Download raw body.
histedit folding "delete, re-add" is broken
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() {
histedit folding "delete, re-add" is broken