From: Mark Jamsek <mark@jamsek.com>
Subject: Re: make worktree diffs record xbit of new files
To: Game of Trees <gameoftrees@openbsd.org>
Date: Fri, 23 Sep 2022 00:20:26 +1000

On 22-09-22 05:40PM, Mark Jamsek wrote:
> On 22-09-21 10:22PM, Mark Jamsek wrote:
> > Related to the recent work: when running `got diff` in a worktree with
> > a file that has the x-bit and has been added with `got add`, we don't
> > presently record this mode like we do with `got diff obj1 obj2` or
> > `got diff -c obj`.
> > 
> > The below change makes `got diff` in a worktree record this information
> > like other diff modes.
> 
> Actually, there's more to consider;
> ...
> What do we actually want to do here? It seems at present we only record
> the file mode for newly added files with all `got diff` formats _except_
> for worktree diffs. I think we should be consistent and show the same
> info in all `got diff` modes.  Otherwise, the proposed diff should be
> modified to only show the file mode in worktree diffs for new
> files--like we already do for our other diffs.
> 
> I think at least one of the above is needed, however, because at
> present, a worktree diff of a new added file with +x doesn't contain
> that information. So `got patch` will fail to set the x-bit.
> 
> In sum:
> 
>   - `got diff obj obj` and `got diff -c obj [-c obj]` show the file mode
>     for _new_ +x files; however, this is not shown for changed files
>   - `got diff` in a work tree does not show this information for neither
>     new nor changed files
>   - all versions of `git diff` show the file mode for both new and
>     changed files

After thinking about this more, I think it makes sense to make `got
diff` in a worktree display the same information as `got diff obj1 obj2`
and `got diff -c obj [-c obj]` and only show the file mode for new added
files. This enables `got patch` to successfully apply patches made with
`got diff` of changes in the worktree. I think it's safe to assume that
there's enough information on disk to successfully apply patches
involving managed files, and showing the file mode all the time is
overkill--we don't currently do this so why start now.

The following diff makes this change. I've also tentatively updated diff
regress tests in a separate diff (at the end of this mail) so you can
run the existing tests first to easily see the change this diff makes.

-----------------------------------------------
commit 3bdd394922c72b9006b302e441f6fbcd211791c9
from: Mark Jamsek <mark@jamsek.dev>
date: Thu Sep 22 14:10:18 2022 UTC
 
 show file mode for new added files in worktree diffs
 
