[PATCH 0/1] Teach git reset to optionally read the pathspecs from stdin

[PATCH 0/1] Teach git reset to optionally read the pathspecs from stdin

[Johannes Schindelin via GitGitGadget]

Especially in 3rd-party tools, where the shape of the repository/worktree is
unpredictable, it is a wise thing to have an option to pass pathspec
parameters via stdin rather than via the command line, as the latter has
size restrictions that the former does not have.

Johannes Schindelin (1):
  reset: support the --stdin option

 Documentation/git-reset.txt | 11 +++++++++
 builtin/reset.c             | 45 ++++++++++++++++++++++++++++++++++++-
 t/t7107-reset-stdin.sh      | 41 +++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 1 deletion(-)
 create mode 100755 t/t7107-reset-stdin.sh


base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-133%2Fdscho%2Freset-stdin-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-133/dscho/reset-stdin-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/133
-- 
gitgitgadget

[PATCH 1/1] reset: support the --stdin option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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. This make the implementation simpler than adding
the pathspecs immediately after the corresponding lines are read.

Note also that we specifically handle CR/LF line endings in `stdin` to
support Windows better.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-reset.txt | 11 +++++++++
 builtin/reset.c             | 45 ++++++++++++++++++++++++++++++++++++-
 t/t7107-reset-stdin.sh      | 41 +++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 1 deletion(-)
 create mode 100755 t/t7107-reset-stdin.sh

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 132f8e55f6..e03014eb04 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git reset' [-q] [<tree-ish>] [--] <paths>...
 'git reset' (--patch | -p) [<tree-ish>] [--] [<paths>...]
+'git reset' [-q] [--stdin [-z]] [<tree-ish>]
 'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [<commit>]
 
 DESCRIPTION
@@ -100,6 +101,16 @@ OPTIONS
 	`reset.quiet` config option. `--quiet` and `--no-quiet` will
 	override the default behavior.
 
+--stdin::
+	Instead of taking the list of paths from the command line,
+	read the list of paths from the standard input. The paths are
+	read verbatim, i.e. without special pathspecs handling. Paths
+	are expected to be separated by LF or CR/LF, unless the `-z`
+	option is in effect.
+
+-z::
+	Only meaningful with `--stdin`; paths are separated with
+	NUL character instead of LF.
 
 EXAMPLES
 --------
diff --git a/builtin/reset.c b/builtin/reset.c
index 4d18a461fa..f8d06223d6 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -25,12 +25,16 @@
 #include "cache-tree.h"
 #include "submodule.h"
 #include "submodule-config.h"
+#include "strbuf.h"
+#include "quote.h"
+#include "argv-array.h"
 
 #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
 
 static const char * const git_reset_usage[] = {
 	N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
 	N_("git reset [-q] [<tree-ish>] [--] <paths>..."),
+	N_("git reset [-q] [--stdin [-z]] [<tree-ish>]"),
 	N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
 	NULL
 };
@@ -284,7 +288,8 @@ static int git_reset_config(const char *var, const char *value, void *cb)
 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;
+	struct argv_array stdin_paths = ARGV_ARRAY_INIT;
 	const char *rev;
 	struct object_id oid;
 	struct pathspec pathspec;
@@ -306,6 +311,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
 		OPT_BOOL('N', "intent-to-add", &intent_to_add,
 				N_("record only the fact that removed paths will be added later")),
+		OPT_BOOL('z', NULL, &nul_term_line,
+				N_("paths are separated with NUL character")),
+		OPT_BOOL(0, "stdin", &read_from_stdin,
+				N_("read paths from <stdin>")),
 		OPT_END()
 	};
 
