[PATCH 0/2] Support `git reset --stdin`

[PATCH 0/2] Support `git reset --stdin`

[Johannes Schindelin]

This feature was missing, and made it cumbersome for third-party
tools to reset a lot of paths in one go.

Support for --stdin has been added, following builtin/checkout-index.c's
example.


Johannes Schindelin (2):
  reset: fix usage
  reset: support the --stdin option

 Documentation/git-reset.txt | 10 +++++++-
 builtin/reset.c             | 56 +++++++++++++++++++++++++++++++++++++++++++--
 t/t7107-reset-stdin.sh      | 33 ++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 3 deletions(-)
 create mode 100755 t/t7107-reset-stdin.sh


base-commit: 8a36cd87b7c85a651ab388d403629865ffa3ba0d
Published-As: https://github.com/dscho/git/releases/tag/reset-stdin-v1
Fetch-It-Via: git fetch https://github.com/dscho/git reset-stdin-v1

-- 
2.10.1.513.g00ef6dd

[PATCH 1/2] reset: fix usage

[Johannes Schindelin]

The <tree-ish> parameter is actually optional (see man page).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 5aa8607..c04ac07 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -24,7 +24,7 @@
 
 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] [<tree-ish>] [--] <paths>..."),
 	N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
 	NULL
 };
-- 
2.10.1.513.g00ef6dd

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

[Johannes Schindelin]

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: to keep things simple, we first parse the entire list and perform
the actual reset action only in a second phase.

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

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 25432d9..533ef69 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -8,7 +8,7 @@ git-reset - Reset current HEAD to the specified state
 SYNOPSIS
 --------
 [verse]
-'git reset' [-q] [<tree-ish>] [--] <paths>...
+'git reset' [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>...
 'git reset' (--patch | -p) [<tree-ish>] [--] [<paths>...]
 'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [<commit>]
 
@@ -97,6 +97,14 @@ OPTIONS
 --quiet::
 	Be quiet, only report errors.
 
+--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.
 
 EXAMPLES
 --------
diff --git a/builtin/reset.c b/builtin/reset.c
index c04ac07..018735f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -21,10 +21,12 @@
 #include "parse-options.h"
 #include "unpack-trees.h"
 #include "cache-tree.h"
+#include "strbuf.h"
+#include "quote.h"
 
 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>] [--] <paths>..."),
 	N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
 	NULL
 };
@@ -267,7 +269,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;
 	const char *rev;
 	struct object_id oid;
 	struct pathspec pathspec;
@@ -286,6 +290,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()
 	};
 
@@ -295,6 +303,44 @@ 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;
+		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"));
+
+		if (patch_mode)
+			flags |= PATHSPEC_PREFIX_ORIGIN;
+
+		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);
+			}
+			ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
+			stdin_paths[stdin_nr++] = xstrdup(buf.buf);
+			strbuf_reset(&buf);
+		}
+		strbuf_release(&unquoted);
+		strbuf_release(&buf);
+
+		ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
+		stdin_paths[stdin_nr++] = NULL;
+		parse_pathspec(&pathspec, 0, flags, prefix,
+			       (const char **)stdin_paths);
+	} else if (nul_term_line)
+		die(_("-z requires --stdin"));
+
 	unborn = !strcmp(rev, "HEAD") && get_sha1("HEAD", oid.hash);
 	if (unborn) {
 		/* reset on unborn branch: treat as reset to empty tree */
@@ -385,5 +431,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (!pathspec.nr)
 		remove_branch_state();
 
+	if (stdin_paths) {
+		while (stdin_nr)
+			free(stdin_paths[--stdin_nr]);
+		free(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 0000000..997dc42
--- /dev/null
+++ b/t/t7107-reset-stdin.sh
@@ -0,0 +1,33 @@
+#!/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_done
+
-- 
2.10.1.513.g00ef6dd

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

[Junio C Hamano]

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

> +	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;
> +		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"));
> +
> +		if (patch_mode)
> +			flags |= PATHSPEC_PREFIX_ORIGIN;

Didn't we already die above under that mode?

> +		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);
> +			}
> +			ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> +			stdin_paths[stdin_nr++] = xstrdup(buf.buf);
> +			strbuf_reset(&buf);
> +		}
> +		strbuf_release(&unquoted);
> +		strbuf_release(&buf);
> +
> +		ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> +		stdin_paths[stdin_nr++] = NULL;

