Re: [PATCH] revision: introduce prepare_revision_walk_extended()

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 19, 2017, 18:33 UTC
Message-ID: <xmqq7etiworw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171219114906.GB24558@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

> On Mon, Dec 18, 2017 at 08:18:19PM +0100, René Scharfe wrote:
>
>> > The root of the matter is that the revision-walking code doesn't clean
>> > up after itself. In every case, the caller is just saving these to clean
>> > up commit marks, isn't it?
>> 
>> bundle also checks if the pending objects exists.
>
> Thanks, I missed that one. So just adding a feature to clean up commit
> marks wouldn't be sufficient to cover that case.

OK.

Show 14 quoted lines

>> > That sidesteps all of the memory ownership issues by just creating a
>> > copy. That's less efficient, but I'd be surprised if it matters in
>> > practice (we tend to do one or two revisions per process, there don't
>> > tend to be a lot of pending tips, and we're really just talking about
>> > copying some pointers here).
>> [...]
>> I don't know if there can be real-world use cases with millions of
>> entries (when it would start to hurt).
>
> I've seen repos which have tens of thousands of tags. Something like
> "rev-list --all" would have tens of thousands of pending objects.
> I think in practice it's limited to the number of objects (though in
> practice more like the number of commits).
> ...

OK again; that is an argument against "let's copy the array".

>> Why does prepare_revision_walk() clear the list of pending objects at
>> all?  Assuming the list is append-only then perhaps remembering the
>> last handled index would suffice.

For that matter, why does it copy, instead of using revs->pending directly? I do not think I can answer this, as I think the design decisions led to this code predates me.

In any case, it seems that the discussion has veered into an interesting tangent but at this point it seems to me that it is not likely to produce an immediate improvement over the posted patch.

Should we take the patch posted as-is and move forward?