Skip to content

Teach git reset to optionally read the pathspecs from stdin #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Teach git reset to optionally read the pathspecs from stdin #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Documentation/git-reset.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ SYNOPSIS
[verse]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

On Wed, Sep 4, 2019 at 5:38 PM Johannes Schindelin via GitGitGadget
<[email protected]> 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 <[email protected]>
> ---
> 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);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Eric Sunshine <[email protected]> writes:

> On Wed, Sep 4, 2019 at 5:38 PM Johannes Schindelin via GitGitGadget
> <[email protected]> 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 <[email protected]>
>> ---
>> 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/[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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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.

'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
Expand Down Expand Up @@ -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
--------
Expand Down
45 changes: 44 additions & 1 deletion builtin/reset.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down Expand Up @@ -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;
Expand All @@ -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()
};

Expand All @@ -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 */
Expand Down Expand Up @@ -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;
}
41 changes: 41 additions & 0 deletions t/t7107-reset-stdin.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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