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

From:
Stefan Sperling <stsp@stsp.name>
Subject:
make 'got merge' detect additional problem cases
To:
gameoftrees@openbsd.org
Date:
Mon, 27 Sep 2021 17:44:22 +0200

Download raw body.

Thread 
This patch adds stat counters for undeleted and unversioned files
found during merges.

These are now displayed by 'got merge', 'got rebase', and any other
applicable commands which merge changes into the work tree without
also updating the work tree's base commit (i.e. pretty much every
command except 'got update' and 'got branch').

For 'got merge', interrupt the merge if any file deletions failed, or
if merged changes targeted any unversioned files. Such problems are signs
of an unclean merge and warrant the user's attention before the merge
result is committed. I plan to apply this same mechanism to 'got rebase'
and 'got histedit' later on as well.

ok?

(applies on top of my 'got merge -c' output patch I sent earlier today)

diff refs/heads/merge-c-output refs/heads/merge-stats
blob - f7b20149b9b681897e3a952de08646564000698f
blob + 5aa3eb413b775cdcac39caf15b875eb40f76ad63
--- got/got.c
+++ got/got.c
@@ -3062,6 +3062,8 @@ struct got_update_progress_arg {
 	int obstructed;
 	int not_updated;
 	int missing;
+	int not_deleted;
+	int unversioned;
 	int verbosity;
 };
 
@@ -3081,6 +3083,35 @@ print_update_progress_stats(struct got_update_progress
 		    "conflicts: %d\n", upa->not_updated);
 }
 
+/*
+ * The meaning of some status codes differs between merge-style operations and
+ * update operations. For example, the ! status code means "file was missing"
+ * if changes were merged into the work tree, and "missing file was restored"
+ * if the work tree was updated. This function should be used by any operation
+ * which merges changes into the work tree without updating the work tree.
+ */
+void
+print_merge_progress_stats(struct got_update_progress_arg *upa)
+{
+	if (!upa->did_something)
+		return;
+
+	if (upa->conflicts > 0)
+		printf("Files with new merge conflicts: %d\n", upa->conflicts);
+	if (upa->obstructed > 0)
+		printf("File paths obstructed by a non-regular file: %d\n",
+		    upa->obstructed);
+	if (upa->missing > 0)
+		printf("Files which had incoming changes but could not be "
+		    "found in the work tree: %d\n", upa->missing);
+	if (upa->not_deleted > 0)
+		printf("Files not deleted due to differences in deleted "
+		    "content: %d\n", upa->not_deleted);
+	if (upa->unversioned > 0)
+		printf("Files not merged because an unversioned file was "
+		    "found in the work tree: %d\n", upa->unversioned);
+}
+
 __dead static void
 usage_update(void)
 {
@@ -3113,6 +3144,10 @@ update_progress(void *arg, unsigned char status, const
 		upa->not_updated++;
 	if (status == GOT_STATUS_MISSING)
 		upa->missing++;
+	if (status == GOT_STATUS_CANNOT_DELETE)
+		upa->not_deleted++;
+	if (status == GOT_STATUS_UNVERSIONED)
+		upa->unversioned++;
 
 	while (path[0] == '/')
 		path++;
@@ -8087,7 +8122,7 @@ cmd_cherrypick(int argc, char *argv[])
 
 	if (upa.did_something)
 		printf("Merged commit %s\n", commit_id_str);
-	print_update_progress_stats(&upa);
+	print_merge_progress_stats(&upa);
 done:
 	if (commit)
 		got_object_commit_close(commit);
@@ -8198,7 +8233,7 @@ cmd_backout(int argc, char *argv[])
 
 	if (upa.did_something)
 		printf("Backed out commit %s\n", commit_id_str);
-	print_update_progress_stats(&upa);
+	print_merge_progress_stats(&upa);
 done:
 	if (commit)
 		got_object_commit_close(commit);
@@ -8914,7 +8949,7 @@ cmd_rebase(int argc, char *argv[])
 		if (error)
 			goto done;
 		printf("Rebase of %s aborted\n", got_ref_get_name(branch));
-		print_update_progress_stats(&upa);
+		print_merge_progress_stats(&upa);
 		goto done; /* nothing else to do */
 	}
 
@@ -9002,7 +9037,7 @@ cmd_rebase(int argc, char *argv[])
 				goto done;
 			printf("Rebase of %s aborted\n",
 			    got_ref_get_name(branch));
-			print_update_progress_stats(&upa);
+			print_merge_progress_stats(&upa);
 
 		}
 		error = got_error(GOT_ERR_EMPTY_REBASE);