It makes sense to collect, but...

> +		parse_pathspec(&pathspec, 0, flags, prefix,
> +			       (const char **)stdin_paths);

...letting them be used as if they are pathspec is wrong when
stdin_paths[] contain wildcard, isn't it?  

I think flags |= PATHSPEC_LITERAL_PATH can help fixing it.  0/2 said
this mimicks checkout-index and I think it should by not treating
the input as wildcarded patterns (i.e. "echo '*.c' | reset --stdin"
shouldn't be the way to reset all .c files --- that's something we
would want to add to the test, I guess).

Thanks.

Re: [PATCH 0/2] Support `git reset --stdin`

[Jeff King]

On Tue, Oct 11, 2016 at 06:08:56PM +0200, Johannes Schindelin wrote:

> This feature was missing, and made it cumbersome for third-party
> tools to reset a lot of paths in one go.
> 
> Support for --stdin has been added, following builtin/checkout-index.c's
> example.

Is git-reset the right layer to add scripting features? I thought we
usually pushed people doing mass index manipulation to use update-index
or read-tree. Is there something that reset makes easy that is hard with
those tools (I could imagine "--hard", but I see it is not supported
with your patch).

Not that I'm necessarily opposed to the patch, I was just surprised.

-Peff

Re: [PATCH 0/2] Support `git reset --stdin`

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Is git-reset the right layer to add scripting features? I thought we
> usually pushed people doing mass index manipulation to use update-index
> or read-tree. Is there something that reset makes easy that is hard with
> those tools (I could imagine "--hard", but I see it is not supported
> with your patch).
>
> Not that I'm necessarily opposed to the patch, I was just surprised.

