rebase suffers '?' conflicts

On Thu, Apr 13, 2023 at 12:13:45AM +0200, Christian Weisgerber wrote: > Stefan Sperling: > > > Can you try this? If this works then I'll try to add regress coverage > > as well before committing it. > > Yes!! With that my reproducer script completes successfully. Here is a more complete diff which implements the fix for rebase, histedit, and merge. With an added safeguard for files which appear in added status during the revert step but were not merged into the work tree as part of the most recently rebased/edited/merged commit. Deleting such files could result in data loss which is a big no-no for a version control system. It could be argued that such files should left in added status rather than unversioned. But it seems cleaner to me to leave the work tree behind without anything scheduled for commit. Also adds test coverage. OK? ----------------------------------------------- when aborting rebase/histedit/merge, unlink files added by merged changes Otherwise we leave unversioned files behind in the work tree which may interfere with new attempts to rebase or merge the changes again. Problem found by naddy@ diff 8cdd231889a848b735f84ed6772eab46c2512db9 a3a41f3a8c0cf88fc117c5edae388d564084f0cd commit - 8cdd231889a848b735f84ed6772eab46c2512db9 commit + a3a41f3a8c0cf88fc117c5edae388d564084f0cd blob - b08ea7c98c8952d224400390c47972f2e057ee4e blob + cad03d0a864965a0acebeb1425c9c753c3d3449e --- lib/worktree.c +++ lib/worktree.c @@ -4700,6 +4700,7 @@ struct revert_file_args { void *patch_arg; struct got_repository *repo; int unlink_added_files; + struct got_pathlist_head *added_files_to_unlink; }; static const struct got_error * @@ -5002,16 +5003,33 @@ revert_file(void *arg, unsigned char status, unsigned goto done; got_fileindex_entry_remove(a->fileindex, ie); if (a->unlink_added_files) { - if (asprintf(&ondisk_path, "%s/%s", - got_worktree_get_root_path(a->worktree), - relpath) == -1) { - err = got_error_from_errno("asprintf"); - goto done; + int do_unlink = a->added_files_to_unlink ? 0 : 1; + + if (a->added_files_to_unlink) { + struct got_pathlist_entry *pe; + + TAILQ_FOREACH(pe, a->added_files_to_unlink, + entry) { + if (got_path_cmp(pe->path, relpath, + pe->path_len, strlen(relpath))) + continue; + do_unlink = 1; + break; + } } - if (unlink(ondisk_path) == -1) { - err = got_error_from_errno2("unlink", - ondisk_path); - break; + + if (do_unlink) { + if (asprintf(&ondisk_path, "%s/%s", + got_worktree_get_root_path(a->worktree), + relpath) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } + if (unlink(ondisk_path) == -1) { + err = got_error_from_errno2("unlink", + ondisk_path); + break; + } } } break; @@ -7328,6 +7346,131 @@ const struct got_error * return err; } +static const struct got_error * +get_paths_changed_between_commits(struct got_pathlist_head *paths, + struct got_object_id *id1, struct got_object_id *id2, + struct got_repository *repo) +{ + const struct got_error *err; + struct got_commit_object *commit1 = NULL, *commit2 = NULL; + struct got_tree_object *tree1 = NULL, *tree2 = NULL; + + if (id1) { + err = got_object_open_as_commit(&commit1, repo, id1); + if (err) + goto done; + + err = got_object_open_as_tree(&tree1, repo, + got_object_commit_get_tree_id(commit1)); + if (err) + goto done; + } + + if (id2) { + err = got_object_open_as_commit(&commit2, repo, id2); + if (err) + goto done; + + err = got_object_open_as_tree(&tree2, repo, + got_object_commit_get_tree_id(commit2)); + if (err) + goto done; + } + + err = got_diff_tree(tree1, tree2, NULL, NULL, -1, -1, "", "", repo, + got_diff_tree_collect_changed_paths, paths, 0); + if (err) + goto done; +done: + if (commit1) + got_object_commit_close(commit1); + if (commit2) + got_object_commit_close(commit2); + if (tree1) + got_object_tree_close(tree1); + if (tree2) + got_object_tree_close(tree2); + return err; +} + +static const struct got_error * +get_paths_added_between_commits(struct got_pathlist_head *added_paths, + struct got_object_id *id1, struct got_object_id *id2, + const char *path_prefix, struct got_repository *repo) +{ + const struct got_error *err; + struct got_pathlist_head merged_paths; + struct got_pathlist_entry *pe; + char *abspath = NULL, *wt_path = NULL; + + TAILQ_INIT(&merged_paths); + + err = get_paths_changed_between_commits(&merged_paths, id1, id2, repo); + if (err) + goto done; + + TAILQ_FOREACH(pe, &merged_paths, entry) { + struct got_diff_changed_path *change = pe->data; + + if (change->status != GOT_STATUS_ADD) + continue; + + if (got_path_is_root_dir(path_prefix)) { + wt_path = strdup(pe->path); + if (wt_path == NULL) { + err = got_error_from_errno("strdup"); + goto done; + } + } else { + if (asprintf(&abspath, "/%s", pe->path) == -1) { + err = got_error_from_errno("strdup"); + goto done; + } + + err = got_path_skip_common_ancestor(&wt_path, + path_prefix, abspath); + if (err) + goto done; + } + + err = got_pathlist_append(added_paths, wt_path, NULL); + if (err) + goto done; + wt_path = NULL; + } + +done: + got_pathlist_free(&merged_paths, GOT_PATHLIST_FREE_ALL); + free(abspath); + free(wt_path); + return err; +} + +static const struct got_error * +get_paths_added_in_commit(struct got_pathlist_head *added_paths, + struct got_object_id *id, const char *path_prefix, + struct got_repository *repo) +{ + const struct got_error *err; + struct got_commit_object *commit = NULL; + struct got_object_qid *pid; + + err = got_object_open_as_commit(&commit, repo, id); + if (err) + goto done; + + pid = STAILQ_FIRST(got_object_commit_get_parent_ids(commit)); + + err = get_paths_added_between_commits(added_paths, + pid ? &pid->id : NULL, id, path_prefix, repo); + if (err) + goto done; +done: + if (commit) + got_object_commit_close(commit); + return err; +} + const struct got_error * got_worktree_rebase_abort(struct got_worktree *worktree, struct got_fileindex *fileindex, struct got_repository *repo, @@ -7337,15 +7480,44 @@ got_worktree_rebase_abort(struct got_worktree *worktre const struct got_error *err, *unlockerr, *sync_err; struct got_reference *resolved = NULL; struct got_object_id *commit_id = NULL; + struct got_object_id *merged_commit_id = NULL; struct got_commit_object *commit = NULL; char *fileindex_path = NULL; + char *commit_ref_name = NULL; + struct got_reference *commit_ref = NULL; struct revert_file_args rfa; struct got_object_id *tree_id = NULL; + struct got_pathlist_head added_paths; + TAILQ_INIT(&added_paths); + err = lock_worktree(worktree, LOCK_EX); if (err) return err; + err = get_rebase_commit_ref_name(&commit_ref_name, worktree); + if (err) + goto done; + + err = got_ref_open(&commit_ref, repo, commit_ref_name, 0); + if (err) + goto done; + + err = got_ref_resolve(&merged_commit_id, repo, commit_ref); + if (err) + goto done; + + /* + * Determine which files in added status can be safely removed + * from disk while reverting changes in the work tree. + * We want to avoid deleting unrelated files which were added by + * the user for conflict resolution purposes. + */ + err = get_paths_added_in_commit(&added_paths, merged_commit_id, + got_worktree_get_path_prefix(worktree), repo); + if (err) + goto done; + err = got_ref_open(&resolved, repo, got_ref_get_symref_target(new_base_branch), 0); if (err) @@ -7393,7 +7565,8 @@ got_worktree_rebase_abort(struct got_worktree *worktre rfa.patch_cb = NULL; rfa.patch_arg = NULL; rfa.repo = repo; - rfa.unlink_added_files = 0; + rfa.unlink_added_files = 1; + rfa.added_files_to_unlink = &added_paths; err = worktree_status(worktree, "", fileindex, repo, revert_file, &rfa, NULL, NULL, 1, 0); if (err) @@ -7406,14 +7579,19 @@ done: if (sync_err && err == NULL) err = sync_err; done: + got_pathlist_free(&added_paths, GOT_PATHLIST_FREE_PATH); got_ref_close(resolved); free(tree_id); free(commit_id); + free(merged_commit_id); if (commit) got_object_commit_close(commit); if (fileindex) got_fileindex_free(fileindex); free(fileindex_path); + free(commit_ref_name); + if (commit_ref) + got_ref_close(commit_ref); unlockerr = lock_worktree(worktree, LOCK_SH); if (unlockerr && err == NULL) @@ -7709,14 +7887,49 @@ got_worktree_histedit_abort(struct got_worktree *workt const struct got_error *err, *unlockerr, *sync_err; struct got_reference *resolved = NULL; char *fileindex_path = NULL; + struct got_object_id *merged_commit_id = NULL; struct got_commit_object *commit = NULL; + char *commit_ref_name = NULL; + struct got_reference *commit_ref = NULL; struct got_object_id *tree_id = NULL; struct revert_file_args rfa; + struct got_pathlist_head added_paths; + TAILQ_INIT(&added_paths); + err = lock_worktree(worktree, LOCK_EX); if (err) return err; + err = get_histedit_commit_ref_name(&commit_ref_name, worktree); + if (err) + goto done; + + err = got_ref_open(&commit_ref, repo, commit_ref_name, 0); + if (err) { + if (err->code != GOT_ERR_NOT_REF) + goto done; + /* Can happen on early abort due to invalid histedit script. */ + commit_ref = NULL; + } + + if (commit_ref) { + err = got_ref_resolve(&merged_commit_id, repo, commit_ref); + if (err) + goto done; + + /* + * Determine which files in added status can be safely removed + * from disk while reverting changes in the work tree. + * We want to avoid deleting unrelated files added by the + * user during conflict resolution or during histedit -e. + */ + err = get_paths_added_in_commit(&added_paths, merged_commit_id, + got_worktree_get_path_prefix(worktree), repo); + if (err) + goto done; + } + err = got_ref_open(&resolved, repo, got_ref_get_symref_target(branch), 0); if (err) @@ -7755,7 +7968,8 @@ got_worktree_histedit_abort(struct got_worktree *workt rfa.patch_cb = NULL; rfa.patch_arg = NULL; rfa.repo = repo; - rfa.unlink_added_files = 0; + rfa.unlink_added_files = 1; + rfa.added_files_to_unlink = &added_paths; err = worktree_status(worktree, "", fileindex, repo, revert_file, &rfa, NULL, NULL, 1, 0); if (err) @@ -7768,9 +7982,14 @@ done: if (sync_err && err == NULL) err = sync_err; done: - got_ref_close(resolved); + if (resolved) + got_ref_close(resolved); + if (commit_ref) + got_ref_close(commit_ref); + free(merged_commit_id); free(tree_id); free(fileindex_path); + free(commit_ref_name); unlockerr = lock_worktree(worktree, LOCK_SH); if (unlockerr && err == NULL) @@ -8448,12 +8667,42 @@ got_worktree_merge_abort(struct got_worktree *worktree got_worktree_checkout_cb progress_cb, void *progress_arg) { const struct got_error *err, *unlockerr, *sync_err; - struct got_object_id *commit_id = NULL; struct got_commit_object *commit = NULL; char *fileindex_path = NULL; struct revert_file_args rfa; + char *commit_ref_name = NULL; + struct got_reference *commit_ref = NULL; + struct got_object_id *merged_commit_id = NULL; struct got_object_id *tree_id = NULL; + struct got_pathlist_head added_paths; + TAILQ_INIT(&added_paths); + + err = get_merge_commit_ref_name(&commit_ref_name, worktree); + if (err) + goto done; + + err = got_ref_open(&commit_ref, repo, commit_ref_name, 0); + if (err) + goto done; + + err = got_ref_resolve(&merged_commit_id, repo, commit_ref); + if (err) + goto done; + + /* + * Determine which files in added status can be safely removed + * from disk while reverting changes in the work tree. + * We want to avoid deleting unrelated files which were added by + * the user for conflict resolution purposes. + */ + err = get_paths_added_between_commits(&added_paths, + got_worktree_get_base_commit_id(worktree), merged_commit_id, + got_worktree_get_path_prefix(worktree), repo); + if (err) + goto done; + + err = got_object_open_as_commit(&commit, repo, worktree->base_commit_id); if (err) @@ -8480,6 +8729,7 @@ got_worktree_merge_abort(struct got_worktree *worktree rfa.patch_arg = NULL; rfa.repo = repo; rfa.unlink_added_files = 1; + rfa.added_files_to_unlink = &added_paths; err = worktree_status(worktree, "", fileindex, repo, revert_file, &rfa, NULL, NULL, 1, 0); if (err) @@ -8493,12 +8743,15 @@ done: err = sync_err; done: free(tree_id); - free(commit_id); + free(merged_commit_id); if (commit) got_object_commit_close(commit); if (fileindex) got_fileindex_free(fileindex); free(fileindex_path); + if (commit_ref) + got_ref_close(commit_ref); + free(commit_ref_name); unlockerr = lock_worktree(worktree, LOCK_SH); if (unlockerr && err == NULL) blob - efdeafbe917d9f2c945e958629fc06b7bd9af2ba blob + 6080151a8a3e5c278b52bd1288edfdf4aa4383fb --- regress/cmdline/histedit.sh +++ regress/cmdline/histedit.sh @@ -902,20 +902,15 @@ test_histedit_abort() { fi done - echo "new file on master" > $testroot/content.expected - cat $testroot/wt/epsilon/new > $testroot/content - cmp -s $testroot/content.expected $testroot/content - ret=$? - if [ $ret -ne 0 ]; then - diff -u $testroot/content.expected $testroot/content - test_done "$testroot" "$ret" + if [ -e $testroot/wt/epsilon/new ]; then + echo "removed file new still exists on disk" >&2 + test_done "$testroot" "1" return 1 fi (cd $testroot/wt && got status > $testroot/stdout) - echo "? epsilon/new" > $testroot/stdout.expected - echo "? unversioned-file" >> $testroot/stdout.expected + echo "? unversioned-file" > $testroot/stdout.expected cmp -s $testroot/stdout.expected $testroot/stdout ret=$? if [ $ret -ne 0 ]; then blob - 876eced1d6b84caf0d92277204c67399f2876875 blob + 9093dd5b9fb524b14d2d17d4fe754da0ec8ef9f4 --- regress/cmdline/merge.sh +++ regress/cmdline/merge.sh @@ -673,9 +673,14 @@ test_merge_abort() { return 1 fi + # unrelated added file added during conflict resolution + touch $testroot/wt/added-file + (cd $testroot/wt && got add added-file > /dev/null) + (cd $testroot/wt && got status > $testroot/stdout) - echo "C alpha" > $testroot/stdout.expected + echo "A added-file" > $testroot/stdout.expected + echo "C alpha" >> $testroot/stdout.expected echo "D beta" >> $testroot/stdout.expected echo "A epsilon/new" >> $testroot/stdout.expected echo "M gamma/delta" >> $testroot/stdout.expected @@ -697,11 +702,13 @@ test_merge_abort() { return 1 fi - echo "R alpha" > $testroot/stdout.expected + echo "R added-file" > $testroot/stdout.expected + echo "R alpha" >> $testroot/stdout.expected echo "R beta" >> $testroot/stdout.expected echo "R epsilon/new" >> $testroot/stdout.expected echo "R gamma/delta" >> $testroot/stdout.expected echo "R symlink" >> $testroot/stdout.expected + echo "G added-file" >> $testroot/stdout.expected echo "Merge of refs/heads/newbranch aborted" \ >> $testroot/stdout.expected @@ -757,7 +764,8 @@ test_merge_abort() { (cd $testroot/wt && got status > $testroot/stdout) - echo "? unversioned-file" > $testroot/stdout.expected + echo "? added-file" > $testroot/stdout.expected + echo "? unversioned-file" >> $testroot/stdout.expected cmp -s $testroot/stdout.expected $testroot/stdout ret=$? if [ $ret -ne 0 ]; then blob - bb762cd05b066923e9538c4bdf31982707dba134 blob + 1d339f654c90ee75c99eac9cc5a7f39e8b6275ce --- regress/cmdline/rebase.sh +++ regress/cmdline/rebase.sh @@ -419,7 +419,10 @@ test_rebase_abort() { local short_orig_commit1=`trim_obj_id 28 $orig_commit1` echo "modified alpha on branch" > $testroot/repo/alpha - git_commit $testroot/repo -m "committing to alpha on newbranch" + echo "new file on branch" > $testroot/repo/epsilon/new + (cd $testroot/repo && git add epsilon/new) + git_commit $testroot/repo \ + -m "changing alpha and adding new on newbranch" local orig_commit2=`git_show_head $testroot/repo` local short_orig_commit2=`trim_obj_id 28 $orig_commit2` @@ -450,10 +453,12 @@ test_rebase_abort() { >> $testroot/stdout.expected echo ": committing to beta on newbranch" >> $testroot/stdout.expected echo "C alpha" >> $testroot/stdout.expected + echo "A epsilon/new" >> $testroot/stdout.expected echo "Files with new merge conflicts: 1" >> $testroot/stdout.expected echo -n "$short_orig_commit2 -> merge conflict" \ >> $testroot/stdout.expected - echo ": committing to alpha on newbranch" >> $testroot/stdout.expected + echo ": changing alpha and adding new on newbranch" \ + >> $testroot/stdout.expected cmp -s $testroot/stdout.expected $testroot/stdout ret=$? if [ $ret -ne 0 ]; then @@ -490,9 +495,15 @@ test_rebase_abort() { return 1 fi + # unrelated file in work tree added during conflict resolution + touch $testroot/wt/added-file + (cd $testroot/wt && got add added-file > /dev/null) + (cd $testroot/wt && got status > $testroot/stdout) - echo "C alpha" > $testroot/stdout.expected + echo "A added-file" > $testroot/stdout.expected + echo "C alpha" >> $testroot/stdout.expected + echo "A epsilon/new" >> $testroot/stdout.expected echo "? unversioned-file" >> $testroot/stdout.expected cmp -s $testroot/stdout.expected $testroot/stdout ret=$? @@ -508,7 +519,10 @@ test_rebase_abort() { echo "Switching work tree to refs/heads/master" \ > $testroot/stdout.expected + echo 'R added-file' >> $testroot/stdout.expected echo 'R alpha' >> $testroot/stdout.expected + echo 'R epsilon/new' >> $testroot/stdout.expected + echo 'G added-file' >> $testroot/stdout.expected echo 'U beta' >> $testroot/stdout.expected echo "Rebase of refs/heads/newbranch aborted" \ >> $testroot/stdout.expected @@ -556,7 +570,8 @@ test_rebase_abort() { fi (cd $testroot/wt && got status > $testroot/stdout) - echo "? unversioned-file" > $testroot/stdout.expected + echo "? added-file" > $testroot/stdout.expected + echo "? unversioned-file" >> $testroot/stdout.expected cmp -s $testroot/stdout.expected $testroot/stdout ret=$? if [ $ret -ne 0 ]; then

2023-04-12 21:43 Stefan Sperling:
rebase suffers '?' conflicts
- 2023-04-12 22:13 Christian Weisgerber:
  rebase suffers '?' conflicts
- 2023-04-13 10:50 Stefan Sperling:
  rebase suffers '?' conflicts
- 2023-04-13 20:40 Christian Weisgerber:
  rebase suffers '?' conflicts
- 2023-04-14 12:18 Stefan Sperling:
  rebase suffers '?' conflicts