Skip to contentSkip to conversation

Teach git reset to optionally read the pathspecs from stdin

PATCH In review Johannes Schindelin via GitGitGadget 4 participants 6 messages Sep 4, 2019 – Sep 5, 2019 Documentation

lore
Share & tools
lore
#0 lore raw

cover Teach git reset to optionally read the pathspecs from stdin

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

Signature · 1 line
gitgitgadget
#1 lore raw

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

From: Johannes Schindelin <[email protected]>

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.

---
 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
Documentation/git-reset.txt +11 -0
index 132f8e55f6..e03014eb04 100644
@@ -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
 --------
builtin/reset.c +44 -1

80 lines not on this page · raw message

t/t7107-reset-stdin.sh new file +41 -0

41 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#2 lore raw

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

Quoted 9 lines · Johannes Schindelin via GitGitGadget

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

---

builtin/reset.c int cmd_reset(int argc, const char **argv, const char *prefix) · line 331

Quoted 7 lines
builtin/reset.c +7 -0
@@ -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);
#3 lore raw

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

Quoted 22 lines · Eric Sunshine
Quoted 9 lines · Johannes Schindelin via GitGitGadget

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

---

builtin/reset.c int cmd_reset(int argc, const char **argv, const char *prefix) · line 331

Quoted 7 lines
builtin/reset.c +7 -0
@@ -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:

Quoted 1 line
     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.

#4 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

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/[email protected]/

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.

#5 lore raw

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

Quoted 12 lines · 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/[email protected]/

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

Quoted 8 lines

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.