commit ace90326f82adffc32a25213124922899e849771
from: Stefan Sperling <stsp@stsp.name>
via: Thomas Adam <thomas@xteddy.org>
date: Mon Sep 27 06:40:44 2021 UTC

allow bad symlinks to survive a merge

Commands which perform merges will now install bad symlinks as symlinks
in the work tree, instead of creating them as regular files. This means
bad symlinks committed with 'got commit -S' (or Git) will be preserved.
The decision to introduce a bad symlink is done at commit-time and
merges should not forcefully reverse this decision.

The cherrypick and backout commands require a manual commit step, and
a merge result with bad symlinks will require use of 'got commit -S'.

Additional testing by thomas adam

commit - e8256d916369996c214b5f28673d32f910996708
commit + ace90326f82adffc32a25213124922899e849771
blob - edd1cb8a01ae94e9786f52b9546fd6ab207b9882
blob + acf42c1ab053024ca6b05a987842225c8dd06846
--- lib/worktree.c
+++ lib/worktree.c
@@ -1370,7 +1370,8 @@ static const struct got_error *
 install_symlink(int *is_bad_symlink, struct got_worktree *worktree,
     const char *ondisk_path, const char *path, struct got_blob_object *blob,
     int restoring_missing_file, int reverting_versioned_file,
-    int path_is_unversioned, struct got_repository *repo,
+    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;
@@ -1415,7 +1416,7 @@ install_symlink(int *is_bad_symlink, struct got_worktr
 	if (err)
 		return err;
 
-	if (*is_bad_symlink) {
+	if (*is_bad_symlink && !allow_bad_symlinks) {
 		/* install as a regular file */
 		got_object_blob_rewind(blob);
 		err = install_blob(worktree, ondisk_path, path,
@@ -2079,8 +2080,8 @@ update_blob(struct got_worktree *worktree,
 			err = install_symlink(&is_bad_symlink, worktree,
 			    ondisk_path, path, blob,
 			    status == GOT_STATUS_MISSING, 0,
-			    status == GOT_STATUS_UNVERSIONED, repo,
-			    progress_cb, progress_arg);
+			    status == GOT_STATUS_UNVERSIONED, 0,
+			    repo, progress_cb, progress_arg);
 		} else {
 			err = install_blob(worktree, ondisk_path, path,
 			    te->mode, sb.st_mode, blob,
@@ -2857,6 +2858,7 @@ struct merge_file_cb_arg {
     void *cancel_arg;
     const char *label_orig;
     struct got_object_id *commit_id2;
+    int allow_bad_symlinks;
 };
 
 static const struct got_error *
@@ -3101,7 +3103,8 @@ merge_file_cb(void *arg, struct got_blob_object *blob1
 			if (S_ISLNK(mode2)) {
 				err = install_symlink(&is_bad_symlink,
 				    a->worktree, ondisk_path, path2, blob2, 0,
-				    0, 1, repo, a->progress_cb, a->progress_arg);
+				    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,
@@ -3264,6 +3267,7 @@ merge_files(struct got_worktree *worktree, struct got_
 	arg.cancel_arg = cancel_arg;
 	arg.label_orig = label_orig;
 	arg.commit_id2 = commit_id2;
+	arg.allow_bad_symlinks = 1; /* preserve bad symlinks across merges */
 	err = got_diff_tree(tree1, tree2, "", "", repo, merge_file_cb, &arg, 1);
 	sync_err = sync_fileindex(fileindex, fileindex_path);
 	if (sync_err && err == NULL)
@@ -4780,7 +4784,7 @@ revert_file(void *arg, unsigned char status, unsigned 
 				}
 				err = install_symlink(&is_bad_symlink,
 				    a->worktree, ondisk_path, ie->path,
-				    blob, 0, 1, 0, a->repo,
+				    blob, 0, 1, 0, 0, a->repo,
 				    a->progress_cb, a->progress_arg);
 			} else {
 				if (rename(path_content, ondisk_path) == -1) {
@@ -4794,7 +4798,7 @@ revert_file(void *arg, unsigned char status, unsigned 
 			if (te && S_ISLNK(te->mode)) {
 				err = install_symlink(&is_bad_symlink,
 				    a->worktree, ondisk_path, ie->path,
-				    blob, 0, 1, 0, a->repo,
+				    blob, 0, 1, 0, 0, a->repo,
 				    a->progress_cb, a->progress_arg);
 			} else {
 				err = install_blob(a->worktree, ondisk_path,
blob - 1208cc97c5c0e642c229074f383d6763516e54f6
blob + b83c96c06f1e72e3b6208710f2b7a15bd572ae1f
--- regress/cmdline/cherrypick.sh
+++ regress/cmdline/cherrypick.sh
@@ -448,6 +448,29 @@ test_cherrypick_modified_symlinks() {
 	git_commit $testroot/repo -m "add symlinks"
 	local commit_id1=`git_show_head $testroot/repo`
 
+	got tree -r $testroot/repo -R -c $commit_id1 \
+		> $testroot/stdout
+	cat > $testroot/stdout.expected <<EOF
+alpha
+alpha.link@ -> alpha
+beta
+epsilon/
+epsilon/beta.link@ -> ../beta
+epsilon/zeta
+epsilon.link@ -> epsilon
+gamma/
+gamma/delta
+nonexistent.link@ -> nonexistent
+passwd.link@ -> /etc/passwd
+EOF
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
 	got branch -r $testroot/repo foo
 
 	got checkout -b foo $testroot/repo $testroot/wt > /dev/null
@@ -455,7 +478,7 @@ test_cherrypick_modified_symlinks() {
 	(cd $testroot/repo && ln -sf beta alpha.link)
 	(cd $testroot/repo && ln -sfT gamma epsilon.link)
 	(cd $testroot/repo && ln -sf ../gamma/delta epsilon/beta.link)
-	(cd $testroot/repo && ln -sf .got/bar $testroot/repo/dotgotfoo.link)
+	(cd $testroot/repo && ln -sf .got/foo $testroot/repo/dotgotfoo.link)
 	(cd $testroot/repo && git rm -q nonexistent.link)
 	(cd $testroot/repo && ln -sf epsilon/zeta zeta.link)
 	(cd $testroot/repo && git add .)
@@ -495,6 +518,22 @@ test_cherrypick_modified_symlinks() {
 		return 1
 	fi
 
+	if ! [ -h $testroot/wt/dotgotfoo.link ]; then
+		echo "dotgotfoo.link is not a symlink"
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	readlink $testroot/wt/dotgotfoo.link > $testroot/stdout
+	echo ".got/foo" > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
 	if ! [ -h $testroot/wt/epsilon.link ]; then
 		echo "epsilon.link is not a symlink"
 		test_done "$testroot" "1"
@@ -546,7 +585,58 @@ test_cherrypick_modified_symlinks() {
 		return 1
 	fi
 
-	test_done "$testroot" "0"
+	(cd $testroot/wt && got commit -m 'commit cherrypick result' \
+		> /dev/null 2>$testroot/stderr)
+	ret="$?"
+	if [ "$ret" == "0" ]; then
+		echo "got commit succeeded unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+	echo -n "got: $testroot/wt/dotgotfoo.link: symbolic link points " \
+		> $testroot/stderr.expected
+	echo "outside of paths under version control" \
+		>> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got commit -S -m 'commit cherrypick result' \
+		> /dev/null)
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		echo "got commit failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+	local commit_id2=`git_show_head $testroot/repo`
+
+	got tree -r $testroot/repo -R -c $commit_id2 \
+		> $testroot/stdout
+	cat > $testroot/stdout.expected <<EOF
+alpha
+alpha.link@ -> beta
+beta
+dotgotfoo.link@ -> .got/foo
+epsilon/
+epsilon/beta.link@ -> ../gamma/delta
+epsilon/zeta
+epsilon.link@ -> gamma
+gamma/
+gamma/delta
+passwd.link@ -> /etc/passwd
+zeta.link@ -> epsilon/zeta
+EOF
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" "$ret"
 }
 
 test_cherrypick_symlink_conflicts() {
@@ -585,7 +675,7 @@ test_cherrypick_symlink_conflicts() {
 	# modeified symlink to file A vs modified symlink to dir B
 	(cd $testroot/wt && ln -sfT ../gamma epsilon/beta.link)
 	# added regular file A vs added bad symlink to file A
-	(cd $testroot/wt && ln -sf .got/bar dotgotfoo.link)
+	(cd $testroot/wt && ln -sf .got/foo dotgotfoo.link)
 	(cd $testroot/wt && got add dotgotfoo.link > /dev/null)
 	# added bad symlink to file A vs added regular file A
 	echo 'this is regular file bar' > $testroot/wt/dotgotbar.link
@@ -755,7 +845,7 @@ test_cherrypick_symlink_conflicts() {
 		> $testroot/content.expected
 	echo "this is regular file foo" >> $testroot/content.expected
 	echo "=======" >> $testroot/content.expected
-	echo -n ".got/bar" >> $testroot/content.expected
+	echo -n ".got/foo" >> $testroot/content.expected
 	echo '>>>>>>>' >> $testroot/content.expected
 	cp $testroot/wt/dotgotfoo.link $testroot/content
 	cmp -s $testroot/content.expected $testroot/content
blob - 0e1a94a8ea7156a29ef18ee63b4b1d881a0b6d30
blob + 1c7ea8430ba3f5a85df6f00f7a3587641ae683ff
--- regress/cmdline/merge.sh
+++ regress/cmdline/merge.sh
@@ -39,6 +39,10 @@ test_merge_basic() {
 	(cd $testroot/repo && ln -s alpha symlink && git add symlink)
 	git_commit $testroot/repo -m "adding symlink on newbranch"
 	local branch_commit4=`git_show_branch_head $testroot/repo newbranch`
+	(cd $testroot/repo && ln -sf .got/bar dotgotbar.link)
+	(cd $testroot/repo && git add dotgotbar.link)
+	git_commit $testroot/repo -m "adding a bad symlink on newbranch"
+	local branch_commit5=`git_show_branch_head $testroot/repo newbranch`
 
 	got checkout -b master $testroot/repo $testroot/wt > /dev/null
 	ret="$?"
@@ -216,6 +220,7 @@ test_merge_basic() {
 
 	echo "G  alpha" >> $testroot/stdout.expected
 	echo "D  beta" >> $testroot/stdout.expected
+	echo "A  dotgotbar.link" >> $testroot/stdout.expected
 	echo "A  epsilon/new" >> $testroot/stdout.expected
 	echo "G  gamma/delta" >> $testroot/stdout.expected
 	echo "A  symlink" >> $testroot/stdout.expected
@@ -267,6 +272,12 @@ test_merge_basic() {
 		return 1
 	fi
 
+	if [ ! -h $testroot/wt/dotgotbar.link ]; then
+		echo "dotgotbar.link is not a symlink"
+		test_done "$testroot" "1"
+		return 1
+	fi
+
 	readlink $testroot/wt/symlink > $testroot/stdout
 	echo "alpha" > $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
@@ -302,7 +313,10 @@ test_merge_basic() {
 
 	(cd $testroot/wt && got update > $testroot/stdout)
 
-	echo 'Already up-to-date' > $testroot/stdout.expected
+	echo 'U  dotgotbar.link' > $testroot/stdout.expected
+	echo -n "Updated to refs/heads/master: " >> $testroot/stdout.expected
+	git_show_head $testroot/repo >> $testroot/stdout.expected
+	echo >> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
 	if [ "$ret" != "0" ]; then
@@ -311,10 +325,44 @@ test_merge_basic() {
 		return 1
 	fi
 
+	# update has changed the bad symlink into a regular file
+	if [ -h $testroot/wt/dotgotbar.link ]; then
+		echo "dotgotbar.link is a symlink"
+		test_done "$testroot" "1"
+		return 1
+	fi
+
 	# We should have created a merge commit with two parents.
 	(cd $testroot/wt && got log -l1 | grep ^parent > $testroot/stdout)
 	echo "parent 1: $master_commit" > $testroot/stdout.expected
-	echo "parent 2: $branch_commit4" >> $testroot/stdout.expected
+	echo "parent 2: $branch_commit5" >> $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	got tree -r $testroot/repo -c $merge_commit -R > $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		echo "got tree failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	# bad symlink dotgotbar.link appears as a symlink in the merge commit:
+	cat > $testroot/stdout.expected <<EOF
+alpha
+dotgotbar.link@ -> .got/bar
+epsilon/
+epsilon/new
+epsilon/zeta
+gamma/
+gamma/delta
+symlink@ -> alpha
+EOF
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
 	if [ "$ret" != "0" ]; then