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 >