From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Glen Choo <chooglen@google.com>
Cc: git@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] branch: add --recurse-submodules option for branch creation
Date: Wed, 22 Sep 2021 13:10:11 +0200 [thread overview]
Message-ID: <87k0j87tdw.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20210921232529.81811-3-chooglen@google.com>
On Tue, Sep 21 2021, Glen Choo wrote:
> static const char *head;
> static struct object_id head_oid;
> +static int recurse_submodules = 0;
Nit: just s/ = 0// will do here, and is the convention typically...
> + r_start_name = strcmp(start_name, "HEAD") ? start_name :
> + refs_resolve_refdup(get_main_ref_store(r), start_name, 0, NULL, NULL);
IMO clearer just as an if/else.
> + if (strcmp(r_start_name, "HEAD"))
> + skip_prefix(r_start_name, "refs/heads/", &r_start_name);
> + create_branch(r, name, r_start_name, force, clobber_head_ok, reflog,
> + quiet, track);
> +
> + if (!recurse_submodules) {
> + return;
> + }
Can lose the braces here...
> +
> + if (repo_read_index(r) < 0)
> + die(_("index file corrupt"));
...Just as you do here..
> +
> + for (i = 0; i < r->index->cache_nr; i++) {
> + const struct cache_entry *ce = r->index->cache[i];
> + if (!S_ISGITLINK(ce->ce_mode))
> + continue;
> + sub = submodule_from_path(r, null_oid(), ce->name);
> + if (repo_submodule_init(&subrepo, r, sub) < 0) {
> + warning(_("Unable to initialize subrepo %s, skipping."), ce->name);
> + continue;
> + }
> + /*
> + * NEEDSWORK: branch tracking is not supported for
> + * submodules because it is not possible to get the
> + * remotes of a submodule.
> + */
It isn't?
$ git -C sha1collisiondetection/ remote -v
origin https://github.com/cr-marcstevens/sha1collisiondetection.git
> [...]
> +test_expect_success 'setup superproject and submodule' '
> + git init parent &&
> + test_commit -C parent bar &&
> + git init child &&
> + test_commit -C child bar &&
> + git -C parent submodule add ../child sub &&
> + git -C parent commit -m "add submodule"
> +'
> +
> +test_expect_success '--recurse-submodules should create branches' '
> + (
> + cd parent &&
> + git branch --recurse-submodules abc &&
> + test_path_is_file .git/refs/heads/abc &&
> + test_path_is_file .git/modules/sub/refs/heads/abc
> + )
> +'
All this manual file checking should depend on REFFILES, but better yet
is there a reason this can't use rev-parse? I.e. why can't we inpect
this state with 'for-each-ref', 'rev-parse' and the like? Does this test
need to assert that these files end up in these specific locations, or
just the ref store? Ditto for the later ones.
> [...]
> + cd parent &&
> + git -c branch.autoSetupMerge=always branch --recurse-submodules ghi main &&
> + test_path_is_file .git/modules/sub/refs/heads/ghi &&
> + test "$(git config branch.ghi.remote)" = . &&
> + test "$(git config branch.ghi.merge)" = refs/heads/main &&
> + test "z$(git -C ../child config branch.ghi.remote)" = z &&
> + test "z$(git -C ../child config branch.ghi.merge)" = z
Use test_cmp, also for this sort of thing the test "x$y" = "x" idiom
isn't needed unless you've got a computer from the mid-90s or something
:)
next prev parent reply other threads:[~2021-09-22 11:17 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-21 23:25 [RFC PATCH 0/2] branch: implement in-process --recurse-submodules Glen Choo
2021-09-21 23:25 ` [RFC PATCH 1/2] refs: pass struct repository *r through to write_ref_to_lockfile() Glen Choo
2021-09-21 23:25 ` [RFC PATCH 2/2] branch: add --recurse-submodules option for branch creation Glen Choo
2021-09-22 11:10 ` Ævar Arnfjörð Bjarmason [this message]
2021-09-22 16:55 ` Glen Choo
2021-09-22 12:28 ` Philippe Blain
2021-09-22 17:24 ` Glen Choo
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=87k0j87tdw.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=chooglen@google.com \
--cc=git@vger.kernel.org \
/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).