From: Jeff King <peff@peff.net>
To: Elijah Newren <newren@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 3/3] traverse_trees(): use stack array for name entries
Date: Fri, 31 Jan 2020 04:29:36 -0500 [thread overview]
Message-ID: <20200131092936.GA2916051@coredump.intra.peff.net> (raw)
In-Reply-To: <CABPp-BE7E--8Yz3PAcjPQX2RCsbq0Q0gURi3RJuE64KM0eo6mA@mail.gmail.com>
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.
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
next prev parent reply other threads:[~2020-01-31 9:29 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-30 9:51 [PATCH 0/3] some minor memory allocation cleanups Jeff King
2020-01-30 9:52 ` [PATCH 1/3] normalize_path_copy(): document "dst" size expectations Jeff King
2020-01-30 20:12 ` Taylor Blau
2020-01-31 8:45 ` Jeff King
2020-01-30 9:52 ` [PATCH 2/3] walker_fetch(): avoid raw array length computation Jeff King
2020-01-30 9:53 ` [PATCH 3/3] traverse_trees(): use stack array for name entries Jeff King
2020-01-30 14:57 ` Elijah Newren
2020-01-31 9:29 ` Jeff King [this message]
2020-01-31 18:52 ` Elijah Newren
2020-02-01 11:39 ` [PATCH] tree-walk.c: break circular dependency with unpack-trees Jeff King
2020-02-01 15:32 ` Elijah Newren
2020-01-30 14:59 ` [PATCH 0/3] some minor memory allocation cleanups Elijah Newren
2020-01-30 23:03 ` Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200131092936.GA2916051@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=newren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.