make worktree diffs record xbit of new files

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

2022-09-21 12:22 Mark Jamsek:
make worktree diffs record xbit of new files
- 2022-09-22 07:40 Mark Jamsek:
  make worktree diffs record xbit of new files
- - 2022-09-22 14:20 Mark Jamsek:
    make worktree diffs record xbit of new files
  - - 2022-09-22 14:45 Stefan Sperling:
      make worktree diffs record xbit of new files