git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: chooglen@google.com
Cc: git@vger.kernel.org, jonathantanmy@google.com,
	steadmon@google.com, emilyshaffer@google.com
Subject: Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation
Date: Tue, 23 Nov 2021 17:31:53 -0800	[thread overview]
Message-ID: <20211124013153.3619998-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20211122223252.19922-5-chooglen@google.com>

Glen Choo <chooglen@google.com> writes:
> +submodule.propagateBranches::
> +	[EXPERIMENTAL] A boolean that enables branching support with
> +	submodules. This allows certain commands to accept
> +	`--recurse-submodules` (`git branch --recurse-submodules` will
> +	create branches recursively), and certain commands that already
> +	accept `--recurse-submodules` will now consider branches (`git
> +	switch --recurse-submodules` will switch to the correct branch
> +	in all submodules).

After some thought, I think that the way in this patch is the best way
to do it, even if it's unfortunate that a natural-looking "git branch
--recurse-submodules" wouldn't work without
"submodule.propagateBranches" (or whatever we decide to call the
variable).

Right now, as far as I know, "--recurse-submodules" in all Git commands
(and equivalently, the configuration variable "submodule.recurse") does
the same thing but in submodules too. For example, "fetch" fetches in
submodules, "switch" switches in submodules, and so on. It would be
plausible for "branch --recurse-submodules" to also create a branch in
submodules, but if a user just used it as-is, it would be surprising if
subsequent commands like "switch --recurse-submodules" or "-c
submodule.recurse=true switch" didn't use the branches that were just
created.

So OK, this makes sense.

> +static int submodule_create_branch(struct repository *r, const char *name,
> +				   const char *start_oid,
> +				   const char *start_name, int force,
> +				   int reflog, int quiet,
> +				   enum branch_track track, char **err_msg)
> +{
> +	int ret = 0;
> +	struct child_process child = CHILD_PROCESS_INIT;
> +	struct strbuf child_err = STRBUF_INIT;
> +	child.git_cmd = 1;
> +	child.err = -1;
> +
> +	prepare_other_repo_env(&child.env_array, r->gitdir);
> +	strvec_pushl(&child.args, "submodule--helper", "create-branch", NULL);

Before this function is a function that calls "git branch" directly -
couldn't this function do the same? (And then you can make both of them
into one function.) The functionality should be exactly the same except
that one has "--dry-run" and the other doesn't.

> @@ -874,6 +894,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  			FREE_AND_NULL(unused_full_ref);
>  			return 0;
>  		}
> +		if (recurse_submodules) {
> +			create_submodule_branches(the_repository, branch_name,
> +						  start_name, force, reflog,
> +						  quiet, track);
> +			return 0;
> +		}
>  		create_branch(the_repository, branch_name, start_name, force, 0,
>  			      reflog, quiet, track);
>  	} else

create_submodule_branches() here is a bit misleading since it also
creates branches in the_repository. Might be better to write explicitly
check in submodules -> create in main repository -> create in
submodules. Or, if you want to combine all the submodule code in one
function, (check in submodules -> create in submodules) -> create in
main repository.

> +test_expect_success 'setup superproject and submodule' '
> +	git init super &&
> +	test_commit foo &&
> +	git init sub-upstream &&
> +	test_commit -C sub-upstream foo &&
> +	git -C super submodule add ../sub-upstream sub &&
> +	git -C super commit -m "add submodule" &&
> +	git -C super config submodule.propagateBranches true
> +'

If making each test independent is important (which seems like a good
goal to me, although I know that the Git tests are inconsistent on
that), we could make this into a bash function (with test_when_finished)
that gets called in every test. It doesn't violate the t/README request
to put all test code inside test_expect_success assertions (since the
function is still being run inside an assertion).

In the general case, it will make test code slower to run, but if you're
going to have to cleanup branches, I think it's better to just recreate
the repo. In any case, for the general case, I can start a separate
email thread for this discussion.

> +test_expect_success '--recurse-submodules should be ignored if submodule.propagateBranches is false' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git -c submodule.propagateBranches=false branch --recurse-submodules branch-a &&
> +		git rev-parse branch-a &&
> +		test_must_fail git -C sub rev-parse branch-a
> +	)
> +'

This doesn't sound like the right behavior to me - I think it's fine if
it was the config "submodule.recurse" instead of "--recurse-submodules",
but if the argument is given on CLI, it should be a fatal error.

> +test_expect_success 'should create branch when submodule is in .git/modules but not .gitmodules' '
> +	test_when_finished "cleanup_branches super branch-a branch-b branch-c" &&
> +	(
> +		cd super &&
> +		git branch branch-a &&
> +		git checkout -b branch-b &&
> +		git submodule add ../sub-upstream sub2 &&
> +		# branch-b now has a committed submodule not in branch-a
> +		git commit -m "add second submodule" &&
> +		git checkout branch-a &&
> +		git branch --recurse-submodules branch-c branch-b &&
> +		git rev-parse branch-c &&
> +		git -C sub rev-parse branch-c &&
> +		git checkout --recurse-submodules branch-c &&
> +		git -C sub2 rev-parse branch-c
> +	)
> +'

