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

[Junio C Hamano <gitster@pobox.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.

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