Re: [PATCH v2 1/1] reset: support the --stdin option
flat view
From: Jeff King <hidden>
Date: 2017-01-27 17:11:42
On Fri, Jan 27, 2017 at 01:38:55PM +0100, Johannes Schindelin wrote:
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:
quoted hunk ↗ jump to hunk
+--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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
+ if (stdin_paths) { + while (stdin_nr) + free(stdin_paths[--stdin_nr]); + free(stdin_paths); + }
argv_array_clear(&stdin_paths);
quoted hunk ↗ jump to hunk
@@ -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...
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ } else if (nul_term_line) + die(_("-z requires --stdin")); +
Hmm, there's our brace question coming up again. :)
quoted hunk ↗ jump to hunk
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