From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Weird behaviour when staging binary files interactively
To: Mark Jamsek <mark@jamsek.com>
Cc: gameoftrees@openbsd.org, Johannes Thyssen Tishman <johannes@thyssentishman.com>
Date: Mon, 4 Nov 2024 11:42:24 +0100

On Mon, Nov 04, 2024 at 05:02:08PM +1100, Mark Jamsek wrote:
> I went for a minimally invasive fix rather than change the UI. I think
> it makes sense to keep the same prompt and use the same "binary files
> ... differ" prompt for consistency. Regress is happy but I still haven't
> had time to expand it. The diff contains a basic `stage -p` test that I
> used while cooking the fix, but I want to expand on this a fair bit
> later tonight with plaintext and binary files in the changeset, y/n
> responses, and tests for unstage and revert.
> 
> This also fixes the diffstat insofar as it shows "1 file changed ..."
> instead of 0, but it still shows "0 insertions/deletions" which we do
> for binary files unless the "-a" flag to force ascii is used, in which
> case the insertions/deletions are computed. I don't know if we want to
> change this to always force ascii so they're unconditionally shown for
> binary files; I lean to no but I'm not married to that position.

Running the diff code on binaries to produce hunk info would just be a
pointless waste of CPU time.

One apparent problem in the diff:

> @@ -5273,11 +5318,23 @@ revert_file(void *arg, unsigned char status, unsigned 
>  
>  		if (a->patch_cb && (status == GOT_STATUS_MODIFY ||
>  		    status == GOT_STATUS_CONFLICT)) {
> -			int is_bad_symlink = 0;
> -			err = create_patched_content(&path_content, 1, &id,
> -			    ondisk_path, dirfd, de_name, ie->path, a->repo,
> +			int is_bad_symlink = 0, revert_binary_file = 0;
> +
> +			err = create_patched_content(&path_content,
> +			    &revert_binary_file, 1, &id, ondisk_path, dirfd,
> +			    de_name, ie->path, a->repo,
>  			    a->patch_cb, a->patch_arg);
> -			if (err || path_content == NULL)
> +			if (err != NULL)
> +				goto done;
> +			if (revert_binary_file){
> +				err = install_blob(a->worktree, ondisk_path,
> +				    ie->path,
> +				    te ? te->mode : GOT_DEFAULT_FILE_MODE,
> +				    got_fileindex_perms_to_st(ie), blob,
> +				    0, 1, 0, 0, NULL, a->repo,
> +				    a->progress_cb, a->progress_arg);

Should errors returned from install_blob() not be checked here?

> +			}
> +			if (path_content == NULL)
>  				break;
>  			if (te && S_ISLNK(te->mode)) {
>  				if (unlink(path_content) == -1) {