If read-tree had pathspec support (i.e. "read these and only these
paths given from the command line into the index from a given
tree-ish"), that would have been the most natural place to extend
with "oh, by the way, instead of the command line, you can feed the
paths on the standard input".  

But it doesn't have one.

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

[Jakub Narębski]

W dniu 11.10.2016 o 18:09, Johannes Schindelin pisze:

>  SYNOPSIS
>  --------
>  [verse]
> -'git reset' [-q] [<tree-ish>] [--] <paths>...
> +'git reset' [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>...

I think you meant here

  +'git reset' [-q] [--stdin [-z]] [<tree-ish>]

Because you say "*Instead*" below.

> +--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.

And die if <paths> were supplied:

> +		if (pathspec.nr)
> +			die(_("--stdin is incompatible with path arguments"));

Of course you need to fix it in built-in synopsis as well:

> +	N_("git reset [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>..."),
>  	N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),

-- 
Jakub Narębski

Re: [PATCH 0/2] Support `git reset --stdin`

[Jeff King]

On Tue, Oct 11, 2016 at 12:27:21PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Is git-reset the right layer to add scripting features? I thought we
> > usually pushed people doing mass index manipulation to use update-index
> > or read-tree. Is there something that reset makes easy that is hard with
> > those tools (I could imagine "--hard", but I see it is not supported
> > with your patch).
> >
> > Not that I'm necessarily opposed to the patch, I was just surprised.
> 
> If read-tree had pathspec support (i.e. "read these and only these
> paths given from the command line into the index from a given
> tree-ish"), that would have been the most natural place to extend
> with "oh, by the way, instead of the command line, you can feed the
> paths on the standard input".
> 
> But it doesn't have one.

True. I'd have done something more like:

  git ls-tree -r $paths | git update-index --index-info

but there are some corner cases around deleting paths from the index.

-Peff

Re: [PATCH 0/2] Support `git reset --stdin`

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> If read-tree had pathspec support (i.e. "read these and only these
>> paths given from the command line into the index from a given
>> tree-ish"), that would have been the most natural place to extend
>> with "oh, by the way, instead of the command line, you can feed the
>> paths on the standard input".
>> 
>> But it doesn't have one.
>
> True. I'd have done something more like:
>
>   git ls-tree -r $paths | git update-index --index-info
>
> but there are some corner cases around deleting paths from the index.

Ah, I would think read-tree has the exact same issue, even if we
added pathspec support, around removal.

So it is more like

	(
		printf "0 0000000000000000000000000000000000000000\t%s\n" $paths
		git --literal-pathspecs ls-tree -r --ignore-missing $paths
	) | git update-index --index-info

which does not look too bad, even though this

	printf "%s\n" $paths | git reset --stdin

does look shorter.

Re: [PATCH 0/2] Support `git reset --stdin`

[Jeff King]

On Tue, Oct 11, 2016 at 02:36:31PM -0700, Junio C Hamano wrote:

> > True. I'd have done something more like:
> >
> >   git ls-tree -r $paths | git update-index --index-info
> >
> > but there are some corner cases around deleting paths from the index.
> 
> Ah, I would think read-tree has the exact same issue, even if we
> added pathspec support, around removal.
> 
> So it is more like
> 
> 	(
> 		printf "0 0000000000000000000000000000000000000000\t%s\n" $paths
> 		git --literal-pathspecs ls-tree -r --ignore-missing $paths
> 	) | git update-index --index-info
> 
> which does not look too bad, even though this
> 
> 	printf "%s\n" $paths | git reset --stdin
> 
> does look shorter.

Of course neither of ours solutions works when "$paths" is coming on
stdin, rather than in a variable, which I suspect was Dscho's original
motivation. :)

One reason not to do the unconditional $z40 in yours is that without it,
I would hope that update-index is smart enough not to discard the stat
information for entries which are unchanged.

I suspect the best answer is more like:

  git diff-index --cached HEAD | git update-index --index-info

except that you have to munge the data in between, because update-index
does not know how to pick the correct data out of the --raw diff output.
But that's probably closer to what git-reset does internally.

Anyway, the existence of this discussion is probably a good argument in
favor of Dscho's patch. I was mostly curious how close our plumbing
tools could come.

-Peff

Re: [PATCH 0/2] Support `git reset --stdin`

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Anyway, the existence of this discussion is probably a good argument in
> favor of Dscho's patch. I was mostly curious how close our plumbing
> tools could come.

Sure, in 100% agreement.

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]

Hi Kuba,

On Tue, 11 Oct 2016, Jakub Narębski wrote:

> W dniu 11.10.2016 o 18:09, Johannes Schindelin pisze:
> 
> >  SYNOPSIS
> >  --------
> >  [verse]
> > -'git reset' [-q] [<tree-ish>] [--] <paths>...
> > +'git reset' [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>...
> 
> I think you meant here
> 
>   +'git reset' [-q] [--stdin [-z]] [<tree-ish>]

Good point. I overlooked that the <paths>... are not optional here.

Thanks,
Dscho

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

[Johannes Schindelin]

Hi Junio,

On Tue, 11 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +	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;
> > +		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"));
> > +
> > +		if (patch_mode)
> > +			flags |= PATHSPEC_PREFIX_ORIGIN;
> 
> Didn't we already die above under that mode?

Oh right. Copy/paste fail.

> > +		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);
> > +			}
> > +			ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> > +			stdin_paths[stdin_nr++] = xstrdup(buf.buf);
> > +			strbuf_reset(&buf);
> > +		}
> > +		strbuf_release(&unquoted);
> > +		strbuf_release(&buf);
> > +
> > +		ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> > +		stdin_paths[stdin_nr++] = NULL;
> 
> It makes sense to collect, but...

It does, doesn't it? I really would have loved to start resetting right
away, but if the list were not sorted and traversed at the same time as
the tree-ish, the performance would just be suboptimal.

I think that is an important point and I adjusted the commit message
accordingly.

> > +		parse_pathspec(&pathspec, 0, flags, prefix,
> > +			       (const char **)stdin_paths);
> 
> ...letting them be used as if they are pathspec is wrong when
> stdin_paths[] contain wildcard, isn't it?  
> 
> I think flags |= PATHSPEC_LITERAL_PATH can help fixing it.  0/2 said
> this mimicks checkout-index and I think it should by not treating
> the input as wildcarded patterns (i.e. "echo '*.c' | reset --stdin"
> shouldn't be the way to reset all .c files --- that's something we
> would want to add to the test, I guess).

True. I adjust the flags accordingly now.

Thanks,
Dscho

[PATCH v2 0/1] Support `git reset --stdin`

[Johannes Schindelin]

