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

From:
Stefan Sperling <stsp@stsp.name>
Subject:
replacing symlinks with files and vice versa
To:
gameoftrees@openbsd.org
Date:
Fri, 6 Nov 2020 23:05:41 +0100

Download raw body.

Thread
  • Stefan Sperling:

    replacing symlinks with files and vice versa

jrick sent me a patch for test which shows that 'got integrate' fails if
the branch being integrated replaces a symlink with a regular file.
This test was committed in 9314b9f4.

The patch below makes it possible to integrate such a branch.
I have also added a another test case which verifies that the inverse
also works, i.e. a symlink being replaced with a regular file.

There are two unrelated changes in this diff:

1) While debugging the issue I noticed that got commit never clears file
type information for staged commitables. This is being fixed here as
well though I would commit this seperately. It's not a real problem
since the staged filetype is only evaluated if the staged flag is set.
But partial staged info in meta-data looks weird.

2) Fix missing unlink(tmppath) in error cases of install_blob().

ok?

diff 9314b9f4a55e5ebd1a98fa58bcd4e429e8b46163 /home/stsp/src/got
blob - b055ee4a9241e2cbc52608a69d97a87cd1161bb8
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -1496,7 +1496,8 @@ install_blob(struct got_worktree *worktree, const char
 				    GOT_STATUS_UNVERSIONED, path);
 				goto done;
 			}
