{"thread":{"id":"39742","subject":"[PATCH] rerere: fix an off-by-one non-bug","startedAt":"2015-06-28T21:47:02Z","lastAt":"2015-06-28T22:26:21Z","messageCount":2,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"265098","messageId":"xmqqa8vjeca1.fsf@gitster.dls.corp.google.com","threadId":"39742","inReplyTo":null,"subject":"[PATCH] rerere: fix an off-by-one non-bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-28T21:47:02Z","receivedAt":"2015-06-28T21:47:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When ac49f5ca (rerere \"remaining\", 2011-02-16) created a new helper\nfunction check_one_conflict() out of find_conflict() function, and\nmade the latter use the returned value from the new helper to update\nthe loop control variable that is an index into the active_cache[],\nthe new variable was incremented by one too many when it pointed at\na path with only stage #1 entry at the very end of the cache.\n\nThis \"strange\" return value does not have any effect on the loop\ncontrol of two callers of this function, as they all notice that\nactive_nr+2 is larger than active_nr just like active_nr+1 is, but\nnevertheless it puzzles the readers when they are trying to figure\nout what the function is trying to do.\n\nHelp future readers from unnecessary confusion by dropping an\nunnecessary +1 from the returned value..\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n rerere.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 31644de..543df47 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -371,7 +371,7 @@ static int check_one_conflict(int i, int *type)\n \t*type = PUNTED;\n \tif (ce_stage(e) == 1) {\n \t\tif (active_nr <= ++i)\n-\t\t\treturn i + 1;\n+\t\t\treturn i;\n \t}\n \n \t/* Only handle regular files with both stages #2 and #3 */\n-- \n2.5.0-rc0-168-g2f1df93\n"},{"id":"265099","messageId":"xmqq6167eagi.fsf@gitster.dls.corp.google.com","threadId":"39742","inReplyTo":"xmqqa8vjeca1.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] rerere: fix an off-by-one non-bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-28T22:26:21Z","receivedAt":"2015-06-28T22:26:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When ac49f5ca (rerere \"remaining\", 2011-02-16) split out a new\nhelper function check_one_conflict() out of find_conflict()\nfunction, so that the latter will use the returned value from the\nnew helper to update the loop control variable that is an index into\nactive_cache[], the new variable incremented the index by one too\nmany when it found a path with only stage #1 entry at the very end\nof active_cache[].\n\nThis \"strange\" return value does not have any effect on the loop\ncontrol of two callers of this function, as they all notice that\nactive_nr+2 is larger than active_nr just like active_nr+1 is, but\nnevertheless it puzzles the readers when they are trying to figure\nout what the function is trying to do.\n\nIn fact, there is no need to do an early return.  The code that\nfollows after skipping the stage #1 entry is fully prepared to\nhandle a case where the entry is at the very end of active_cache[].\n\nHelp future readers from unnecessary confusion by dropping an early\nreturn.  We skip the stage #1 entry, and if there are stage #2 and\nstage #3 entries for the same path, we diagnose the path as\nTHREE_STAGED (otherwise we say PUNTED), and then we skip all entries\nfor the same path.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Heh, I was an idiot.  There is nothing gained by keeping an early\n   return in this codepath other than unnecessary complexity.\n\n rerere.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 31644de..e307711 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -369,10 +369,8 @@ static int check_one_conflict(int i, int *type)\n \t}\n \n \t*type = PUNTED;\n-\tif (ce_stage(e) == 1) {\n-\t\tif (active_nr <= ++i)\n-\t\t\treturn i + 1;\n-\t}\n+\tif (ce_stage(e) == 1)\n+\t\ti++;\n \n \t/* Only handle regular files with both stages #2 and #3 */\n \tif (i + 1 < active_nr) {\n-- \n2.5.0-rc0-168-g2f1df93\n"}]}