This feature was missing, and made it cumbersome for third-party
tools to reset a lot of paths in one go.

Support for --stdin has been added, following builtin/checkout-index.c's
example.

Changes since v1:

- adjusted commit message to explain why we read everything before
  resetting

- fixed synopsis (`--stdin` does not work with `-- <path>...`)

- avoid handling patch_mode twice

- use PATHSPEC_LITERAL_PATH to avoid interpreting input as wildcards


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

 Documentation/git-reset.txt |  9 ++++++++
 builtin/reset.c             | 54 ++++++++++++++++++++++++++++++++++++++++++++-
 t/t7107-reset-stdin.sh      | 33 +++++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100755 t/t7107-reset-stdin.sh


base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/reset-stdin-v2
Fetch-It-Via: git fetch https://github.com/dscho/git reset-stdin-v2

Interdiff vs v1:

 diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
 index 533ef69f91..abb71bb805 100644
 --- a/Documentation/git-reset.txt
 +++ b/Documentation/git-reset.txt
 @@ -8,8 +8,9 @@ git-reset - Reset current HEAD to the specified state
  SYNOPSIS
  --------
  [verse]
 -'git reset' [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>...
 +'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
 diff --git a/builtin/reset.c b/builtin/reset.c
 index 6de3936aed..1d3075b7ee 100644
 --- a/builtin/reset.c
 +++ b/builtin/reset.c
 @@ -26,7 +26,8 @@
  
  static const char * const git_reset_usage[] = {
  	N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
 -	N_("git reset [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>..."),
 +	N_("git reset [-q] [<tree-ish>] [--] <paths>..."),
 +	N_("git reset [-q] [--stdin [-z]] [<tree-ish>]"),
  	N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
  	NULL
  };
 @@ -317,9 +318,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
  		if (pathspec.nr)
  			die(_("--stdin is incompatible with path arguments"));
  
 -		if (patch_mode)
 -			flags |= PATHSPEC_PREFIX_ORIGIN;
 -
  		while (getline_fn(&buf, stdin) != EOF) {
  			if (!nul_term_line && buf.buf[0] == '"') {
  				strbuf_reset(&unquoted);
 @@ -336,8 +334,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
  
  		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);
 +
  	} else if (nul_term_line)
  		die(_("-z requires --stdin"));
  

-- 
2.11.1.windows.prerelease.2.9.g3014b57

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

[Johannes Schindelin]

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.

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

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 25432d9257..abb71bb805 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
@@ -97,6 +98,14 @@ OPTIONS
 --quiet::
 	Be quiet, only report errors.
 
+--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.
 
 EXAMPLES
 --------
diff --git a/builtin/reset.c b/builtin/reset.c
index 8ab915bfcb..1d3075b7ee 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -21,10 +21,13 @@
 #include "parse-options.h"
 #include "unpack-trees.h"
 #include "cache-tree.h"
+#include "strbuf.h"
+#include "quote.h"
 
 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
 };
@@ -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;
 	const char *rev;
 	struct object_id oid;
 	struct pathspec pathspec;
@@ -286,6 +291,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()
 	};
 