Hmm...how is this submodule in .git/modules but not .gitmodules? It
looks like a normal submodule to me.

> +test_expect_success 'should not create branch when submodule is not in .git/modules' '

The title of this test contradicts the title of the test that I quoted
previously.

> +test_expect_success 'should not fail when unable to set up tracking in submodule' '
> +	test_when_finished "cleanup_branches super-clone branch-b" &&
> +	(
> +		cd super-clone &&
> +		git branch --recurse-submodules branch-b origin/branch-b
> +	)
> +'

Is there a warning printed that we can check?

Also, this patch set doesn't discuss the case in which the branch in a
submodule already exists, but it points to the exact commit that we
want. What is the functionality in that case? I would say that the user
should be able to recursively create the branch in this case, but am
open to other opinions. In any case, that case should be tested.

  parent reply	other threads:[~2021-11-24  1:32 UTC|newest]

Thread overview: 110+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-22 22:32 [PATCH 0/4] implement branch --recurse-submodules Glen Choo
2021-11-22 22:32 ` [PATCH 1/4] submodule-config: add submodules_of_tree() helper Glen Choo
2021-11-23  2:12   ` Jonathan Tan
2021-11-23 19:48     ` Glen Choo
2021-11-23 10:53   ` Ævar Arnfjörð Bjarmason
2021-11-23 18:35     ` Glen Choo
2021-11-23 22:46   ` Junio C Hamano
2021-11-22 22:32 ` [PATCH 2/4] branch: refactor out branch validation from create_branch() Glen Choo
2021-11-22 22:32 ` [PATCH 3/4] branch: add --dry-run option to branch Glen Choo
2021-11-23 10:42   ` Ævar Arnfjörð Bjarmason
2021-11-23 18:42     ` Glen Choo
2021-11-23 23:10   ` Jonathan Tan
2021-11-24  0:52     ` Glen Choo
2021-11-22 22:32 ` [PATCH 4/4] branch: add --recurse-submodules option for branch creation Glen Choo
2021-11-23 10:45   ` Ævar Arnfjörð Bjarmason
2021-11-23 18:56     ` Glen Choo
2021-11-23 19:41   ` Philippe Blain
2021-11-23 23:43     ` Glen Choo
2021-11-24  1:31   ` Jonathan Tan [this message]
2021-11-24 18:18     ` Glen Choo
2021-11-29 21:01       ` Jonathan Tan
2021-12-06 21:55 ` [PATCH v2 0/3] implement branch --recurse-submodules Glen Choo
2021-12-06 21:55   ` [PATCH v2 1/3] branch: move --set-upstream-to behavior to setup_tracking() Glen Choo
2021-12-06 22:48     ` Junio C Hamano
2021-12-08 18:48       ` Glen Choo
2021-12-06 23:28     ` Junio C Hamano
2021-12-08 17:09       ` Glen Choo
2021-12-06 21:55   ` [PATCH v2 2/3] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-06 21:55   ` [PATCH v2 3/3] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-09 18:49   ` [PATCH v3 0/5] implement branch --recurse-submodules Glen Choo
2021-12-09 18:49     ` [PATCH v3 1/5] branch: move --set-upstream-to behavior to setup_tracking() Glen Choo
2021-12-09 21:19       ` Jonathan Tan
2021-12-09 22:16         ` Glen Choo
2021-12-09 18:49     ` [PATCH v3 2/5] branch: remove forward declaration of validate_branch_start() Glen Choo
2021-12-09 18:49     ` [PATCH v3 3/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-09 21:23       ` Jonathan Tan
2021-12-09 21:57         ` Glen Choo
2021-12-09 18:49     ` [PATCH v3 4/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-11 18:08       ` Philippe Blain
2021-12-14 20:08         ` Glen Choo
2021-12-09 18:49     ` [PATCH v3 5/5] branch.c: replace questionable exit() codes Glen Choo
2021-12-10  2:21       ` Ævar Arnfjörð Bjarmason
2021-12-10 17:43         ` Glen Choo
2021-12-13  9:02         ` Junio C Hamano
2021-12-13  9:19           ` Ævar Arnfjörð Bjarmason
2021-12-13 19:26             ` Junio C Hamano
2021-12-09 21:59     ` [PATCH v3 0/5] implement branch --recurse-submodules Jonathan Tan
2021-12-09 22:21       ` Glen Choo
2021-12-13 23:20         ` Jonathan Tan
2021-12-14 18:47           ` Glen Choo
2021-12-16  0:32     ` [PATCH v4 " Glen Choo
2021-12-16  0:32       ` [PATCH v4 1/5] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2021-12-16  0:32       ` [PATCH v4 2/5] branch: make create_branch() always create a branch Glen Choo
2021-12-16  0:32       ` [PATCH v4 3/5] branch: add a dry_run parameter to create_branch() Glen Choo
2021-12-16  0:32       ` [PATCH v4 4/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-16  0:32       ` [PATCH v4 5/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-16 23:33       ` [PATCH v5 0/5] implement branch --recurse-submodules Glen Choo
2021-12-16 23:33         ` [PATCH v5 1/5] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2021-12-16 23:33         ` [PATCH v5 2/5] branch: make create_branch() always create a branch Glen Choo
2021-12-16 23:33         ` [PATCH v5 3/5] branch: add a dry_run parameter to create_branch() Glen Choo
2021-12-16 23:33         ` [PATCH v5 4/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-16 23:33         ` [PATCH v5 5/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-17  0:34         ` [PATCH v5 0/5] implement branch --recurse-submodules Junio C Hamano
2021-12-17  0:45           ` Junio C Hamano
2021-12-20 19:09             ` Glen Choo
2021-12-20 19:50               ` Junio C Hamano
2021-12-20 20:25                 ` Glen Choo
2021-12-20 23:34         ` [PATCH v6 " Glen Choo
2021-12-20 23:34           ` [PATCH v6 1/5] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2022-01-11  2:09             ` Jonathan Tan
2022-01-11 17:29               ` Glen Choo
2022-01-11 20:03                 ` Jonathan Tan
2021-12-20 23:34           ` [PATCH v6 2/5] branch: make create_branch() always create a branch Glen Choo
2022-01-11  2:19             ` Jonathan Tan
2022-01-11 17:51               ` Glen Choo
2021-12-20 23:34           ` [PATCH v6 3/5] branch: add a dry_run parameter to create_branch() Glen Choo
2021-12-20 23:34           ` [PATCH v6 4/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-20 23:34           ` [PATCH v6 5/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-26  4:09             ` Junio C Hamano
2022-01-11  3:28             ` Jonathan Tan
2022-01-11 18:11               ` Glen Choo
2022-01-11 20:15                 ` Jonathan Tan
2022-01-11 23:22                   ` Glen Choo
2021-12-20 23:36           ` [PATCH v6 0/5] implement branch --recurse-submodules Glen Choo
2021-12-21  1:07           ` Junio C Hamano
2021-12-21 17:51             ` Glen Choo
2022-01-24 20:44           ` [PATCH v7 0/6] " Glen Choo
2022-01-24 20:44             ` [PATCH v7 1/6] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2022-01-24 20:44             ` [PATCH v7 2/6] branch: make create_branch() always create a branch Glen Choo
2022-01-24 20:44             ` [PATCH v7 3/6] branch: add a dry_run parameter to create_branch() Glen Choo
2022-01-24 20:44             ` [PATCH v7 4/6] builtin/branch: consolidate action-picking logic in cmd_branch() Glen Choo
2022-01-24 20:44             ` [PATCH v7 5/6] branch: add --recurse-submodules option for branch creation Glen Choo
2022-01-27 20:29               ` Jonathan Tan
2022-01-27 21:32                 ` Glen Choo
2022-01-27 22:42                   ` Glen Choo
2022-01-24 20:44             ` [PATCH v7 6/6] branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks Glen Choo
2022-01-27 22:15               ` Junio C Hamano
2022-01-28 19:44                 ` Glen Choo
2022-01-29  0:04             ` [PATCH v8 0/6] implement branch --recurse-submodules Glen Choo
2022-01-29  0:04               ` [PATCH v8 1/6] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2022-01-29  0:04               ` [PATCH v8 2/6] branch: make create_branch() always create a branch Glen Choo
2022-02-01 22:20                 ` Junio C Hamano
2022-01-29  0:04               ` [PATCH v8 3/6] branch: add a dry_run parameter to create_branch() Glen Choo
2022-01-29  0:04               ` [PATCH v8 4/6] builtin/branch: consolidate action-picking logic in cmd_branch() Glen Choo
2022-01-29  0:04               ` [PATCH v8 5/6] branch: add --recurse-submodules option for branch creation Glen Choo
2022-02-04  1:10                 ` Glen Choo
2022-02-04 16:15                   ` Junio C Hamano
2022-02-04 18:10                     ` Glen Choo
2022-01-29  0:04               ` [PATCH v8 6/6] branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks Glen Choo
2022-02-01 17:43               ` [PATCH v8 0/6] implement branch --recurse-submodules Jonathan Tan

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=20211124013153.3619998-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=chooglen@google.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=steadmon@google.com \
    /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).