Re: [PATCH 1/1] revision: don't set parents as uninteresting if exclude promisor objects

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2024, 16:45 UTC
Message-ID: <xmqq4j82euvr.fsf@gitster.g>
In-Reply-To: <20240802073143.56731-2-hanyang.tony@bytedance.com>

Han Young <hanyang.tony@bytedance.com> writes:

> In revision.c, `process_parents()` recursively marks commit parents 
> as UNINTERESTING if the commit itself is UNINTERESTING.

Makes sense.

> `--exclude-promisor-objects` is implemented as 
> "iterate all objects in promisor packfiles, mark them as UNINTERESTING".

Also makes sense.

> So when we find a commit object in a promisor packfile, we also set its ancestors 
> as UNINTERESTING, whether the ancestor is a promisor object or not.
> This causes normal objects to be falsely marked as promisor objects 
> and removed by `git repack`.

Ahh, that is not desirable. So the need to do something to fix it is well established here.

> Signed-off-by: Han Young <hanyang.tony@bytedance.com>
> Helped-by: C O Xing Xin <xingxin.xx@bytedance.com>
> ---

Please order these trailer lines logically in chronological order, i.e. you get helped by others to complete the change and then seal it by signing it off at the end. I'll swap these two while queuing.

Show 13 quoted lines

> diff --git a/revision.c b/revision.c
> index 1c0192f522..eacb0c909d 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1164,7 +1164,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
>  	 * wasn't uninteresting), in which case we need
>  	 * to mark its parents recursively too..
>  	 */
> -	if (commit->object.flags & UNINTERESTING) {
> +	if (!revs->exclude_promisor_objects && commit->object.flags & UNINTERESTING) {
>  		while (parent) {
>  			struct commit *p = parent->item;
>  			parent = parent->next;

But if the iteration is over all objects in certain packfiles to mark them all uninteresting, shouldn't the caller avoid the call to process_parents() in the first place? Letting process_parents() to do other things and only refrain from doing the "this commit is marked uninteresting" part does not quite match what you are trying to do, at least to me.

Please check "git blame" to find those who are likely to know better about the code around the area and ask for help from them. I think the bulk of the logic related came from the series that led to f3d618d2 (Merge branch 'jh/fsck-promisors', 2018-02-13), so I added the authors of the series.

It apepars to me that its approach to exclude the objects that appear in the promisor packs may be sound, but the design and implementation of it is dubious. Shouldn't it be getting the list of objects with get_object_list() WITHOUT paying any attention to --exclude-promisor-objects flag, and then filtering objects that appear in the promisor packs out of that list, without mucking with the object and commit traversal in revision.c at all?

Thanks.