"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
Re: histedit folding "delete, re-add" is broken
To:
Christian Weisgerber <naddy@mips.inka.de>, gameoftrees@openbsd.org
Date:
Wed, 8 Mar 2023 16:35:14 +0100

Download raw body.

Thread
On Tue, Mar 07, 2023 at 09:37:06AM +0100, Stefan Sperling wrote:
> On Tue, Mar 07, 2023 at 12:24:00AM +0100, Christian Weisgerber wrote:
> > Histedit folding of a file deletion and subsequent addition of the
> > same file is broken.
> > 
> > You have a file.
> > In commit #1 you delete that file.
> > In commit #2 you put a modified version of that file back into place.
> > Then you histedit and fold commits #1 and #2.
> > 
> > I would expect that this operation coalesces the commits into one
> > that changes the file.  In reality, the file is simply deleted.
> > I guess commit #1 preempts further changes.
> > 
> > By contrast, Git behaves as I would expect.
> > 
> > (I wasn't idly looking for a corner case.  One lazy way to, say,
> > switch the FreeBSD got port to got-portable would be to simply
> > delete the old port, put a newly created one into the same place,
> > fold the commits, and let God^Ht sort them out...)
> > 
> > xfail regression test below.  OK?
> 
> Yes, thanks!
> 
> This is a delete (local change) vs. add (incoming change) on a file, and
> hence a tree conflict (per classification the SVN project has been using).
> 
> This appears as a tree conflict during histedit fold because fold is
> implemented by merging (cherrypicking) multiple commits into the work 
> tree in sequence, with the changes from commit N appearing as local
> modifications when commit N+1 gets merged. In 'got status' terms we see
> local state 'D alpha', and the change being merged is 'A alpha'.
> 
> I agree we should tweak our merge logic to "restore" the added incoming
> file even if that file has been locally deleted. There are two similar
> cases to consider.
> 
> One case is where alpha was renamed, rather than deleted:
> 
>   git mv alpha alpha2
>   git commit
>   echo "unrelated content for unrelated file alpha" > alpha
>   git add alpha
> 
> And this case which is covered by your test:
> 
>   git rm alpha
>   git commit
>   echo "restore alpha, perhaps with tweaks" > alpha
>   git add alpha
> 
> In either case a deletion event for file alpha will appear while merging
> the changes in sequence, followed by an addition event. In both cases the
> desired merge result would likely be to re-add file alpha with its new
> content. I don't see an obvious alternative way of resolving these cases.
> So your proposal makes sense to me.

Here is a fix for this. ok?


-----------------------------------------------
 support histedit fold operations which delete a file and then add it again
 
 problem found by naddy@
 
diff cee3836880940ba0d1203e0682a43a680132bc27 4931293229292383086fb2ddd615c3df18de0bba
commit - cee3836880940ba0d1203e0682a43a680132bc27
commit + 4931293229292383086fb2ddd615c3df18de0bba
blob - f391c9b29f69de0b5abb466e2cd2bd6e65ae9dde
blob + 19b8418d8d3b6252476d3f2048056a0c997b1377
--- lib/fileindex.c
+++ lib/fileindex.c
@@ -128,7 +128,7 @@ got_fileindex_entry_update(struct got_fileindex_entry 
 	}
 
 	if (blob_sha1) {
-		memcpy(ie->blob_sha1, blob_sha1, SHA1_DIGEST_LENGTH);
+		memmove(ie->blob_sha1, blob_sha1, SHA1_DIGEST_LENGTH);
 		ie->flags &= ~GOT_FILEIDX_F_NO_BLOB;
 	} else
 		ie->flags |= GOT_FILEIDX_F_NO_BLOB;
blob - d7bd842e2f1e138573c94334a50105954443372d
blob + ce1ab2adbfadcbf6d57d031d9390a6d8a3940fb8
--- lib/worktree.c
+++ lib/worktree.c
@@ -2853,6 +2853,66 @@ struct merge_file_cb_arg {
 	return err;
 }
 
