On Fri, Apr 5, 2019 at 8:03 AM Elijah Newren <newren@xxxxxxxxx> wrote:
>
> This series adds a new configuration option, merge.directoryRenames,
> for setting how to make use of directory rename detection heuristics.
> The default becomes "conflict", meaning that conflicts are reported
> instead of silently moving paths according to the heuristics. Also,
> even when merge.directoryRenames config setting is "true", this series
> changes behavior in that it now prints informational messages about
> paths that are adjusted by the directory rename detection heuristics.
>
I read through the v2 series, and the range diff on v3. I thought it
looked good.
> Changes since v2 (range-diff below):
> * Made use of git_parse_maybe_bool() as suggested by Ævar, and made
> the parsing of the merge.directoryRenames setting look more like
> that for merge.ff.
>
> I didn't get much review of round 2, which maybe means everyone is
> happy with what they see. If anyone would like to take a look at just
> part of the series, the pieces I'd most like folks to look at are:
> * Patch 15, particularly looking over the new testcases (13a-13d) in
> t6043 and the documentation.
The documentation made sense to me. I can't speak much towards the
implementation, but I definitely agree that conflicting is a suitable
default, and better than silently renaming.
> * Should I have switched the type of "mode" from 'unsigned short' to
> 'unsigned' instead of vice-versa in patch 1?
I think switching to unsigned short is better. Unless we need the full
integer width for some reason? but since it's already short I don't
see why we would..
> * Similarly, does anyone have a reason to prefer oid,mode pair over
> using a diff_filespec (in patch 11 I convert half the sites to the
> latter)?
>
>
> Elijah Newren (15):
> Use 'unsigned short' for mode, like diff_filespec does
> merge-recursive: rename merge_options argument from 'o' to 'opt'
> merge-recursive: rename diff_filespec 'one' to 'o'
> merge-recursive: rename locals 'o' and 'a' to 'obuf' and 'abuf'
> merge-recursive: use 'ci' for rename_conflict_info variable name
> merge-recursive: move some struct declarations together
> merge-recursive: shrink rename_conflict_info
> merge-recursive: remove ren[12]_other fields from rename_conflict_info
> merge-recursive: track branch where rename occurred in rename struct
> merge-recursive: cleanup handle_rename_* function signatures
> merge-recursive: switch from (oid,mode) pairs to a diff_filespec
> t6043: fix copied test description to match its purpose
> merge-recursive: track information associated with directory renames
> merge-recursive: give callers of handle_content_merge() access to
> contents
> merge-recursive: switch directory rename detection default
>
> Documentation/config/merge.txt | 19 +-
> archive.c | 2 +-
> blame.c | 2 +-
> blame.h | 2 +-
> builtin/rm.c | 2 +-
> builtin/update-index.c | 2 +-
> cache.h | 2 +-
> fsck.c | 2 +-
> line-log.c | 2 +-
> match-trees.c | 8 +-
> merge-recursive.c | 1853 ++++++++++++------------
> notes.c | 2 +-
> sha1-name.c | 2 +-
> t/t3401-rebase-and-am-rename.sh | 8 +-
> t/t6043-merge-rename-directories.sh | 462 +++++-
> t/t6046-merge-skip-unneeded-updates.sh | 8 +-
> tree-diff.c | 2 +-
> tree-walk.c | 6 +-
> tree-walk.h | 6 +-
> 19 files changed, 1367 insertions(+), 1025 deletions(-)
>
> Range-diff:
> 1: bb5b410a61 = 1: bb5b410a61 Use 'unsigned short' for mode, like diff_filespec does
> 2: f91c28257e = 2: f91c28257e merge-recursive: rename merge_options argument from 'o' to 'opt'
> 3: e3fe8baa15 = 3: e3fe8baa15 merge-recursive: rename diff_filespec 'one' to 'o'
> 4: c6bd963ffb = 4: c6bd963ffb merge-recursive: rename locals 'o' and 'a' to 'obuf' and 'abuf'
> 5: eca30e7571 = 5: eca30e7571 merge-recursive: use 'ci' for rename_conflict_info variable name
> 6: 07f0d5fa8e = 6: 07f0d5fa8e merge-recursive: move some struct declarations together
> 7: 4cdd1ecbcb = 7: 4cdd1ecbcb merge-recursive: shrink rename_conflict_info
> 8: 3490324bdd = 8: 3490324bdd merge-recursive: remove ren[12]_other fields from rename_conflict_info
> 9: fb73a2c55d = 9: fb73a2c55d merge-recursive: track branch where rename occurred in rename struct
> 10: 124ee08ed8 = 10: 124ee08ed8 merge-recursive: cleanup handle_rename_* function signatures
> 11: 78a5916efe = 11: 78a5916efe merge-recursive: switch from (oid,mode) pairs to a diff_filespec
> 12: a8309326c1 = 12: a8309326c1 t6043: fix copied test description to match its purpose
> 13: b362f4db1e = 13: b362f4db1e merge-recursive: track information associated with directory renames
> 14: 2e0258a358 = 14: 2e0258a358 merge-recursive: give callers of handle_content_merge() access to contents
> 15: 719c25afaf ! 15: 428cdf62b3 merge-recursive: switch directory rename detection default
> @@ -262,17 +262,12 @@
> free(value);
> }
> + if (!git_config_get_string("merge.directoryrenames", &value)) {
> -+ if (!strcasecmp(value, "true"))
> -+ opt->detect_directory_renames = 2;
> -+ else if (!strcasecmp(value, "false"))
> -+ opt->detect_directory_renames = 0;
> -+ else if (!strcasecmp(value, "conflict"))
> ++ int boolval = git_parse_maybe_bool(value);
> ++ if (0 <= boolval) {
> ++ opt->detect_directory_renames = boolval ? 2 : 0;
> ++ } else if (!strcasecmp(value, "conflict")) {
> + opt->detect_directory_renames = 1;
> -+ else {
> -+ error(_("Invalid value for merge.directoryRenames: %s"),
> -+ value);
> -+ opt->detect_directory_renames = 1;
> -+ }
> ++ } /* avoid erroring on values from future versions of git */
> + free(value);
> + }
> git_config(git_xmerge_config, NULL);
> --
> 2.21.0.211.g719c25afaf.dirty
>
