Re: [PATCH 2/2] rebase -i: silence stash apply

[Phillip Wood]

Hi Johannes

Thanks for your reply
On 18/05/17 11:49, Johannes Schindelin wrote:
Hi Phillip,
On Thu, 18 May 2017, Phillip Wood wrote:

diff --git a/sequencer.c b/sequencer.c
index 
f8bc18badf1a3fb1b39656501c5a316e229968d2..311728a145dfc66e230334221a2610468239932d
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1914,6 +1914,8 @@ static int apply_autostash(struct replay_opts *opts)
        strbuf_trim(&stash_sha1);
  
  	child.git_cmd = 1;
+       child.no_stdout = 1;
+       child.no_stderr = 1;
        argv_array_push(&child.args, "stash");
        argv_array_push(&child.args, "apply");
        argv_array_push(&child.args, stash_sha1.buf);

If I remember correctly, then the shell version prints the output in case
of an error.

The shell version prints it's own error message if there's an error, the 
C version does this as well

Shell version:
apply_autostash () {
        if test -f "$state_dir/autostash"
        then
                stash_sha1=$(cat "$state_dir/autostash")
                if git stash apply $stash_sha1 2>&1 >/dev/null
                then
                        echo "$(gettext 'Applied autostash.')"
                else
                        git stash store -m "autostash" -q $stash_sha1 ||
                        die "$(eval_gettext "Cannot store \$stash_sha1")"
                        gettext 'Applying autostash resulted in conflicts.
Your changes are safe in the stash.
You can run "git stash pop" or "git stash drop" at any time.
'
                fi
        fi
}

C version:
static int apply_autostash(struct replay_opts *opts)
{
        struct strbuf stash_sha1 = STRBUF_INIT;
        struct child_process child = CHILD_PROCESS_INIT;
        int ret = 0;

        if (!read_oneliner(&stash_sha1, rebase_path_autostash(), 1)) {
                strbuf_release(&stash_sha1);
                return 0;
        }
        strbuf_trim(&stash_sha1);

        child.git_cmd = 1;
        child.no_stdout = 1;
        child.no_stderr = 1;
        argv_array_push(&child.args, "stash");
        argv_array_push(&child.args, "apply");
        argv_array_push(&child.args, stash_sha1.buf);
        if (!run_command(&child))
                printf(_("Applied autostash.\n"));
        else {
                struct child_process store = CHILD_PROCESS_INIT;

                store.git_cmd = 1;
                argv_array_push(&store.args, "stash");
                argv_array_push(&store.args, "store");
                argv_array_push(&store.args, "-m");
                argv_array_push(&store.args, "autostash");
                argv_array_push(&store.args, "-q");
                argv_array_push(&store.args, stash_sha1.buf);
                if (run_command(&store))
                        ret = error(_("cannot store %s"), stash_sha1.buf);
                else
                        printf(_("Applying autostash resulted in conflicts.\n"
                                "Your changes are safe in the stash.\n"
                                "You can run \"git stash pop\" or"
                                " \"git stash drop\" at any time.\n"));
        }

        strbuf_release(&stash_sha1);
        return ret;
}


We already imitated that behavior in `git commit`
(https://github.com/git-for-windows/git/blob/v2.13.0.windows.1/sequencer.c#L674-L684):

                /* hide stderr on success */
                struct strbuf buf = STRBUF_INIT;
                int rc = pipe_command(&cmd,
                                      NULL, 0,
                                      /* stdout is already redirected */
                                      NULL, 0,
                                      &buf, 0);
                if (rc)
                        fputs(buf.buf, stderr);
                strbuf_release(&buf);

I think that would be the appropriate approach here, too.

I can change it to do this if you think it is an improvement but the 
existing error message seems fairly clear to me and I think (though I 
haven't checked) that the call to stash store should print out the id of 
the stash for the user

P.S.: it may be a very good idea to accompany this patch (as well as the
previous one) by a patch to the test suite to verify that the fixed code
does not regress.

I agree that a test for this (and probably my other changes) would be 
useful, I'll try and put something together but it won't be until after 
the end of next week.

Thanks

Phillip