git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jacob Keller <jacob.keller@gmail.com>
Date
Apr 5, 2019, 16:32 UTC
Message-ID
<CA+P7+xrbd3c7J5tW+LOXKq7g62HL_0GdDOBZhT-oNgRPHOeS+A@mail.gmail.com>
In-Reply-To
<20190405150026.5260-1-newren@gmail.com>
On Fri, Apr 5, 2019 at 8:03 AM Elijah Newren <newren@gmail.com> wrote:
Show 9 quoted lines
>
> 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.

Show 10 quoted lines
> 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)?
>
Show 82 quoted lines
>
> 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
>
Previous: Elijah Newren
Message 49 of 49 in “[BUG] All files in folder are moved when cherry-picking commit that moves fewer files”
  1. Linus NilssonFeb 27, 2019
  2. Phillip WoodFeb 27, 2019
  3. Elijah NewrenFeb 27, 2019
  4. Jeff KingFeb 27, 2019
  5. Elijah NewrenFeb 27, 2019
  6. Linus NilssonFeb 28, 2019
  7. Junio C HamanoMar 1, 2019
  8. Elijah NewrenMar 2, 2019
  9. Junio C HamanoMar 3, 2019
  10. Elijah NewrenMar 6, 2019
  11. Junio C HamanoMar 6, 2019
  12. Elijah NewrenMar 7, 2019
  13. Junio C HamanoMar 7, 2019
  14. Junio C HamanoMar 7, 2019
  15. 00/15 Switch directory rename detection defaultElijah Newren, Mar 30, 2019
  16. 08/15 merge-recursive: remove ren[12]_other fields from rename_conflict_infoElijah Newren, Mar 30, 2019
  17. 10/15 merge-recursive: cleanup handle_rename_* function signaturesElijah Newren, Mar 30, 2019
  18. 01/15 Use 'unsigned short' for mode, like diff_filespec doesElijah Newren, Mar 30, 2019
  19. 04/15 merge-recursive: rename locals 'o' and 'a' to 'obuf' and 'abuf'Elijah Newren, Mar 30, 2019
  20. 14/15 merge-recursive: give callers of handle_content_merge() access to contentsElijah Newren, Mar 30, 2019
  21. 11/15 merge-recursive: switch from (oid,mode) pairs to a diff_filespecElijah Newren, Mar 30, 2019
  22. 07/15 merge-recursive: shrink rename_conflict_infoElijah Newren, Mar 30, 2019
  23. 12/15 t6043: fix copied test description to match its purposeElijah Newren, Mar 30, 2019
  24. 09/15 merge-recursive: track branch where rename occurred in rename structElijah Newren, Mar 30, 2019
  25. 13/15 merge-recursive: track information associated with directory renamesElijah Newren, Mar 30, 2019
  26. 15/15 merge-recursive: switch directory rename detection defaultElijah Newren, Mar 30, 2019
  27. Ævar Arnfjörð BjarmasonMar 30, 2019
  28. Elijah NewrenApr 1, 2019
  29. 02/15 merge-recursive: rename merge_options argument from 'o' to 'opt'Elijah Newren, Mar 30, 2019
  30. 05/15 merge-recursive: use 'ci' for rename_conflict_info variable nameElijah Newren, Mar 30, 2019
  31. 03/15 merge-recursive: rename diff_filespec 'one' to 'o'Elijah Newren, Mar 30, 2019
  32. 06/15 merge-recursive: move some struct declarations togetherElijah Newren, Mar 30, 2019
  33. 00/15 Switch directory rename detection defaultElijah Newren, Apr 5, 2019
  34. 03/15 merge-recursive: rename diff_filespec 'one' to 'o'Elijah Newren, Apr 5, 2019
  35. 05/15 merge-recursive: use 'ci' for rename_conflict_info variable nameElijah Newren, Apr 5, 2019
  36. 09/15 merge-recursive: track branch where rename occurred in rename structElijah Newren, Apr 5, 2019
  37. 06/15 merge-recursive: move some struct declarations togetherElijah Newren, Apr 5, 2019
  38. 07/15 merge-recursive: shrink rename_conflict_infoElijah Newren, Apr 5, 2019
  39. 08/15 merge-recursive: remove ren[12]_other fields from rename_conflict_infoElijah Newren, Apr 5, 2019
  40. 04/15 merge-recursive: rename locals 'o' and 'a' to 'obuf' and 'abuf'Elijah Newren, Apr 5, 2019
  41. 15/15 merge-recursive: switch directory rename detection defaultElijah Newren, Apr 5, 2019
  42. 14/15 merge-recursive: give callers of handle_content_merge() access to contentsElijah Newren, Apr 5, 2019
  43. 12/15 t6043: fix copied test description to match its purposeElijah Newren, Apr 5, 2019
  44. 13/15 merge-recursive: track information associated with directory renamesElijah Newren, Apr 5, 2019
  45. 02/15 merge-recursive: rename merge_options argument from 'o' to 'opt'Elijah Newren, Apr 5, 2019
  46. 10/15 merge-recursive: cleanup handle_rename_* function signaturesElijah Newren, Apr 5, 2019
  47. 01/15 Use 'unsigned short' for mode, like diff_filespec doesElijah Newren, Apr 5, 2019
  48. 11/15 merge-recursive: switch from (oid,mode) pairs to a diff_filespecElijah Newren, Apr 5, 2019
  49. Jacob KellerApr 5, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.