git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Glen Choo <chooglen@google.com>
Cc: git@vger.kernel.org
Subject: Re: [RFC PATCH 1/4] submodule--helper: teach --toplevel-cwd-prefix
Date: Wed, 09 Nov 2022 03:37:27 +0100	[thread overview]
Message-ID: <221109.867d04rfnt.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <20221109004708.97668-2-chooglen@google.com>


On Tue, Nov 08 2022, Glen Choo wrote:

> The top-level "--super-prefix" option (i.e. "git --super-prefix") is
> overloaded:
>
> - "git submodule--helper" passes the relative path between the top-level
>   "git" process's cwd and the current repository
> - "git read-tree" passes the path from the root of top-level
>   superproject's tree to the the current repository.
>
> In both cases, "--super-prefix" is only used to display meaningful paths
> from the superproject to a nested submodule.

The "overloaded" here could really use some elaboration.

Yes, we have different built-in commands that use it, but their use is
mostly mutually exclusive, i.e. when "submodule status" recurses into
itself it using an option called "--super-prefix" doesn't impact "git
read-tree", even though it uses the same option & env variable to
communicate the same concept.

But I'm totally willing to buy that it's simpler to e.g. have "git
submodule--helper status" or whatever use its own option to carry this
forward, rather than some option to "git" itself. Or if some commands
just need a --some-option, but not the environment variable.

But this doesn't really address that and ....

> Let's address this overloading by breaking it up into its constituent
> use cases. Teach "git submodule--helper" the "--toplevel-cwd-prefix"
> option, which replaces its usage of "git --super-prefix". (A future
> patch will address the "git read-tree" use case.) This value is only
> used in builtin/submodule--helper.c, but it is stored in submodule.c,
> since it may be needed by other builtins in the future.

...Uh, other built-ins might use it in the future? :) Wouldn't we be
right back to a --super-prefix then, which is a cross-built-in
semi-standard way to communicate this sort of information? So instead of
a hypothetical current:

	git submodule some-subcommand
		=> git --super-prefix submodule/ submodule -C submodule/ some-subcommand
			=> git --super-prefix submodule/ fetch

Or whatever, we'd do what? Have each of the now split-up commands take
their own flag for this? E.g.:

	git submodule some-subcommand
		=> git submodule -C submodule/ --toplevel-cwd-prefix submodule/ some-subcommand
			=> git fetch --submodule-prefix submodule/

Or are you really trying to say that it was a hassle to pass down this
through the various functions to all the callers, so emulating what we
did with the previous global was easier if it was a new global?
I.e. it's not about passing it to other built-ins, but between
submodule--helper itself?

I think doing that and chipping away at this from the bottom-up would be
a much better approach, at least for some cases.

E.g. just to have "absorbgitdirs" itself support a "--super-prefix"
option, not "git" or "git submodule--helper".

