Re: [PATCH 3/3] traverse_trees(): use stack array for name entries

From: Elijah Newren <newren@gmail.com>
Date: Jan 31, 2020, 18:52 UTC
Message-ID: <CABPp-BFXwin5mR7z7Ocx9bq5LadFF4CxZR03HzZxh5f=1+F6NQ@mail.gmail.com>
In-Reply-To: <20200131092936.GA2916051@coredump.intra.peff.net>

On Fri, Jan 31, 2020 at 1:29 AM Jeff King <peff@peff.net> wrote:

Show 21 quoted lines

>
> On Thu, Jan 30, 2020 at 06:57:26AM -0800, Elijah Newren wrote:
>
> > > This does increase the coupling between tree-walk and unpack-trees a
> > > bit. I'd be OK just switching to ALLOC_ARRAY(), too. I doubt the
> > > performance improvement is measurable, and the cleanup free() calls are
> > > already there.
> >
> > Could we undo this cyclic dependency between tree-walk and
> > unpack-trees by defining MAX_TRAVERSE_TREES in tree-walk.h, making
> > MAX_UNPACK_TREES in unpack-trees.h be defined to MAX_TRAVERSE_TREES,
> > and remove the include of unpack-trees.h in tree-walk.c?
>
> I don't mind doing that, but I had a hard time trying to write a commit
> message. I.e., I can explain the current use of MAX_UNPACK_TREES here,
> or defining MAX_TRAVERSE_TREES as MAX_UNPACK_TREES by saying "this is an
> arbitrary limit, but it's the highest value any caller would use with
> us".
>
> But to define MAX_UNPACK_TREES in terms of MAX_TRAVERSE_TREES, I feel
> we've created a circular reasoning in the justification.

It may be circular in how it came about, but I don't see how it's circular from first principles: unpack_trees() uses traverse_trees() to do its work (whereas tree-walk.c shouldn't call any functions from unpack-trees). For simplicity and lacking any justification for huge numbers of trees, we want to impose a small limit on both for how many trees they can simultaneously operate on -- though we want to leave a little wiggle room by generously setting the limit much higher than we expect any caller to ever want. unpack-trees certainly can't have a higher limit than tree-walk, and we don't know of callers outside unpack_trees that would want to traverse more trees than unpack_trees() does, so we just set them both to the same value, 8.

Show 23 quoted lines

> I'm not even sure whether the current value of 8 is meaningful. It comes
> from this commit:
>
>   commit ca885a4fe6444bed840295378848904106c87c85
>   Author: Junio C Hamano <gitster@pobox.com>
>   Date:   Thu Mar 13 22:07:18 2008 -0700
>
>       read-tree() and unpack_trees(): use consistent limit
>
>       read-tree -m can read up to MAX_TREES, which was arbitrarily set to 8 since
>       August 2007 (4 is needed to deal with 2 merge-base case).
>
>       However, the updated unpack_trees() code had an advertised limit of 4
>       (which it enforced).  In reality the code was prepared to take only 3
>       trees and giving 4 caused it to stomp on its stack.  Rename the MAX_TREES
>       constant to MAX_UNPACK_TREES, move it to the unpack-trees.h common header
>       file, and use it from both places to avoid future confusion.
>
> which kind of makes me wonder if we should just go back to calling it
> MAX_TREES. I guess that's too vague, though.
>
> So I dunno. It would be easy to do as you asked, but I'm not convinced
> it actually untangles anything meaningful.

As far as I can tell, before your change, we can remove the
   #include "unpack-trees.h"
from tree-walk.c; nothing uses it.  I do like snipping includes and
dependencies where they aren't necessary, and this one seems like one
that could be removed.

But, it's not a big deal; if you want to leave that for future work for someone else (perhaps even asking me to turn my paragraph above into a commit message that rips it out and defines one #define in terms of the other), that's fine.