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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 22, 2021, 11:10 UTC
Message-ID: <87k0j87tdw.fsf@evledraar.gmail.com>
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);

Show 6 quoted lines

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

Show 15 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.git

> [...]

Show 17 quoted lines

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

Show 8 quoted lines

> [...]
> +		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 :)