From: Stefan Beller <sbeller@google.com>
To: Duy Nguyen <pclouds@gmail.com>,
"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Reviews for the first patches of pclouds/narrow-checkout
Date: Mon, 23 May 2016 11:26:16 -0700 [thread overview]
Message-ID: <CAGZ79kb-y6gUHEBSdVB6Y+A=EjykZgcNsLynXcwSMyVNHiDU2Q@mail.gmail.com> (raw)
Hi,
so I started looking into your narrow checkout branch and started reviewing
the patches. Thanks for working on the narrow checkout!
Reviewed-by: Stefan Beller <sbeller@google.com> (just asking for signoff)
tree.c: break long lines
read-cache: realign some statements
read-cache: add sort_index()
read-cache: description for base_name_compare()
unpack-trees: break down long lines
Regarding:
base_name_compare: do not assume name[len] == '\0'
Could you clarify if this is an existing bug or a preparation for a future
different use?
read-cache: new sort compare function that pays attention to ce_mode
Would a new mode (0100 (directory)) require bumping the index version?
Or the other way round: What If I use these patches and then try to use
another version of Git without this feature?
read-cache: refactor check_ce_order()
Would it make sense to avoid the yoda condition?
(i.e. "cmp > 0" instead of "0 < cmp" -> die("unordered stage entries"))
Or rather: I found it confusing that cmp is on both sides of the < in two
different conditions, i.e. it looks like you prefer to keep the "<" sign
constant, whereas I would have written
if (cmp < 0)
continue;
if (cmp > 0)
die(...);
It's a style thing, so I guess either is fine.
I would however put the case for (cmp < 0) first as that is the expected case?
read-cache: check ce_mode in check_ce_order()
Can we replace
cmp = (c1 < c2) ? -1 : (c1 > c2) ? 1 : 0;
by:
cmp = c1 - c2;
as it is only used in comparisons lesser/greater than 0 afterwards.
Again, I would put the "continue" case first (cmp >0)
read-cache: index_name_stage_pos() => index_name_mode_stage_pos()
"After this read-cache.c code is pretty much ready for accepting dir
entries in the index."
What is missing?
So that's a review for the first third of the patches. :)
I wonder how much is left for actually finishing the narrow checkout,
as I could not find documentation or code the user interacts with.
(i.e. I would like to use a narrow checkout. How do I start? Where do I put
the pathspec of things I would like to use? Or are you envisioning a git wrapper
for that? "git narrow [make-go-away, revive] <pathspec>" ?)
Thanks,
Stefan
next reply other threads:[~2016-05-23 18:31 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-23 18:26 Stefan Beller [this message]
2016-05-24 12:02 ` Reviews for the first patches of pclouds/narrow-checkout Duy Nguyen
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGZ79kb-y6gUHEBSdVB6Y+A=EjykZgcNsLynXcwSMyVNHiDU2Q@mail.gmail.com' \
--to=sbeller@google.com \
--cc=git@vger.kernel.org \
--cc=pclouds@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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).