From: Emily Shaffer <emilyshaffer@google.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2] clone: pass --single-branch during --recurse-submodules
Date: Thu, 20 Feb 2020 18:53:06 -0800 [thread overview]
Message-ID: <20200221025306.GK2447@google.com> (raw)
In-Reply-To: <20200130102303.GD840531@coredump.intra.peff.net>
On Thu, Jan 30, 2020 at 05:23:03AM -0500, Jeff King wrote:
> On Tue, Jan 28, 2020 at 02:17:36PM -0800, Emily Shaffer wrote:
Ouch. I forgot about this review for some time. Sorry :)
>
> > Previously, performing "git clone --recurse-submodules --single-branch"
> > resulted in submodules cloning all branches even though the superproject
> > cloned only one branch. Pipe --single-branch through the submodule
> > helper framework to make it to 'clone' later on.
>
> This makes sense to me, bearing in mind that I'm not at all a good
> person to point out subtleties with submodules that could bite us.
I wonder if it makes sense to ship this to our submodule-using masses
here for a little while and see how it works / whether anybody yells?
This might be too small for that kind of thing.
>
> > As discussed in the thread for v1 of this patch, in cases when the
> > submodule commit referenced by the specified superproject branch isn't
> > the same as the HEAD of the submodule repo known by the server side,
> > this still works in kind of a non-obvious way. In these cases, first we
> > fetch the single branch that is the ancestor of the server's HEAD; then
> > we fetch the commit needed by the superproject (and its ancestry). So
> > while this change prevents us from fetching *all* branches on clone, it
> > doesn't necessarily limit us to a single branch as described.
>
> Is it worth adding a test that we do the right thing here? Not so much
> to prove that it works now, but to protect us against future changes. It
> seems like the sort of thing that could get subtly broken.
What did you have in mind beyond the test I added already?
>
> The patch looks mostly good to me (my, that was a lot of plumbing
> through that option); here are a few minor comments:
>
> > -update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]::
> > +update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--jobs <n>] [--single-branch] [--] [<path>...]::
>
> This line is horrendously long. Not new in your patch, but I wonder if
> the time might have come to break it up.
I dug around in Asciidoc doc and couldn't find a good way to do so. The
trailing :: means this command listing is done as a "definition list",
and I just didn't see any way to multiline an entry for such a thing. :(
>
> > +--single-branch::
> > + This option is only valid for the update command.
> > + Clone only one branch during update, HEAD or --branch.
>
> For some reason my brain insists on parsing this second sentence as a
> 3-item list without an Oxford comma. I wonder if it would be more clear
> as:
>
> Clone only one branch during update: HEAD or one specified by
> --branch.
>
> or similar.
Took it verbatim, I agree.
>
> > #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
> > SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, 0, \
> > - NULL, NULL, NULL, \
> > + NULL, NULL, NULL, 0,\
> > NULL, 0, 0, 0, NULL, 0, 0, 1}
>
> Wow. Also not new in your patch, but I think we're moving towards the
> use of C99 named initializers, which would make this a bit less
> daunting (all of the NULL/0 items could be omitted!).
Hrm. Yeah, I'll add a quick patch before this one to do so. It's pretty
gross :)
> > +test_expect_success 'clone with --single-branch' '
> > + test_when_finished "rm -rf super_clone" &&
> > + git clone --recurse-submodules --single-branch "file://$pwd/." super_clone &&
> > + (
> > + cd super_clone/sub &&
> > + git branch -a >branches &&
> > + test_must_fail grep other branches
> > + )
> > +'
>
> Don't use test_must_fail with non-Git commands; you can just say "! grep".
>
> We usually try to avoid scripting around git-branch output (although I
ooof
> find it pretty unlikely that future changes would break this particular
> case). git-for-each-ref would be a better pick, but I wonder if:
>
> git rev-parse --verify origin/master &&
> test_must_fail git rev-parse --verify origin/other
>
> might express the expectation more clearly.
Sure, done.
Sorry again for the long wait, and thanks for the effort on the review.
New revision coming momentarily.
- Emily
next prev parent reply other threads:[~2020-02-21 2:53 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-08 23:19 [PATCH] clone: teach --single-branch and --branch during --recurse Emily Shaffer
2020-01-08 23:39 ` Emily Shaffer
2020-01-09 8:11 ` Jeff King
2020-01-16 22:38 ` Emily Shaffer
2020-01-17 21:03 ` Jeff King
2020-01-27 22:20 ` Emily Shaffer
2020-01-27 22:49 ` Emily Shaffer
2020-01-27 23:10 ` Jeff King
2020-01-28 1:08 ` Emily Shaffer
2020-01-28 1:31 ` Jeff King
2020-01-28 2:10 ` Emily Shaffer
2020-01-27 23:00 ` Jeff King
2020-01-28 22:17 ` [PATCH v2] clone: pass --single-branch during --recurse-submodules Emily Shaffer
2020-01-30 10:23 ` Jeff King
2020-02-21 2:53 ` Emily Shaffer [this message]
2020-02-21 3:16 ` Jeff King
2020-02-21 3:10 ` [PATCH v3 0/2] propagate --single-branch during clone --recurse-submodules Emily Shaffer
2020-02-21 3:10 ` [PATCH v3 1/2] submodule--helper: use C99 named initializer Emily Shaffer
2020-02-21 5:26 ` Jeff King
2020-02-22 20:13 ` Junio C Hamano
2020-02-21 3:10 ` [PATCH v3 2/2] clone: pass --single-branch during --recurse-submodules Emily Shaffer
2020-02-21 5:28 ` Jeff King
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=20200221025306.GK2447@google.com \
--to=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/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).