Re: [PATCH v2 20/20] transport-helper: implement marks location as capability

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 20, 2011, 01:29 UTC
Message-ID: <20110620012937.GB25836@elie>
In-Reply-To: <1308496725-22329-21-git-send-email-srabbelier@gmail.com>

Sverre Rabbelier wrote:

Show 6 quoted lines

> [Subject: transport-helper: implement marks location as capability]
>
> While this requires the helper to flush stdout after listing 'gitdir'
> as capability, and read a command (the 'gitdir' response from the
> remote helper infrastructure) right after that, this is more elegant
> and does not require an ad-hoc exchange of values.

Probably I'm not thinking straight, but it's not obvious how the subject connects to the content. I suppose the pieces I'm missing are that:

 - this patch introduces new undocumented "import-marks" and
   "export-marks" capabilities that take a pathname as parameter;

 - for remote-testgit, that pathname is based on $GIT_DIR, so to
   retrieve that value we flush stdout and read the gitdir immediately
   rather than hoping the gitdir arrives on stdin in time for it to
   be used later.

But that still leaves a couple of other questions unanswered:

Why do we use a "gitdir" capability for this at all, instead of exporting the GIT_DIR environment variable as Tomas has suggested? (Not about this patch, but a separate patch explaining that in the documentation would be nice.)

How does this interact with fast-import's --relative-marks feature (if at all)?

[...]

> +++ b/transport-helper.c

[...]

Show 17 quoted lines

> @@ -718,26 +728,6 @@ static int push_refs_with_export(struct transport *transport,
>  
>  	write_constant(helper->in, "export\n");
>  
> -	recvline(data, &buf);
> -	if (debug)
> -		fprintf(stderr, "Debug: Got export_marks '%s'\n", buf.buf);
> -	if (buf.len) {
> -		struct strbuf arg = STRBUF_INIT;
> -		strbuf_addstr(&arg, "--export-marks=");
> -		strbuf_addbuf(&arg, &buf);
> -		export_marks = strbuf_detach(&arg, NULL);
> -	}
> -
> -	recvline(data, &buf);
> -	if (debug)
> -		fprintf(stderr, "Debug: Got import_marks '%s'\n", buf.buf);

Hm, it seems there is a silent (but good) change in the behavior of the "export" command, too.

Except where noted already, the patches look good, and the series seems to make the remote helper infrastructure a little saner. Thanks.