git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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) {
Previous: Junio C HamanoNext: Junio C Hamano
Message 17 of 39 in “"rerere" minor clean-up”
  1. 00/13 "rerere" minor clean-upJunio C Hamano, Jul 1, 2015
  2. 01/13 rerere: fix an off-by-one non-bugJunio C Hamano, Jul 1, 2015
  3. 02/13 rerere: plug conflict ID leaksJunio C Hamano, Jul 1, 2015
  4. 03/13 rerere: lift PATH_MAX limitationJunio C Hamano, Jul 1, 2015
  5. 04/13 rerere: write out each record of MERGE_RR in one goJunio C Hamano, Jul 1, 2015
  6. 05/13 rerere: report autoupdated paths only after actually updating themJunio C Hamano, Jul 1, 2015
  7. 06/13 rerere: drop want_sp parameter from is_cmarker()Junio C Hamano, Jul 1, 2015
  8. 07/13 rerere: stop looping unnecessarilyJunio C Hamano, Jul 1, 2015
  9. 08/13 rerere: explain the rerere I/O abstractionJunio C Hamano, Jul 1, 2015
  10. 09/13 rerere: explain MERGE_RR management helpersJunio C Hamano, Jul 1, 2015
  11. 10/13 rerere: explain the primary codepathJunio C Hamano, Jul 1, 2015
  12. 11/13 rerere: explain "rerere forget" codepathJunio C Hamano, Jul 1, 2015
  13. 12/13 rerere: explain the remainderJunio C Hamano, Jul 1, 2015
  14. 13/13 rerere: refactor "replay" part of do_plain_rerere()Junio C Hamano, Jul 1, 2015
  15. 00/18 "rerere" preparatory clean-upJunio C Hamano, Jul 17, 2015
  16. 01/18 rerere: fix an off-by-one non-bugJunio C Hamano, Jul 17, 2015
  17. Junio C HamanoJul 24, 2015
  18. 02/18 rerere: plug conflict ID leaksJunio C Hamano, Jul 17, 2015
  19. 03/18 rerere: lift PATH_MAX limitationJunio C Hamano, Jul 17, 2015
  20. 04/18 rerere: write out each record of MERGE_RR in one goJunio C Hamano, Jul 17, 2015
  21. 05/18 rerere: report autoupdated paths only after actually updating themJunio C Hamano, Jul 17, 2015
  22. 06/18 rerere: drop want_sp parameter from is_cmarker()Junio C Hamano, Jul 17, 2015
  23. Philip OakleyJul 18, 2015
  24. Eric SunshineJul 18, 2015
  25. 07/18 rerere: stop looping unnecessarilyJunio C Hamano, Jul 17, 2015
  26. Junio C HamanoJul 24, 2015
  27. 08/18 rerere: explain the rerere I/O abstractionJunio C Hamano, Jul 17, 2015
  28. Junio C HamanoJul 24, 2015
  29. 09/18 rerere: explain MERGE_RR management helpersJunio C Hamano, Jul 17, 2015
  30. 10/18 rerere: explain the primary codepathJunio C Hamano, Jul 17, 2015
  31. 11/18 rerere: explain "rerere forget" codepathJunio C Hamano, Jul 17, 2015
  32. 12/18 rerere: explain the remainderJunio C Hamano, Jul 17, 2015
  33. 13/18 rerere: refactor "replay" part of do_plain_rerere()Junio C Hamano, Jul 17, 2015
  34. 14/18 rerere: further de-dent do_plain_rerere()Junio C Hamano, Jul 17, 2015
  35. 15/18 rerere: further clarify do_rerere_one_path()Junio C Hamano, Jul 17, 2015
  36. 16/18 rerere: call conflict-ids IDsJunio C Hamano, Jul 17, 2015
  37. 17/18 rerere: use "struct rerere_id" instead of "char *" for conflict IDJunio C Hamano, Jul 17, 2015
  38. Eric SunshineJul 18, 2015
  39. 18/18 rerere: un-nest merge() furtherJunio C Hamano, Jul 17, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.