[PATCH] rerere: fix an off-by-one non-bug

Junio C Hamano· Jun 28, 2015, 21:47 UTC · lore

When ac49f5ca (rerere "remaining", 2011-02-16) created a new helper function check_one_conflict() out of find_conflict() function, and made the latter use the returned value from the new helper to update the loop control variable that is an index into the active_cache[], the new variable was incremented by one too many when it pointed at a path with only stage #1 entry at the very end of the cache.

This "strange" return value does not have any effect on the loop control of two callers of this function, as they all notice that active_nr+2 is larger than active_nr just like active_nr+1 is, but nevertheless it puzzles the readers when they are trying to figure out what the function is trying to do.

Help future readers from unnecessary confusion by dropping an unnecessary +1 from the returned value..

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 rerere.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to rerere.c +1 −1

diff --git a/rerere.c b/rerere.c
index 31644de..543df47 100644
--- a/rerere.c
+++ b/rerere.c
@@ -371,7 +371,7 @@ static int check_one_conflict(int i, int *type)
 	*type = PUNTED;
 	if (ce_stage(e) == 1) {
 		if (active_nr <= ++i)
-			return i + 1;
+			return i;
 	}
 
 	/* Only handle regular files with both stages #2 and #3 */

-- 
2.5.0-rc0-168-g2f1df93

Junio C Hamano· Jun 28, 2015, 22:26 UTC · re: Junio C Hamano · lore

[PATCH v2] rerere: fix an off-by-one non-bug

When ac49f5ca (rerere "remaining", 2011-02-16) split out a new helper function check_one_conflict() out of find_conflict() function, so that the latter will use the returned value from the new helper to update the loop control variable that is an index into active_cache[], the new variable incremented the index by one too many when it found a path with only stage #1 entry at the very end of active_cache[].

In fact, there is no need to do an early return. The code that follows after skipping the stage #1 entry is fully prepared to handle a case where the entry is at the very end of active_cache[].

Help future readers from unnecessary confusion by dropping an early return. We skip the stage #1 entry, and if there are stage #2 and stage #3 entries for the same path, we diagnose the path as THREE_STAGED (otherwise we say PUNTED), and then we skip all entries for the same path.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Heh, I was an idiot.  There is nothing gained by keeping an early
   return in this codepath other than unnecessary complexity.

 rerere.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Show changes to rerere.c +2 −4

diff --git a/rerere.c b/rerere.c
index 31644de..e307711 100644
--- a/rerere.c
+++ b/rerere.c
@@ -369,10 +369,8 @@ static int check_one_conflict(int i, int *type)
 	}
 
 	*type = PUNTED;
-	if (ce_stage(e) == 1) {
-		if (active_nr <= ++i)
-			return i + 1;
-	}
+	if (ce_stage(e) == 1)
+		i++;
 
 	/* Only handle regular files with both stages #2 and #3 */
 	if (i + 1 < active_nr) {

-- 
2.5.0-rc0-168-g2f1df93

patchrerere: fix an off-by-one non-bug