Re: [RFC PATCH 2/2] branch: add --recurse-submodules option for branch creation

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>  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

>> +	if (!recurse_submodules) {
>> +		return;
>> +	}
>
> Can lose the braces here...

Ah, yes. Old habits die hard.. Thanks!

>> +
>> +	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

Ah, 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 :)

> 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:
 
>> * 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.