Teach git reset to optionally read the pathspecs from stdin

[dscho]

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.

[dscho]

/submit

[gitgitgadget]

Submitted as pull.133.git.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

[SyntevoAlex]

FYI @dscho #445

[dscho]

@SyntevoAlex no need to ping me, this caused a failure in the Azure Pipeline that tries to rebase all of Git for Windows' patches on top of pu continuously.

Since the --stdin flag is in use by some MinGit users, I had to forward-port it. It is not entirely clear to me how deviating from the existing practices makes sense (there are tons of --stdin/-z options in Git, and no --pathspec-from-file), but then, I have no interest in discussing this at this point, either.

[dscho]

This was actually superseded a long time ago by c58ae96