Re: [PATCH 3/4] fast-import: let importers retrieve blobs

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 4, 2010, 02:35 UTC
Message-ID: <20101204023515.GA18735@burratino>
In-Reply-To: <20101128194501.GD19998@burratino>

Jonathan Nieder wrote:

Show 5 quoted lines

>                                                              The value
> for cat-blob-fd cannot be specified in the stream because that would
> be a layering violation: the decision of where to direct a stream has
> to be made when fast-import is started anyway, so we might as well
> make the stream format is independent of that detail.

Ungrammatical.  I think I meant:

 There is no POSIX facility to open a file descriptor from outside
 after a process has already started; therefore, the frontend has to
 prepare a file descriptor for writing blobs before executing
 git fast-import.  The --cat-blob-fd command line option indicates
 which file descriptor that is, defaulting to 1.

 It does not make sense to wait until the stream starts to specify
 which fd so it is not allowed, avoiding a potential layering
 violation.  Other fast-import backends might provide other ways to
 specify where the blob stream should be written.

Show 7 quoted lines

> +++ b/fast-import.c
> @@ -2824,6 +2910,8 @@ static int parse_one_feature(const char *feature, int from_stream)
>  		option_import_marks(feature + 13, from_stream);
>  	} else if (!prefixcmp(feature, "export-marks=")) {
>  		option_export_marks(feature + 13);
> +	} else if (!strcmp(feature, "cat-blob")) {
> +		; /* Don't die - this feature is supported */

Implies support for a "--cat-blob" command line option that checks for cat-blob support. Is this wanted?

(If so, it should be documented. If not, the condition should be "from_stream && !strcmp(...)".)

Show 9 quoted lines

> @@ -2918,6 +3006,11 @@ static void parse_argv(void)
>  		if (parse_one_feature(a + 2, 0))
>  			continue;
>  
> +		if (!prefixcmp(a + 2, "cat-blob-fd=")) {
> +			option_cat_blob_fd(a + 2 + strlen("cat-blob-fd="));
> +			continue;
> +		}
> +

Would be simpler and more explicit to put in parse_one_feature:

	} else if (!from_stream && !prefixcmp(feature, "cat-blob-fd=")) {

Sorry this is taking so long to get right. :-/ Jonathan