Re: [PATCH v3 1/2] commit-reach: properly peel tags

[Jeff King <peff@peff.net>]

On Fri, Sep 21, 2018 at 08:05:26AM -0700, Derrick Stolee via GitGitGadget wrote:

> diff --git a/commit-reach.c b/commit-reach.c
> index 86715c103c..e748414d04 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -544,20 +544,42 @@ int can_all_from_reach_with_flag(struct object_array *from,
>  {
>  	struct commit **list = NULL;
>  	int i;
> +	int nr_commits;
>  	int result = 1;
>  
>  	ALLOC_ARRAY(list, from->nr);
> +	nr_commits = 0;
>  	for (i = 0; i < from->nr; i++) {
> -		list[i] = (struct commit *)from->objects[i].item;
> +		struct object *from_one = from->objects[i].item;
>  
> -		if (parse_commit(list[i]) ||
> -		    list[i]->generation < min_generation)
> -			return 0;
> +		if (!from_one || from_one->flags & assign_flag)
> +			continue;
> +
> +		from_one = deref_tag(the_repository, from_one,
> +				     "a from object", 0);
> +		if (!from_one || from_one->type != OBJ_COMMIT) {
> +			/* no way to tell if this is reachable by
> +			 * looking at the ancestry chain alone, so
> +			 * leave a note to ourselves not to worry about
> +			 * this object anymore.
> +			 */

A minor nit, but the original comment you restored here has a style
violation. Might be worth fixing up (but certainly not worth a re-roll
on its own).

> +			from->objects[i].item->flags |= assign_flag;

OK, so here we mark the original tag with a flag...

> +
> +		list[nr_commits] = (struct commit *)from_one;
> +		if (parse_commit(list[nr_commits]) ||
> +		    list[nr_commits]->generation < min_generation) {
> +			result = 0;
> +			goto cleanup;
> +		}

And we jump to the cleanup here, but...

> @@ -600,7 +622,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
>  	}
>  
>  cleanup:
> -	for (i = 0; i < from->nr; i++) {
> +	for (i = 0; i < nr_commits; i++) {
>  		clear_commit_marks(list[i], RESULT);
>  		clear_commit_marks(list[i], assign_flag);

This walks over the items in the list array, which don't include the tag
we marked. Did we decide that's OK? I think it's actually how the
original code behaved, too, but it seems funny. At the least we might
want to call it out in the commit message.

Should we also be walking from->objects and clearing the flags from
there (maybe not RESULT, but assign_flag)? It's not clear to me if it's
unintentional for those to be marked after the function leaves or not.

Also, a minor aside, but I think it would be slightly more efficient for
those final two lines to do:

  clear_commit_marks(list[i], RESULT | assign_flag);

Of course, that's totally orthogonal to this patch, but it may make you
feel better to offset the other round of clearing I'm suggesting. ;)

> diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
> index eb21103998..08d2ea68e8 100644
> --- a/t/helper/test-reach.c
> +++ b/t/helper/test-reach.c

These bits all looked good to me.

-Peff