[patch, needs testing] implement fast-forward merges

The below patch makes "got merge" fast-forward when possible, instead of aborting and advising the user to run "got integrate" instead. I put "needs testing" in the subject line because I haven't really used this patch except to try out a couple of basic operations. Perhaps the new regression tests are enough, though. A possible follow-up change would be an -M option to force creating a merge commit. I split part of got_worktree_merge_prepare into a new function, got_worktree_merge_write_refs, since that part doesn't make sense in the fast-forward case. The code implementing the fast-forward is based on a similar case in cmd_rebase. Maybe there is potential to simplify by putting common code into a new function. -- James diff refs/heads/main refs/heads/falsifian/mg_ff commit - 2f3ccc5ff648594ff13619e304c2a139a917f2d2 commit + e0d373cccbb1462edd46bd882dfc9aedcd0d15e0 blob - c0f43abe63fe3e59e6b05e48efdcb881e2654b2f blob + 234fc25420a18e672baeecd57954bc2d03dc2880 --- TODO +++ TODO @@ -8,15 +8,9 @@ got: instead of the root directory checked out at /usr/src. The next LLVM release 12.1 would later be committed onto the llvm-12 branch and then merged into main at /usr/src/gnu/llvm in the same way. -- Teach 'got merge' to forward a branch reference if possible, instead of - creating a merge commit. Forwarding should only be done if linear - history exists from the tip of the branch being merged to the tip of - the work tree's branch, and if the tip of the work tree's branch is - itself not a merge commit (this makes "stacked" merges possible - by default, and prevents a 'main' branch reference from being forwarded - to a vendor branch in case no new commits were added to 'main' since - the previous vendor merge). Provide an option (-M) which forces creation - of a merge commit, for cases where users deem forwarding undesirable. +- Provide an option (-M) to 'got merge' which forces creation of a merge commit + even when forwarding is possible, for cases where users deem forwarding + undesirable. - When a clone fails the HEAD symref will always point to "refs/heads/main" (ie. the internal default HEAD symref of Got). Resuming a failed clone with 'got fetch' is supposed to work. To make this easier, if the HEAD symref blob - cf38fa07face9d2a811a09f32c2b6e6bc5cff414 blob + af131cc34188e7ce130a0b3d83844ce7fa07edeb --- got/got.1 +++ got/got.1 @@ -2796,9 +2796,18 @@ Create a merge commit based on the current branch of t .Op Ar branch .Xc .Dl Pq alias: Cm mg -Create a merge commit based on the current branch of the work tree and -the specified -.Ar branch . +Merge the specified +.Ar branch +into the current branch of the work tree. +If the branches have diverged, creates a merge commit. +Otherwise, if +.Ar branch +already includes all commits from the work tree's branch, update the work +tree's branch to be the same as +.Ar branch +without creating a commit, and update the work tree to the most recent commit +on the branch. +.Pp If a linear project history is desired, then use of .Cm got rebase should be preferred over @@ -2854,14 +2863,6 @@ If history of the .Pp .Cm got merge will refuse to run if certain preconditions are not met. -If history of the -.Ar branch -is based on the work tree's current branch, then no merge commit can -be created and -.Cm got integrate -may be used to integrate the -.Ar branch -instead. If the work tree is not yet fully updated to the tip commit of its branch, then the work tree must first be updated with .Cm got update . blob - b713aa2e5f2e3920e0055fb2366825c0e1487eff blob + 5b4dd802382f2f11d1eb84a746581ea23b5fcc96 --- got/got.c +++ got/got.c @@ -13313,19 +13313,52 @@ cmd_merge(int argc, char *argv[]) GOT_ERR_MERGE_PATH, repo); if (error) goto done; + error = got_worktree_merge_prepare(&fileindex, worktree, repo); + if (error) + goto done; if (yca_id && got_object_id_cmp(wt_branch_tip, yca_id) == 0) { - static char msg[512]; - snprintf(msg, sizeof(msg), - "cannot create a merge commit because %s is based " - "on %s; %s can be integrated with 'got integrate' " - "instead", branch_name, - got_worktree_get_head_ref_name(worktree), - branch_name); - error = got_error_msg(GOT_ERR_SAME_BRANCH, msg); + struct got_pathlist_head paths; + if (interrupt_merge) { + error = got_error_msg(GOT_ERR_SAME_BRANCH, + "merge is a fast-forward; this is " + "incompatible with got merge -n"); + goto done; + } + printf("Forwarding %s to %s\n", + got_ref_get_name(wt_branch), branch_name); + error = got_ref_change_ref(wt_branch, branch_tip); + if (error) + goto done; + error = got_ref_write(wt_branch, repo); + if (error) + goto done; + error = got_worktree_set_base_commit_id(worktree, repo, + branch_tip); + if (error) + goto done; + TAILQ_INIT(&paths); + error = got_pathlist_append(&paths, "", NULL); + if (error) + goto done; + error = got_worktree_checkout_files(worktree, + &paths, repo, update_progress, &upa, + check_cancelled, NULL); + got_pathlist_free(&paths, GOT_PATHLIST_FREE_NONE); + if (error) + goto done; + if (upa.did_something) { + char *id_str; + error = got_object_id_str(&id_str, branch_tip); + if (error) + goto done; + printf("Updated to commit %s\n", id_str); + free(id_str); + } else + printf("Already up-to-date\n"); + print_update_progress_stats(&upa); goto done; } - error = got_worktree_merge_prepare(&fileindex, worktree, - branch, repo); + error = got_worktree_merge_write_refs(worktree, branch, repo); if (error) goto done; blob - f7de090bfb2bd10312bdd0c389b5eabe41a33fdd blob + 5b0b7259e6ff5fc0edf8711ac6780dd49315fe54 --- include/got_worktree.h +++ include/got_worktree.h @@ -485,16 +485,22 @@ const struct got_error *got_worktree_merge_in_progress struct got_worktree *, struct got_repository *); /* - * Prepare for merging a branch into the work tree's current branch. + * Prepare for merging a branch into the work tree's current branch: lock the + * worktree and check that preconditions are satisfied. The function also + * returns a pointer to a fileindex which must be passed back to other + * merge-related functions. + */ +const struct got_error *got_worktree_merge_prepare(struct got_fileindex **, + struct got_worktree *, struct got_repository *); + +/* * This function creates a reference to the branch being merged, and to * this branch's current tip commit, in the "got/worktree/merge/" namespace. * These references are used to keep track of merge operation state and are * used as input and/or output arguments with other merge-related functions. - * The function also returns a pointer to a fileindex which must be - * passed back to other merge-related functions. */ -const struct got_error *got_worktree_merge_prepare(struct got_fileindex **, - struct got_worktree *, struct got_reference *, struct got_repository *); +const struct got_error *got_worktree_merge_write_refs(struct got_worktree *, + struct got_reference *, struct got_repository *); /* * Continue an interrupted merge operation. blob - e97f10e73cd0df0521ce4e36b40897121837e8d5 blob + 95e2e6bfa46ac12fd077b083c122ade77e54106f --- lib/worktree.c +++ lib/worktree.c @@ -8486,14 +8486,12 @@ const struct got_error *got_worktree_merge_prepare( const struct got_error *got_worktree_merge_prepare( struct got_fileindex **fileindex, struct got_worktree *worktree, - struct got_reference *branch, struct got_repository *repo) + struct got_repository *repo) { const struct got_error *err = NULL; char *fileindex_path = NULL; - char *branch_refname = NULL, *commit_refname = NULL; - struct got_reference *wt_branch = NULL, *branch_ref = NULL; - struct got_reference *commit_ref = NULL; - struct got_object_id *branch_tip = NULL, *wt_branch_tip = NULL; + struct got_reference *wt_branch = NULL; + struct got_object_id *wt_branch_tip = NULL; struct check_rebase_ok_arg ok_arg; *fileindex = NULL; @@ -8514,14 +8512,6 @@ const struct got_error *got_worktree_merge_prepare( if (err) goto done; - err = get_merge_branch_ref_name(&branch_refname, worktree); - if (err) - return err; - - err = get_merge_commit_ref_name(&commit_refname, worktree); - if (err) - return err; - err = got_ref_open(&wt_branch, repo, worktree->head_ref_name, 0); if (err) @@ -8536,6 +8526,38 @@ const struct got_error *got_worktree_merge_prepare( goto done; } +done: + free(fileindex_path); + if (wt_branch) + got_ref_close(wt_branch); + free(wt_branch_tip); + if (err) { + if (*fileindex) { + got_fileindex_free(*fileindex); + *fileindex = NULL; + } + lock_worktree(worktree, LOCK_SH); + } + return err; +} + +const struct got_error *got_worktree_merge_write_refs( + struct got_worktree *worktree, struct got_reference *branch, + struct got_repository *repo) +{ + const struct got_error *err = NULL; + char *branch_refname = NULL, *commit_refname = NULL; + struct got_reference *branch_ref = NULL, *commit_ref = NULL; + struct got_object_id *branch_tip = NULL; + + err = get_merge_branch_ref_name(&branch_refname, worktree); + if (err) + return err; + + err = get_merge_commit_ref_name(&commit_refname, worktree); + if (err) + return err; + err = got_ref_resolve(&branch_tip, repo, branch); if (err) goto done; @@ -8557,21 +8579,11 @@ done: done: free(branch_refname); free(commit_refname); - free(fileindex_path); if (branch_ref) got_ref_close(branch_ref); if (commit_ref) got_ref_close(commit_ref); - if (wt_branch) - got_ref_close(wt_branch); - free(wt_branch_tip); - if (err) { - if (*fileindex) { - got_fileindex_free(*fileindex); - *fileindex = NULL; - } - lock_worktree(worktree, LOCK_SH); - } + free(branch_tip); return err; } blob - ca27e157f924c0abac516cda4f26581c04e3cc21 blob + 315385f3c9a0c45b75d3dd9921caf1f7b9fe18c1 --- regress/cmdline/merge.sh +++ regress/cmdline/merge.sh @@ -52,31 +52,7 @@ test_merge_basic() { return 1 fi - # need a divergant commit on the main branch for 'got merge' - (cd $testroot/wt && got merge newbranch \ - > $testroot/stdout 2> $testroot/stderr) - ret=$? - if [ $ret -eq 0 ]; then - echo "got merge succeeded unexpectedly" >&2 - test_done "$testroot" "1" - return 1 - fi - echo -n "got: cannot create a merge commit because " \ - > $testroot/stderr.expected - echo -n "refs/heads/newbranch is based on refs/heads/master; " \ - >> $testroot/stderr.expected - echo -n "refs/heads/newbranch can be integrated with " \ - >> $testroot/stderr.expected - echo "'got integrate' instead" >> $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 - - # create the required dirvergant commit + # create a divergent commit (cd $testroot/repo && git checkout -q master) echo "modified zeta on master" > $testroot/repo/epsilon/zeta git_commit $testroot/repo -m "committing to zeta on master" @@ -123,7 +99,7 @@ test_merge_basic() { ret=$? if [ $ret -eq 0 ]; then echo "got merge succeeded unexpectedly" >&2 - test_done "$testroot" "$ret" + test_done "$testroot" "1" return 1 fi echo -n "got: work tree contains files from multiple base commits; " \ @@ -371,6 +347,158 @@ test_merge_continue() { test_done "$testroot" "$ret" } +test_merge_forward() { + local testroot=`test_init merge_forward` + local commit0=`git_show_head $testroot/repo` + + # Create a commit before branching, which will be used to help test + # preconditions for "got merge". + echo "modified alpha" > $testroot/repo/alpha + git_commit $testroot/repo -m "common commit" + local commit1=`git_show_head $testroot/repo` + + (cd $testroot/repo && git checkout -q -b newbranch) + echo "modified beta on branch" > $testroot/repo/beta + git_commit $testroot/repo -m "committing to beta on newbranch" + local commit2=`git_show_head $testroot/repo` + + got checkout -b master $testroot/repo $testroot/wt > /dev/null + ret=$? + if [ $ret -ne 0 ]; then + echo "got checkout failed unexpectedly" >&2 + test_done "$testroot" "$ret" + return 1 + fi + + # must not use a mixed-commit work tree with 'got merge' + (cd $testroot/wt && got update -c $commit0 alpha > /dev/null) + ret=$? + if [ $ret -ne 0 ]; then + echo "got update failed unexpectedly" >&2 + test_done "$testroot" "$ret" + return 1 + fi + (cd $testroot/wt && got merge newbranch \ + > $testroot/stdout 2> $testroot/stderr) + ret=$? + if [ $ret -eq 0 ]; then + echo "got merge succeeded unexpectedly" >&2 + test_done "$testroot" "$ret" + return 1 + fi + echo -n "got: work tree contains files from multiple base commits; " \ + > $testroot/stderr.expected + echo "the entire work tree must be updated first" \ + >> $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 update > /dev/null) + ret=$? + if [ $ret -ne 0 ]; then + echo "got update failed unexpectedly" >&2 + test_done "$testroot" "$ret" + return 1 + fi + + # 'got merge -n' refuses to fast-forward + (cd $testroot/wt && got merge -n newbranch \ + > $testroot/stdout 2> $testroot/stderr) + ret=$? + if [ $ret -eq 0 ]; then + echo "got merge succeeded unexpectedly" >&2 + test_done "$testroot" "1" + return 1 + fi + echo -n "got: merge is a fast-forward; " > $testroot/stderr.expected + echo "this is incompatible with got merge -n" \ + >> $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 merge newbranch \ + > $testroot/stdout 2> $testroot/stderr) + ret=$? + if [ $ret -ne 0 ]; then + echo "got merge failed unexpectedly" >&2 + test_done "$testroot" "$ret" + return 1 + fi + + echo "Forwarding refs/heads/master to refs/heads/newbranch" \ + > $testroot/stdout.expected + echo "U beta" >> $testroot/stdout.expected + echo "Updated to commit $commit2" \ + >> $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 log | grep ^commit > $testroot/stdout) + echo -n "commit $commit2 " > $testroot/stdout.expected + echo "(master, newbranch)" >> $testroot/stdout.expected + echo "commit $commit1" >> $testroot/stdout.expected + echo "commit $commit0" >> $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" "$ret" +} + +test_merge_backward() { + local testroot=`test_init merge_backward` + local commit0=`git_show_head $testroot/repo` + + (cd $testroot/repo && git checkout -q -b newbranch) + (cd $testroot/repo && git checkout -q master) + echo "modified alpha on master" > $testroot/repo/alpha + git_commit $testroot/repo -m "committing to alpha on master" + + got checkout -b master $testroot/repo $testroot/wt > /dev/null + ret=$? + if [ $ret -ne 0 ]; then + echo "got checkout failed unexpectedly" >&2 + test_done "$testroot" "$ret" + return 1 + fi + + (cd $testroot/wt && got merge newbranch \ + > $testroot/stdout 2> $testroot/stderr) + ret=$? + if [ $ret -ne 0 ]; then + echo "got merge failed unexpectedly" >&2 + test_done "$testroot" "$ret" + return 1 + fi + echo "Already up-to-date" > $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" "$ret" +} + test_merge_continue() { local testroot=`test_init merge_continue` local commit0=`git_show_head $testroot/repo` @@ -1566,6 +1694,8 @@ run_test test_merge_continue test_parseargs "$@" run_test test_merge_basic +run_test test_merge_forward +run_test test_merge_backward run_test test_merge_continue run_test test_merge_continue_new_commit run_test test_merge_abort

2023-06-17 20:37 James Cook:
[patch, needs testing] implement fast-forward merges
- 2023-06-19 07:13 Stefan Sperling:
  [patch, needs testing] implement fast-forward merges