Re: [PATCH] sub-process: print the cmd when a capability is unsupported

From: Jonathan Tan <jonathantanmy@google.com>
Date: Aug 17, 2017, 21:49 UTC
Message-ID: <20170817144939.72fcd998@twelve2.svl.corp.google.com>
In-Reply-To: <25BB25E4-DD3F-46A5-8CD5-D784CBAB6F52@gmail.com>

On Thu, 17 Aug 2017 23:34:33 +0200 Lars Schneider <larsxschneider@gmail.com> wrote:

Show 16 quoted lines

> 
> > On 17 Aug 2017, at 23:01, Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > Christian Couder <christian.couder@gmail.com> writes:
> > 
> >> ... but I think we should then emphasize more in our test
> >> scripts (maybe by giving a good example) and perhaps also in the doc
> >> that the filters/sub-processes should really pay attention and not
> >> output any capability that are not supported by Git.
> > 
> > Oh, absolutely.  If you know there is such a breakage in our test
> > script, please do fix it.
> > 
> > Thanks.
> 
> Junio's reasoning [1] is spot on from my point of view.

Agreed.

> 
> I intentionally did not add the negotiation to the test code to keep
> the test as simple as possible.

I think this is the correct approach - the test was testing Git's behavior, not the filter's behavior. (Although, if someone wanted to add a test for a misbehaving filter, that would be great, although such a test would have hardcoded output from the filter anyway.)

Show 10 quoted lines

> However, I wrote this in the
> gitattributes docs [2]:
> 
>   After the version negotiation Git sends a list of all capabilities that
>   it supports and a flush packet. Git expects to read a list of desired
>   capabilities, which must be a subset of the supported capabilities list,
>   and a flush packet as response:
> 
> Maybe we should revive "Documentation/technical/api-sub-process.txt" [3]
> after all to explain these kind of things?

As for reviving that specific file, I saw you wrote a similar comment but I didn't reply to it - sorry. The commit in question is 7e2e1bb ("Documentation: migrate sub-process docs to header", 2017-07-26), and in that commit, everything in api-sub-process.txt was migrated. I think it's better for such documentation to be in a .h file, rather than a .txt file.

As for describing the Long Running Process Protocol in a Documentation/.../txt file, my plan was to create a new file specifically for that, as you can see in an e-mail I sent [4]. I'm OK with putting it in a different place, but it probably shouldn't be named "api", because those are for internal Git APIs.

[4] https://public-inbox.org/git/eadce97b6a1e80345a2621e71ce187e9e6bc05bf.1501532294.git.jonathantanmy@google.com/

> [1] https://public-inbox.org/git/xmqq8tijpkrv.fsf@gitster.mtv.corp.google.com/
> [2] https://github.com/git/git/blob/b3622a4ee94e4916cd05e6d96e41eeb36b941182/Documentation/gitattributes.txt#L408-L411
> [3] https://public-inbox.org/git/20170807102136.30b23023@twelve2.svl.corp.google.com/