@@ -9058,7 +9093,7 @@ cmd_rebase(int argc, char *argv[])
 		if (error)
 			goto done;
 
-		print_update_progress_stats(&upa);
+		print_merge_progress_stats(&upa);
 		if (upa.conflicts > 0)
 			rebase_status = GOT_STATUS_CONFLICT;
 
@@ -10147,7 +10182,7 @@ cmd_histedit(int argc, char *argv[])
 			goto done;
 		printf("Histedit of %s aborted\n",
 		    got_ref_get_symref_target(branch));
-		print_update_progress_stats(&upa);
+		print_merge_progress_stats(&upa);
 		goto done; /* nothing else to do */
 	} else if (abort_edit) {
 		error = got_error(GOT_ERR_NOT_HISTEDIT);
@@ -10258,7 +10293,7 @@ cmd_histedit(int argc, char *argv[])
 				got_worktree_histedit_abort(worktree, fileindex,
 				    repo, branch, base_commit_id,
 				    update_progress, &upa);
-				print_update_progress_stats(&upa);
+				print_merge_progress_stats(&upa);
 				goto done;
 			}
 		} else {
@@ -10272,7 +10307,7 @@ cmd_histedit(int argc, char *argv[])
 				got_worktree_histedit_abort(worktree, fileindex,
 				    repo, branch, base_commit_id,
 				    update_progress, &upa);
-				print_update_progress_stats(&upa);
+				print_merge_progress_stats(&upa);
 				goto done;
 			}
 
@@ -10284,7 +10319,7 @@ cmd_histedit(int argc, char *argv[])
 			got_worktree_histedit_abort(worktree, fileindex,
 			    repo, branch, base_commit_id,
 			    update_progress, &upa);
-			print_update_progress_stats(&upa);
+			print_merge_progress_stats(&upa);
 			goto done;
 		}
 
@@ -10368,7 +10403,7 @@ cmd_histedit(int argc, char *argv[])
 		got_object_commit_close(commit);
 		commit = NULL;
 
-		print_update_progress_stats(&upa);
+		print_merge_progress_stats(&upa);
 		if (upa.conflicts > 0)
 			rebase_status = GOT_STATUS_CONFLICT;
 
@@ -10773,7 +10808,7 @@ cmd_merge(int argc, char *argv[])
 		    check_cancelled, NULL);
 		if (error)
 			goto done;
