git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Elijah Newren <newren@gmail.com>
Cc: git@vger.kernel.org, torvalds@linux-foundation.org, sbeller@google.com
Subject: Re: [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths
Date: Mon, 16 Apr 2018 09:37:04 +0900	[thread overview]
Message-ID: <xmqqo9ikyojz.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180413195607.18091-4-newren@gmail.com> (Elijah Newren's message of "Fri, 13 Apr 2018 12:56:06 -0700")

Elijah Newren <newren@gmail.com> 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)

  reply	other threads:[~2018-04-16  0:37 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-11  7:19 [Git] recursive merge on 'master' severely broken? Junio C Hamano
2018-04-11  9:06 ` Junio C Hamano
2018-04-11  9:57   ` Junio C Hamano
2018-04-11 15:51 ` Elijah Newren
2018-04-12  1:37   ` Junio C Hamano
2018-04-13 19:56 ` [RFC PATCH v9 0/30] Add directory rename detection to git Elijah Newren
2018-04-13 19:56   ` [PATCH v9 29.25/30] merge-recursive: improve output precision around skipping updates Elijah Newren
2018-04-13 19:56   ` [PATCH v9 29.50/30] t6046: testcases checking whether updates can be skipped in a merge Elijah Newren
2018-04-13 21:03     ` Stefan Beller
2018-04-13 19:56   ` [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths Elijah Newren
2018-04-16  0:37     ` Junio C Hamano [this message]
2018-04-16 21:21       ` Elijah Newren
2018-04-13 19:56   ` [PATCH v9 30/30] merge-recursive: fix check for skipability of working tree updates Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo9ikyojz.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    --cc=sbeller@google.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).