Re: [PATCH v6 1/4] git: make super-prefix option

2016-10-04 Thread Jeff King
On Tue, Oct 04, 2016 at 10:31:51AM -0700, Stefan Beller wrote:

> On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams  wrote:
> 
> >
> > +const char *get_super_prefix(void)
> > +{
> > +   if (!super_prefix)
> > +   super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
> > +   return super_prefix;
> > +}
> > +
> 
> As said earlier, is the following a valid thought:
> 
> > The getenv() function returns a pointer to the value in the
> > environment, or NULL if there is no match.
> > So in case this is not set (when e.g. the user did not specify the
> > super prefix), we would probe it a couple of times.
> > The caching effect only occurs when the string is set. So this looks
> > like we save repetitive calls, but we do not always do that.

I think your concern is valid. If it is not set, we will do an O(n)
search through the whole environment on each call.

I also think the result of getenv() needs to be copied. In some
implementations it persists for the life of the program, but that's not
guaranteed; it may be overwritten by unrelated calls to getenv() or
setenv().

-Peff


Re: [PATCH v6 1/4] git: make super-prefix option

2016-10-04 Thread Junio C Hamano
Stefan Beller  writes:

> On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams  wrote:
>
>>
>> +const char *get_super_prefix(void)
>> +{
>> +   if (!super_prefix)
>> +   super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
>> +   return super_prefix;
>> +}
>> +
>
> As said earlier, is the following a valid thought:
>
>> The getenv() function returns a pointer to the value in the
>> environment, or NULL if there is no match.
>> So in case this is not set (when e.g. the user did not specify the
>> super prefix), we would probe it a couple of times.
>> The caching effect only occurs when the string is set. So this looks
>> like we save repetitive calls, but we do not always do that.

That reading is correct.  If the code wants to do the caching, it
should do so correctly.  Unless we expect that some callers might
want to be able to invalidate the cache,

get_super_prefix(void)
{
static int initialized;
if (!initialized) {
super_prefix = getenv(...);
initialized = 1;
}
return super_prefix;
}

would suffice.

Thanks for careful reading.


Re: [PATCH v6 1/4] git: make super-prefix option

2016-10-04 Thread Stefan Beller
On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams  wrote:

>
> +const char *get_super_prefix(void)
> +{
> +   if (!super_prefix)
> +   super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
> +   return super_prefix;
> +}
> +

As said earlier, is the following a valid thought:

> The getenv() function returns a pointer to the value in the
> environment, or NULL if there is no match.
> So in case this is not set (when e.g. the user did not specify the
> super prefix), we would probe it a couple of times.
> The caching effect only occurs when the string is set. So this looks
> like we save repetitive calls, but we do not always do that.

>
> +   if (get_super_prefix()) {
> +   die("%s doesn't support --super-prefix", argv[0]);
> +   }
> +

Nit: no braces, please.


[PATCH v6 1/4] git: make super-prefix option

2016-09-29 Thread Brandon Williams
Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
which can be used to specify a path from above a repository down to its
root.  When such a super-prefix is specified, the paths reported by Git
are prefixed with it to make them relative to that directory "above".
The paths given by the user on the command line
(e.g. "git subcmd --output-file=path/to/a/file" and pathspecs) are taken
relative to the directory "above" to match.

The immediate use of this option is by commands which have a
--recurse-submodule option in order to give context to submodules about
how they were invoked.  This option is currently only allowed for
builtins which support a super-prefix.

Signed-off-by: Brandon Williams 
---
 Documentation/git.txt |  6 ++
 cache.h   |  2 ++
 environment.c | 10 ++
 git.c | 26 ++
 4 files changed, 44 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7913fc2..2188ae6 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=]
+[--super-prefix=]
  []
 
 DESCRIPTION
@@ -601,6 +602,11 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
details.  Equivalent to setting the `GIT_NAMESPACE` environment
variable.
 
+--super-prefix=::
+   Currently for internal use only.  Set a prefix which gives a path from
+   above a repository down to its root.  One use is to give submodules
+   context about the superproject that invoked it.
+
 --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..8cf495d 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_SUPER_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUPER_PREFIX"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
@@ -468,6 +469,7 @@ extern int get_common_dir_noenv(struct strbuf *sb, const 
char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
 extern const char *strip_namespace(const char *namespaced_ref);
+extern const char *get_super_prefix(void);
 extern const char *get_git_work_tree(void);
 
 /*
diff --git a/environment.c b/environment.c
index ca72464..13f3d70 100644
--- a/environment.c
+++ b/environment.c
@@ -100,6 +100,8 @@ static char *work_tree;
 static const char *namespace;
 static size_t namespace_len;
 
+static const char *super_prefix;
+
 static const char *git_dir, *git_common_dir;
 static char *git_object_dir, *git_index_file, *git_graft_file;
 int git_db_env, git_index_env, git_graft_env, git_common_dir_env;
@@ -120,6 +122,7 @@ const char * const local_repo_env[] = {
NO_REPLACE_OBJECTS_ENVIRONMENT,
GIT_REPLACE_REF_BASE_ENVIRONMENT,
GIT_PREFIX_ENVIRONMENT,
+   GIT_SUPER_PREFIX_ENVIRONMENT,
GIT_SHALLOW_FILE_ENVIRONMENT,
GIT_COMMON_DIR_ENVIRONMENT,
NULL
@@ -222,6 +225,13 @@ const char *strip_namespace(const char *namespaced_ref)
return namespaced_ref + namespace_len;
 }
 
+const char *get_super_prefix(void)
+{
+   if (!super_prefix)
+   super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
+   return super_prefix;
+}
+
 static int git_work_tree_initialized;
 
 /*
diff --git a/git.c b/git.c
index 1c61151..f756b62 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, "--super-prefix")) {
+   if (*argc < 2) {
+   fprintf(stderr, "No prefix given for 
--super-prefix.\n" );
+   usage(git_usage_string);
+   }
+   setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1);
+   if (envchanged)
+   *envchanged = 1;
+   (*argv)++;
+   (*argc)--;
+   } else if (skip_prefix(cmd, "--super-prefix=", &cmd)) {
+   setenv(GIT_SUPER_PREFIX_ENVIRONMENT, cmd, 1);
+   if (envchanged)
+   *envchanged = 1;
} else if (!strcmp(cmd, "--bare")) {
char *cwd = xgetcwd();