Re: [PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 14, 2012, 01:33 UTC
Message-ID: <7vfwee9p1t.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120213092730.GB15585@t1405.greatnet.de>

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 5 quoted lines

> Previously it was not possible to iterate revisions twice using the
> revision walking api. We add a reset_revision_walk() which clears the
> used flags. This allows us to do multiple sequencial revision walks.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>

I am kind of surprised that this is already its 5th round.

Show 19 quoted lines

> diff --git a/object.c b/object.c
> index 6b06297..6291ce9 100644
> --- a/object.c
> +++ b/object.c
> @@ -275,3 +275,14 @@ void object_array_remove_duplicates(struct object_array *array)
>  		array->nr = dst;
>  	}
>  }
> +
> +void clear_object_flags(unsigned flags)
> +{
> +	int i;
> +	struct object *obj;
> +
> +	for (i=0; i < obj_hash_size; i++) {
> +		if ((obj = obj_hash[i]) && obj->flags & flags)
> +			obj->flags &= ~flags;
> +	}
> +}

Minimally,

        void clear_object_flags(unsigned flags)
        {
                int i;

                for (i = 0; i < obj_hash_size; i++) {
                        struct object *obj = obj_hash[i];
                        if (obj && (obj->flags & flags))
                                obj->flags &= ~flags;
                }
        }

I am not sure if the "If there is any bit set we care about, drop them" buys us anything, though.

Show 9 quoted lines

> diff --git a/revision.c b/revision.c
> index c97d834..77ce6bd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2061,6 +2061,11 @@ static void set_children(struct rev_info *revs)
>  	}
>  }
>  
> +void reset_revision_walk()

	void reset_revision_walk(void)

> +{
> +	clear_object_flags(SEEN | ADDED | SHOWN);
> +}

But is this really the right API? After a particular program finishes using the revision walker, wouldn't it want to clear both the set of these standard flag bits used by the traversal machinery, as well as whatever program specific bits it used to mark the objects with?

Show 7 quoted lines

> diff --git a/revision.h b/revision.h
> index b8e9223..3535733 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -192,6 +192,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct
>  ...
> +extern void reset_revision_walk();

Likewise, "extern void reset_revision_walk(void);".

Show 20 quoted lines

> diff --git a/submodule.c b/submodule.c
> index 9a28060..645ff5d 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -404,6 +404,7 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
>  	while ((commit = get_revision(&rev)) && !needs_pushing)
>  		commit_need_pushing(commit, &needs_pushing);
>  
> +	reset_revision_walk();
>  	free(sha1_copy);
>  	strbuf_release(&remotes_arg);
>  
> @@ -741,6 +742,7 @@ static int find_first_merges(struct object_array *result, const char *path,
>  		if (in_merge_bases(b, &commit, 1))
>  			add_object_array(o, NULL, &merges);
>  	}
> +	reset_revision_walk();
>  
>  	/* Now we've got all merges that contain a and b. Prune all
>  	 * merges that contain another found merge and save them in

These two hunk look like a *BUGFIX* to me (certainly it does not look like this is an addition of any new feature).

What bug does this fix, and how is the current submodule code broken without this patch? Can you describe the problem in the log message, and add a test to demonstrate the existing breakage?