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