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

On Mon, Nov 13, 2017 at 3:39 PM, Elijah Newren [off-list ref] wrote:

On Mon, Nov 13, 2017 at 2:12 PM, Stefan Beller [off-list ref] wrote:

quoted

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)

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 :/