-		print_update_progress_stats(&upa);
+		print_merge_progress_stats(&upa);
 		if (!upa.did_something) {
 			error = got_worktree_merge_abort(worktree, fileindex,
 			    repo, update_progress, &upa);
@@ -10789,24 +10824,26 @@ cmd_merge(int argc, char *argv[])
 		if (error)
 			goto done;
 		printf("Merge of %s interrupted on request\n", branch_name);
-	} else if (upa.conflicts > 0 || upa.missing > 0) {
+	} else if (upa.conflicts > 0 || upa.missing > 0 ||
+	    upa.not_deleted > 0 || upa.unversioned > 0) {
 		error = got_worktree_merge_postpone(worktree, fileindex);
 		if (error)
 			goto done;
-		if (upa.conflicts > 0 && upa.missing == 0) {
+		if (upa.conflicts > 0 && upa.missing == 0 &&
+		    upa.not_deleted == 0 && upa.unversioned == 0) {
 			error = got_error_msg(GOT_ERR_CONFLICTS,
 			    "conflicts must be resolved before merging "
 			    "can continue");
 		} else if (upa.conflicts > 0) {
 			error = got_error_msg(GOT_ERR_CONFLICTS,
 			    "conflicts must be resolved before merging "
-			    "can continue; changes destined for missing "
+			    "can continue; changes destined for some "
 			    "files were not yet merged and "
 			    "should be merged manually if required before the "
 			    "merge operation is continued");
 		} else {
 			error = got_error_msg(GOT_ERR_CONFLICTS,
-			    "changes destined for missing "
+			    "changes destined for some "
 			    "files were not yet merged and should be "
 			    "merged manually if required before the "
 			    "merge operation is continued");
@@ -11097,7 +11134,7 @@ cmd_unstage(int argc, char *argv[])
 	error = got_worktree_unstage(worktree, &paths, update_progress,
 	    &upa, pflag ? choose_patch : NULL, &cpa, repo);
 	if (!error)
-		print_update_progress_stats(&upa);
+		print_merge_progress_stats(&upa);
 done:
 	if (patch_script_file && fclose(patch_script_file) == EOF &&
 	    error == NULL)
blob - 219e1fb124feebc4ea8b077b5fbe2cc78e072f34
blob + 548dac7cec6a0bef830836a1220db040da604b7e
--- regress/cmdline/backout.sh
+++ regress/cmdline/backout.sh
@@ -117,6 +117,9 @@ test_backout_edits_for_file_since_deleted() {
 
 	echo "!  alpha" > $testroot/stdout.expected
 	echo "Backed out commit $bad_commit" >> $testroot/stdout.expected
+	echo -n "Files which had incoming changes but could not be found " \
+		>> $testroot/stdout.expected
+	echo "in the work tree: 1" >> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
 	if [ "$ret" != "0" ]; then
@@ -168,6 +171,9 @@ test_backout_next_commit() {
 	echo "G  epsilon/zeta" >> $testroot/stdout.expected
 	echo "!  new" >> $testroot/stdout.expected
 	echo "Backed out commit $bad_commit" >> $testroot/stdout.expected
+	echo -n "Files which had incoming changes but could not be found " \
+		>> $testroot/stdout.expected
+	echo "in the work tree: 1" >> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
 	if [ "$ret" != "0" ]; then
blob - 1ac117d1cc3d254e8d0fcb2c29016b27ed7bb4d9
blob + e75e24e3911844ab65c99352267aa5df1c5a4f3d
--- regress/cmdline/cherrypick.sh
+++ regress/cmdline/cherrypick.sh
@@ -708,6 +708,12 @@ test_cherrypick_symlink_conflicts() {
 	echo "C  new.link" >> $testroot/stdout.expected
 	echo "Merged commit $commit_id2" >> $testroot/stdout.expected
 	echo "Files with new merge conflicts: 6" >> $testroot/stdout.expected
+	echo -n "Files which had incoming changes but could not be found " \
+		>> $testroot/stdout.expected
+	echo "in the work tree: 1" >> $testroot/stdout.expected
+	echo -n "Files not merged because an unversioned file was found in " \
+		>> $testroot/stdout.expected
+	echo "the work tree: 1" >> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
 	if [ "$ret" != "0" ]; then
@@ -1406,6 +1412,9 @@ test_cherrypick_same_branch() {
 	echo "!  beta" >> $testroot/stdout.expected
 	echo "G  epsilon/new" >> $testroot/stdout.expected
 	echo "Merged commit $branch_rev" >> $testroot/stdout.expected
+	echo -n "Files which had incoming changes but could not be found " \
+		>> $testroot/stdout.expected
+	echo "in the work tree: 1" >> $testroot/stdout.expected
 
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
blob - d4e0cb2db7f4ccaf1b56d880da1adfd94b2707d6
blob + be7e58787cda7e3de992a5142542bebcf76ab385
--- regress/cmdline/merge.sh
+++ regress/cmdline/merge.sh
@@ -986,6 +986,9 @@ test_merge_missing_file() {
 
 	echo "!  alpha" > $testroot/stdout.expected
 	echo "G  gamma/delta" >> $testroot/stdout.expected
+	echo -n "Files which had incoming changes but could not be found " \
+		>> $testroot/stdout.expected
+	echo "in the work tree: 1" >> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
 	if [ "$ret" != "0" ]; then
@@ -994,7 +997,7 @@ test_merge_missing_file() {
 		return 1
 	fi
 
-	echo -n "got: changes destined for missing files " \
+	echo -n "got: changes destined for some files " \
 		> $testroot/stderr.expected
 	echo -n "were not yet merged and should be merged manually if " \
 		>> $testroot/stderr.expected
blob - a3608c60299d965c964b1a34f3b87ad35addc9c6
blob + eafa4a62215e1fb630f5480357bafc155aa31309
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -1212,10 +1212,14 @@ test_rebase_delete_missing_file() {
 	echo ": committing to delta on newbranch" >> $testroot/stdout.expected
 	echo "!  beta" >> $testroot/stdout.expected
 	echo "!  d/f/g/new" >> $testroot/stdout.expected
+	echo -n "Files which had incoming changes but could not be found " \
+		>> $testroot/stdout.expected
+	echo "in the work tree: 2" >> $testroot/stdout.expected
 	echo -n "$short_orig_commit2 -> no-op change" \
 		>> $testroot/stdout.expected
 	echo ": removing beta and d/f/g/new on newbranch" \
 		>> $testroot/stdout.expected
+		>> $testroot/stdout.expected
 	echo "Switching work tree to refs/heads/newbranch" \
 		>> $testroot/stdout.expected