Re: [PATCH v2 1/1] reset: support the --stdin option
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
Johannes Schindelin
