From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Date: Tue, 05 Nov 2019 19:42:40 GMT
Subject: Re: [PATCH 5/5] commit: support the --pathspec-from-file option
Message-ID: <73fb5aa2-6cdb-424a-5696-a2609d74a034@syntevo.com>
In-Reply-To: <9ca7fa57-c438-7243-6ab1-956d8f132d37@gmail.com>

On 05.11.2019 17:27, Phillip Wood wrote:

>> Also add new '--pathspec-file-null' switch that mirrors '-z' used in
>> various places. Some porcelain commands, such as `git commit`, already
>> use '-z', therefore it needed a new unambiguous name.
> 
> It might be worth tailoring the message to the command rather than 
> having exactly the same message for commit and reset

I also was somewhat unhappy about duplication. But I didn't figure how 
to do that correctly. Currently the messages for 'git reset' and 'git 
commit' are almost identical.

Maybe in 2nd commit I should say something like "Extend 
`--pathspec-file-null` support to `git commit` (see previous patch for 
`git reset`)" ?

> I think my comments from patch 3 about <pathspecs> and the option names 
> apply here as well

Yes, sure, I will try to apply your suggestions to all patches. 
Hopefully without forgetting things :)

>> +    if (!pathspec.nr && (also || (only && !amend && !allow_empty)))
>> +        die(_("No paths with --include/--only does not make sense."));
> 
> I wonder if there is a way of calling parse_pathspec_file() from 
> parse_and_validate_options() instead. Otherwise we end up validating 
> options here instead which is a bit messy.

Yes, I was also somewhat unhappy about that. I will give it more thought.

> Overall this series is nicely structured and is looking pretty good

Thanks, and also thanks for reviewing my patches!