Re: [PATCH v1 3/3] convert: add filter.<driver>.useProtocol option

> On 24 Jul 2016, at 22:14, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 2016-07-24 o 20:36, Lars Schneider pisze:
>>> On 23 Jul 2016, at 02:11, Jakub Narębski <jnareb@gmail.com> wrote:
>>>> W dniu 2016-07-22 o 17:49, larsxschneider@gmail.com pisze:
>>>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>>
>>>> Nb. this line is only needed if you want author name and/or date
>>>> different from the email sender, or if you have sender line misconfigured
>>>> (e.g. lacking the human readable name).
>>>
>>> I use "git format-patch" to generate these emails:
>>>
>>> git format-patch --cover-letter --subject-prefix="PATCH ..." -M $BASE -o $OUTPUT
>>>
>>> How would I disable this line? (I already checked the man page to no avail).
>>
>> If you are using `git send-email` or equivalent, I think it is
>> stripped automatically if it is not needed (in you case it was,
>> because Sender was lacking human readable name... at least I think
>> it was because of what my email reader inserted as reply line).
>> If you are using an ordinary email client, you need to remove it
>> yourself, if needed.
> 
> Weird. I am sending the patches with this command:
> 
> git send-email mystuff/* --to=git@vger.kernel.org --cc=...
> 
> Maybe I need to try the "--suppress-from" switch?!

>>>> I also agree that we might want to be able to keep clean and smudge
>>>> filters separate, but be able to run a single program if they are
>>>> both the same. I think there is a special case for filter unset,
>>>> and/or filter being "cat" -- we would want to keep that.
>>>
>>> Since 1a8630d there is a more efficient way to unset a filter ;-)
>>> Can you think of other cases where the separation would be useful?
>>
>> I can't think of any, but it doesn't mean that it does not exist.
>> It also does not mean that you need to consider situation that may
>> not happen. Covering one-way filters, like "indent" filter for `clean`,
>> should be enough... they do work with your proposal, don't they?
> 
> This should work right now but it would be a bit inefficient (the filter
> would just pass the data unchanged through the smudge command). I plan to
> add a "capabilities" flag to the protocol. Then you can define only
> the "clean" capability and nothing or the current filter mechanism 
> would happen for smudge (I will make a test case to demonstrate that
> behavior in v2).

>>>> 	Git <-- Filter: "capabilities clean smudge\n"
>>>>
>>>> Or we can add capabilities in later version...
>>>
>>> That is an interesting idea. My initial thought was to make the capabilities
>>> of a certain version fix. If we want to add new capabilities then we would 
>>> bump the version. I wonder what others think about your suggestion!
>>
>> Using capabilities (like git-upload-pack / git-receive-pack, that is
>> smart Git transfer protocols do) is probably slightly more difficult on
>> the Git side (assuming no capabilities negotiation), but also much more
>> flexible than pure version numbers.
>>
>> One possible idea for a capability is support for passing input
>> and output of a filter via filesystem, like cleanToFile and smudgeFromFile
>> proposal in 'jh/clean-smudge-annex' (in 'pu').
>>
>> For example:
>>
>> 	Git <-- Filter: "capabilities clean smudge cleanToFile smudgeFromFile\n"
> 
> Yes, I like that very much. As stated above, I will add that in v2.

>>>>> 	Git --> Filter: "testfile.dat\n"
>>>>
>>>> Unfortunately, while sane filenames should not contain newlines[1],
>>>> the unfortunate fact is that *filenames can include newlines*, and
>>>> you need to be able to handle that[2].  Therefore you need either to
>>>> choose a different separator (the only one that can be safely used
>>>> is "\0", i.e. the NUL character - but it is not something easy to
>>>> handle by shell scripts), or C-quote filenames as needed, or always
>>>> C-quote filenames.  C-quoting at minimum should include quoting newline
>>>> character, and the escape character itself.
>>>>
>>>> BTW. is it the basename of a file, or a full pathname?
>>>>
>>>> [1]: http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html
>>>> [2]: http://www.dwheeler.com/essays/filenames-in-shell.html
>>>
>>> Thanks for this explanation. A bash version of the protocol is not
>>> trivial (I tried it but ended up using Perl). Therefore I think "\0"
>>> would be a good choice?
>>
>> That, or use git convention of surrounding C-quoted filenames in
>> double quotes (which means that if it begins with quote, it is C-quoted).
>>
>> For example:
>>
>>  $ git commit -m 'Initial commit'
>>  [master (root-commit) 266dab0] Initial commit
>>   2 files changed, 2 insertions(+)
>>   create mode 100644 foo
>>   create mode 100644 "foo \" \\ ,"
>>  $ ls -1
>>  foo " \ ,
>>  foo
>>
>> I'm not sure which solution would be easier for filter writers,
>> NUL termination, or C-quoting.
> 
> Unless someone has a convincing argument for one solution or the other
> I will go with the \0 termination as it seems easier.

>>>>> 	Git --> Filter: "7\n"
>>>>
>>>> That's the content size in bytes written as an ASCII number.
>>>
>>> Correct.
>>
>> But not obvious from the description / documentation.
> 
> I will improve that in v2. Should I add the info that it is base10 or
> would you consider that a given?

>>>>> ---
>>>>> Documentation/gitattributes.txt |  41 +++++++-
>>>>> convert.c                       | 210 ++++++++++++++++++++++++++++++++++++++--
>>>>> t/t0021-conversion.sh           | 170 ++++++++++++++++++++++++++++++++
>>>>
>>>> Wouldn't it be better to name the test case something more
>>>> descriptive, for example
>>>>
>>>>  t/t0021-filter-driver-useProtocol.sh
>>>>
>>>> The name of test should be adjusted to final name of the feature,
>>>> of course.
>>>
>>> I think the prefix numbers should be unique, no? And t0022 is already taken.
>>
>> I meant here that the "conversion" part of "t/t0021-conversion.sh" test
>> filename is not descriptive enough.
> 
> Ah, I see. You suggest to rename the test case? Would that be OK with the Git
> community?

>>>             What do you mean by "discrepancy in how config variables are 
>>> referenced"?
>>
>> What I meant here that filter.<driver>.smudge and filter.<driver>.clean
>> were referenced as "`smudge` command" and "`clean` command" in the paragraph
>> you modified.
>>
>> Perhaps filter.<driver>.useProtocol is all right (I have not looked further),
>> but it should be formatted as `filter.<driver>.useProtocol` IMVHO.
> 
> Initially I thought so, too. But "filter.<driver>.required", which is already
> mentioned in gitattributes.txt, does not use this style. Should I change that, too,
> or use the existing style?

>> The problem is do the protocol need to have some way of communicating
>> errors from the filter to Git?  Perhaps using stderr would be enough
>> (but then Git would need to drain it, I think... unless it is not
>> redirected), perhaps some command is needed?
>>
>> For example, instead of:
>>
>> 	Git <-- Filter: "15\n"
>> 	Git <-- Filter: "SMUDGED_CONTENT"
>>
>> perhaps filter should return
>>
>> 	Git <-- Filter: "error\n"
>> 	Git <-- Filter: "ONE_LINE_OF_ERROR_DESCRIPTION\n"
>>
>> on error? Or if printing expected output length upfront is easier,
>> use a signal (but that is supposedly not that reliable as message
>> passing mechanism)?
>>
>> It might be the case that some files return errors, but some do not.
> 
> I would prefer it if the filter just dies in case of trouble and that
> way communicates to Git that something went wrong. Everything else
> just complicates the protocol.