Elijah Newren <newren@xxxxxxxxx> writes:
> @@ -362,13 +363,17 @@ static int git_merge_trees(struct merge_options *o,
> init_tree_desc_from_tree(t+2, merge);
>
> rc = unpack_trees(3, t, &o->unpack_opts);
> + cache_tree_free(&active_cache_tree);
> +
> + o->orig_index = the_index;
> + the_index = tmp_index;
> +
> /*
> - * unpack_trees NULLifies src_index, but it's used in verify_uptodate,
> - * so set to the new index which will usually have modification
> - * timestamp info copied over.
> + * src_index is used in verify_uptodate, but was NULLified in
> + * unpack_trees, so we need to set it back to the original index.
> */
Was NULLified? I thought that the point of src/dst distinction
Linus introduced long time ago at 34110cd4 ("Make 'unpack_trees()'
have a separate source and destination index", 2008-03-06) was that
we can then keep the source side of the traversal unmodified.
> - o->unpack_opts.src_index = &the_index;
> - cache_tree_free(&active_cache_tree);
> + o->unpack_opts.src_index = &o->orig_index;
> -static int was_tracked(const char *path)
> +/*
> + * Returns whether path was tracked in the index before the merge started
> + */
> +static int was_tracked(struct merge_options *o, const char *path)
> {
> - int pos = cache_name_pos(path, strlen(path));
> + int pos = index_name_pos(&o->orig_index, path, strlen(path));
>
> if (0 <= pos)
> - /* we have been tracking this path */
> + /* we were tracking this path before the merge */
> return 1;
>
> - /*
> - * Look for an unmerged entry for the path,
> - * specifically stage #2, which would indicate
> - * that "our" side before the merge started
> - * had the path tracked (and resulted in a conflict).
> - */
> - for (pos = -1 - pos;
> - pos < active_nr && !strcmp(path, active_cache[pos]->name);
> - pos++)
> - if (ce_stage(active_cache[pos]) == 2)
> - return 1;
> return 0;
> }
I do agree with the simplicity of the new code that directly asks
exactly what we want to ask. However, there is one thing that is
puzzling below...
> static int would_lose_untracked(const char *path)
> {
> - return !was_tracked(path) && file_exists(path);
> + /*
> + * This may look like it can be simplified to:
> + * return !was_tracked(o, path) && file_exists(path)
> + * but it can't. This function needs to know whether path was
> + * in the working tree due to EITHER having been tracked in the
> + * index before the merge OR having been put into the working copy
> + * and index by unpack_trees(). Due to that either-or requirement,
> + * we check the current index instead of the original one.
> + */
If this path was created by merge-recursive, not by unpack_trees(),
what does this function want to say? Say, we are looking at path P,
the other branch we are merging moved some other path Q to P (while
our side modified contents at path Q). Then path P we are looking
at has contents of Q at the merge base at stage #1, the contents of
Q from our HEAD at stage #2 and the contents of P from the other
branch at stage #3. The code below says "path P is OK, we won't
lose it" in such a case, but it is unclear if the above comment
wants to also cover that case.
> + int pos = cache_name_pos(path, strlen(path));
> +
> + if (pos < 0)
> + pos = -1 - pos;
> + while (pos < active_nr &&
> + !strcmp(path, active_cache[pos]->name)) {
> + /*
> + * If stage #0, it is definitely tracked.
> + * If it has stage #2 then it was tracked
> + * before this merge started. All other
> + * cases the path was not tracked.
> + */
> + switch (ce_stage(active_cache[pos])) {
> + case 0:
> + case 2:
> + return 0;
> + }
> + pos++;
> + }
> + return file_exists(path);
> }
>
> static int was_dirty(struct merge_options *o, const char *path)
