Đoàn Trần Công Danh <congdanhqx@xxxxxxxxx> writes: >> @@ -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; Sounds like a reasonable improvement of readability. >> 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 personally do not mind this one. It is crystal clear from the patch text: "We used to use only one and managed to get away without blob1/blob2 but now we use more than 1, so let's use names with number suffix". On the other hand, a "preparatory patch" that renames blob to blob_file1 before we need the second one is a noise. > *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. The latter clean-up to make sure we won't notice Git failure outside test_expect_* block may make sense, but I do not know if we want to make it a preparatory clean-up or "remember to do so later when the dust settles". If this single-patch topic needs to touch only a small part of the existing test to do its job, and such a clean-up ends up touching far wider parts of the script, then I would say we can do so as a post-patch clean-up, not as a part of the topic. > > 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. Thanks for a careful review, again.
