Re: [PATCH 2/9] merge-recursive: clarify code in was_tracked()

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2016, 15:31 UTC
Message-ID: <xmqq7fd51ijr.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1607011057180.12947@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Show 22 quoted lines

> 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.

Show 19 quoted lines

> +	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;
>  }
>