@@ -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;
+		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);
+			}
+			ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
+			stdin_paths[stdin_nr++] = xstrdup(buf.buf);
+			strbuf_reset(&buf);
+		}
+		strbuf_release(&unquoted);
+		strbuf_release(&buf);
+
+		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);
+
+	} else if (nul_term_line)
+		die(_("-z requires --stdin"));
+
 	unborn = !strcmp(rev, "HEAD") && get_sha1("HEAD", oid.hash);
 	if (unborn) {
 		/* reset on unborn branch: treat as reset to empty tree */
@@ -385,5 +431,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (!pathspec.nr)
 		remove_branch_state();
 
+	if (stdin_paths) {
+		while (stdin_nr)
+			free(stdin_paths[--stdin_nr]);
+		free(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..997dc42dd2
--- /dev/null
+++ b/t/t7107-reset-stdin.sh
@@ -0,0 +1,33 @@
+#!/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_done
+
-- 
2.11.1.windows.prerelease.2.9.g3014b57

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

[Jeff King]

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:

> +--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.

> @@ -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.

> +	if (stdin_paths) {
> +		while (stdin_nr)
> +			free(stdin_paths[--stdin_nr]);
> +		free(stdin_paths);
> +	}

  argv_array_clear(&stdin_paths);

> @@ -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...

> +		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. :)

> 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

[PATCH v3 0/1] Support `git reset --stdin`

[Johannes Schindelin]

This feature was missing, and made it cumbersome for third-party
tools to reset a lot of paths in one go.

Support for --stdin has been added, following builtin/checkout-index.c's
example.

Changes since v2:

- the documentation clarifies that --stdin does not treat the input as
  pathspecs

- the code now uses struct argv_array instead of rolling its own


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

 Documentation/git-reset.txt | 10 ++++++++++
 builtin/reset.c             | 47 ++++++++++++++++++++++++++++++++++++++++++++-
 t/t7107-reset-stdin.sh      | 33 +++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+), 1 deletion(-)
 create mode 100755 t/t7107-reset-stdin.sh


base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/reset-stdin-v3
Fetch-It-Via: git fetch https://github.com/dscho/git reset-stdin-v3

Interdiff vs v2:

 diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
 index abb71bb805..d319ed9b20 100644
 --- a/Documentation/git-reset.txt
 +++ b/Documentation/git-reset.txt
 @@ -100,7 +100,8 @@ OPTIONS
  
  --stdin::
  	Instead of taking list of paths from the command line,
 -	read list of paths from the standard input.  Paths are
 +	read list of paths from the standard input.  The paths are
 +	read verbatim, i.e. not handled as pathspecs.  Paths are
  	separated by LF (i.e. one path per line) by default.
  
  -z::
 diff --git a/builtin/reset.c b/builtin/reset.c
 index 1d3075b7ee..fe7723c179 100644
 --- a/builtin/reset.c
 +++ b/builtin/reset.c
 @@ -23,6 +23,7 @@
  #include "cache-tree.h"
  #include "strbuf.h"
  #include "quote.h"
 +#include "argv-array.h"
  
  static const char * const git_reset_usage[] = {
  	N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
 @@ -271,8 +272,7 @@ 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, nul_term_line = 0, read_from_stdin = 0, unborn;
 -	char **stdin_paths = NULL;
 -	int stdin_nr = 0, stdin_alloc = 0;
 +	struct argv_array stdin_paths = ARGV_ARRAY_INIT;
  	const char *rev;
  	struct object_id oid;
  	struct pathspec pathspec;
 @@ -325,18 +325,15 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
  					die(_("line is badly quoted"));
  				strbuf_swap(&buf, &unquoted);
  			}
 -			ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
 -			stdin_paths[stdin_nr++] = xstrdup(buf.buf);
 +			argv_array_push(&stdin_paths, buf.buf);
  			strbuf_reset(&buf);
  		}
  		strbuf_release(&unquoted);
  		strbuf_release(&buf);
  
 -		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);
 +			       stdin_paths.argv);
  
  	} else if (nul_term_line)
  		die(_("-z requires --stdin"));
 @@ -431,11 +428,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
  	if (!pathspec.nr)
  		remove_branch_state();
  
 -	if (stdin_paths) {
 -		while (stdin_nr)
 -			free(stdin_paths[--stdin_nr]);
 -		free(stdin_paths);
 -	}
 +	argv_array_clear(&stdin_paths);
  
  	return update_ref_status;
  }

-- 
2.11.1.windows.prerelease.2.9.g3014b57

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

[Johannes Schindelin]

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.

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

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 25432d9257..d319ed9b20 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
@@ -97,6 +98,15 @@ OPTIONS
 --quiet::
 	Be quiet, only report errors.
 
+--stdin::
+	Instead of taking list of paths from the command line,
+	read list of paths from the standard input.  The paths are
+	read verbatim, i.e. not handled as pathspecs.  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.
 
 EXAMPLES
 --------
diff --git a/builtin/reset.c b/builtin/reset.c
index 8ab915bfcb..fe7723c179 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -21,10 +21,14 @@
 #include "parse-options.h"
 #include "unpack-trees.h"
 #include "cache-tree.h"
+#include "strbuf.h"
+#include "quote.h"
+#include "argv-array.h"
 
 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
 };
@@ -267,7 +271,8 @@ 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;
+	struct argv_array stdin_paths = ARGV_ARRAY_INIT;
 	const char *rev;
 	struct object_id oid;
 	struct pathspec pathspec;
