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

Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue

From
Stefan Beller <sbeller@google.com>
Date
Nov 13, 2017, 23:46 UTC
Message-ID
<CAGZ79ka=7NPU7vm5Onx89+qR3wU6UqDuPVbWsmwpk2Q_b1pjGA@mail.gmail.com>
In-Reply-To
<CABPp-BFynaf6abbd2ywzjuQnus5BFp=DQmKd80qHj9vFOqEmig@mail.gmail.com>
On Mon, Nov 13, 2017 at 3:39 PM, Elijah Newren <newren@gmail.com> wrote:
Show 11 quoted lines
> On Mon, Nov 13, 2017 at 2:12 PM, Stefan Beller <sbeller@google.com> wrote:
>> I wanted to debug a very similar issue today just after reviewing this
>> series, see
>> https://public-inbox.org/git/743acc29-85bb-3773-b6a0-68d4a0b8fd63@ispras.ru/
>
> Oh, bleh.  That's not a D/F conflict at all, it's the code assuming
> there's a D/F conflict because the entry it is processing ("sub") is a
> submodule rather than a file, and it panics when it sees "a directory
> in the way" -- a directory that just so happens to be named "sub" and
> which is in fact the desired submodule, meaning that the working
> directory is already good and needs no changes.

yup, I came to find the same snippet of code to be the offender, I just haven't figured out how to fix this bug.

Thanks for taking a look!

As you have a lot of fresh knowledge in the merge-recursive case currently, how would we approach the fix here?

(there is a test available at remotes/origin/sb/test-cherry-pick-submodule-getting-in-a-way)

