Hi Junio,
On Fri, 1 Jul 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
>
> > I would like to propose this diff instead (it is larger, but with a net
> > savings of one line):
> >
> > -- snipsnap --
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index d5a593c..0eda51a 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -658,24 +658,22 @@ static int was_tracked(const char *path)
> > {
> > 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 (pos >= 0)
> > + return pos < active_nr;
> > + /*
> > + * cache_name_pos() looks for stage == 0, even if we did not ask for
> > + * it. Let's look for stage == 2 now.
> > + */
>
> I think this keeps the same phrasing from the original that makes
> the comment misleading. It "looks for stage == 0" is not the whole
> story but only half.
Yes, it is the relevant part of the story to explain why we're not done
when pos < 0.
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*. And the reason is that cache_name_pos()
does not quite do what the name suggests.
I am sorry to disagree with you here: I really find it important to
document this potential misunderstanding.
> It looks for a place to insert the path at stage #0" is. Your half is
> used by the "if (0 <= pos)" you split out into a separate statement
> above already, and the untold half is needed to explain why this loop is
> correct.
True. I did not bother to document that part. Because even if I was
puzzled by the logic handling a negative return value, the assignment "pos
= -1 - pos" made it very clear to me what was happening. I am not *that*
easily puzzled.
> It returns the place to insert stage #0 entry, so if you are looking
> for stage #1 or higher, you only have to loop while the path
> matches, because the entries are sorted by <path, stage>.
>
> And with that understanding, there is no strong reason to special
> case "ah, we found stage #0 entry" at all.
There is one, and I mentioned it. In the common case (i.e. we found an
entry right away because it is in stage 0), the loop unnecessarily
compares the name *again*: index_name_pos() already performed that
comparison and if it returned a non-negative value, we know that that
comparison was successful.
I find the combination of clarification, code reduction, and separation
between conflated cases (stage 0 does not need that loop at all)
compelling enough to state that my patch is an improvement overall. We can
discuss about wording and what details to mention in the comments, of
course.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html