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

[Jonathan Nieder <jrnieder@gmail.com>]

Sverre Rabbelier wrote:

> [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
[...]
> @@ -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.