On 05.11.2019 16:03, Phillip Wood wrote:
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.
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.
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'
+'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@xxxxxxxxxxxxxxxxxxxxxxxxx/
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
+ 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.
+ } 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.
+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?
