Download raw body.
Weird behaviour when staging binary files interactively
Stefan Sperling <stsp@stsp.name> wrote:
> 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.
I agree!
> 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?
We break on the next line and go straight to done: already because
path_content is NULL when revert_binary_file is set, which is why I
left it out, but it's much clearer to have an explicit check there.
The below diff adds that, which is the only change to lib/worktree.c
plus expands the stage -p binary test.
I still need to add unstage and revert coverage, though, but I'm not
sure I'll do that tonight as my eyes are falling out of their sockets
already :)
> > + }
> > + if (path_content == NULL)
> > break;
> > if (te && S_ISLNK(te->mode)) {
> > if (unlink(path_content) == -1) {
M lib/worktree.c | 101+ 26-
M regress/cmdline/stage.sh | 258+ 0-
2 files changed, 359 insertions(+), 26 deletions(-)
commit - e37a5589c16266235a9b0d3b6d7be7ec67b46390
commit + 192e8698a24abf91db4978a2895b9db33d2416ac
blob - bb9125ced48b15709c711e4a7f24e2e35e0a2c0d
blob + 28806a15451e401fae7d46afe3aa5c1ca838385b
--- lib/worktree.c
+++ lib/worktree.c
@@ -4811,6 +4811,43 @@ copy_remaining_content(FILE *f1, FILE *f2, int *line_c
}
static const struct got_error *
+accept_or_reject_binary_change(int *choice, const char *path,
+ got_worktree_patch_cb patch_cb, void *patch_arg)
+{
+ const struct got_error *err;
+ FILE *f;
+
+ *choice = GOT_PATCH_CHOICE_NONE;
+
+ f = got_opentemp();
+ if (f == NULL)
+ return got_error_from_errno("got_opentemp");
+
+ if (fprintf(f, "Binary files %s and %s differ\n", path, path) < 0) {
+ err = got_error_msg(GOT_ERR_IO, "fprintf");
+ goto done;
+ }
+ if (fseeko(f, 0L, SEEK_SET) == -1) {
+ err = got_error_from_errno("fseeko");
+ goto done;
+ }
+
+ err = (*patch_cb)(choice, patch_arg, GOT_STATUS_MODIFY, path, f, 1, 1);
+
+done:
+ if (f != NULL && fclose(f) == EOF && err == NULL)
+ err = got_error_from_errno("fclose");
+ return err;
+}
+
+static int
+diff_result_has_binary(struct diff_result *r)
+{
+ return (r->left->atomizer_flags | r->right->atomizer_flags) &
+ DIFF_ATOMIZER_FOUND_BINARY_DATA;
+}
+
+static const struct got_error *
apply_or_reject_change(int *choice, int *nchunks_used,
struct diff_result *diff_result, int n,
const char *relpath, FILE *f1, FILE *f2, int *line_cur1, int *line_cur2,
@@ -4905,8 +4942,8 @@ struct revert_file_args {
};
static const struct got_error *
-create_patched_content(char **path_outfile, int reverse_patch,
- struct got_object_id *blob_id, const char *path2,
+create_patched_content(char **path_outfile, int *confirm_binary_change,
+ int reverse_patch, struct got_object_id *blob_id, const char *path2,
int dirfd2, const char *de_name2,
const char *relpath, struct got_repository *repo,
got_worktree_patch_cb patch_cb, void *patch_arg)
@@ -4920,10 +4957,11 @@ create_patched_content(char **path_outfile, int revers
char *path1 = NULL, *id_str = NULL;
struct stat sb2;
struct got_diffreg_result *diffreg_result = NULL;
- int line_cur1 = 1, line_cur2 = 1, have_content = 0;
+ int choice, line_cur1 = 1, line_cur2 = 1, have_content = 0;
int i = 0, n = 0, nchunks_used = 0, nchanges = 0;
*path_outfile = NULL;
+ *confirm_binary_change = 0;
err = got_object_id_str(&id_str, blob_id);
if (err)
@@ -5015,6 +5053,14 @@ create_patched_content(char **path_outfile, int revers
if (err)
goto done;
+ if (diff_result_has_binary(diffreg_result->result)) {
+ err = accept_or_reject_binary_change(&choice, relpath,
+ patch_cb, patch_arg);
+ if (err == NULL && choice == GOT_PATCH_CHOICE_YES)
+ *confirm_binary_change = 1;
+ goto done;
+ }
+
err = got_opentemp_named(path_outfile, &outfile, "got-patched-content",
"");
if (err)
@@ -5033,7 +5079,6 @@ create_patched_content(char **path_outfile, int revers
nchanges++;
}
for (n = 0; n < diffreg_result->result->chunks.len; n += nchunks_used) {
- int choice;
err = apply_or_reject_change(&choice, &nchunks_used,
diffreg_result->result, n, relpath, f1, f2,
&line_cur1, &line_cur2,
@@ -5273,11 +5318,25 @@ 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);
+ if (err != NULL)
+ goto done;
+ }
+ if (path_content == NULL)
break;
if (te && S_ISLNK(te->mode)) {
if (unlink(path_content) == -1) {
@@ -9236,11 +9295,15 @@ stage_path(void *arg, unsigned char status,
if (choice != GOT_PATCH_CHOICE_YES)
break;
} else {
- err = create_patched_content(&path_content, 0,
+ int stage_binary_file = 0;
+
+ err = create_patched_content(&path_content,
+ &stage_binary_file, 0,
staged_blob_id ? staged_blob_id : blob_id,
ondisk_path, dirfd, de_name, ie->path,
a->repo, a->patch_cb, a->patch_arg);
- if (err || path_content == NULL)
+ if (!stage_binary_file &&
+ (err || path_content == NULL))
break;
}
}
@@ -9449,9 +9512,9 @@ struct unstage_path_arg {
static const struct got_error *
create_unstaged_content(char **path_unstaged_content,
- char **path_new_staged_content, struct got_object_id *blob_id,
- struct got_object_id *staged_blob_id, const char *relpath,
- struct got_repository *repo,
+ char **path_new_staged_content, int *confirm_binary_change,
+ struct got_object_id *blob_id, struct got_object_id *staged_blob_id,
+ const char *relpath, struct got_repository *repo,
got_worktree_patch_cb patch_cb, void *patch_arg)
{
const struct got_error *err, *free_err;
@@ -9459,10 +9522,11 @@ create_unstaged_content(char **path_unstaged_content,
FILE *f1 = NULL, *f2 = NULL, *outfile = NULL, *rejectfile = NULL;
char *path1 = NULL, *path2 = NULL, *label1 = NULL;
struct got_diffreg_result *diffreg_result = NULL;
- int line_cur1 = 1, line_cur2 = 1, n = 0, nchunks_used = 0;
+ int choice, line_cur1 = 1, line_cur2 = 1, n = 0, nchunks_used = 0;
int have_content = 0, have_rejected_content = 0, i = 0, nchanges = 0;
int fd1 = -1, fd2 = -1;
+ *confirm_binary_change = 0;
*path_unstaged_content = NULL;
*path_new_staged_content = NULL;
@@ -9511,6 +9575,14 @@ create_unstaged_content(char **path_unstaged_content,
if (err)
goto done;
+ if (diff_result_has_binary(diffreg_result->result)) {
+ err = accept_or_reject_binary_change(&choice, relpath,
+ patch_cb, patch_arg);
+ if (err == NULL && choice == GOT_PATCH_CHOICE_YES)
+ *confirm_binary_change = 1;
+ goto done;
+ }
+
err = got_opentemp_named(path_unstaged_content, &outfile,
"got-unstaged-content", "");
if (err)
@@ -9536,7 +9608,6 @@ create_unstaged_content(char **path_unstaged_content,
nchanges++;
}
for (n = 0; n < diffreg_result->result->chunks.len; n += nchunks_used) {
- int choice;
err = apply_or_reject_change(&choice, &nchunks_used,
diffreg_result->result, n, relpath, f1, f2,
&line_cur1, &line_cur2,
@@ -9600,11 +9671,11 @@ done:
}
static const struct got_error *
-unstage_hunks(struct got_object_id *staged_blob_id,
- struct got_blob_object *blob_base,
- struct got_object_id *blob_id, struct got_fileindex_entry *ie,
- const char *ondisk_path, const char *label_orig,
- struct got_worktree *worktree, struct got_repository *repo,
+unstage_hunks(int *confirm_binary_change, struct got_object_id *staged_blob_id,
+ struct got_blob_object *blob_base, struct got_object_id *blob_id,
+ struct got_fileindex_entry *ie, const char *ondisk_path,
+ const char *label_orig, struct got_worktree *worktree,
+ struct got_repository *repo,
got_worktree_patch_cb patch_cb, void *patch_arg,
got_worktree_checkout_cb progress_cb, void *progress_arg)
{
@@ -9618,8 +9689,8 @@ unstage_hunks(struct got_object_id *staged_blob_id,
struct stat sb;
err = create_unstaged_content(&path_unstaged_content,
- &path_new_staged_content, blob_id, staged_blob_id,
- ie->path, repo, patch_cb, patch_arg);
+ &path_new_staged_content, confirm_binary_change,
+ blob_id, staged_blob_id, ie->path, repo, patch_cb, patch_arg);
if (err)
return err;
@@ -9823,12 +9894,16 @@ unstage_path(void *arg, unsigned char status,
if (choice != GOT_PATCH_CHOICE_YES)
break;
} else {
- err = unstage_hunks(staged_blob_id,
- blob_base, blob_id, ie, ondisk_path,
- label_orig, a->worktree, a->repo,
+ int unstage_binary_file = 0;
+
+ err = unstage_hunks(&unstage_binary_file,
+ staged_blob_id, blob_base, blob_id,
+ ie, ondisk_path, label_orig,
+ a->worktree, a->repo,
a->patch_cb, a->patch_arg,
a->progress_cb, a->progress_arg);
- break; /* Done with this file. */
+ if (!unstage_binary_file)
+ break; /* Done with this file. */
}
}
err = got_object_open_as_blob(&blob_staged, a->repo,
blob - bb8f0dfe722dc734a56ccff46a57c1cf44e3005b
blob + 001262301f5016d81a6b4944240cc53a7e8a0144
--- regress/cmdline/stage.sh
+++ regress/cmdline/stage.sh
@@ -3041,6 +3041,263 @@ EOF
test_done "$testroot" "0"
}
+test_stage_patch_binary() {
+ local testroot=$(test_init stage_patch_binary)
+
+ dd if=/dev/urandom of=$testroot/repo/binary bs=1024 count=16 \
+ > /dev/null 2>&1
+ git -C $testroot/repo add binary
+ git_commit $testroot/repo -m "add binary file"
+ local id=$(git_show_head $testroot/repo)
+
+ got checkout $testroot/repo $testroot/wt > /dev/null
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ ed -s $testroot/wt/binary <<-EOF
+ 2m8
+ 7m16
+ 5m24
+ 22m32
+ w
+ EOF
+
+ # 'n' response to reject stage of binary change
+ printf "n\n" > $testroot/patchscript
+ (cd $testroot/wt && got stage -F $testroot/patchscript -p binary \
+ > $testroot/stdout 2> $testroot/stderr)
+ ret=$?
+ if [ $ret -eq 0 ]; then
+ echo "got stage command succeeded unexpectedly" >&2
+ test_done "$testroot" "1"
+ return 1
+ fi
+
+ cat > $testroot/stdout.expected <<-EOF
+ -----------------------------------------------
+ Binary files binary and binary differ
+ -----------------------------------------------
+ M binary (change 1 of 1)
+ stage this change? [y/n/q] n
+ EOF
+
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ echo "got: no changes to stage" > $testroot/stderr.expected
+ cmp -s $testroot/stderr.expected $testroot/stderr
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stderr.expected $testroot/stderr
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ (cd $testroot/wt && got status > $testroot/stdout)
+ echo "M binary" > $testroot/stdout.expected
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ # 'q' response to reject stage of binary change
+ printf "q\n" > $testroot/patchscript
+ (cd $testroot/wt && got stage -F $testroot/patchscript -p binary \
+ > $testroot/stdout 2> $testroot/stderr)
+ ret=$?
+ if [ $ret -eq 0 ]; then
+ echo "got stage command succeeded unexpectedly" >&2
+ test_done "$testroot" "1"
+ return 1
+ fi
+
+ cat > $testroot/stdout.expected <<-EOF
+ -----------------------------------------------
+ Binary files binary and binary differ
+ -----------------------------------------------
+ M binary (change 1 of 1)
+ stage this change? [y/n/q] q
+ EOF
+
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ echo "got: no changes to stage" > $testroot/stderr.expected
+ cmp -s $testroot/stderr.expected $testroot/stderr
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stderr.expected $testroot/stderr
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ (cd $testroot/wt && got status > $testroot/stdout)
+ echo "M binary" > $testroot/stdout.expected
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ # 'y' response to stage binary change
+ printf "y\n" > $testroot/patchscript
+ (cd $testroot/wt && got stage -F $testroot/patchscript -p binary \
+ > $testroot/stdout 2> $testroot/stderr)
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ echo "got stage command failed unexpectedly" >&2
+ test_done "$testroot" "1"
+ return 1
+ fi
+
+ cat > $testroot/stdout.expected <<-EOF
+ -----------------------------------------------
+ Binary files binary and binary differ
+ -----------------------------------------------
+ M binary (change 1 of 1)
+ stage this change? [y/n/q] y
+ EOF
+
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ (cd $testroot/wt && got status > $testroot/stdout)
+ echo " M binary" > $testroot/stdout.expected
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ (cd $testroot/wt && got commit -m "changed binary" > /dev/null)
+
+ seq 16 > $testroot/wt/numbers
+ (cd $testroot/wt && got add numbers > /dev/null)
+ (cd $testroot/wt && got commit -m "add numbers" > /dev/null)
+
+ ed -s $testroot/wt/numbers <<-EOF
+ ,s/^2$/x/
+ ,s/^8$/y/
+ ,s/^16$/z/
+ w
+ EOF
+
+ ed -s $testroot/wt/binary <<-EOF
+ 2m8
+ 7m16
+ 5m24
+ 22m32
+ w
+ EOF
+
+ # stage first numbers hunk and binary file
+ printf "y\ny\nn\nn\n" > $testroot/patchscript
+ (cd $testroot/wt && got stage -F $testroot/patchscript -p \
+ > $testroot/stdout)
+
+ cat > $testroot/stdout.expected <<-EOF
+ -----------------------------------------------
+ Binary files binary and binary differ
+ -----------------------------------------------
+ M binary (change 1 of 1)
+ stage this change? [y/n/q] y
+ -----------------------------------------------
+ @@ -1,5 +1,5 @@
+ 1
+ -2
+ +x
+ 3
+ 4
+ 5
+ -----------------------------------------------
+ M numbers (change 1 of 3)
+ stage this change? [y/n/q] y
+ -----------------------------------------------
+ @@ -5,7 +5,7 @@
+ 5
+ 6
+ 7
+ -8
+ +y
+ 9
+ 10
+ 11
+ -----------------------------------------------
+ M numbers (change 2 of 3)
+ stage this change? [y/n/q] n
+ -----------------------------------------------
+ @@ -13,4 +13,4 @@
+ 13
+ 14
+ 15
+ -16
+ +z
+ -----------------------------------------------
+ M numbers (change 3 of 3)
+ stage this change? [y/n/q] n
+ EOF
+
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ (cd $testroot/wt && got status > $testroot/stdout)
+ echo " M binary" > $testroot/stdout.expected
+ echo "MM numbers" >> $testroot/stdout.expected
+
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ (cd $testroot/wt && got diff -s binary | grep '^Binary files' \
+ > $testroot/stdout)
+ echo "Binary files binary and binary differ" \
+ > $testroot/stdout.expected
+
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ test_done "$testroot" 0
+}
+
test_parseargs "$@"
run_test test_stage_basic
run_test test_stage_no_changes
@@ -3072,3 +3329,4 @@ run_test test_stage_patch_quit
run_test test_stage_patch_incomplete_script
run_test test_stage_symlink
run_test test_stage_patch_symlink
+run_test test_stage_patch_binary
--
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68
Weird behaviour when staging binary files interactively