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

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Date: Nov 5, 2019, 19:36 UTC
Message-ID: <8548babb-db15-3fc8-f99f-678032e476c3@syntevo.com>
In-Reply-To: <b93592df-561f-36d4-09ce-e02d570c60da@gmail.com>

On 05.11.2019 16:03, Phillip Wood wrote:

Show 5 quoted lines

>> The new option allows to read either a specified file or `stdin`.
>> Reading from file is a good way to avoid competing for stdin, and
>> also gives some extra flexibility.
> 
> I think this flexibility is a good idea

Thanks for your support :) Previously the opinions were mixed and I was a bit afraid that this will invoke a new round of discussions.

Show 5 quoted lines

>> Decisions taken for simplicity:
>> 1) The new option is declared incompatible with other options that
>>     could use `stdin`.
> 
> I'm confused reset does not use stdin does it?

I understand that '--patch' interacts with user via stdin. Will double-check tomorrow.

>> 2) It is not allowed to pass some refspecs in args and others in file.
> 
> s/refspecs/pathspecs/ ?

Thanks! Not quite used to git speak and mix up things sometimes.

Show 8 quoted lines

>> 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.
> 
> As the 'lines' in the file are nul terminated perhaps it would be better 
> to call this --pathspec-file-nul or --nul-termination. I think the use 
> of --null to mean nul termination for config was a mistake (for grep it 
> matches what GUN grep does but it's still unfortunate).

OK, will change to '--pathspec-file-nul'

Show 5 quoted lines

>> +'git reset' [-q] [--pathspec-from-file=<file> [--pathspec-file-null]] 
>> [<tree-ish>]
> 
> --pathspec-file would be shorter and still conveys the intent of the 
> option. Is this line missing a leading space?

'--pathspec-from-file' was kind of suggested instead of '--paths-file' by Junio here: https://public-inbox.org/git/xmqqtv9qr82q.fsf@gitster-ct.c.googlers.com/

so Junio added '-from' to previous writing. Hmm. What do you think, taking Junio's message into account?

As for whitespaces, sorry, will fix.

>> +'git reset' [-q] [--pathspec-from-file=<file> [--pathspec-file-null]] 
>> [<tree-ish>]::
> 
> Alignment again

Will fix

>> +    These forms reset the index entries for all `<pathspec>` to their
> 
> The new form does not mention <pathspec> so this could be confusing

Shall I replace '<pathspec>' with 'pathspecs' ?

> As we have a separate synopsis line for --pathspec-from-file which does 
> not mention <pathspec> it might be better just to say "read pathspecs 
> from `<file>` instead of the command line".

OK

Show 6 quoted lines

>> +    if (pathspec_from_file) {
>> +        if (patch_mode)
>> +            die(_("--pathspec-from-file is incompatible with --patch"));
> 
> This is sensible as -p is interactive so we wouldn't expect command line 
> length to be an issue

Yes, I also thought so. I doubt that user is willing to interactively decide on hundreds of files.

Show 6 quoted lines

>> +    } else if (pathspec_file_null)
>> +        die(_("--pathspec-file-null requires --pathspec-from-file"));
> 
> Style nit: the coding guidelines state that if any branch of an if 
> statement requires braces then all the branches should be braced. This 
> is widely ignored though.

Took this code from a guy who ignored it :) But sure, will change.

> These days we tend to set up the expected files within the relevant test 
> case using <<-\EOF to allow indentation and disallow substitution 
> (unless it's needed of course)

I'll try to change this.

Show 10 quoted lines

>> +test_expect_success 'quotes' '
>> +    restore_checkpoint &&
>> +
>> +    git rm fileA.t &&
>> +    printf "\"file\\101.t\"" | git reset --pathspec-from-file=- &&
>> +
>> +    verify_state expect.a
> 
> If I've understood correctly this doesn't test if a path is correctly 
> unquoted, only that it is accepted.

In my understanding, 'verify_state expect.a' should test that it's correctly understood. Am I wrong?

>> +test_expect_success '--pathspec-from-file is not compatible with 
>> --soft --hard' '
> 
> s/--soft --hard/--soft or --hard/

Good idea.

>> +test_expect_success 'only touches what was listed' '
> 
> s/^/--pathspec-from-file / ?

I thought that whole test package is about '--pathspec-from-file' so I'd rather not repeat that in every test name. Shall I change that?