Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option

From: Karthik Nayak <karthik.188@gmail.com>
Date: Oct 25, 2023, 09:34 UTC
Message-ID: <CAOLa=ZQ6K8g-r+Oq8Jq53QbB4qhKr6HLgpfT5OjtgzBW7DMzbg@mail.gmail.com>
In-Reply-To: <xmqqbkcn52z4.fsf@gitster.g>

On Tue, Oct 24, 2023 at 7:45 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 9 quoted lines

> I would suspect that swapping if/else would make it easier to
> follow.  Everybody else in the patch guards the use of the oidset
> with "were we told not to die on missing objects?", i.e.,
>
>         if (revs->do_not_die_on_missing_objects)
>                 oidset_insert(&revs->missing_objects, &p->object.oid);
>         else
>                 return -1; /* corrupt repository */
>

Makes sense. Will change.

Show 16 quoted lines

> > @@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)
> >                                      FOR_EACH_OBJECT_PROMISOR_ONLY);
> >       }
> >
> > +     if (revs->do_not_die_on_missing_objects)
> > +             oidset_init(&revs->missing_objects, 0);
>
> I read the patch to make sure that .missing_objects oidset is used
> only when .do_not_die_on_missing_objects is set and the oidset is
> untouched unless it is initialized.  Well done.
>
> I know I floated "perhaps oidset can replace the object bits,
> especially because the number of objects that need marking is
> expected to be small", but I am curious what the performance
> implication of this would be.  Is this something we can create
> comparison easily?

I did a simple comparision here, I randomly deleted commits which had child commits with greater than 2 parents.

$ git rev-list --missing=print HEAD | grep "?" | wc -l 828

Using the flag bit:

$ hyperfine -w 3 "~/git/bin-wrappers/git rev-list --missing=allow-any HEAD"
Benchmark 1: ~/git/bin-wrappers/git rev-list --missing=allow-any HEAD
  Time (mean ± σ):     860.5 ms ±  15.2 ms    [User: 375.3 ms, System: 467.5 ms]
  Range (min … max):   835.2 ms … 881.0 ms    10 runs

Using the oidset:

$ hyperfine -w 3 "~/git/bin-wrappers/git rev-list --missing=allow-any HEAD"
Benchmark 1: ~/git/bin-wrappers/git rev-list --missing=allow-any HEAD
  Time (mean ± σ):     901.3 ms ±   9.6 ms    [User: 394.3 ms, System: 488.3 ms]
  Range (min … max):   885.0 ms … 913.2 ms    10 runs

Its definitely slower, but not by much.

Show 27 quoted lines

>
> > I noticed that nobody releases the resource held by this new oidset.
> > Shouldn't we do so in revision.c:release_revisions()?
>
> It seems that linux-leaks CI job noticed the same.
>
> https://github.com/git/git/actions/runs/6633599458/job/18021612518#step:5:2949
>
> I wonder if the following is sufficient?
>
>  revision.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git c/revision.c w/revision.c
> index 724a116401..7a67ff74dc 100644
> --- c/revision.c
> +++ w/revision.c
> @@ -3136,6 +3136,8 @@ void release_revisions(struct rev_info *revs)
>         clear_decoration(&revs->merge_simplification, free);
>         clear_decoration(&revs->treesame, free);
>         line_log_free(revs);
> +       if (revs->do_not_die_on_missing_objects)
> +               oidset_clear(&revs->missing_objects);
>  }
>
>  static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
>

Yup, this seems right and was missed, will add.

On Wed, Oct 25, 2023 at 8:40 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 9 quoted lines

>
> >
> >       if (commit->object.flags & ADDED)
> >               return 0;
> > +     if (revs->do_not_die_on_missing_objects &&
> > +             oidset_contains(&revs->missing_objects, &commit->object.oid))
>
> Nit: indentation is off here.
>

Thanks, will fix.

Show 10 quoted lines

> > +
> > +     /* Missing objects to be tracked without failing traversal. */
> > +     struct oidset missing_objects;
>
> As far as I can see we only use this set to track missing commits, but
> none of the other objects. The name thus feels a bit misleading to me,
> as a reader might rightfully assume that it contains _all_ missing
> objects after the revwalk. Should we rename it to `missing_commits` to
> clarify?
>

Fair enough, I was thinking of being future compatible. But probably best to be specific now and refactor as needed in the future. Will change.

Thanks both for the review!