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

From:
Stefan Sperling <stsp@stsp.name>
Subject:
allow bad symlinks to survive a merge
To:
gameoftrees@openbsd.org
Date:
Sat, 25 Sep 2021 01:16:41 +0200

Download raw body.

Thread 
At present, got merge and other commands which use the file merge
code (rebase/histedit/cherrypick/backout) convert bad symlinks
into regular files in the work tree. When this gets committed the
bad symlink is converted into a regular file in the repository.

I think we should allow such symlinks to survive a merge as symlinks.
They are already in the repository so either 'got commit -S' or Git were
used to create such a link. In the latter case, people who use Git and Got
on the same project might be surprised to learn that Got is always replacing
such symlinks with regular files during merges.

With this patch bad symlinks are preserved in resulting commits.
For 'got merge' this happens automatically. For 'got cherrypick' the
subsequent commit command needs to use the '-S' option to succeed.

I am only testing 'got merge' and 'got cherrypick' for now.
Test for other affected commands do not yet cover bad symlinks at all.

ok?

diff f6764181d7e0fc68673b90cbc93d6064509d0bd7 /home/stsp/src/got
blob - 8699471c6069c7d4bc2a233ca24d70202cb6d22b
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -1372,7 +1372,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;
@@ -1417,7 +1418,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,
@@ -2081,8 +2082,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,
@@ -2859,6 +2860,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 *
@@ -3103,7 +3105,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,
@@ -3266,6 +3269,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)
@@ -4782,7 +4786,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) {
@@ -4796,7 +4800,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 - fb48a56bcaee4c1b91848964e57ec9ebfcfef84d
file + regress/cmdline/cherrypick.sh
--- 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 -sfh 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 -sfh ../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
file + regress/cmdline/merge.sh
--- 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,15 +325,49 @@ 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
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
 	test_done "$testroot" "$ret"
 }