Re: [PATCH 2/9] merge-recursive: clarify code in was_tracked()

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 6, 2016, 15:30 UTC
Message-ID: <xmqqr3b6u6mb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1607020906560.12947@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> To understand why we're not done yet, the crucial point is *not* that the
> return value encodes the insert position. The crucial point is that
> despite asking for an index entry matching a specific name, we might not
> find one, *even if there is one*.

I've been wondering why you keep saying "even though we didn't ask, we look for stage#0", and now I see why. The cache_pos() interface *is* about finding the stage#0 entry for the given path.

When it finds none, it indicates where a stage#0 entry of that path would be inserted, which by the sort-order would give us where higher stage entries for the path would be found (if there is any). There is no parameter for you to tell it to find stage#2, and "even though we didn't ask" is showing (and being the source of) the confusion.

And I did not want a misleading comment to spread the confusion; that is why I was reacting strongly.

As you pointed out, we can return early without falling into the generic "we are still looking at the same path" codepath when we find thestage#0 entry, so I wouldn't mind doing something like the following.

static int was_tracked(const char *path)
{
	int pos = cache_name_pos(path, strlen(path));

        if (0 <= pos)
	        /* we have been tracking this path */
        	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;
}