@@ -286,6 +291,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()
 	};
 
@@ -295,6 +304,40 @@ 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;
+		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_sha1("HEAD", oid.hash);
 	if (unborn) {
 		/* reset on unborn branch: treat as reset to empty tree */
@@ -385,5 +428,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (!pathspec.nr)
 		remove_branch_state();
 
+	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..997dc42dd2
--- /dev/null
+++ b/t/t7107-reset-stdin.sh
@@ -0,0 +1,33 @@
+#!/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_done
+
-- 
2.11.1.windows.prerelease.2.9.g3014b57

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

[Johannes Schindelin]

Hi Peff,

On Fri, 27 Jan 2017, Jeff King wrote:

> On Fri, Jan 27, 2017 at 01:38:55PM +0100, Johannes Schindelin wrote:
> 
> A few minor suggestions:
> 
> > +--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.

I think this would be overkill. In reality, --stdin without -z does not
make much sense, anyway.

If you feel strongly about it, I encourage you to submit a follow-up
patch.

The rest of your suggestions have been implemented in v3.

Ciao,
Johannes

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

[Jeff King]

On Fri, Jan 27, 2017 at 06:34:46PM +0100, Johannes Schindelin wrote:

> > 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.
> 
> I think this would be overkill. In reality, --stdin without -z does not
> make much sense, anyway.

I think with most Unix tools people tend to use the non-z forms until
they break, and then switch to the z form. :)

And in that sense, this transparently Just Works because the output will
often come from another git tool, which will quote as appropriate.

> If you feel strongly about it, I encourage you to submit a follow-up
> patch.
> 
> The rest of your suggestions have been implemented in v3.

Thanks. I think the path/pathspec thing was the more important of the
two suggestions.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> A few minor suggestions:
>
>> +--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.

If the code forces literal pathspecs, then what the user feeds to
the command is no longer pathspecs from the user's point of view,
and I agree that the distinction is important.  

If the remainder of the documentation is loose in terminology and
the reason why we were able to get away with it was because we
consistently used "paths" when we meant "pathspec", these existing
mention of "paths" have to be tightened, either in this patch or a
preparatory step patch before this one, because the addition of
"this thing reads paths not pathspecs" is what makes them ambiguous.

Thanks.  I re-read the part of the code that reads and unquotes as
necessary in the patch and they looked correct.

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

[Jeff King]

On Fri, Jan 27, 2017 at 10:30:48AM -0800, Junio C Hamano wrote:

> > 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.
> 
> If the code forces literal pathspecs, then what the user feeds to
> the command is no longer pathspecs from the user's point of view,
> and I agree that the distinction is important.  
> 
> If the remainder of the documentation is loose in terminology and
> the reason why we were able to get away with it was because we
> consistently used "paths" when we meant "pathspec", these existing
> mention of "paths" have to be tightened, either in this patch or a
> preparatory step patch before this one, because the addition of
> "this thing reads paths not pathspecs" is what makes them ambiguous.

I think a lot of the documentation uses <paths> to refer to pathspecs
(e.g., git-log(1), git-diff(1), etc).  As long as we're consistent with
that convention, I don't think it's that big a deal.

This spot needs a specific mention because it violates the convention.

I don't know if the are other spots where it might be unclear, but I
think we're probably better to tighten those as they come up, rather
than switch to saying "<pathspecs>" everywhere.

That's outside the scope of this series, though, I would think.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think a lot of the documentation uses <paths> to refer to pathspecs
> (e.g., git-log(1), git-diff(1), etc).  As long as we're consistent with
> that convention, I don't think it's that big a deal.
>
> This spot needs a specific mention because it violates the convention.

Yup, I think we are in agreement.

> I don't know if the are other spots where it might be unclear, but I
> think we're probably better to tighten those as they come up, rather
> than switch to saying "<pathspecs>" everywhere.

Oh, I do not think I would disagree.  As I think this change is an
instancethat makes the resulting text unclear, it would set a good
example to tighten existing one as part of its clean-up.

It can be done as a follow-up bugfix patch (i.e. "previous one made
the resulting document uncleasr and here is to fix it"), but that
would not serve as good ra ole model to mentor newer contributor as
doing the other way around.