Re: [PATCH v3 00/15] Switch directory rename detection default

[Jacob Keller <jacob.keller@gmail.com>]

On Fri, Apr 5, 2019 at 8:03 AM Elijah Newren <newren@gmail.com> 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
>