diff 611e5fc2074d428e17f920dc595496af4dd0dc77 3bdd394922c72b9006b302e441f6fbcd211791c9
commit - 611e5fc2074d428e17f920dc595496af4dd0dc77
commit + 3bdd394922c72b9006b302e441f6fbcd211791c9
blob - 7c95dd13a7945672200a10ba9c507d183e842406
blob + ecd8277b6cdd19cdad9bcbf64a4136745c824888
--- got/got.c
+++ got/got.c
@@ -4717,8 +4717,10 @@ print_diff(void *arg, unsigned char status, unsigned c
 	char *abspath = NULL, *label1 = NULL;
 	struct stat sb;
 	off_t size1 = 0;
-	int f2_exists = 1;
+	int f2_exists = 0;
 
+	memset(&sb, 0, sizeof(sb));
+
 	if (a->diff_staged) {
 		if (staged_status != GOT_STATUS_MODIFY &&
 		    staged_status != GOT_STATUS_ADD &&
@@ -4850,8 +4852,8 @@ print_diff(void *arg, unsigned char status, unsigned c
 					goto done;
 			}
 		}
-		if (fstat(fd, &sb) == -1) {
-			err = got_error_from_errno2("fstat", abspath);
+		if (fstatat(fd, abspath, &sb, AT_SYMLINK_NOFOLLOW) == -1) {
+			err = got_error_from_errno2("lstat", abspath);
 			goto done;
 		}
 		f2 = fdopen(fd, "r");
@@ -4860,9 +4862,7 @@ print_diff(void *arg, unsigned char status, unsigned c
 			goto done;
 		}
 		fd = -1;
-	} else {
-		sb.st_size = 0;
-		f2_exists = 0;
+		f2_exists = 1;
 	}
 
 	if (blob1) {
@@ -4873,8 +4873,8 @@ print_diff(void *arg, unsigned char status, unsigned c
 	}
 
 	err = got_diff_blob_file(blob1, a->f1, size1, label1, f2 ? f2 : a->f2,
-	    f2_exists, sb.st_size, path, GOT_DIFF_ALGORITHM_PATIENCE,
-	    a->diff_context, a->ignore_whitespace, a->force_text_diff, stdout);
+	    f2_exists, &sb, path, GOT_DIFF_ALGORITHM_PATIENCE, a->diff_context,
+	    a->ignore_whitespace, a->force_text_diff, stdout);
 done:
 	if (fd1 != -1 && close(fd1) == -1 && err == NULL)
 		err = got_error_from_errno("close");
blob - bc4bc871ee90bf32c4fd2bd3b2b452f29d5822a0
blob + 26617c2f087a790bbb5f19d32d57f83246fcb532
--- include/got_diff.h
+++ include/got_diff.h
@@ -74,7 +74,7 @@ const struct got_error *got_diff_blob(struct got_diff_
  * Whitespace differences may optionally be ignored.
  */
 const struct got_error *got_diff_blob_file(struct got_blob_object *, FILE *,
-    off_t, const char *, FILE *, int, size_t, const char *,
+    off_t, const char *, FILE *, int, struct stat *, const char *,
     enum got_diff_algorithm, int, int, int, FILE *);
 
 /*
blob - ec271a03249db4f54f213d8fd71a18c6220153ef
blob + 58baf2baa359d6bb49f1d7c1b7bd6af3f15bbb36
--- lib/diff.c
+++ lib/diff.c
@@ -221,7 +221,7 @@ got_diff_blob(struct got_diff_line **lines, size_t*nli
 static const struct got_error *
 diff_blob_file(struct got_diffreg_result **resultp,
     struct got_blob_object *blob1, FILE *f1, off_t size1, const char *label1,
-    FILE *f2, int f2_exists, size_t size2, const char *label2,
+    FILE *f2, int f2_exists, struct stat *sb2, const char *label2,
     enum got_diff_algorithm diff_algo, int diff_context, int ignore_whitespace,
     int force_text_diff, FILE *outfile)
 {
@@ -239,9 +239,22 @@ diff_blob_file(struct got_diffreg_result **resultp,
 		idstr1 = "/dev/null";
 
 	if (outfile) {
+		char	*mode = NULL;
+
+		/* display file mode for new added files only */
+		if (f2_exists && blob1 == NULL) {
+			int mmask = (S_IRWXU | S_IRWXG | S_IRWXO);
+
+			if (S_ISLNK(sb2->st_mode))
+				mmask = S_IFLNK;
+			if (asprintf(&mode, " (mode %o)",
+			    sb2->st_mode & mmask) == -1)
+				return got_error_from_errno("asprintf");
+		}
 		fprintf(outfile, "blob - %s\n", label1 ? label1 : idstr1);
-		fprintf(outfile, "file + %s\n",
-		    f2_exists ? label2 : "/dev/null");
+		fprintf(outfile, "file + %s%s\n",
+		    f2_exists ? label2 : "/dev/null", mode ? mode : "");
+		free(mode);
 	}
 
 	err = got_diffreg(&result, f1, f2, diff_algo, ignore_whitespace,
@@ -272,13 +285,13 @@ done:
 
 const struct got_error *
 got_diff_blob_file(struct got_blob_object *blob1, FILE *f1, off_t size1,
-    const char *label1, FILE *f2, int f2_exists, size_t size2,
+    const char *label1, FILE *f2, int f2_exists, struct stat *sb2,
     const char *label2, enum got_diff_algorithm diff_algo, int diff_context,
     int ignore_whitespace, int force_text_diff, FILE *outfile)
 {
 	return diff_blob_file(NULL, blob1, f1, size1, label1, f2, f2_exists,
-	    size2, label2, diff_algo, diff_context, ignore_whitespace,
-	    force_text_diff, outfile );
+	    sb2, label2, diff_algo, diff_context, ignore_whitespace,
+	    force_text_diff, outfile);
 }
 
 static const struct got_error *
blob - b01acc2d4192df91e119abcea319ac890c164de0
blob + becf27d3e00feb1fd2b32823931dc0d1b580cfb6
--- libexec/got-read-patch/got-read-patch.c
+++ libexec/got-read-patch/got-read-patch.c
@@ -231,7 +231,8 @@ find_diff(int *done, int *next, FILE *fp, int git, con
 		} else if (!strncmp(line, "+++ ", 4)) {
 			free(new);
 			err = filename(line+4, &new);
-		} else if (!strncmp(line, "blob + ", 7)) {
+		} else if (!strncmp(line, "blob + ", 7) ||
+		    !strncmp(line, "file + ", 7)) {
 			xbit = filexbit(line);
 		} else if (!git && !strncmp(line, "blob - ", 7)) {
 			free(blob);
blob - 43f41172dfa14e7ca103f635f10f4b25a6f11cf8
blob + 33d639ca47bfa61715eeea8b3f15c073985d8d05
--- regress/cmdline/diff.sh
+++ regress/cmdline/diff.sh
@@ -1327,6 +1327,53 @@ EOF
 	test_done "$testroot" $ret
 }
 
+test_diff_worktree_newfile_xbit() {
+	local testroot=`test_init diff_worktree_newfile_xbit`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	echo xfile > $testroot/wt/xfile
+	chmod +x $testroot/wt/xfile
+	(cd $testroot/wt && got add xfile) > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+	(cd $testroot/wt && got diff) > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	local commit_id=`git_show_head $testroot/repo`
+	cat <<EOF > $testroot/stdout.expected
+diff $testroot/wt
+commit - $commit_id
+path + $testroot/wt
+blob - /dev/null
+file + xfile (mode 755)
+--- /dev/null
++++ xfile
+@@ -0,0 +1 @@
++xfile
+EOF
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "failed to record mode 755"
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" $ret
+}
+
 test_parseargs "$@"
 run_test test_diff_basic
 run_test test_diff_shows_conflict
@@ -1340,3 +1387,4 @@ run_test test_diff_binary_files
 run_test test_diff_commits
 run_test test_diff_ignored_file
 run_test test_diff_crlf
+run_test test_diff_worktree_newfile_xbit


Regress tests for above changes:

diff 3bdd394922c72b9006b302e441f6fbcd211791c9 98df6478da8fc43c30fea8901c047f20180daa07
commit - 3bdd394922c72b9006b302e441f6fbcd211791c9
commit + 98df6478da8fc43c30fea8901c047f20180daa07
blob - 33d639ca47bfa61715eeea8b3f15c073985d8d05
blob + be07e3211fa6b1491159fb5458fdf2f901864d68
--- regress/cmdline/diff.sh
+++ regress/cmdline/diff.sh
@@ -54,7 +54,7 @@ test_diff_basic() {
 	echo '@@ -1 +0,0 @@' >> $testroot/stdout.expected
 	echo '-beta' >> $testroot/stdout.expected
 	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + new' >> $testroot/stdout.expected
+	echo 'file + new (mode 644)' >> $testroot/stdout.expected
 	echo '--- /dev/null' >> $testroot/stdout.expected
 	echo '+++ new' >> $testroot/stdout.expected
 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
@@ -148,7 +148,7 @@ test_diff_basic() {
 	echo '-zeta' >> $testroot/stdout.expected
 	echo '+modified zeta' >> $testroot/stdout.expected
 	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + new' >> $testroot/stdout.expected
+	echo 'file + new (mode 644)' >> $testroot/stdout.expected
 	echo '--- /dev/null' >> $testroot/stdout.expected
 	echo '+++ new' >> $testroot/stdout.expected
 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
@@ -269,13 +269,13 @@ test_diff_basic() {
 	echo "commit - $head_rev" >> $testroot/stdout.expected
 	echo "path + $testroot/wt" >> $testroot/stdout.expected
 	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + master' >> $testroot/stdout.expected
+	echo 'file + master (mode 644)' >> $testroot/stdout.expected
 	echo '--- /dev/null' >> $testroot/stdout.expected
 	echo '+++ master' >> $testroot/stdout.expected
 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
 	echo '+master' >> $testroot/stdout.expected
 	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + new' >> $testroot/stdout.expected
+	echo 'file + new (mode 644)' >> $testroot/stdout.expected
 	echo '--- /dev/null' >> $testroot/stdout.expected
 	echo '+++ new' >> $testroot/stdout.expected
 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
@@ -311,7 +311,7 @@ test_diff_basic() {
 	echo "commit - $head_rev" >> $testroot/stdout.expected
 	echo "path + $testroot/wt" >> $testroot/stdout.expected
 	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + new' >> $testroot/stdout.expected
+	echo 'file + new (mode 644)' >> $testroot/stdout.expected
 	echo '--- /dev/null' >> $testroot/stdout.expected
 	echo '+++ new' >> $testroot/stdout.expected
 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
@@ -365,7 +365,7 @@ test_diff_basic() {
 	echo "commit - $head_rev" >> $testroot/stdout.expected
 	echo "path + $testroot/wt" >> $testroot/stdout.expected
 	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + new' >> $testroot/stdout.expected
+	echo 'file + new (mode 644)' >> $testroot/stdout.expected
 	echo '--- /dev/null' >> $testroot/stdout.expected
 	echo '+++ new' >> $testroot/stdout.expected
 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
@@ -798,7 +798,7 @@ test_diff_symlinks_in_work_tree() {
 	echo '-nonexistent' >> $testroot/stdout.expected
 	echo '\ No newline at end of file' >> $testroot/stdout.expected
 	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + zeta.link' >> $testroot/stdout.expected
+	echo 'file + zeta.link (mode 120000)' >> $testroot/stdout.expected
 	echo '--- /dev/null' >> $testroot/stdout.expected
 	echo '+++ zeta.link' >> $testroot/stdout.expected
 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
@@ -950,7 +950,7 @@ test_diff_binary_files() {
 	echo "commit - $head_rev" >> $testroot/stdout.expected
 	echo "path + $testroot/wt" >> $testroot/stdout.expected
 	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + foo' >> $testroot/stdout.expected
+	echo 'file + foo (mode 644)' >> $testroot/stdout.expected
 	echo "Binary files /dev/null and foo differ" \
 		>> $testroot/stdout.expected
 
@@ -967,7 +967,7 @@ test_diff_binary_files() {
 	echo "commit - $head_rev" >> $testroot/stdout.expected
 	echo "path + $testroot/wt" >> $testroot/stdout.expected
 	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + foo' >> $testroot/stdout.expected
+	echo 'file + foo (mode 644)' >> $testroot/stdout.expected
 	echo '--- /dev/null' >> $testroot/stdout.expected
 	echo '+++ foo' >> $testroot/stdout.expected
 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68