@@ -316,6 +325,38 @@ 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;
+		int flags = PATHSPEC_PREFER_FULL;
+		struct strbuf buf = STRBUF_INIT;
+		struct strbuf unquoted = STRBUF_INIT;
+
+		if (patch_mode)
+			die(_("--stdin is incompatible with --patch"));
+
+		if (pathspec.nr)
+			die(_("--stdin is incompatible with path arguments"));
+
+		while (getline_fn(&buf, stdin) != EOF) {
+			if (!nul_term_line && buf.buf[0] == '"') {
+				strbuf_reset(&unquoted);
+				if (unquote_c_style(&unquoted, buf.buf, NULL))
+					die(_("line is badly quoted"));
+				strbuf_swap(&buf, &unquoted);
+			}
+			argv_array_push(&stdin_paths, buf.buf);
+			strbuf_reset(&buf);
+		}
+		strbuf_release(&unquoted);
+		strbuf_release(&buf);
+
+		flags |= PATHSPEC_LITERAL_PATH;
+		parse_pathspec(&pathspec, 0, flags, prefix, stdin_paths.argv);
+
+	} else if (nul_term_line)
+		die(_("-z requires --stdin"));
+
 	unborn = !strcmp(rev, "HEAD") && get_oid("HEAD", &oid);
 	if (unborn) {
 		/* reset on unborn branch: treat as reset to empty tree */
@@ -416,5 +457,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (!pathspec.nr)
 		remove_branch_state(the_repository);
 
+	argv_array_clear(&stdin_paths);
+
 	return update_ref_status;
 }
diff --git a/t/t7107-reset-stdin.sh b/t/t7107-reset-stdin.sh
new file mode 100755
index 0000000000..db5483b8f1
--- /dev/null
+++ b/t/t7107-reset-stdin.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='reset --stdin'
+
+. ./test-lib.sh
+
+test_expect_success 'reset --stdin' '
+	test_commit hello &&
+	git rm hello.t &&
+	test -z "$(git ls-files hello.t)" &&
+	echo hello.t | git reset --stdin &&
+	test hello.t = "$(git ls-files hello.t)"
+'
+
+test_expect_success 'reset --stdin -z' '
+	test_commit world &&
+	git rm hello.t world.t &&
+	test -z "$(git ls-files hello.t world.t)" &&
+	printf world.tQworld.tQhello.tQ | q_to_nul | git reset --stdin -z &&
+	printf "hello.t\nworld.t\n" >expect &&
+	git ls-files >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--stdin requires --mixed' '
+	echo hello.t >list &&
+	test_must_fail git reset --soft --stdin <list &&
+	test_must_fail git reset --hard --stdin <list &&
+	git reset --mixed --stdin <list
+'
+
+
+test_expect_success '--stdin trims carriage returns' '
+	test_commit endline &&
+	git rm endline.t &&
+	printf "endline.t\r\n" >list &&
+	git reset --stdin <list &&
+	test endline.t = "$(git ls-files endline.t)"
+'
+
+test_done
-- 
gitgitgadget

Re: [PATCH 1/1] reset: support the --stdin option

[Eric Sunshine]

