Re: [PATCH v2 1/1] reset: support the --stdin option
- From
Jeff King <peff@peff.net>
- Date
- Jan 27, 2017, 17:04 UTC
- Message-ID
- <20170127170422.lvpghp6jdud37zxx@sigill.intra.peff.net>
- In-Reply-To
- <3b0bad26045ed175f40f050e15b65cb6a8f2368c.1485520718.git.johannes.schindelin@gmx.de>
On Fri, Jan 27, 2017 at 01:38:55PM +0100, Johannes Schindelin wrote:
Show 9 quoted lines
> Just like with other Git commands, this option makes it read the paths > from the standard input. It comes in handy when resetting many, many > paths at once and wildcards are not an option (e.g. when the paths are > generated by a tool). > > Note: we first parse the entire list and perform the actual reset action > only in a second phase. Not only does this make things simpler, it also > helps performance, as do_diff_cache() traverses the index and the > (sorted) pathspecs in simultaneously to avoid unnecessary lookups.
This looks OK to me. At first I wondered if using PATHSPEC_LITERAL_PATH was consistent with other "--stdin" tools. I think it mostly is (or at least consistent with checkout-index). The exception is "rev-list --stdin", but that's probably fine. It is taking rev-list arguments in the first place, not a list of paths.
A few minor suggestions:
Show 8 quoted lines
> +--stdin:: > + Instead of taking list of paths from the command line, > + read list of paths from the standard input. Paths are > + separated by LF (i.e. one path per line) by default. > + > +-z:: > + Only meaningful with `--stdin`; paths are separated with > + NUL character instead of LF.
Is it worth clarifying that these are paths, not pathspecs? The word "paths" is used to refer to the pathspecs on the command-line elsewhere in the document.
It might also be worth mentioning the quoting rules for the non-z case.
Show 8 quoted lines
> @@ -267,7 +270,9 @@ static int reset_refs(const char *rev, const struct object_id *oid)
> int cmd_reset(int argc, const char **argv, const char *prefix)
> {
> int reset_type = NONE, update_ref_status = 0, quiet = 0;
> - int patch_mode = 0, unborn;
> + int patch_mode = 0, nul_term_line = 0, read_from_stdin = 0, unborn;
> + char **stdin_paths = NULL;
> + int stdin_nr = 0, stdin_alloc = 0;This list is a good candidate for an argv_array, I think. So:
struct argv_array stdin_paths = ARGV_ARRAY_INIT;
> + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc); > + stdin_paths[stdin_nr++] = xstrdup(buf.buf);
argv_array_push(&stdin_paths, buf.buf);
> + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc); > + stdin_paths[stdin_nr++] = NULL;
This goes away because argv_array handles it for you.
Show 5 quoted lines
> + if (stdin_paths) {
> + while (stdin_nr)
> + free(stdin_paths[--stdin_nr]);
> + free(stdin_paths);
> + }argv_array_clear(&stdin_paths);
Show 9 quoted lines
> @@ -295,6 +304,43 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
> PARSE_OPT_KEEP_DASHDASH);
> parse_args(&pathspec, argv, prefix, patch_mode, &rev);
>
> + if (read_from_stdin) {
> + strbuf_getline_fn getline_fn = nul_term_line ?
> + strbuf_getline_nul : strbuf_getline_lf;
> + int flags = PATHSPEC_PREFER_FULL |
> + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP;You set two flags here...
Show 5 quoted lines
> + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc); > + stdin_paths[stdin_nr++] = NULL; > + flags |= PATHSPEC_LITERAL_PATH; > + parse_pathspec(&pathspec, 0, flags, prefix, > + (const char **)stdin_paths);
...and then one more here. They all seem to be set unconditionally, and we never look at "flags" between the two lines. I think it would be more obvious to set them all in the same place.
> + } else if (nul_term_line)
> + die(_("-z requires --stdin"));
> +Hmm, there's our brace question coming up again. :)
Show 9 quoted lines
> diff --git a/t/t7107-reset-stdin.sh b/t/t7107-reset-stdin.sh > new file mode 100755 > index 0000000000..997dc42dd2 > --- /dev/null > +++ b/t/t7107-reset-stdin.sh > @@ -0,0 +1,33 @@ > +#!/bin/sh > + > +test_description='reset --stdin'
Here are a few things we might want to test that I didn't see covered:
- feeding path X does not reset path Y
- de-quoting in the non-z case
-Peff