stash: fix "--staged" with binary files
PATCH 1/1 stash: fix "--staged" with binary files
From: Adam Johnson <[email protected]>
"git stash --staged" would crash with binary files, after saving the stash. This behaviour dates back to the addition of the feature in 41a28eb6c1 (stash: implement '--staged' option for 'push' and 'save', 2021-10-18). 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.
---
stash: fix "--staged" with binary files
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1722%2Fadamchainz%2Faj%2Fstash-binary-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1722/adamchainz/aj/stash-binary-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1722
builtin/stash.c | 4 ++-- t/t3903-stash.sh | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-)
builtin/stash.c +2 -2
@@ -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;
t/t3903-stash.sh +9 -0
@@ -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 +' + test_expect_success 'dont assume push with non-option args' ' test_must_fail git stash -q drop 2>err && test_grep -e "subcommand wasn'\''t specified; '\''push'\'' can'\''t be assumed due to unexpected token '\''drop'\''" err
base-commit: ae3196a5ea84a9e88991d576020cf66512487088
Signature · 1 line
gitgitgadget
Quoted 3 lines · Adam Johnson via GitGitGadget
From: Adam Johnson <[email protected]>
"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.
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 4 lines
Helped-By Jeff King <[email protected]>Helped-By Randall S. Becker <[email protected]>Signed-off-by Adam Johnson <[email protected]>---
Will queue.
Thanks.
Quoted 31 lines
builtin/stash.c +2
-2@@ -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;t/t3903-stash.sh +8
-0@@ -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 +'
Thank you for the feedback, that was clearly explained. I will try to remember to CC more relevant folks in the future.
Quoted 77 lines · Junio C Hamano
Quoted 3 lines · Adam Johnson via GitGitGadget
From: Adam Johnson <[email protected]>
"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.
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 4 lines
Helped-By Jeff King <[email protected]>Helped-By Randall S. Becker <[email protected]>Signed-off-by Adam Johnson <[email protected]>---
Will queue.
Thanks.
Quoted 31 lines
builtin/stash.c +2
-2@@ -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;t/t3903-stash.sh +8
-0@@ -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 +'
Quoted 9 lines · Adam Johnson via GitGitGadget
From: Adam Johnson <[email protected]>
"git stash --staged" would crash with binary files, after saving the stash. This behaviour dates back to the addition of the feature in
41a28eb6c1(stash: implement '--staged' option for 'push' and 'save', 2021-10-18). Adding the "--binary" option of "diff-tree" fixes this. The "diff-tree" call instash_patch()also omits "--binary", but that is fine since binary files cannot be selected interactively.
Quoted 3 lines
Helped-By Jeff King <[email protected]>Helped-By Randall S. Becker <[email protected]>Signed-off-by Adam Johnson <[email protected]>
I had to dig in the archive to remember what I might have helped with. ;)
The patch looks good to me, though one thing I noticed:
t/t3903-stash.sh test_expect_success 'stash --staged' ' · line 403
Quoted 8 lines
t/t3903-stash.sh +8
-0+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 +'
I wonder if test_cmp would ever have problems with binary files on any platforms (and yes, I was the one who suggested using it). We use "diff" on most platforms, but a few (like old SunOS) set GIT_TEST_CMP to "cmp" because they don't have "diff -u" at all. I'm content to leave it as-is until somebody turns up a platform that complains, and then the escape hatch is "OK, so set GIT_TEST_CMP=cmp".
-Peff