Hi Junio,
On Wed, 29 Jun 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>
> > It can be puzzling to see that was_tracked() tries to match an index
> > entry by name even if cache_name_pos() returned a negative value. Let's
> > clarify that cache_name_pos() implicitly looks for stage 0, while we are
> > also okay with finding other stages.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> > ---
> > merge-recursive.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index 98f4632..bcb53f0 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -658,6 +658,7 @@ static int was_tracked(const char *path)
> > {
> > int pos = cache_name_pos(path, strlen(path));
> >
> > + /* cache_name_pos() looks for stage == 0, so pos may be < 0 */
>
> It returns >= if found at stage #0, or a negative (counting from -1)
> to indicate where the path would be inserted if it were to be added
> at stage #0.
Right. Please note that nothing in the function call to cache_name_pos()
specifies that we are looking for any particular stage. You are probably
too intimate with the implementation details, so naturally you assume that
cache_name_pos() looks for a precise match *with stage 0*. I am not that
familiar with that part of the implementation, so I *was* puzzled.
> The new comment does not explain how "pos may be < 0" leads to
> "hence pos = -1 - pos is the right thing to do here". It is
> misleading and we probably are better off without.
I agree that the comment is not very good currently. But I disagree that
we are better off without any comment here.
So maybe a comment is not really enough here. In case the entry is in
stage 0, we will have an exact match, so there is no need to go to the
lengths of a while() loop.
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.
+ */
+ for (pos = -1 - pos; pos < active_nr &&
+ !strcmp(path, active_cache[pos]->name); pos++)
/*
* 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:
+ if (ce_stage(active_cache[pos]) == 2)
return 1;
- }
- pos++;
- }
return 0;
}
--
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