Web lists-archives.com

Re: [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths




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)