Hi Laurent, On 2020-05-18 11:40:21+0200, Laurent Arnoud <laurent@xxxxxxxxxx> wrote: > The `diff.relative` boolean option set to `true` show only changes on > the current directory and show relative pathnames to the current > directory. > > Teach --no-relative to override earlier --relative > > Signed-off-by: Laurent Arnoud <laurent@xxxxxxxxxx> Thanks for addressing my comment. Sorry for detecting this late. Since I usually only look into end of mail-archive to see any interesting topic. And sorry for this late email, I want to run full test before replying with this. I've just seen this: https://lore.kernel.org/git/xmqq1rnk923o.fsf@xxxxxxxxxxxxxxxxxxxxxx/ I've written some test and concluded that we'll need this fix-up to make sure git-format-patch(1) doesn't generate broken patch: ----------------8<---------------- builtin/log.c | 1 + t/t4045-diff-relative.sh | 11 ++++++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index d104d5c688..5949a4883e 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1744,6 +1744,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) rev.diff = 1; rev.max_parents = 1; rev.diffopt.flags.recursive = 1; + rev.diffopt.flags.relative_name = 0; rev.subject_prefix = fmt_patch_subject_prefix; memset(&s_r_opt, 0, sizeof(s_r_opt)); s_r_opt.def = "HEAD"; diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh index ac264ccc2a..a73b104d66 100755 --- a/t/t4045-diff-relative.sh +++ b/t/t4045-diff-relative.sh @@ -11,7 +11,8 @@ test_expect_success 'setup' ' blob_file1=$(git hash-object file1) && blob_file2=$(git hash-object subdir/file2) && git add . && - git commit -m one + git commit -m one && + git format-patch -1 --stdout >expect.patch ' check_diff () { @@ -107,7 +108,9 @@ check_diff_relative_option () { test_expect_success "config diff.relative $relative_opt -p $*" " test_config -C $dir diff.relative $relative_opt && git -C '$dir' diff -p $* HEAD^ >actual && - test_cmp expected actual + test_cmp expected actual && + git -C '$dir' format-patch -1 --stdout >actual.patch && + test_cmp expect.patch actual.patch " } @@ -140,7 +143,9 @@ check_diff_no_relative_option () { test_config -C $dir diff.relative $relative_opt && git -C '$dir' diff -p $* HEAD^ >actual && git -C '$dir' diff -p $* HEAD^ >/tmp/actual && - test_cmp expected actual + test_cmp expected actual && + git -C '$dir' format-patch -1 --stdout >actual.patch && + test_cmp expect.patch actual.patch " } --------------------8<------------------------- > @@ -4558,6 +4563,8 @@ void repo_diff_setup(struct repository *r, struct diff_options *options) > options->b_prefix = "b/"; > } > > + options->flags.relative_name = diff_relative; > + Nitpick: I don't think this option is too special to add a newline to separate it from the rest :) Sorry about not seeing this earlier, I'm a very careless person. Anyway, I think (just a matter of my _personal_ preference), it's better to move it up 21 lines, together with: options->flags.rename_empty = 1; > diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh > index 258808708e..ac264ccc2a 100755 > --- a/t/t4045-diff-relative.sh > +++ b/t/t4045-diff-relative.sh > @@ -8,7 +8,8 @@ test_expect_success 'setup' ' > echo content >file1 && > mkdir subdir && > echo other content >subdir/file2 && > - blob=$(git hash-object subdir/file2) && > + blob_file1=$(git hash-object file1) && > + blob_file2=$(git hash-object subdir/file2) && This rename from blob to blob_file2 is a noise to this patch. Not sure if we should make a preparatory patch to rename, though. *I* would say yes, and another patch to move all git-related code into test_expect_* family. Then, all new testing code for git in this patch should be placed inside test_expect_*, too. I think it's better to wait for other's opinions :) > @@ -86,4 +87,80 @@ do > check_$type . dir/file2 --relative=sub > done > > + diff --git a/$expect b/$expect > + new file mode 100644 > + index 0000000..$short_blob_file2 > + --- /dev/null > + +++ b/$expect > + @@ -0,0 +1 @@ > + +other content > + EOF > + test_expect_success "config diff.relative $relative_opt -p $*" " > + test_config -C $dir diff.relative $relative_opt && > + git -C '$dir' diff -p $* HEAD^ >actual && > + git -C '$dir' diff -p $* HEAD^ >/tmp/actual && Please this leftover from debugging. -- Danh
