Re: [PATCH v10 13/14] convert: add filter.<driver>.process option

[Lars Schneider <larsxschneider@gmail.com>]

@Peff: If you have time, it would be great if you could comment on
one question below prefixed with "@Peff". Thanks!


> On 12 Oct 2016, at 03:54, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> W dniu 12.10.2016 o 00:26, Lars Schneider pisze: 
>>> On 09 Oct 2016, at 01:06, Jakub Narębski <jnareb@gmail.com> wrote:
>>>> 
>> 
>>>> After the filter started
>>>> Git sends a welcome message ("git-filter-client"), a list of
>>>> supported protocol version numbers, and a flush packet. Git expects
>>>> +to read a welcome response message ("git-filter-server") and exactly
>>>> +one protocol version number from the previously sent list. All further
>>>> +communication will be based on the selected version. The remaining
>>>> +protocol description below documents "version=2". Please note that
>>>> +"version=42" in the example below does not exist and is only there
>>>> +to illustrate how the protocol would look like with more than one
>>>> +version.
>>>> +
>>>> +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:
>>>> +------------------------
>>>> +packet:          git> git-filter-client
>>>> +packet:          git> version=2
>>>> +packet:          git> version=42
>>>> +packet:          git> 0000
>>>> +packet:          git< git-filter-server
>>>> +packet:          git< version=2
>>>> +packet:          git> clean=true
>>>> +packet:          git> smudge=true
>>>> +packet:          git> not-yet-invented=true
>>>> +packet:          git> 0000
>>>> +packet:          git< clean=true
>>>> +packet:          git< smudge=true
>>>> +packet:          git< 0000
>>> 
>>> WARNING: This example is different from description!!!
>> 
>> Can you try to explain the difference more clearly? I read it multiple
>> times and I think this is sound.
> 
> I'm sorry it was *my mistake*.  I have read the example exchange wrong.
> 
> On the other hand that means that I have other comment, which I though
> was addressed already in v10, namely that not all exchanges ends with
> flush packet (inconsistency, and I think a bit of lack of extendability).

Well, this part of the protocol is not supposed to be extensible because
it is supposed to deal *only* with the version number. It needs to keep 
the same structure to ensure forward and backward compatibility.

However, for consistency sake I will add a flush packet.


>>> In description above the example you have 4-part handshake, not 3-part;
>>> the filter is described to send list of supported capabilities last
>>> (a subset of what Git command supports).
>> 
>> Part 1: Git sends a welcome message...
>> Part 2: Git expects to read a welcome response message...
>> Part 3: After the version negotiation Git sends a list of all capabilities...
>> Part 4: Git expects to read a list of desired capabilities...
>> 
>> I think example and text match, no?
> 
> Yes, it does; as I have said already, I have misread the example. 
> 
> Anyway, in some cases 4-way handshake, where Git sends list of
> supported capabilities first, is better.  If the protocol has
> to prepare something for each of capabilities, and perhaps check
> those preparation status, it can do it after Git sends what it
> could need, and before it sends what it does support.
> 
> Though it looks a bit strange that client (as Git is client here)
> sends its capabilities first...

Git tells the filter what it can do. Then the filter decides what
features it supports. I would prefer to keep it that way as I don't
see a strong advantage for the other way around.


>>> By the way, now I look at it, the argument for using the
>>> "<capability>=true" format instead of "capability=<capability>"
>>> (or "supported-command=<capability>") is weak.  The argument for
>>> using "<variable>=<value>" to make it easier to implement parsing
>>> is sound, but the argument for "<capability>=true" is weak.
>>> 
>>> The argument was that with "<capability>=true" one can simply
>>> parse metadata into hash / dictionary / hashmap, and choose
>>> response based on that.  Hash / hashmap / associative array
>>> needs different keys, so the reasoning went for "<capability>=true"
>>> over "capability=<capability>"... but the filter process still
>>> needs to handle lines with repeating keys, namely "version=<N>"
>>> lines!
>>> 
>>> So the argument doesn't hold water IMVHO, and we can choose
>>> version which reads better / is more natural.
>> 
>> I have to agree that "capability=<capability>" might read a
>> little bit nicer. However, Peff suggested "<capability>=true" 
>> as his preference and this is absolutely OK with me.
> 
> From what I remember it was Peff stating that he thinks "<foo>=true"
> is easier for parsing (it is, but we still need to support the harder
> way parsing anyway), and offered that "<foo>" is good enough (if less
> consistent).
> 
>> I am happy to change that if a second reviewer shares your
>> opinion.
> 
> Also, with "capability=<foo>" we can be more self descriptive,
> for example "supported-command=<foo>"; though "capability" is good
> enough for me.
> 
> For example
> 
> packet:          git> wants=clean
> packet:          git> wants=smudge
> packet:          git> wants=size
> packet:          git> 0000
> packet:          git< supports=clean
> packet:          git< supports=smudge
> packet:          git< 0000
> 
> Though coming up with good names is hard; and as I said "capability"
> is good enough; OTOH with "smudge=true" etc. we don't need to come
> up with good name at all... though I wonder if it is a good thing `\_o,_/

How about this (I borrowed these terms from contract negotiation)?

packet:          git> offers=clean
packet:          git> offers=smudge
packet:          git> offers=size
packet:          git> 0000
packet:          git< accepts=clean
packet:          git< accepts=smudge
packet:          git< 0000

@Peff: Would that be OK for you?


>>>> +------------------------
>>>> +packet:          git< status=abort
>>>> +packet:          git< 0000
>>>> +------------------------
>>>> +
>>>> +After the filter has processed a blob it is expected to wait for
>>>> +the next "key=value" list containing a command. Git will close
>>>> +the command pipe on exit. The filter is expected to detect EOF
>>>> +and exit gracefully on its own.
>>> 
>>> Any "kill filter" solutions should probably be put here.
>> 
>> Agreed. 
>> 
>>> I guess
>>> that filter exiting means EOF on its standard output when read
>>> by Git command, isn't it?
>> 
>> Yes, but at this point Git is not listening anymore.
> 
> I think it might be good idea to have here the information about
> what filter process should do if it needs maybe lengthy closing
> process, to not hold/stop Git command or to not be killed.

I've added:

"Git will wait until the filter process has stopped."


Thanks,
Lars