Then we just pass that down to absorb_git_dir_into_superproject(), which
will only invoke "git submodule--helper absorbgitdirs", which we'll then
tell about the super-prefix with a "--super-prefix" option to that
subcommand.

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c75e9e86b0..acb838e4d6 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -115,18 +115,18 @@ static char *resolve_relative_url(const char *rel_url, const char *up_path, int
>  /* the result should be freed by the caller. */
>  static char *get_submodule_displaypath(const char *path, const char *prefix)
>  {
> -	const char *super_prefix = get_super_prefix();
> +	const char *toplevel_cwd_prefix = get_toplevel_cwd_prefix();
>  
> -	if (prefix && super_prefix) {
> -		BUG("cannot have prefix '%s' and superprefix '%s'",
> -		    prefix, super_prefix);
> +	if (prefix && toplevel_cwd_prefix) {
> +		BUG("cannot have prefix '%s' and toplevel_cwd_prefix '%s'",
> +		    prefix, toplevel_cwd_prefix);
>  	} else if (prefix) {
>  		struct strbuf sb = STRBUF_INIT;
>  		char *displaypath = xstrdup(relative_path(path, prefix, &sb));
>  		strbuf_release(&sb);
>  		return displaypath;
> -	} else if (super_prefix) {
> -		return xstrfmt("%s%s", super_prefix, path);
> +	} else if (toplevel_cwd_prefix) {
> +		return xstrfmt("%s%s", toplevel_cwd_prefix, path);
>  	} else {
>  		return xstrdup(path);
>  	}

At the end of this series git.c has stopped knowing about
"--super-prefix", so I don't see why we need the churn of renaming
this. Even if we just make built-in commands take it now, isn't that
sufficient? We'd still need...

> @@ -364,9 +364,10 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
>  		cpr.dir = path;
>  		prepare_submodule_repo_env(&cpr.env);
>  
> -		strvec_pushl(&cpr.args, "--super-prefix", NULL);
> +		strvec_pushl(&cpr.args, "submodule--helper",
> +			     "--toplevel-cwd-prefix", NULL);
>  		strvec_pushf(&cpr.args, "%s/", displaypath);
> -		strvec_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive",
> +		strvec_pushl(&cpr.args, "foreach", "--recursive",
>  			     NULL);
>  

...to do this, but not all the function & variable renames.

>  	struct option options[] = {
> +		OPT_CALLBACK_F(0, "toplevel-cwd-prefix", NULL, "path",
> +			       "path from top level cwd to working tree root",
> +			       0, option_parse_toplevel_cwd_prefix),
>  		OPT_SUBCOMMAND("clone", &fn, module_clone),
>  		OPT_SUBCOMMAND("add", &fn, module_add),
>  		OPT_SUBCOMMAND("update", &fn, module_update),
> @@ -3375,21 +3378,10 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>  		OPT_SUBCOMMAND("set-url", &fn, module_set_url),
>  		OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
>  		OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
> +
>  		OPT_END()
>  	};
>  	argc = parse_options(argc, argv, prefix, options, usage, 0);
> -	subcmd = argv[0];
> -
> -	if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
> -	    strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
> -	    strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
> -	    get_super_prefix())
> -		/*
> -		 * xstrfmt() rather than "%s %s" to keep the translated
> -		 * string identical to git.c's.
> -		 */
> -		die(_("%s doesn't support --super-prefix"),
> -		    xstrfmt("'%s %s'", cmd, subcmd));
>  
>  	return fn(argc, argv, prefix);
>  }

Re some of the things I was aiming for in the WIP submodule built-in
series by splitting up "submodule--helper", before this we'll only
accept it for certain subcommands, e.g.:

	$ ./git --super-prefix=blah/ submodule--helper summary
	fatal: 'submodule--helper summary' doesn't support --super-prefix

Whereas now we'll accept it:

	$ ./git submodule--helper --toplevel-cwd-prefix=blah/ summary
	$

I'm not necessarily against that, but it does seem to run somewhat
counter to the divide-and-conquer of eventually getting rid of this
super-prefix (or whatever we call it) business.

And in any case, if that's what we're going to do let's loosen that up
and justify that change in its own commit, separate from a
rename/refactor. I.e. you could start by removing this strcmp(...) &&
strcmp(...), along with an explanation for why we won't need to care
about which "submodule--helper" subcommand needs it anymore.

> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
> index 2859695c6d..a9efebc7ec 100755
> --- a/t/t7412-submodule-absorbgitdirs.sh
> +++ b/t/t7412-submodule-absorbgitdirs.sh
> @@ -54,15 +54,24 @@ test_expect_success 'setup nested submodule' '
>  '
>  
>  test_expect_success 'absorb the git dir in a nested submodule' '
> +	# Touch the files so that they show up in git status
> +	>expect.err &&
> +	>actual.err &&
>  	git status >expect.1 &&
>  	git -C sub1/nested rev-parse HEAD >expect.2 &&
> -	git submodule absorbgitdirs &&
> +	git submodule absorbgitdirs 2>actual.err &&
>  	test -f sub1/nested/.git &&
>  	test -d .git/modules/sub1/modules/nested &&
>  	git status >actual.1 &&
>  	git -C sub1/nested rev-parse HEAD >actual.2 &&
>  	test_cmp expect.1 actual.1 &&
> -	test_cmp expect.2 actual.2
> +	test_cmp expect.2 actual.2 &&
> +	cat >expect.err <<-EOF &&
> +	Migrating git directory of ${SQ}sub1/nested${SQ} from
> +	${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/sub1/nested/.git${SQ} to
> +	${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/.git/modules/sub1/modules/nested${SQ}
> +	EOF
> +	test_cmp expect.err actual.err

I'm afraid I'll need to change my name & OS for that to pass for me :)

This reminded me to send in:
https://lore.kernel.org/git/patch-1.1-34b54fdd9bb-20221109T020347Z-avarab@gmail.com/
; which fixes this message, & adds more tests for this output.

  reply	other threads:[~2022-11-09  3:05 UTC|newest]

Thread overview: 105+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-09  0:47 [RFC PATCH 0/4] git: remove --super-prefix Glen Choo
2022-11-09  0:47 ` [RFC PATCH 1/4] submodule--helper: teach --toplevel-cwd-prefix Glen Choo
2022-11-09  2:37   ` Ævar Arnfjörð Bjarmason [this message]
2022-11-09  0:47 ` [RFC PATCH 2/4] fetch: refactor --submodule-prefix Glen Choo
2022-11-09  3:06   ` Ævar Arnfjörð Bjarmason
2022-11-09  0:47 ` [RFC PATCH 3/4] read-tree: teach --submodule-prefix Glen Choo
2022-11-09  3:13   ` Ævar Arnfjörð Bjarmason
2022-11-09  0:47 ` [RFC PATCH 4/4] git: remove --super-prefix Glen Choo
2022-11-09 19:34 ` [RFC PATCH 0/8] Get rid of "git --super-prefix" Ævar Arnfjörð Bjarmason
2022-11-09 19:34   ` [RFC PATCH 1/8] submodule--helper: don't use global --super-prefix in "absorbgitdirs" Ævar Arnfjörð Bjarmason
2022-11-11  0:12     ` Glen Choo
2022-11-09 19:34   ` [RFC PATCH 2/8] submodule--helper: "deinit" has never used "--super-prefix" Ævar Arnfjörð Bjarmason
2022-11-09 19:34   ` [RFC PATCH 3/8] submodule--helper: convert "foreach" to its own "--super-prefix" Ævar Arnfjörð Bjarmason
2022-11-09 19:34   ` [RFC PATCH 4/8] submodule--helper: convert "sync" " Ævar Arnfjörð Bjarmason
2022-11-09 19:34   ` [RFC PATCH 5/8] submodule--helper: convert "status" " Ævar Arnfjörð Bjarmason
2022-11-09 19:34   ` [RFC PATCH 6/8] submodule--helper: convert "{update,clone}" to their " Ævar Arnfjörð Bjarmason
2022-11-09 19:34   ` [RFC PATCH 7/8] submodule tests: test "git branch -t" output and stderr Ævar Arnfjörð Bjarmason
2022-11-09 19:34   ` [RFC PATCH 8/8] read-tree: add "--super-prefix" option, eliminate global Ævar Arnfjörð Bjarmason
2022-11-11  0:40     ` Glen Choo
2022-11-09 21:21   ` [RFC PATCH 0/8] Get rid of "git --super-prefix" Taylor Blau
2022-11-09 21:47     ` Ævar Arnfjörð Bjarmason
2022-11-09 22:27       ` Taylor Blau
2022-11-09 22:54         ` Ævar Arnfjörð Bjarmason
2022-11-10  0:45   ` Glen Choo
2022-11-10 10:51     ` Ævar Arnfjörð Bjarmason
2022-11-11  1:07       ` Glen Choo
2022-11-11 18:29         ` Glen Choo
2022-11-11 21:17           ` Ævar Arnfjörð Bjarmason
2022-11-11 21:51             ` Taylor Blau
2022-11-12  1:10             ` Glen Choo
2022-11-14 10:09               ` Ævar Arnfjörð Bjarmason
2022-11-14 23:33                 ` Glen Choo
2022-11-15  1:37                   ` Ævar Arnfjörð Bjarmason
2022-11-14 10:08   ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
2022-11-14 10:08     ` [PATCH v2 01/10] read-tree + fetch tests: test failing "--super-prefix" interaction Ævar Arnfjörð Bjarmason
2022-11-14 19:00       ` Glen Choo
2022-11-14 19:14         ` Ævar Arnfjörð Bjarmason
2022-11-14 19:49           ` Glen Choo
2022-11-14 10:08     ` [PATCH v2 02/10] submodule--helper: don't use global --super-prefix in "absorbgitdirs" Ævar Arnfjörð Bjarmason
2022-11-14 21:22       ` Glen Choo
2022-11-17 18:10         ` Ævar Arnfjörð Bjarmason
2022-11-14 10:08     ` [PATCH v2 03/10] submodule--helper: "deinit" has never used "--super-prefix" Ævar Arnfjörð Bjarmason
2022-11-14 10:08     ` [PATCH v2 04/10] submodule--helper: convert "foreach" to its own "--super-prefix" Ævar Arnfjörð Bjarmason
2022-11-14 21:56       ` Glen Choo
2022-11-17 18:14         ` Ævar Arnfjörð Bjarmason
2022-11-14 10:08     ` [PATCH v2 05/10] submodule--helper: convert "sync" " Ævar Arnfjörð Bjarmason
2022-11-14 10:08     ` [PATCH v2 06/10] submodule--helper: convert "status" " Ævar Arnfjörð Bjarmason
2022-11-14 10:08     ` [PATCH v2 07/10] submodule--helper: convert "{update,clone}" to their " Ævar Arnfjörð Bjarmason
2022-11-14 22:04       ` Glen Choo
2022-11-14 10:08     ` [PATCH v2 08/10] submodule tests: test "git branch -t" output and stderr Ævar Arnfjörð Bjarmason
2022-11-14 22:20       ` Glen Choo
2022-11-14 10:08     ` [PATCH v2 09/10] read-tree: add "--super-prefix" option, eliminate global Ævar Arnfjörð Bjarmason
2022-11-14 22:28       ` Glen Choo
2022-11-14 10:08     ` [PATCH v2 10/10] fetch: rename "--submodule-prefix" to "--super-prefix" Ævar Arnfjörð Bjarmason
2022-11-14 22:31       ` Glen Choo
2022-11-14 21:59     ` [PATCH v2 00/10] Get rid of "git --super-prefix" Taylor Blau
2022-11-14 23:20     ` Glen Choo
2022-11-14 23:39     ` Glen Choo
2022-11-15  1:27       ` Ævar Arnfjörð Bjarmason
2022-11-16 21:07         ` Glen Choo
2022-11-17 18:07           ` Ævar Arnfjörð Bjarmason
2022-11-21 19:16             ` Glen Choo
2022-11-19 12:41     ` [PATCH v3 0/9] " Ævar Arnfjörð Bjarmason
2022-11-19 12:41       ` [PATCH v3 1/9] read-tree + fetch tests: test failing "--super-prefix" interaction Ævar Arnfjörð Bjarmason
2022-11-19 12:41       ` [PATCH v3 2/9] submodule.c & submodule--helper: pass along "super_prefix" param Ævar Arnfjörð Bjarmason
2022-11-19 12:41       ` [PATCH v3 3/9] submodule--helper: don't use global --super-prefix in "absorbgitdirs" Ævar Arnfjörð Bjarmason
2022-11-22 19:53         ` Glen Choo
2022-11-19 12:41       ` [PATCH v3 4/9] submodule--helper: convert "foreach" to its own "--super-prefix" Ævar Arnfjörð Bjarmason
2022-11-19 12:41       ` [PATCH v3 5/9] submodule--helper: convert "sync" " Ævar Arnfjörð Bjarmason
2022-11-19 12:41       ` [PATCH v3 6/9] submodule--helper: convert "status" " Ævar Arnfjörð Bjarmason
2022-11-19 12:41       ` [PATCH v3 7/9] submodule--helper: convert "{update,clone}" to their " Ævar Arnfjörð Bjarmason
2022-11-19 12:41       ` [PATCH v3 8/9] read-tree: add "--super-prefix" option, eliminate global Ævar Arnfjörð Bjarmason
2022-11-22 19:57         ` Glen Choo
2022-11-19 12:41       ` [PATCH v3 9/9] fetch: rename "--submodule-prefix" to "--super-prefix" Ævar Arnfjörð Bjarmason
2022-11-22 22:29       ` [PATCH v3 0/9] Get rid of "git --super-prefix" Glen Choo
2022-12-15  9:32       ` [PATCH v4 " Ævar Arnfjörð Bjarmason
2022-12-15  9:32         ` [PATCH v4 1/9] submodule absorbgitdirs tests: add missing "Migrating git..." tests Ævar Arnfjörð Bjarmason
2022-12-15 20:54           ` Glen Choo
2022-12-20 10:32             ` Ævar Arnfjörð Bjarmason
2022-12-15  9:32         ` [PATCH v4 2/9] read-tree + fetch tests: test failing "--super-prefix" interaction Ævar Arnfjörð Bjarmason
2022-12-15  9:32         ` [PATCH v4 3/9] submodule.c & submodule--helper: pass along "super_prefix" param Ævar Arnfjörð Bjarmason
2022-12-15  9:32         ` [PATCH v4 4/9] submodule--helper: don't use global --super-prefix in "absorbgitdirs" Ævar Arnfjörð Bjarmason
2022-12-15 21:05           ` Glen Choo
2022-12-15  9:32         ` [PATCH v4 5/9] submodule--helper: convert "foreach" to its own "--super-prefix" Ævar Arnfjörð Bjarmason
2022-12-15  9:32         ` [PATCH v4 6/9] submodule--helper: convert "sync" " Ævar Arnfjörð Bjarmason
2022-12-15  9:32         ` [PATCH v4 7/9] submodule--helper: convert "status" " Ævar Arnfjörð Bjarmason
2022-12-15  9:32         ` [PATCH v4 8/9] submodule--helper: convert "{update,clone}" to their " Ævar Arnfjörð Bjarmason
2022-12-15  9:32         ` [PATCH v4 9/9] read-tree: add "--super-prefix" option, eliminate global Ævar Arnfjörð Bjarmason
2022-12-15 21:19         ` [PATCH v4 0/9] Get rid of "git --super-prefix" Glen Choo
2022-12-15 22:19           ` Junio C Hamano
2022-12-15 22:12         ` Junio C Hamano
2022-12-20 12:39         ` [PATCH v5 " Ævar Arnfjörð Bjarmason
2022-12-20 12:39           ` [PATCH v5 1/9] submodule absorbgitdirs tests: add missing "Migrating git..." tests Ævar Arnfjörð Bjarmason
2022-12-20 12:39           ` [PATCH v5 2/9] read-tree + fetch tests: test failing "--super-prefix" interaction Ævar Arnfjörð Bjarmason
2022-12-20 12:39           ` [PATCH v5 3/9] submodule.c & submodule--helper: pass along "super_prefix" param Ævar Arnfjörð Bjarmason
2022-12-20 12:39           ` [PATCH v5 4/9] submodule--helper: don't use global --super-prefix in "absorbgitdirs" Ævar Arnfjörð Bjarmason
2022-12-20 12:39           ` [PATCH v5 5/9] submodule--helper: convert "foreach" to its own "--super-prefix" Ævar Arnfjörð Bjarmason
2022-12-20 12:39           ` [PATCH v5 6/9] submodule--helper: convert "sync" " Ævar Arnfjörð Bjarmason
2022-12-20 12:39           ` [PATCH v5 7/9] submodule--helper: convert "status" " Ævar Arnfjörð Bjarmason
2022-12-20 12:39           ` [PATCH v5 8/9] submodule--helper: convert "{update,clone}" to their " Ævar Arnfjörð Bjarmason
2022-12-20 12:39           ` [PATCH v5 9/9] read-tree: add "--super-prefix" option, eliminate global Ævar Arnfjörð Bjarmason
2022-11-09 21:16 ` [RFC PATCH 0/4] git: remove --super-prefix Taylor Blau
2022-11-09 23:55   ` Glen Choo
2022-11-10  2:14     ` Taylor Blau
2022-11-10 23:49       ` 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=221109.867d04rfnt.gmgdl@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).