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)
next prev parent 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).