From: "Adam Johnson" <me@adamj.eu>
To: "Junio C Hamano" <gitster@pobox.com>,
"Adam Johnson via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
"Randall S. Becker" <randall.becker@nexbridge.ca>
Subject: Re: [PATCH] stash: fix "--staged" with binary files
Date: Mon, 22 Apr 2024 21:48:55 +0100 [thread overview]
Message-ID: <f5da1265-004d-487f-ac65-deee50aaca0a@app.fastmail.com> (raw)
In-Reply-To: <xmqqsezdw7br.fsf@gitster.g>
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@gmail.com> writes:
>
> > From: Adam Johnson <me@adamj.eu>
> >
> > "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@peff.net>
> > Helped-By: Randall S. Becker <randall.becker@nexbridge.ca>
> > Signed-off-by: Adam Johnson <me@adamj.eu>
> > ---
>
> 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
> > +'
>
>
next prev parent reply other threads:[~2024-04-22 20:49 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-22 10:28 [PATCH] stash: fix "--staged" with binary files Adam Johnson via GitGitGadget
2024-04-22 19:00 ` Junio C Hamano
2024-04-22 20:48 ` Adam Johnson [this message]
2024-04-23 21:44 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f5da1265-004d-487f-ac65-deee50aaca0a@app.fastmail.com \
--to=me@adamj.eu \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=randall.becker@nexbridge.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).