| Message ID | 3334d4cb6f302a35986d94ea8ffcd1ee9c6aae5d.1647274230.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash' | expand |
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Victoria Dye <vdye@github.com> > > Add the options '-q' and '--refresh' to the 'git reset' executed in > 'reset_head()', and '--refresh' to the 'git reset -q' executed in > 'do_push_stash(...)'. > > 'stash' is implemented such that git commands invoked as part of it (e.g., > 'clean', 'read-tree', 'reset', etc.) have their informational output > silenced. However, the 'reset' in 'reset_head()' is *not* called with '-q', > leading to the potential for a misleading printout from 'git stash apply > --index' if the stash included a removed file: > > Unstaged changes after reset: D <deleted file> > > Not only is this confusing in its own right (since, after the reset, 'git > stash' execution would stage the deletion in the index), it would be printed > even when the stash was applied with the '-q' option. As a result, the > messaging is removed entirely by calling 'git status' with '-q'. > > Additionally, because the default behavior of 'git reset -q' is to skip > refreshing the index, but later operations in 'git stash' subcommands expect > a non-stale index, enable '--refresh' as well. > > Signed-off-by: Victoria Dye <vdye@github.com> > --- > builtin/stash.c | 5 +++-- > t/t3903-stash.sh | 12 ++++++++++++ > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/builtin/stash.c b/builtin/stash.c > index 3e8af210fde..91407d9bbe0 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -310,7 +310,7 @@ static int reset_head(void) > * API for resetting. > */ > cp.git_cmd = 1; > - strvec_push(&cp.args, "reset"); > + strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL); > > return run_command(&cp); > } > @@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q > struct child_process cp = CHILD_PROCESS_INIT; > > cp.git_cmd = 1; > - strvec_pushl(&cp.args, "reset", "-q", "--", NULL); > + strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--", > + NULL); > add_pathspecs(&cp.args, ps); > if (run_command(&cp)) { > ret = -1; > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index f36e121210e..17f2ad2344c 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' ' > test_must_be_empty output.out > ' > > +test_expect_success 'apply --index -q is quiet' ' > + # Added file, deleted file, modified file all staged for commit > + echo foo >new-file && > + echo test >file && > + git add new-file file && > + git rm other-file && > + > + git stash && Hpmh. The hunk that updates reset_head() does get exercised by "apply --index", so testing that is OK, but we are also patching "do_push_stash()" to be quiet, so don't we care the chattyness of this step, too? In these steps, we also want the same "did the command refresh the index?" tests, no? > + git stash apply --index -q >output.out 2>&1 && > + test_must_be_empty output.out > +' > + > test_expect_success 'save -q is quiet' ' > git stash save --quiet >output.out 2>&1 && > test_must_be_empty output.out Other than these nits I noticed, the overall idea of the topic is well presented. Thanks for working on this.
Junio C Hamano wrote: > "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Victoria Dye <vdye@github.com> >> >> Add the options '-q' and '--refresh' to the 'git reset' executed in >> 'reset_head()', and '--refresh' to the 'git reset -q' executed in >> 'do_push_stash(...)'. >> >> 'stash' is implemented such that git commands invoked as part of it (e.g., >> 'clean', 'read-tree', 'reset', etc.) have their informational output >> silenced. However, the 'reset' in 'reset_head()' is *not* called with '-q', >> leading to the potential for a misleading printout from 'git stash apply >> --index' if the stash included a removed file: >> >> Unstaged changes after reset: D <deleted file> >> >> Not only is this confusing in its own right (since, after the reset, 'git >> stash' execution would stage the deletion in the index), it would be printed >> even when the stash was applied with the '-q' option. As a result, the >> messaging is removed entirely by calling 'git status' with '-q'. >> >> Additionally, because the default behavior of 'git reset -q' is to skip >> refreshing the index, but later operations in 'git stash' subcommands expect >> a non-stale index, enable '--refresh' as well. >> >> Signed-off-by: Victoria Dye <vdye@github.com> >> --- >> builtin/stash.c | 5 +++-- >> t/t3903-stash.sh | 12 ++++++++++++ >> 2 files changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/builtin/stash.c b/builtin/stash.c >> index 3e8af210fde..91407d9bbe0 100644 >> --- a/builtin/stash.c >> +++ b/builtin/stash.c >> @@ -310,7 +310,7 @@ static int reset_head(void) >> * API for resetting. >> */ >> cp.git_cmd = 1; >> - strvec_push(&cp.args, "reset"); >> + strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL); >> >> return run_command(&cp); >> } >> @@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q >> struct child_process cp = CHILD_PROCESS_INIT; >> >> cp.git_cmd = 1; >> - strvec_pushl(&cp.args, "reset", "-q", "--", NULL); >> + strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--", >> + NULL); >> add_pathspecs(&cp.args, ps); >> if (run_command(&cp)) { >> ret = -1; >> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh >> index f36e121210e..17f2ad2344c 100755 >> --- a/t/t3903-stash.sh >> +++ b/t/t3903-stash.sh >> @@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' ' >> test_must_be_empty output.out >> ' >> >> +test_expect_success 'apply --index -q is quiet' ' >> + # Added file, deleted file, modified file all staged for commit >> + echo foo >new-file && >> + echo test >file && >> + git add new-file file && >> + git rm other-file && >> + >> + git stash && > > Hpmh. The hunk that updates reset_head() does get exercised by > "apply --index", so testing that is OK, but we are also patching > "do_push_stash()" to be quiet, so don't we care the chattyness of > this step, too? > The '-q' option was already present in the reset in 'do_push_stash()', but I did add the '--refresh' option to ensure that (for example) 'git stash push --staged' refreshes the index. With that in mind... > In these steps, we also want the same "did the command refresh the > index?" tests, no? > yes, both cases where '--refresh' was added should be tested (since both will fail if they don't include that option). I'll add them in the next iteration. >> + git stash apply --index -q >output.out 2>&1 && >> + test_must_be_empty output.out >> +' >> + >> test_expect_success 'save -q is quiet' ' >> git stash save --quiet >output.out 2>&1 && >> test_must_be_empty output.out > > Other than these nits I noticed, the overall idea of the topic is > well presented. Thanks for working on this. >
diff --git a/builtin/stash.c b/builtin/stash.c index 3e8af210fde..91407d9bbe0 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -310,7 +310,7 @@ static int reset_head(void) * API for resetting. */ cp.git_cmd = 1; - strvec_push(&cp.args, "reset"); + strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL); return run_command(&cp); } @@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q struct child_process cp = CHILD_PROCESS_INIT; cp.git_cmd = 1; - strvec_pushl(&cp.args, "reset", "-q", "--", NULL); + strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--", + NULL); add_pathspecs(&cp.args, ps); if (run_command(&cp)) { ret = -1; diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index f36e121210e..17f2ad2344c 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' ' test_must_be_empty output.out ' +test_expect_success 'apply --index -q is quiet' ' + # Added file, deleted file, modified file all staged for commit + echo foo >new-file && + echo test >file && + git add new-file file && + git rm other-file && + + git stash && + git stash apply --index -q >output.out 2>&1 && + test_must_be_empty output.out +' + test_expect_success 'save -q is quiet' ' git stash save --quiet >output.out 2>&1 && test_must_be_empty output.out