{"thread":{"id":"61353","subject":"[PATCH] stash: fix \"--staged\" with binary files","startedAt":"2024-04-22T10:28:18Z","lastAt":"2024-04-23T21:44:20Z","messageCount":4,"participants":["Adam Johnson via GitGitGadget","Junio C Hamano","Adam Johnson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"493303","messageId":"pull.1722.git.1713781694490.gitgitgadget@gmail.com","threadId":"61353","inReplyTo":null,"subject":"[PATCH] stash: fix \"--staged\" with binary files","fromName":"Adam Johnson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-04-22T10:28:14Z","receivedAt":"2024-04-22T10:28:18Z","isPatch":true,"sender":{"key":"me@adamj.eu","avatar":"https://avatars.githubusercontent.com/u/857609?v=4"},"body":"From: Adam Johnson \n\n\"git stash --staged\" would crash with binary files, after saving the stash.\nThis behaviour dates back to the addition of the feature in 41a28eb6c1\n(stash: implement '--staged' option for 'push' and 'save', 2021-10-18).\nAdding the \"--binary\" option of \"diff-tree\" fixes this. The \"diff-tree\" call\nin stash_patch() also omits \"--binary\", but that is fine since binary files\ncannot be selected interactively.\n\nHelped-By: Jeff King \nHelped-By: Randall S. Becker \nSigned-off-by: Adam Johnson \n---\n stash: fix \"--staged\" with binary files\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1722%2Fadamchainz%2Faj%2Fstash-binary-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1722/adamchainz/aj/stash-binary-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1722\n\n builtin/stash.c | 4 ++--\n t/t3903-stash.sh | 9 +++++++++\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 062be1fbc07..7751bca868e 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1205,8 +1205,8 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,\n \t}\n \n \tcp_diff_tree.git_cmd = 1;\n-\tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"-U1\", \"HEAD\",\n-\t\t oid_to_hex(&info->w_tree), \"--\", NULL);\n+\tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"--binary\",\n+\t\t \"-U1\", \"HEAD\", oid_to_hex(&info->w_tree), \"--\", NULL);\n \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n \t\tret = -1;\n \t\tgoto done;\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 00db82fb245..a7f71f8126f 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -393,6 +393,15 @@ test_expect_success 'stash --staged' '\n \ttest bar,bar4 = $(cat file),$(cat file2)\n '\n \n+test_expect_success 'stash --staged with binary file' '\n+\tprintf \"\\0\" >file &&\n+\tgit add file &&\n+\tgit stash --staged &&\n+\tgit stash pop &&\n+\tprintf \"\\0\" >expect &&\n+\ttest_cmp expect file\n+'\n+\n test_expect_success 'dont assume push with non-option args' '\n \ttest_must_fail git stash -q drop 2>err &&\n \ttest_grep -e \"subcommand wasn'\\''t specified; '\\''push'\\'' can'\\''t be assumed due to unexpected token '\\''drop'\\''\" err\n\nbase-commit: ae3196a5ea84a9e88991d576020cf66512487088\n-- \ngitgitgadget\n"},{"id":"493315","messageId":"xmqqsezdw7br.fsf@gitster.g","threadId":"61353","inReplyTo":"pull.1722.git.1713781694490.gitgitgadget@gmail.com","subject":"Re: [PATCH] stash: fix \"--staged\" with binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-22T19:00:56Z","receivedAt":"2024-04-22T19:01:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Adam Johnson via GitGitGadget\" writes:\n\n> From: Adam Johnson \n>\n> \"git stash --staged\" would crash with binary files, after saving the stash.\n\n\"would crash with\" -> \"errors out when given\", probably.\n\nWe would be better off reserving the word \"crash\" to refer to an\nuncontrolled exit with non-zero status of the command, possibly\nkilled via system signals like SEGV and SIGBUS. In this case, I\nsuspect that you would get an \"error:\" from underlying apply\nmachinery, which leads the caller to exit with a non-zero status\ndue to an error.\n\n> This behaviour dates back to the addition of the feature in 41a28eb6c1\n> (stash: implement '--staged' option for 'push' and 'save', 2021-10-18).\n\nWhen you find the commit that introduced the problem you are fixing,\nespecially when the author of the commit is still active on the\nmailing list, it would be nice to give a carbon-copy of the message\nto them.\n\n> Adding the \"--binary\" option of \"diff-tree\" fixes this. The \"diff-tree\" call\n> in stash_patch() also omits \"--binary\", but that is fine since binary files\n> cannot be selected interactively.\n\nI love seeing an explanation like this in a proposed commit log\nmessage. It is concise but very clear at the same time. It is so\nclearly written that anybody who is reasonably familiar with the\ncode does not even need to look at the patch text itself to see what\nthe actual fix would look like.\n\nVery nicely explained.\n\n> Helped-By: Jeff King \n> Helped-By: Randall S. Becker \n> Signed-off-by: Adam Johnson \n> ---\n\nWill queue.\n\nThanks.\n\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 062be1fbc07..7751bca868e 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -1205,8 +1205,8 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,\n> \t}\n> \n> \tcp_diff_tree.git_cmd = 1;\n> -\tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"-U1\", \"HEAD\",\n> -\t\t oid_to_hex(&info->w_tree), \"--\", NULL);\n> +\tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"--binary\",\n> +\t\t \"-U1\", \"HEAD\", oid_to_hex(&info->w_tree), \"--\", NULL);\n> \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n> \t\tret = -1;\n> \t\tgoto done;\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 00db82fb245..a7f71f8126f 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -393,6 +393,15 @@ test_expect_success 'stash --staged' '\n> \ttest bar,bar4 = $(cat file),$(cat file2)\n> '\n> \n> +test_expect_success 'stash --staged with binary file' '\n> +\tprintf \"\\0\" >file &&\n> +\tgit add file &&\n> +\tgit stash --staged &&\n> +\tgit stash pop &&\n> +\tprintf \"\\0\" >expect &&\n> +\ttest_cmp expect file\n> +'\n\n"},{"id":"493319","messageId":"f5da1265-004d-487f-ac65-deee50aaca0a@app.fastmail.com","threadId":"61353","inReplyTo":"xmqqsezdw7br.fsf@gitster.g","subject":"Re: [PATCH] stash: fix \"--staged\" with binary files","fromName":"Adam Johnson","fromEmail":"me@adamj.eu","sentAt":"2024-04-22T20:48:55Z","receivedAt":"2024-04-22T20:49:17Z","isPatch":true,"sender":{"key":"me@adamj.eu","avatar":"https://avatars.githubusercontent.com/u/857609?v=4"},"body":"Thank you for the feedback, that was clearly explained. I will try to remember to CC more relevant folks in the future.\n\nOn Mon, 22 Apr 2024, at 20:00, Junio C Hamano wrote:\n> \"Adam Johnson via GitGitGadget\" writes:\n> \n> > From: Adam Johnson \n> >\n> > \"git stash --staged\" would crash with binary files, after saving the stash.\n> \n> \"would crash with\" -> \"errors out when given\", probably.\n> \n> We would be better off reserving the word \"crash\" to refer to an\n> uncontrolled exit with non-zero status of the command, possibly\n> killed via system signals like SEGV and SIGBUS. In this case, I\n> suspect that you would get an \"error:\" from underlying apply\n> machinery, which leads the caller to exit with a non-zero status\n> due to an error.\n> \n> > This behaviour dates back to the addition of the feature in 41a28eb6c1\n> > (stash: implement '--staged' option for 'push' and 'save', 2021-10-18).\n> \n> When you find the commit that introduced the problem you are fixing,\n> especially when the author of the commit is still active on the\n> mailing list, it would be nice to give a carbon-copy of the message\n> to them.\n> \n> > Adding the \"--binary\" option of \"diff-tree\" fixes this. The \"diff-tree\" call\n> > in stash_patch() also omits \"--binary\", but that is fine since binary files\n> > cannot be selected interactively.\n> \n> I love seeing an explanation like this in a proposed commit log\n> message. It is concise but very clear at the same time. It is so\n> clearly written that anybody who is reasonably familiar with the\n> code does not even need to look at the patch text itself to see what\n> the actual fix would look like.\n> \n> Very nicely explained.\n> \n> > Helped-By: Jeff King \n> > Helped-By: Randall S. Becker \n> > Signed-off-by: Adam Johnson \n> > ---\n> \n> Will queue.\n> \n> Thanks.\n> \n> > diff --git a/builtin/stash.c b/builtin/stash.c\n> > index 062be1fbc07..7751bca868e 100644\n> > --- a/builtin/stash.c\n> > +++ b/builtin/stash.c\n> > @@ -1205,8 +1205,8 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,\n> > }\n> > \n> > cp_diff_tree.git_cmd = 1;\n> > - strvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"-U1\", \"HEAD\",\n> > - oid_to_hex(&info->w_tree), \"--\", NULL);\n> > + strvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"--binary\",\n> > + \"-U1\", \"HEAD\", oid_to_hex(&info->w_tree), \"--\", NULL);\n> > if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n> > ret = -1;\n> > goto done;\n> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> > index 00db82fb245..a7f71f8126f 100755\n> > --- a/t/t3903-stash.sh\n> > +++ b/t/t3903-stash.sh\n> > @@ -393,6 +393,15 @@ test_expect_success 'stash --staged' '\n> > test bar,bar4 = $(cat file),$(cat file2)\n> > '\n> > \n> > +test_expect_success 'stash --staged with binary file' '\n> > + printf \"\\0\" >file &&\n> > + git add file &&\n> > + git stash --staged &&\n> > + git stash pop &&\n> > + printf \"\\0\" >expect &&\n> > + test_cmp expect file\n> > +'\n> \n> \n"},{"id":"493375","messageId":"20240423214419.GB1172807@coredump.intra.peff.net","threadId":"61353","inReplyTo":"pull.1722.git.1713781694490.gitgitgadget@gmail.com","subject":"Re: [PATCH] stash: fix \"--staged\" with binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-23T21:44:19Z","receivedAt":"2024-04-23T21:44:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 22, 2024 at 10:28:14AM +0000, Adam Johnson via GitGitGadget wrote:\n\n> From: Adam Johnson \n> \n> \"git stash --staged\" would crash with binary files, after saving the stash.\n> This behaviour dates back to the addition of the feature in 41a28eb6c1\n> (stash: implement '--staged' option for 'push' and 'save', 2021-10-18).\n> Adding the \"--binary\" option of \"diff-tree\" fixes this. The \"diff-tree\" call\n> in stash_patch() also omits \"--binary\", but that is fine since binary files\n> cannot be selected interactively.\n> \n> Helped-By: Jeff King \n> Helped-By: Randall S. Becker \n> Signed-off-by: Adam Johnson \n\nI had to dig in the archive to remember what I might have helped with. ;)\n\nThe patch looks good to me, though one thing I noticed:\n\n> +test_expect_success 'stash --staged with binary file' '\n> +\tprintf \"\\0\" >file &&\n> +\tgit add file &&\n> +\tgit stash --staged &&\n> +\tgit stash pop &&\n> +\tprintf \"\\0\" >expect &&\n> +\ttest_cmp expect file\n> +'\n\nI wonder if test_cmp would ever have problems with binary files on any\nplatforms (and yes, I was the one who suggested using it). We use \"diff\"\non most platforms, but a few (like old SunOS) set GIT_TEST_CMP to \"cmp\"\nbecause they don't have \"diff -u\" at all. I'm content to leave it as-is\nuntil somebody turns up a platform that complains, and then the escape\nhatch is \"OK, so set GIT_TEST_CMP=cmp\".\n\n-Peff\n"}]}