On Wed, Sep 4, 2019 at 5:38 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> 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).
> [...]
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/builtin/reset.c b/builtin/reset.c
> @@ -316,6 +325,38 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
> +       if (read_from_stdin) {
> +               [...]
> +               while (getline_fn(&buf, stdin) != EOF) {
> +                       if (!nul_term_line && buf.buf[0] == '"') {
> +                               strbuf_reset(&unquoted);
> +                               if (unquote_c_style(&unquoted, buf.buf, NULL))
> +                                       die(_("line is badly quoted"));

Perhaps include the offending line in the error message to make it
easier for the user to understand what went wrong:

     die(_("line is badly quoted: %s"), buf.buf);

Re: [PATCH 1/1] reset: support the --stdin option

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Sep 4, 2019 at 5:38 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> 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).
>> [...]
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> diff --git a/builtin/reset.c b/builtin/reset.c
>> @@ -316,6 +325,38 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>> +       if (read_from_stdin) {
>> +               [...]
>> +               while (getline_fn(&buf, stdin) != EOF) {
>> +                       if (!nul_term_line && buf.buf[0] == '"') {
>> +                               strbuf_reset(&unquoted);
>> +                               if (unquote_c_style(&unquoted, buf.buf, NULL))
>> +                                       die(_("line is badly quoted"));
>
> Perhaps include the offending line in the error message to make it
> easier for the user to understand what went wrong:
>
>      die(_("line is badly quoted: %s"), buf.buf);

I think the patch is solving the right problem by adding a feature
that is useful to work around the command line limit, but I doubt
the wisdom of limiting the input to "paths" by default.  

It would be perfect if the command gets taught to take pathspec,
with "git --[no-]literal-pathspec reset --stdin" as an escape hatch.

Re: [PATCH 1/1] reset: support the --stdin option

[Alexandr Miloslavskiy]

Johannes, thanks for working on this problem!

In the previous discussion, there was a suggestion to change
'--stdin' to '--paths-file', where '--paths-file -' would mean stdin:
https://public-inbox.org/git/066cfd61-9700-e154-042f-fc9cffbd6346@web.de/

I believe that was a good suggestion for a few reasons:
1) Supporting files in addition to stdin sounds reasonable for its cost.
2) '--paths-file' will support files and stdin with the same "interface",
    avoiding the possible need for another interface later.
3) '--paths-file' sounds more self-documented then '--stdin'.

Later, we intend to provide patches to extend the same feature to
multiple other commands, at least {add, checkout, commit, rm, stash},
and I'm merely trying to avoid possible design issues for this
larger-scale change.

If you don't mind the suggestion but not willing to spend time
implementing it, we'd like to step in and contribute the remaining
work.

Re: [PATCH 1/1] reset: support the --stdin option

[Junio C Hamano]

Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> writes:

> Johannes, thanks for working on this problem!
>
> In the previous discussion, there was a suggestion to change
> '--stdin' to '--paths-file', where '--paths-file -' would mean stdin:
> https://public-inbox.org/git/066cfd61-9700-e154-042f-fc9cffbd6346@web.de/
>
> I believe that was a good suggestion for a few reasons:
> 1) Supporting files in addition to stdin sounds reasonable for its cost.
> 2) '--paths-file' will support files and stdin with the same "interface",
>    avoiding the possible need for another interface later.
> 3) '--paths-file' sounds more self-documented then '--stdin'.
>
> Later, we intend to provide patches to extend the same feature to
> multiple other commands, at least {add, checkout, commit, rm, stash},
> and I'm merely trying to avoid possible design issues for this
> larger-scale change.
>
> If you don't mind the suggestion but not willing to spend time
> implementing it, we'd like to step in and contribute the remaining
> work.

Ahh, I completely forgot about the earlier exchange.  Thanks for a
pointer.

While I do like the idea of adding an option to take the pathspec,
which you would usually give from the command line, from the
standard input, as a standard technique to lift the command line
length limit on various platforms, what I do not find right in the
posted patch is that it conflates it with something orthogonal.

When you already have a set of paths you would want to give to the
command, and when these paths may have wildcard letters in them, you
would want to tell the command that you do not want wildcards in the
pathspec to take effect.  That is a valid wish and we already have a
standard solution for that (i.e. the "--literal-pathspecs" option).

But it is orthogonal to how you feed these paths to the command.  I
would not say "unrelated", in that there may be correlation between
the cases where you would want to feed pathspec from the standard
input and the cases where you would want these pathspec taken
litearlly, but the choice is still "orthogonal".  

If we said "when feeding from the standard input, they are taken
literally by default, but on the command line, the wildcards are
expanded", that is bad enough---I do not think we want to hear any
unnecessary "Git UI is inconsistent" noise raised from such a design
choice.

The posted patch does not even do that.  It takes its input as
literal pathspec and it is done unconditionally without any way to
override it.

If we introduce a new --paths-from-file and make it take only paths
and never pathspecs (i.e. no wildcard expansion, a directory does
not mean everything underneath it), that would be less worrisome wrt
the UI inconsistency front (but we may need to commit to teach
existing commands that take pathspec from the standard input the new
option, too, so that all subcommands that can take some form of
paths specification would work the same way with --paths-from-file).

Or we can do --stdin (or --pathspec-from-file=-) that takes pathspecs,
whose literal-ness is controlled by the '--literal-pathspecs' option.

Thanks.