Subject: Re: [PATCH 1/4 v4] submodules: make submodule-prefix option

From: Junio C Hamano <[email-masked]>
Date: Tue, 27 Sep 2016 11:17:57 -0700

Brandon Williams <[email-masked]> writes:

> +--submodule-prefix=<path>::
> +	Set a prefix which gives submodules context about the superproject that
> +	invoked it.  Only allowed for commands which support submodules.

This, and also the message in die(), uses a phrase "support
submodules", but it is unclear what it exactly means to the end
users and readers.

A "ls-files" that is recursively run as an implementation detail of
the "grep --recurse-submodules" would be taught to support this
option with this series.  Who is supporting submodules in that
context?

I'd imagine (close to) 100% of the people would say it is "grep"
that is supporting submodules, not "ls-files", but what this
paragraph and die() message want to express by the phrase "support
submodules" is the fact that "ls-files" knows how to react to
"--submodule-prefix" option.

I'd suggest not to worry too much about this phrasing at this point,
until we figure out exactly how we want to present these to end
users.  For now, perhaps drop the second sentence and replace it
with "The end-users are not expected to use this option" or
something like that?

> diff --git a/git.c b/git.c
> index 1c61151..b2b096a 100644
> --- a/git.c
> +++ b/git.c
> @@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
>  			if (envchanged)
>  				*envchanged = 1;
> +		} else if (!strcmp(cmd, "--submodule-prefix")) {
> +			if (*argc < 2) {
> +				fprintf(stderr, "No prefix given for --submodule-prefix.\n" );
> +				usage(git_usage_string);
> +			}
> +			setenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT, (*argv)[1], 1);
> +			if (envchanged)
> +				*envchanged = 1;
> +			(*argv)++;
> +			(*argc)--;
> +		} else if (skip_prefix(cmd, "--submodule-prefix=", &cmd)) {
> +			setenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT, cmd, 1);
> +			if (envchanged)
> +				*envchanged = 1;
>  		} else if (!strcmp(cmd, "--bare")) {
>  			char *cwd = xgetcwd();
>  			is_bare_repository_cfg = 1;
> @@ -310,6 +324,7 @@ static int handle_alias(int *argcp, const char ***argv)
>   * RUN_SETUP for reading from the configuration file.
>   */
>  #define NEED_WORK_TREE		(1<<3)
> +#define SUPPORT_SUBMODULES	(1<<4)
>  
>  struct cmd_struct {
>  	const char *cmd;
> @@ -344,6 +359,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>  	}
>  	commit_pager_choice();
>  
> +	if (!help && (getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT) &&
> +		      !(p->option & SUPPORT_SUBMODULES)))
> +		die("%s doesn't support submodules", p->cmd);

s/submodules/submodule-prefix/ at least.

>  	if (!help && p->option & NEED_WORK_TREE)
>  		setup_work_tree();