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

From: Jeff King <peff@peff.net>
Date: Jan 31, 2020, 09:29 UTC
Message-ID: <20200131092936.GA2916051@coredump.intra.peff.net>
In-Reply-To: <CABPp-BE7E--8Yz3PAcjPQX2RCsbq0Q0gURi3RJuE64KM0eo6mA@mail.gmail.com>

On Thu, Jan 30, 2020 at 06:57:26AM -0800, Elijah Newren wrote:

Show 9 quoted lines

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

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.

-Peff