Re: [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 16, 2018, 00:37 UTC
- Message-ID
- <xmqqo9ikyojz.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180413195607.18091-4-newren@gmail.com>
Elijah Newren <newren@gmail.com> writes:
Show 16 quoted lines
> @@ -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;
Show 27 quoted lines
> -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...
Show 12 quoted lines
> 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.
Show 23 quoted lines
> + 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)