+static const struct got_error *
+add_file(struct got_worktree *worktree, struct got_fileindex *fileindex,
+    struct got_fileindex_entry *ie, const char *ondisk_path,
+    const char *path2, struct got_blob_object *blob2, mode_t mode2,
+    int restoring_missing_file, int reverting_versioned_file,
+    int path_is_unversioned, int allow_bad_symlinks,
+    struct got_repository *repo,
+    got_worktree_checkout_cb progress_cb, void *progress_arg)
+{
+	const struct got_error *err = NULL;
+	int is_bad_symlink = 0;
+
+	if (S_ISLNK(mode2)) {
+		err = install_symlink(&is_bad_symlink,
+		    worktree, ondisk_path, path2, blob2,
+		    restoring_missing_file,
+		    reverting_versioned_file,
+		    path_is_unversioned, allow_bad_symlinks,
+		    repo, progress_cb, progress_arg);
+	} else {
+		err = install_blob(worktree, ondisk_path, path2,
+		    mode2, GOT_DEFAULT_FILE_MODE, blob2,
+		    restoring_missing_file, reverting_versioned_file, 0,
+		    path_is_unversioned, repo, progress_cb, progress_arg);
+	}
+	if (err)
+		return err;
+	if (ie == NULL) {
+		/* Adding an unversioned file. */
+		err = got_fileindex_entry_alloc(&ie, path2);
+		if (err)
+			return err;
+		err = got_fileindex_entry_update(ie,
+		    worktree->root_fd, path2, NULL, NULL, 1);
+		if (err) {
+			got_fileindex_entry_free(ie);
+			return err;
+		}
+		err = got_fileindex_entry_add(fileindex, ie);
+		if (err) {
+			got_fileindex_entry_free(ie);
+			return err;
+		}
+	} else {
+		/* Re-adding a locally deleted file. */
+		err = got_fileindex_entry_update(ie,
+		    worktree->root_fd, path2, ie->blob_sha1,
+		    worktree->base_commit_id->sha1, 0);
+		if (err)
+			return err;
+	}
+
+	if (is_bad_symlink) {
+		got_fileindex_entry_filetype_set(ie,
+		    GOT_FILEIDX_MODE_BAD_SYMLINK);
+	}
+
+	return NULL;
+}
+
 struct merge_file_cb_arg {
     struct got_worktree *worktree;
     struct got_fileindex *fileindex;
@@ -3070,7 +3130,8 @@ merge_file_cb(void *arg, struct got_blob_object *blob1
 			if (status != GOT_STATUS_NO_CHANGE &&
 			    status != GOT_STATUS_MODIFY &&
 			    status != GOT_STATUS_CONFLICT &&
-			    status != GOT_STATUS_ADD) {
+			    status != GOT_STATUS_ADD &&
+			    status != GOT_STATUS_DELETE) {
 				err = (*a->progress_cb)(a->progress_arg,
 				    status, path2);
 				goto done;
@@ -3092,52 +3153,28 @@ merge_file_cb(void *arg, struct got_blob_object *blob1
 				    sb.st_mode, a->label_orig, blob2,
 				    a->commit_id2, repo, a->progress_cb,
 				    a->progress_arg);
-			} else {
+			} else if (status != GOT_STATUS_DELETE) {
 				err = got_error_path(ondisk_path,
 				    GOT_ERR_FILE_OBSTRUCTED);
 			}
 			if (err)
 				goto done;
 			if (status == GOT_STATUS_DELETE) {
-				err = got_fileindex_entry_update(ie,
-				    a->worktree->root_fd, path2, blob2->id.sha1,
-				    a->worktree->base_commit_id->sha1, 0);
+				/* Re-add file with content from new blob. */
+				err = add_file(a->worktree, a->fileindex, ie,
+				    ondisk_path, path2, blob2, mode2,
+				    0, 0, 0, a->allow_bad_symlinks,
+				    repo, a->progress_cb, a->progress_arg);
 				if (err)
 					goto done;
 			}
 		} else {
-			int is_bad_symlink = 0;
-			sb.st_mode = GOT_DEFAULT_FILE_MODE;
-			if (S_ISLNK(mode2)) {
-				err = install_symlink(&is_bad_symlink,
-				    a->worktree, ondisk_path, path2, blob2, 0,
-				    0, 1, a->allow_bad_symlinks, repo,
-				    a->progress_cb, a->progress_arg);
-			} else {
-				err = install_blob(a->worktree, ondisk_path, path2,
-				    mode2, sb.st_mode, blob2, 0, 0, 0, 1, repo,
-				    a->progress_cb, a->progress_arg);
-			}
+			err = add_file(a->worktree, a->fileindex, NULL,
+			     ondisk_path, path2, blob2, mode2,
+			     0, 0, 1, a->allow_bad_symlinks,
+			    repo, a->progress_cb, a->progress_arg);
 			if (err)
 				goto done;
-			err = got_fileindex_entry_alloc(&ie, path2);
-			if (err)
-				goto done;
-			err = got_fileindex_entry_update(ie,
-			    a->worktree->root_fd, path2, NULL, NULL, 1);
-			if (err) {
-				got_fileindex_entry_free(ie);
-				goto done;
-			}
-			err = got_fileindex_entry_add(a->fileindex, ie);
-			if (err) {
-				got_fileindex_entry_free(ie);
-				goto done;
-			}
-			if (is_bad_symlink) {
-				got_fileindex_entry_filetype_set(ie,
-				    GOT_FILEIDX_MODE_BAD_SYMLINK);
-			}
 		}
 	}
 done:
blob - b4517778fe90cf2ff3cb089a57a6fc0b911217dc
blob + 4837661a3f1d3724a4c7b8a235329ae2d6330c0f
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -1590,18 +1590,20 @@ test_histedit_fold_delete_add() {
 	echo "Switching work tree to refs/heads/master" \
 		>> $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
+	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
 
 	if [ ! -e $testroot/wt/alpha ]; then
-		ret="xfail fold deleted and added file"
+		echo "file alpha is missing on disk" >&2
+		test_done "$testroot" "1"
+		return 1
 	fi
-	test_done "$testroot" "$ret"
+	test_done "$testroot" "0"
 }
 
 test_histedit_fold_only() {