Johannes Schindelin [off-list ref] writes:
I agree that the comment is not very good currently. But I disagree that
we are better off without any comment here.
I meant we are better off without your particular version of comment
which is misleading. I am all for a better comment to help those
who are new to the codepath.
quoted hunk
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. 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.
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.
+ 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;
}
Hi Junio,
On Fri, 1 Jul 2016, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
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