-			if (!S_ISREG(st_mode) && !installing_bad_symlink) {
+			if (!(S_ISLNK(st_mode) && S_ISREG(te_mode)) &&
+			    !S_ISREG(st_mode) && !installing_bad_symlink) {
 				/* TODO file is obstructed; do something */
 				err = got_error_path(ondisk_path,
 				    GOT_ERR_FILE_OBSTRUCTED);
@@ -1558,17 +1559,24 @@ install_blob(struct got_worktree *worktree, const char
 	}
 
 	if (update) {
+		if (S_ISLNK(st_mode) && unlink(ondisk_path) == -1) {
+			err = got_error_from_errno2("unlink", ondisk_path);
+			goto done;
+		}
 		if (rename(tmppath, ondisk_path) != 0) {
 			err = got_error_from_errno3("rename", tmppath,
 			    ondisk_path);
-			unlink(tmppath);
 			goto done;
 		}
+		free(tmppath);
+		tmppath = NULL;
 	}
 
 done:
 	if (fd != -1 && close(fd) != 0 && err == NULL)
 		err = got_error_from_errno("close");
+	if (tmppath != NULL && unlink(tmppath) == -1 && err == NULL)
+		err = got_error_from_errno2("unlink", tmppath);
 	free(tmppath);
 	return err;
 }
@@ -5287,6 +5295,7 @@ update_fileindex_after_commit(struct got_pathlist_head
 			    ct->staged_status == GOT_STATUS_MODIFY) {
 				got_fileindex_entry_stage_set(ie,
 				    GOT_FILEIDX_STAGE_NONE);
+				got_fileindex_entry_staged_filetype_set(ie, 0);
 				err = got_fileindex_entry_update(ie,
 				    ct->ondisk_path, ct->staged_blob_id->sha1,
 				    new_base_commit_id->sha1,
@@ -7602,8 +7611,10 @@ unstage_hunks(struct got_object_id *staged_blob_id,
 	if (new_staged_blob_id) {
 		memcpy(ie->staged_blob_sha1, new_staged_blob_id->sha1,
 		    SHA1_DIGEST_LENGTH);
-	} else
+	} else {
 		got_fileindex_entry_stage_set(ie, GOT_FILEIDX_STAGE_NONE);
+		got_fileindex_entry_staged_filetype_set(ie, 0);
+	}
 done:
 	free(new_staged_blob_id);
 	if (path_unstaged_content &&
@@ -7724,9 +7735,11 @@ unstage_path(void *arg, unsigned char status,
 			err = got_error_path(relpath, GOT_ERR_BAD_FILETYPE);
 			break;
 		}
-		if (err == NULL)
+		if (err == NULL) {
 			got_fileindex_entry_stage_set(ie,
 			    GOT_FILEIDX_STAGE_NONE);
+			got_fileindex_entry_staged_filetype_set(ie, 0);
+		}
 		break;
 	case GOT_STATUS_DELETE:
 		if (a->patch_cb) {
@@ -7743,6 +7756,7 @@ unstage_path(void *arg, unsigned char status,
 			}
 		}
 		got_fileindex_entry_stage_set(ie, GOT_FILEIDX_STAGE_NONE);
+		got_fileindex_entry_staged_filetype_set(ie, 0);
 		err = get_file_status(&status, &sb, ie, ondisk_path,
 		    dirfd, de_name, a->repo);
 		if (err)
blob - c347b9ec6713d7f7eb5960370b109310af7802ba
file + regress/cmdline/integrate.sh
--- regress/cmdline/integrate.sh
+++ regress/cmdline/integrate.sh
@@ -385,8 +385,8 @@ test_integrate_backwards_in_time() {
 	test_done "$testroot" "$ret"
 }
 
-test_integrate_obstructed_symlink() {
-	local testroot=`test_init update_replace_symlink`
+test_integrate_replace_symlink_with_file() {
+	local testroot=`test_init integrate_replace_symlink_with_file`
 
 	got checkout $testroot/repo $testroot/wt > /dev/null
 	ret="$?"
@@ -401,29 +401,98 @@ test_integrate_obstructed_symlink() {
 	(cd $testroot/wt && got commit -m "add regular file and symlink" \
 		>/dev/null)
 
-	(cd $testroot/wt && got br replace_symlink >/dev/null)
+	(cd $testroot/wt && got br replace_symlink_with_file >/dev/null)
 	(cd $testroot/wt && rm alpha.link >/dev/null)
 	(cd $testroot/wt && cp alpha alpha.link)
 	(cd $testroot/wt && got stage alpha.link >/dev/null)
 	(cd $testroot/wt && got commit -m "replace symlink" >/dev/null)
 
 	(cd $testroot/wt && got up -b master >/dev/null)
-	(cd $testroot/wt && got integrate replace_symlink \
-		2> $testroot/stderr)
+	(cd $testroot/wt && got integrate replace_symlink_with_file \
+		> $testroot/stdout)
 
-	echo "got: $testroot/wt/alpha.link: file is obstructed" \
-		> $testroot/stderr.expected
-	cmp -s $testroot/stderr.expected $testroot/stderr
+	echo "U  alpha.link" > $testroot/stdout.expected
+	echo -n "Integrated refs/heads/replace_symlink_with_file " \
+		>> $testroot/stdout.expected
+	echo "into refs/heads/master" >> $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
 	if [ "$ret" != "0" ]; then
-		diff -u $testroot/stderr.expected $testroot/stderr
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
 	fi
+
+	if [ -h $testroot/wt/alpha.link ]; then
+		echo "alpha.link is still a symlink"
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	echo "alpha" > $testroot/content.expected
+	cat $testroot/wt/alpha.link > $testroot/content
+
+	cmp -s $testroot/content.expected $testroot/content
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/content.expected $testroot/content
+	fi
 	test_done "$testroot" "$ret"
 }
 
+test_integrate_replace_file_with_symlink() {
+	local testroot=`test_init integrate_replace_file_with_symlink`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		echo "checkout failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got br replace_file_with_symlink >/dev/null)
+	(cd $testroot/wt && rm alpha)
+	(cd $testroot/wt && ln -s beta alpha)
+	(cd $testroot/wt && got commit -m "replace regular file with symlink" \
+		>/dev/null)
+
+	(cd $testroot/wt && got up -b master >/dev/null)
+	(cd $testroot/wt && got integrate replace_file_with_symlink \
+		> $testroot/stdout)
+
+	echo "U  alpha" > $testroot/stdout.expected
+	echo -n "Integrated refs/heads/replace_file_with_symlink " \
+		>> $testroot/stdout.expected
+	echo "into refs/heads/master" >> $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/alpha ]; then
+		echo "alpha is not a symlink"
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	readlink $testroot/wt/alpha > $testroot/stdout
+	echo "beta" > $testroot/stdout.expected
+	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_parseargs "$@"
 run_test test_integrate_basic
 run_test test_integrate_requires_rebase_first
 run_test test_integrate_path_prefix
 run_test test_integrate_backwards_in_time
-run_test test_integrate_obstructed_symlink
+run_test test_integrate_replace_symlink_with_file
+run_test test_integrate_replace_file_with_symlink