Elijah Newren <newren@xxxxxxxxx> writes: > Hi, > > It helps when providing a new iteration of a patch to do two things: > (1) provide a range-diff against the previous version to make it > easier for reviewers to see what has changed > (2) either reply to the previous submission or link to it in your > header so reviewers can find previous comments Thanks, I'll do that for upcoming v3. > > In this case, v1 was over at > https://lore.kernel.org/git/pull.1606.git.1699010701704.gitgitgadget@xxxxxxxxx/. > > On Tue, Dec 26, 2023 at 12:21 PM Sam James <sam@xxxxxxxxxx> wrote: >> >> This patch adds a config value for 'diff.renames' called 'copies-harder' >> which make it so '-C -C' is in effect always passed for 'git log -p', >> 'git diff', etc. >> >> This allows specifying that 'git log -p', 'git diff', etc should always act >> as if '-C --find-copies-harder' was passed. >> >> It has proven this especially useful for certain types of repository (like >> Gentoo's ebuild repositories) because files are often copies of a previous >> version: >> >> Suppose a directory 'sys-devel/gcc' contains recipes for building >> GCC, with one file for each supported upstream branch: >> gcc-13.x.build.recipe >> gcc-12.x.build.recipe >> gcc-11.x.build.recipe >> gcc-10.x.build.recipe >> >> gcc-13.x.build.recipe was started as a copy of gcc-12.x.build.recipe >> (which was started as a copy of gcc-11.x.build.recipe, etc.). Previous versions >> are kept around to support parallel installation of multiple versions. >> >> Being able to easily observe the diff relative to other recipes within the >> directory has been a quality of life improvement for such repo layouts. >> >> Cc: Elijah Newren <newren@xxxxxxxxx> >> Cc: Junio C Hamano <gitster@xxxxxxxxx> >> Signed-off-by: Sam James <sam@xxxxxxxxxx> > > While the "Cc:" lines don't hurt, note that everything above this > point is included in the commit message, so we'd typically rather > those two lines be below the triple-dashed line. ACK. > >> --- >> v2: Improve the commit message using Elijah Newren's example (it is indeed >> precisely what I was thinking of, but phrased better), and improve the documentation >> to explain better what the new config option actually does & refer to git-diff's >> --find-copies-harder. >> >> Documentation/config/diff.txt | 8 +++++--- >> Documentation/config/status.txt | 3 ++- >> diff.c | 12 +++++++++--- >> diff.h | 1 + >> diffcore-rename.c | 4 ++-- >> merge-ort.c | 2 +- >> merge-recursive.c | 2 +- >> 7 files changed, 21 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt >> index bd5ae0c337..cdd8a74ec0 100644 >> --- a/Documentation/config/diff.txt >> +++ b/Documentation/config/diff.txt >> @@ -131,9 +131,11 @@ diff.renames:: >> Whether and how Git detects renames. If set to "false", >> rename detection is disabled. If set to "true", basic rename >> detection is enabled. If set to "copies" or "copy", Git will >> - detect copies, as well. Defaults to true. Note that this >> - affects only 'git diff' Porcelain like linkgit:git-diff[1] and >> - linkgit:git-log[1], and not lower level commands such as >> + detect copies, as well. If set to "copies-harder", Git will spend extra >> + cycles to find more copies even in unmodified paths, see >> + '--find-copies-harder' in linkgit:git-diff[1]. Defaults to true. >> + Note that this affects only 'git diff' Porcelain like linkgit:git-diff[1] >> + and linkgit:git-log[1], and not lower level commands such as >> linkgit:git-diff-files[1]. >> >> diff.suppressBlankEmpty:: >> diff --git a/Documentation/config/status.txt b/Documentation/config/status.txt >> index 2ff8237f8f..7ca7a4becd 100644 >> --- a/Documentation/config/status.txt >> +++ b/Documentation/config/status.txt >> @@ -33,7 +33,8 @@ status.renames:: >> Whether and how Git detects renames in linkgit:git-status[1] and >> linkgit:git-commit[1] . If set to "false", rename detection is >> disabled. If set to "true", basic rename detection is enabled. >> - If set to "copies" or "copy", Git will detect copies, as well. >> + If set to "copies" or "copy", Git will detect copies, as well. If >> + set to "copies-harder", Git will try harder to detect copies. > > It'd be nice to have the improved text from diff.renames here ("If set > to "copies-harder", Git will spend extra cycles to find more copies > even in unmodified paths."). > >> Defaults to the value of diff.renames. >> >> status.showStash:: >> diff --git a/diff.c b/diff.c >> index a2def45644..585acf9a99 100644 >> --- a/diff.c >> +++ b/diff.c >> @@ -206,8 +206,11 @@ int git_config_rename(const char *var, const char *value) >> { >> if (!value) >> return DIFF_DETECT_RENAME; >> + if (!strcasecmp(value, "copies-harder")) >> + return DIFF_DETECT_COPY_HARDER; >> if (!strcasecmp(value, "copies") || !strcasecmp(value, "copy")) >> - return DIFF_DETECT_COPY; >> + return DIFF_DETECT_COPY; >> + > > I pointed out in response to v1 that these last couple lines, while a > nice cleanup, should be in a separate patch. > >> return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0; >> } >> >> @@ -4849,8 +4852,11 @@ void diff_setup_done(struct diff_options *options) >> else >> options->flags.diff_from_contents = 0; >> >> - if (options->flags.find_copies_harder) >> + /* Just fold this in as it makes the patch-to-git smaller */ >> + if (options->flags.find_copies_harder || options->detect_rename == DIFF_DETECT_COPY_HARDER) { > > Again, I already responded to v1 that four of your lines were too long > and needed to be split. All four remain unsplit in your resubmission. > For future reference, when you resubmit a patch, please either (a) > first respond to the review explaining why you don't agree with the > suggested changes, or (b) include the fixes reviewers point out, or > (c) state in your cover letter or explanation after the '---' why you > chose to not include the suggested changes. I apologise for the rookie errors, I don't really have an excuse either. > >> + options->flags.find_copies_harder = 1; >> options->detect_rename = DIFF_DETECT_COPY; >> + } >> >> if (!options->flags.relative_name) >> options->prefix = NULL; >> @@ -5281,7 +5287,7 @@ static int diff_opt_find_copies(const struct option *opt, >> if (*arg != 0) >> return error(_("invalid argument to %s"), opt->long_name); >> >> - if (options->detect_rename == DIFF_DETECT_COPY) >> + if (options->detect_rename == DIFF_DETECT_COPY || options->detect_rename == DIFF_DETECT_COPY_HARDER) >> options->flags.find_copies_harder = 1; >> else >> options->detect_rename = DIFF_DETECT_COPY; >> diff --git a/diff.h b/diff.h >> index 66bd8aeb29..b29e5b777f 100644 >> --- a/diff.h >> +++ b/diff.h >> @@ -555,6 +555,7 @@ int git_config_rename(const char *var, const char *value); >> >> #define DIFF_DETECT_RENAME 1 >> #define DIFF_DETECT_COPY 2 >> +#define DIFF_DETECT_COPY_HARDER 3 >> >> #define DIFF_PICKAXE_ALL 1 >> #define DIFF_PICKAXE_REGEX 2 >> diff --git a/diffcore-rename.c b/diffcore-rename.c >> index 5a6e2bcac7..856291d66f 100644 >> --- a/diffcore-rename.c >> +++ b/diffcore-rename.c >> @@ -299,7 +299,7 @@ static int find_identical_files(struct hashmap *srcs, >> } >> /* Give higher scores to sources that haven't been used already */ >> score = !source->rename_used; >> - if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY) >> + if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY && options->detect_rename != DIFF_DETECT_COPY_HARDER) >> continue; >> score += basename_same(source, target); >> if (score > best_score) { >> @@ -1405,7 +1405,7 @@ void diffcore_rename_extended(struct diff_options *options, >> trace2_region_enter("diff", "setup", options->repo); >> info.setup = 0; >> assert(!dir_rename_count || strmap_empty(dir_rename_count)); >> - want_copies = (detect_rename == DIFF_DETECT_COPY); >> + want_copies = (detect_rename == DIFF_DETECT_COPY || detect_rename == DIFF_DETECT_COPY_HARDER); >> if (dirs_removed && (break_idx || want_copies)) >> BUG("dirs_removed incompatible with break/copy detection"); >> if (break_idx && relevant_sources) >> diff --git a/merge-ort.c b/merge-ort.c >> index 6491070d96..7749835465 100644 >> --- a/merge-ort.c >> +++ b/merge-ort.c >> @@ -4782,7 +4782,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result) >> * sanity check them anyway. >> */ >> assert(opt->detect_renames >= -1 && >> - opt->detect_renames <= DIFF_DETECT_COPY); >> + opt->detect_renames <= DIFF_DETECT_COPY_HARDER); >> assert(opt->verbosity >= 0 && opt->verbosity <= 5); >> assert(opt->buffer_output <= 2); >> assert(opt->obuf.len == 0); >> diff --git a/merge-recursive.c b/merge-recursive.c >> index e3beb0801b..d52dd53660 100644 >> --- a/merge-recursive.c >> +++ b/merge-recursive.c >> @@ -3708,7 +3708,7 @@ static int merge_start(struct merge_options *opt, struct tree *head) >> assert(opt->branch1 && opt->branch2); >> >> assert(opt->detect_renames >= -1 && >> - opt->detect_renames <= DIFF_DETECT_COPY); >> + opt->detect_renames <= DIFF_DETECT_COPY_HARDER); >> assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE && >> opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE); >> assert(opt->rename_limit >= -1); >> -- >> 2.43.0 > > The patch looks close, but it does continue to violate style > guidelines and make unrelated changes, like v1. And the wording in > one of the documentation blurbs could be improved a little. Looking > forward to a v3 with those fixed. I've not finished checking yet, but I think 5825268db1058516d05be03d6a8d8d55eea5a943 fixed a bug which I'd been relying on for something, where I may need to add another option for git log to suppress copies (for scripts). Should I send that in a series when doing v3 if it ends up being required? thanks, sam
Attachment:
signature.asc
Description: PGP signature
