Re: [PATCH v3 01/18] rerere: fix an off-by-one non-bug
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 24, 2015, 19:46 UTC
- Message-ID
- <xmqq7fppmj95.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1437171880-21590-2-git-send-email-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 43 quoted lines
> 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[].
>
> 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.
>
> 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>
> ---
> rerere.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> 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++;As a conflicted index can have multiple stage #1 entries when dealing with a criss-cross merge, this should probably be
while (ce_stage(e) == 1)
i++;instead.
> /* Only handle regular files with both stages #2 and #3 */
> if (i + 1 < active_nr) {