Thank you for the feedback, that was clearly explained. I will try to remember to CC more relevant folks in the future.
On Mon, 22 Apr 2024, at 20:00, Junio C Hamano wrote:
> "Adam Johnson via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Adam Johnson <me@xxxxxxxx>
> >
> > "git stash --staged" would crash with binary files, after saving the stash.
>
> "would crash with" -> "errors out when given", probably.
>
> We would be better off reserving the word "crash" to refer to an
> uncontrolled exit with non-zero status of the command, possibly
> killed via system signals like SEGV and SIGBUS. In this case, I
> suspect that you would get an "error:" from underlying apply
> machinery, which leads the caller to exit with a non-zero status
> due to an error.
>
> > This behaviour dates back to the addition of the feature in 41a28eb6c1
> > (stash: implement '--staged' option for 'push' and 'save', 2021-10-18).
>
> When you find the commit that introduced the problem you are fixing,
> especially when the author of the commit is still active on the
> mailing list, it would be nice to give a carbon-copy of the message
> to them.
>
> > Adding the "--binary" option of "diff-tree" fixes this. The "diff-tree" call
> > in stash_patch() also omits "--binary", but that is fine since binary files
> > cannot be selected interactively.
>
> I love seeing an explanation like this in a proposed commit log
> message. It is concise but very clear at the same time. It is so
> clearly written that anybody who is reasonably familiar with the
> code does not even need to look at the patch text itself to see what
> the actual fix would look like.
>
> Very nicely explained.
>
> > Helped-By: Jeff King <peff@xxxxxxxx>
> > Helped-By: Randall S. Becker <randall.becker@xxxxxxxxxxxx>
> > Signed-off-by: Adam Johnson <me@xxxxxxxx>
> > ---
>
> Will queue.
>
> Thanks.
>
> > diff --git a/builtin/stash.c b/builtin/stash.c
> > index 062be1fbc07..7751bca868e 100644
> > --- a/builtin/stash.c
> > +++ b/builtin/stash.c
> > @@ -1205,8 +1205,8 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
> > }
> >
> > cp_diff_tree.git_cmd = 1;
> > - strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD",
> > - oid_to_hex(&info->w_tree), "--", NULL);
> > + strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "--binary",
> > + "-U1", "HEAD", oid_to_hex(&info->w_tree), "--", NULL);
> > if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
> > ret = -1;
> > goto done;
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index 00db82fb245..a7f71f8126f 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -393,6 +393,15 @@ test_expect_success 'stash --staged' '
> > test bar,bar4 = $(cat file),$(cat file2)
> > '
> >
> > +test_expect_success 'stash --staged with binary file' '
> > + printf "\0" >file &&
> > + git add file &&
> > + git stash --staged &&
> > + git stash pop &&
> > + printf "\0" >expect &&
> > + test_cmp expect file
> > +'
>
>
