Re: [PATCH 3/5] reset: support the --pathspec-from-file option

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Date: Nov 6, 2019, 15:56 UTC
Message-ID: <df4cdf1d-1775-9d25-f32b-1abd57dffc31@syntevo.com>
In-Reply-To: <xmqqd0e5vcq9.fsf@gitster-ct.c.googlers.com>

I think I have implemented most suggestions in PatchV2. Thanks!

Show 5 quoted lines

>> 2) Some UI's have started and grown with porcelain commands. Replacing
>>     existing logic with plumbing commands could be cumbersome and prone
>>     to various new problems.
> 
> There is not a lot of sympathy for such argument ;-)

Let's think that this merely reinforces the first point :)

Show 11 quoted lines

> I do not understand this part.  Wouldn't it natural to expect that
> "-z", when used with "--pathspec-from-file", tell us that the named
> file is NUL delimited collection of records?  What's different about
> "--pathspec-file-null" that it is confusing to all it "-z"?
> 
> I actually do not mind not having "-z" and using only
> "--pathspec-file-null".  A new option with long name that feels
> similar to existing "-z" but does sufficiently different things so
> that it needs a different name, however, feels counter-productive
> for the purpose of using it in the UI that produces commands to be
> learned by end-users.

I didn't want to run into a situation where '-z' suddenly applies to multiple things (for example, if later it's decided to allow --patch with --pathspec-from-file).

Regarding UI, I think that (insert our UI name here) will not use '--pathspec-from-file' unless there are really many files. When it does happen, user will probably have an easier time understanding '--pathspec-file-nul' then '-z'.

Show 7 quoted lines

> When I invented the terms "pathspec", "refspec", etc. for this
> project, I meant them to be collective nouns.  A pathspec is a set
> of pathspec elements, each of which is usually given as a separate
> argument on the command line.  So "Read pathspec from file" is good
> (not "Read pathspecs from file").
> 
> And "Pathspec elements" are separated by LF or CR/LF.

I changed that. However, `git grep pathspecs` is quite interesting. My own taste says that "pathspecs" is more expected then "pathspec".

Show 7 quoted lines

> A tangent.  Since we do not allow NUL in a pathspec element, we do
> not even need the "-z" option.  When we read pathspec from a file,
> we can take any of CRLF, LF or NUL as the separator.
> 
> Ah, sorry, that would not help very much.  With "-z" we are allowing
> to express pathspec elements inside which there are embedded LF or
> CR/LF.  Sorry about the noise.

Yes, NUL delimiter allows to avoid escaping.

>   - Quote end-of-here-document (EOF in the above) if you do not rely
>     on parameter interpolation inside the here document; this helps
>     readers by telling them that what is presented is used verbatim.

Tests overwhelmingly prefer escaped form (\EOF) instead, so I used that.

> This feeds "fileA.t<NUL>fileB.t" without <NUL> after "fileB.t" to
> the command.  Intended?

Kind of intended. I changed tests to always use trailing delimiters, and also added a new dedicated test without trailing delimiter.