Show 21 quoted lines
> In this case, the relevant code from merge-recursive.c is the following:
>
>         /* Case B: Added in one. */
>         /* [nothing|directory] -> ([nothing|directory], file) */
> <snip>
>         if (dir_in_way(path, !o->call_depth,
>                    S_ISGITLINK(a_mode))) {
>             char *new_path = unique_path(o, path, add_branch);
>             clean_merge = 0;
>             output(o, 1, _("CONFLICT (%s): There is a directory with
> name %s in %s. "
>                    "Adding %s as %s"),
>                    conf, path, other_branch, path, new_path);
>
> Note that the comment even explicitly assumes the newly added entry is
> a file.  We should expect there to be a directory present (the
> submodule being added), but the code doesn't have a check for that.
> The S_ISGITLINK(a_mode) makes you think it has special handling for
> the submodule case, but that's for the reverse situation (the
> submodule isn't yet present in the working copy, it came from the
> other side of history, but there is an empty directory present).
oh :/
Previous: Elijah NewrenNext: Elijah Newren
Message 67 of 81 in “Add directory rename detection to git”
  1. 00/30 Add directory rename detection to gitElijah Newren, Nov 10, 2017
  2. 01/30 Tighten and correct a few testcases for merging and cherry-pickingElijah Newren, Nov 10, 2017
  3. Stefan BellerNov 13, 2017
  4. 24/30 merge-recursive: Add computation of collisions due to dir rename & mergingElijah Newren, Nov 10, 2017
  5. René ScharfeJun 10, 2018
  6. René ScharfeJun 10, 2018
  7. Jeff KingJun 10, 2018
  8. Elijah NewrenJun 11, 2018
  9. Junio C HamanoJun 14, 2018
  10. 23/30 merge-recursive: Add a new hashmap for storing file collisionsElijah Newren, Nov 10, 2017
  11. 11/30 directory rename detection: testcases exploring possibly suboptimal mergesElijah Newren, Nov 10, 2017
  12. Stefan BellerNov 14, 2017
  13. Elijah NewrenNov 14, 2017
  14. 16/30 merge-recursive: Introduce new functions to handle rename logicElijah Newren, Nov 10, 2017
  15. Junio C HamanoNov 14, 2017
  16. Junio C HamanoNov 14, 2017
  17. Elijah NewrenNov 14, 2017
  18. 09/30 directory rename detection: testcases checking which side did the renameElijah Newren, Nov 10, 2017
  19. Stefan BellerNov 14, 2017
  20. Elijah NewrenNov 14, 2017
  21. 06/30 directory rename detection: testcases to avoid taking detection too farElijah Newren, Nov 10, 2017
  22. Stefan BellerNov 13, 2017
  23. Elijah NewrenNov 14, 2017
  24. 08/30 directory rename detection: files/directories in the way of some renamesElijah Newren, Nov 10, 2017
  25. Stefan BellerNov 14, 2017
  26. Elijah NewrenNov 14, 2017
  27. 05/30 directory rename detection: directory splitting testcasesElijah Newren, Nov 10, 2017
  28. Stefan BellerNov 13, 2017
  29. 07/30 directory rename detection: partially renamed directory testcase/discussionElijah Newren, Nov 10, 2017
  30. Stefan BellerNov 14, 2017
  31. 26/30 merge-recursive: When comparing files, don't include treesElijah Newren, Nov 10, 2017
  32. 29/30 merge-recursive: Fix overwriting dirty files involved in renamesElijah Newren, Nov 10, 2017
  33. 12/30 directory rename detection: miscellaneous testcases to complete coverageElijah Newren, Nov 10, 2017
  34. Stefan BellerNov 15, 2017
  35. Elijah NewrenNov 16, 2017
  36. 10/30 directory rename detection: more involved edge/corner testcasesElijah Newren, Nov 10, 2017
  37. Stefan BellerNov 14, 2017
  38. Elijah NewrenNov 14, 2017
  39. Stefan BellerNov 14, 2017
  40. 14/30 directory rename detection: tests for handling overwriting dirty filesElijah Newren, Nov 10, 2017
  41. 15/30 merge-recursive: Move the get_renames() functionElijah Newren, Nov 10, 2017
  42. Junio C HamanoNov 14, 2017
  43. Elijah NewrenNov 14, 2017
  44. Junio C HamanoNov 15, 2017
  45. 22/30 merge-recursive: Check for directory level conflictsElijah Newren, Nov 10, 2017
  46. 19/30 merge-recursive: Split out code for determining diff_filepairsElijah Newren, Nov 10, 2017
  47. Junio C HamanoNov 14, 2017
  48. 13/30 directory rename detection: tests for handling overwriting untracked filesElijah Newren, Nov 10, 2017
  49. 27/30 merge-recursive: Apply necessary modifications for directory renamesElijah Newren, Nov 10, 2017
  50. Stefan BellerNov 15, 2017
  51. Elijah NewrenNov 16, 2017
  52. 18/30 merge-recursive: Make !o->detect_rename codepath more obviousElijah Newren, Nov 10, 2017
  53. 17/30 merge-recursive: Fix leaks of allocated renames and diff_filepairsElijah Newren, Nov 10, 2017
  54. Junio C HamanoNov 14, 2017
  55. 28/30 merge-recursive: Avoid clobbering untracked files with directory renamesElijah Newren, Nov 10, 2017
  56. 30/30 merge-recursive: Fix remaining directory rename + dirty overwrite casesElijah Newren, Nov 10, 2017
  57. 21/30 merge-recursive: Add get_directory_renames()Elijah Newren, Nov 10, 2017
  58. Junio C HamanoNov 14, 2017
  59. Elijah NewrenNov 14, 2017
  60. 25/30 merge-recursive: Check for file level conflicts then get new nameElijah Newren, Nov 10, 2017
  61. 20/30 merge-recursive: Add a new hashmap for storing directory renamesElijah Newren, Nov 10, 2017
  62. 02/30 merge-recursive: Fix logic ordering issueElijah Newren, Nov 10, 2017
  63. Stefan BellerNov 13, 2017
  64. Elijah NewrenNov 13, 2017
  65. Stefan BellerNov 13, 2017
  66. Elijah NewrenNov 13, 2017
  67. Stefan BellerNov 13, 2017
  68. 04/30 directory rename detection: basic testcasesElijah Newren, Nov 10, 2017
  69. Stefan BellerNov 13, 2017
  70. Elijah NewrenNov 14, 2017
  71. Stefan BellerNov 14, 2017
  72. Elijah NewrenNov 14, 2017
  73. Junio C HamanoNov 14, 2017
  74. 03/30 merge-recursive: Add explanation for src_entry and dst_entryElijah Newren, Nov 10, 2017
  75. Stefan BellerNov 13, 2017
  76. Elijah NewrenNov 13, 2017
  77. Stefan BellerNov 13, 2017
  78. Junio C HamanoNov 14, 2017
  79. Philip OakleyNov 10, 2017
  80. Elijah NewrenNov 10, 2017
  81. Philip OakleyNov 13, 2017

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.