Re: [PATCH] stash: fix "--staged" with binary files

[Jeff King <peff@peff.net>]

On Mon, Apr 22, 2024 at 10:28:14AM +0000, Adam Johnson via GitGitGadget wrote:

> From: Adam Johnson <me@adamj.eu>
> 
> "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.
> 
> Helped-By: Jeff King <peff@peff.net>
> Helped-By: Randall S. Becker <randall.becker@nexbridge.ca>
> Signed-off-by: Adam Johnson <me@adamj.eu>

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:

> +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