Re: [RFC PATCH 2/2] branch: add --recurse-submodules option for branch creation
- From
Glen Choo <chooglen@google.com>
- Date
- Sep 22, 2021, 16:55 UTC
- Message-ID
- <kl6lv92s35z3.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <87k0j87tdw.fsf@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 5 quoted lines
>> 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...
Thanks for the catch!
>> + 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.
Sounds good, I'll use an if/else instead
Show 5 quoted lines
>> + if (!recurse_submodules) {
>> + return;
>> + }
>
> Can lose the braces here...Ah, yes. Old habits die hard.. Thanks!
Show 20 quoted lines
>> +
>> + 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.gitAh, my comment is ambiguous. I meant that we cannot get submodule remotes in-process because remotes.c stores its state in static variables I believe it implicitly refers to the remotes of the_repository and can't be reused for submodules.
Of course I hope I am wrong, because that would make this task a lot easier :)
Show 5 quoted lines
> 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.
> 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 > :)
The only reason the tests look this way is that I have copied the surrounding tests. From your comments, it seems clear that these tests are fairly out-of-date, so I should probably model them after something else.
I will incorporate Philippe's suggestion
Most tests for submodules are usually in separate test files. I don't think this is a set-in-stone rule, but if more tests are coming in the future, maybe a new test file t????-branch-submodule.sh would be appropriate ? Just a small suggestion.
Then at least my new tests won't look so out of place with the other branch tests.
>> In this patchset, branching works slightly differently between the >> superproject and submodule (skip ahead for specifics). There are two >> very obvious alternatives that can address this:
Show 5 quoted lines
>> * A: only implement --recurse-submodules behavior after we are able to >> eliminate any kind of dependence on the_repository/global state that >> shouldn't be shared. >> * B: implement --recurse-submodules as child processes, which won't be >> bothered by global state.
I was wondering if you had thoughts on this bit in particular. It seems unpleasant for branching to behave differently between superproject and submodule, so I'd like to discard this RFC (or at least the 'disable remotes' behavior) ASAP and start work on a version that serves us better.