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

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> >> s/submodules/submodule-prefix/ at least.
> >
> > So should the #define be SUPPORT_SUBMODULE_PREFIX instead?  That may be
> > too narrow minded and not looking toward future submodule options
> > support but I'm not sure.
> 
> I am not convinced that this prefix needs to be tied/limited to
> submodule, at least not yet, though.  I view it as a prefix that
> points from above the repository's top, of which submodule support
> may be a good sample user, but the caller may not necessarily be
> doing or interested in "submodule support".
> 
> That's also part of figuring out how we want define the semantics of
> this thing and how we want to present it to the end-users, I guess,
> so we may have to rename it when we know more, but that's OK.

K we can leave the name as is for now then.  Thankfully it should be a
simple name change since it only lives in 1 file.

-- 
Brandon Williams

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

[Junio C Hamano]

Brandon Williams  writes:

>> s/submodules/submodule-prefix/ at least.
>
> So should the #define be SUPPORT_SUBMODULE_PREFIX instead?  That may be
> too narrow minded and not looking toward future submodule options
> support but I'm not sure.

I am not convinced that this prefix needs to be tied/limited to
submodule, at least not yet, though.  I view it as a prefix that
points from above the repository's top, of which submodule support
may be a good sample user, but the caller may not necessarily be
doing or interested in "submodule support".

That's also part of figuring out how we want define the semantics of
this thing and how we want to present it to the end-users, I guess,
so we may have to rename it when we know more, but that's OK.

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

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > +--submodule-prefix=::
> > +   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?

K can do.  The intention is that each command has to do whatever
internal rework needed so that it understands how to interact with
subomodules (be that calling another command or internally supporting
it).

> > +   die("%s doesn't support submodules", p->cmd);
> 
> s/submodules/submodule-prefix/ at least.

So should the #define be SUPPORT_SUBMODULE_PREFIX instead?  That may be
too narrow minded and not looking toward future submodule options
support but I'm not sure.

-- 
Brandon Williams

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

[Junio C Hamano]

Brandon Williams  writes:

> +--submodule-prefix=::
> + 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=", )) {
> + 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();

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

[Brandon Williams]

Add a submodule-prefix environment variable
'GIT_INTERNAL_SUBMODULE_PREFIX' which can be used by commands which have
--recurse-submodule options to give context to submodules about how they
were invoked.  This option is only allowed for builtins which have
submodule support.

Signed-off-by: Brandon Williams 
---
 Documentation/git.txt |  5 +
 cache.h   |  1 +
 environment.c |  1 +
 git.c | 19 +++
 4 files changed, 26 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7913fc2..d29967a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,6 +13,7 @@ SYNOPSIS
 [--exec-path[=]] [--html-path] [--man-path] [--info-path]
 [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
 [--git-dir=] [--work-tree=] [--namespace=]
+[--submodule-prefix=]
  []
 
 DESCRIPTION
@@ -601,6 +602,10 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
details.  Equivalent to setting the `GIT_NAMESPACE` environment
variable.
 
+--submodule-prefix=::
+   Set a prefix which gives submodules context about the superproject that
+   invoked it.  Only allowed for commands which support submodules.
+
 --bare::
Treat the repository as a bare repository.  If GIT_DIR
environment is not set, it is set to the current working
diff --git a/cache.h b/cache.h
index 3556326..ae88a35 100644
--- a/cache.h
+++ b/cache.h
@@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int 
mode)
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
+#define GIT_SUBMODULE_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUBMODULE_PREFIX"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
diff --git a/environment.c b/environment.c
index ca72464..7380815 100644
--- a/environment.c
+++ b/environment.c
@@ -120,6 +120,7 @@ const char * const local_repo_env[] = {
NO_REPLACE_OBJECTS_ENVIRONMENT,
GIT_REPLACE_REF_BASE_ENVIRONMENT,
GIT_PREFIX_ENVIRONMENT,
+   GIT_SUBMODULE_PREFIX_ENVIRONMENT,
GIT_SHALLOW_FILE_ENVIRONMENT,
GIT_COMMON_DIR_ENVIRONMENT,
NULL
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=", )) {
+   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);
+
if (!help && p->option & NEED_WORK_TREE)
setup_work_tree();
 
-- 
2.8.0.rc3.226.g39d4020