git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Derrick Stolee <dstolee@microsoft.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	"sbeller@google.com" <sbeller@google.com>,
	"avarab@gmail.com" <avarab@gmail.com>,
	"larsxschneider@gmail.com" <larsxschneider@gmail.com>,
	"gitster@pobox.com" <gitster@pobox.com>
Subject: Re: [PATCH v2 0/4] Lazy-load trees when reading commit-graph
Date: Fri, 6 Apr 2018 15:21:46 -0400	[thread overview]
Message-ID: <20180406192146.GC921@sigill.intra.peff.net> (raw)
In-Reply-To: <20180406190919.167092-1-dstolee@microsoft.com>

On Fri, Apr 06, 2018 at 07:09:30PM +0000, Derrick Stolee wrote:

> Derrick Stolee (4):
>   treewide: rename tree to maybe_tree
>   commit: create get_commit_tree() method
>   treewide: replace maybe_tree with accessor methods
>   commit-graph: lazy-load trees for commits

I gave this only a cursory read, but it addresses my concern from the
previous round.

If I were doing it myself, I probably would have folded patches 1 and 3
together. They are touching all the same spots, and it would be an error
for any case converted in patch 1 to not get converted in patch 3. I'm
assuming you caught them all due to Coccinelle, though IMHO it is
somewhat overkill here. By folding them together the compiler could tell
you which spots you missed.

And going forward, I doubt it is going to be a common error for people
to use maybe_tree directly. Between the name and the warning comment,
you'd have to really try to shoot yourself in the foot with it. The
primary concern was catching people using the existing "tree" name,
whose semantics changed.

All that said, I'm fine with having it done this way, too.

-Peff

  parent reply	other threads:[~2018-04-06 19:21 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-03 12:00 [PATCH 0/3] Lazy-load trees when reading commit-graph Derrick Stolee
2018-04-03 12:00 ` [PATCH 1/3] commit: create get_commit_tree() method Derrick Stolee
2018-04-03 12:00 ` [PATCH 2/3] treewide: use get_commit_tree() for tree access Derrick Stolee
2018-04-03 12:00 ` [PATCH 3/3] commit-graph: lazy-load trees Derrick Stolee
2018-04-03 18:00   ` Stefan Beller
2018-04-03 18:22     ` Derrick Stolee
2018-04-03 18:37       ` Stefan Beller
2018-04-03 12:15 ` [PATCH 0/3] Lazy-load trees when reading commit-graph Derrick Stolee
2018-04-03 13:06 ` Jeff King
2018-04-03 13:14   ` Derrick Stolee
2018-04-03 20:20     ` Jeff King
2018-04-04 12:08       ` Derrick Stolee
2018-04-06 19:09 ` [PATCH v2 0/4] " Derrick Stolee
2018-04-06 19:09   ` [PATCH v2 1/4] treewide: rename tree to maybe_tree Derrick Stolee
2018-04-06 19:09   ` [PATCH v2 2/4] commit: create get_commit_tree() method Derrick Stolee
2018-04-06 19:09   ` [PATCH v2 3/4] treewide: replace maybe_tree with accessor methods Derrick Stolee
2018-04-06 19:09   ` [PATCH v2 4/4] commit-graph: lazy-load trees for commits Derrick Stolee
2018-04-06 19:21   ` Jeff King [this message]
2018-04-06 19:41     ` [PATCH v2 0/4] Lazy-load trees when reading commit-graph Derrick Stolee
2018-04-06 19:45     ` Stefan Beller
2018-04-08 23:18     ` Junio C Hamano
2018-04-09 13:15       ` Derrick Stolee
2018-04-09 17:25         ` Stefan Beller
2018-04-07 18:40   ` Jakub Narebski
2018-04-08  1:17     ` Derrick Stolee
2018-04-11 20:41       ` Jakub Narebski

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=20180406192146.GC921@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=larsxschneider@gmail.com \
    --cc=sbeller@google.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).