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

From: Jeff King <peff@peff.net>
Date: Apr 23, 2024, 21:44 UTC
Message-ID: <20240423214419.GB1172807@coredump.intra.peff.net>
In-Reply-To: <pull.1722.git.1713781694490.gitgitgadget@gmail.com>

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

Show 12 quoted lines

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

Show 8 quoted lines

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