> On Fri, Nov 6, 2020 at 2:52 PM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
> >
> > > + /*
> > > + * Note: We only label files with df_conflict, not directories.
> > > + * Since directories stay where they are, and files move out of the
> > > + * way to make room for a directory, we don't care if there was a
> > > + * directory/file conflict for a parent directory of the current path.
> > > + */
> > > + unsigned df_conflict = (filemask != 0) && (dirmask != 0);
> >
> > Suppose you have:
> >
> > [ours]
> > foo/
> > bar/
> > baz
> > quux
> > [theirs]
> > foo
> >
> > By "we only label files with df_conflict, not directories", are you
> > referring to not labelling "foo/" in [ours], or to "bar/", "baz", and
> > "quux" (so, the files and directories within a directory)? At first I
> > thought you were referring to the former, but perhaps you are referring
> > to the latter.
>
> The former. I was drawing a distinction between how this code
> operates, and how unpack_trees() operates, which probably only matters
> to those familiar with unpack_trees() or who have been reading through
> it recently.
Just for clarification: do you mean "the latter"? (The "not" in my
question might be confusing.)
To be more illustrative in what I meant, at first I thought that you
were NOT labelling "foo/" in [ours], hence:
[ours]
foo/ <- unlabeled
[theirs]
foo <- labeled
In this way, in a sense, you are indeed labelling only the file, not the
directory.
But instead what you seem to be doing is this:
[ours]
foo/ <- labeled
bar/ <- unlabeled
baz <- unlabeled
quux <- unlabeled
[theirs]
foo <- labeled
which is what I meant by NOT labelling "bar/", "baz", and "quux".
> unpack_trees() will note when there is a directory/file
> conflict, and propagates that information to all subtrees, with every
> path specially checking for the o->df_conflict_entry and then handling
> it specially (e.g. keeping higher order stages instead of using an
> aggressive or trivial resolutions).
And here it seems like you're describing that unpack_trees() would label
it in this way:
[ours]
foo/ <- labeled
bar/ <- labeled
baz <- labeled
quux <- labeled
[theirs]
foo <- labeled
(and you're emphasizing by contrast that merge-ort is NOT doing this).
> However, leaving both a file and
> a directory at the same path, while allowed in the index, makes for
> ugliness and difficulty for users to resolve. Plus it isn't allowed
> in the working tree anyway. We decided a while ago that it'd be
> better to represent these conflicts differently[1], [2].
>
> Also, at the time you are unpacking or traversing trees, you only know
> if one side had a directory where the other side had a file. You
> don't know if the final merge result will actually have a
> directory/file conflict. If the file existed in both the base version
> and unmodified on one side, for example, then the file will be removed
> as part of the merge. It is similarly possible that the entire
> directory of files all need to be deleted or are all renamed
> elsewhere. So, you have to keep track of a df_conflict bit, but you
> can't act on it until you've processed several other things first.
>
> Since I already know I'm not going to move a whole directory of files
> out of the way so that a file can be placed in the working tree
> instead of that whole directory, the directory doesn't need to be
> tweaked. I'm not going to propagate any information about a
> directory/file conflict at some path down to all subpaths of the
> directory. I only track it for the file that immediately conflicts,
> and then only take action on it after resolving all the paths under
> the corresponding directory to see if the directory/file conflict
> remains.
>
> [1] https://lore.kernel.org/git/xmqqbmabcuhf.fsf@xxxxxxxxxxxxxxxxxxxxxxxxx/
> and the thread surrounding it
> [2] https://lore.kernel.org/git/f27f12e8e50e56c010c29caa00296475d4de205b.1603731704.git.gitgitgadget@xxxxxxxxx/,
> which is now commit ef52778708 ("merge tests: expect improved
> directory/file conflict handling in ort", 2020-10-26)
Makes sense.
> > > @@ -161,6 +179,13 @@ static int collect_merge_info_callback(int n,
> > > newinfo.name = p->path;
> > > newinfo.namelen = p->pathlen;
> > > newinfo.pathlen = st_add3(newinfo.pathlen, p->pathlen, 1);
> > > + /*
> > > + * If we did care about parent directories having a D/F
> > > + * conflict, then we'd include
> > > + * newinfo.df_conflicts |= (mask & ~dirmask);
> > > + * here. But we don't. (See comment near setting of local
> > > + * df_conflict variable near the beginning of this function).
> > > + */
> >
> > I'm not sure how "mask" and "dirmask" contains information about parent
> > directories. "mask" represents the available entries, and "dirmask"
> > represents which of them are directories, as far as I know. So we can
> > notice when something is missing, but I don't see how this distinguishes
> > between the case that something is missing because it was in a parent
> > directory that got deleted, vs something is missing because it itself
> > got deleted.
>
> Yeah, this is more comparisons to unpack_trees. This code is about to
> set up a recursive call into subdirectories. newinfo is set based on
> the mask and dirmask of the current entry, and then subdirectories can
> consult newinfo.df_conflicts to see if that path is within a directory
> that was involved in a directory/file conflict. For example:
>
> Tree in base version:
> foo/
> bar
> stuff.txt
> Tree on side 1: (adds foo/baz)
> foo/
> bar
> baz
> stuff.txt
> Tree on side 2: (deletes foo/, adds new file foo)
> foo
> stuff.txt
>
> When processing 'foo', we have mask=7, dirmask = 3. So, here
> unpack_trees() would have set newinfo.df_conflicts = (mask & ~dirmask)
> = 4. Then when we process foo/bar or foo/baz, we have mask=2,
> dirmask=0, which looks like there are no directory/file conflicts.
> However, we can note that these paths are under a directory involved
> in a directory/file conflict via info.df_conflicts whose value is 4.
> unpack_trees() cared about paths under a directory that was involved
> in a directory/file conflict, and someone familiar with that code
> might ask why I don't also track the same information. The answer is
> that I don't track it, even though I thought about it, because it's
> useless overhead since I'm going to leave the directory alone and move
> the file out of the way.
>
> Does that make sense?
Ah...yes, that makes sense. I think I didn't notice the "newinfo", so I
didn't realize that we were setting the info of our children based on
ourselves. Perhaps I would have noticed it sooner if the comment had
read "If this file/directory cared about its parent directory (the
current directory) having a D/F conflict, then we'd propagate the masks
in this way:" instead of "If we did care about parent directories having
a D/F conflict", but perhaps the point is already obvious enough.
