Re: [PATCH v5 04/11] setup_git_directory_1(): avoid changing global state

2017-03-10 Thread Junio C Hamano
Johannes Schindelin  writes:

>  /*
>   * We cannot decide in this function whether we are in the work tree or
>   * not, since the config can only be read _after_ this function was called.
> + *
> + * Also, we avoid changing any global state (such as the current working
> + * directory) to allow early callers.
> + *
> + * The directory where the search should start needs to be passed in via the
> + * `dir` parameter; upon return, the `dir` buffer will contain the path of
> + * the directory where the search ended, and `gitdir` will contain the path 
> of
> + * the discovered .git/ directory, if any. This path may be relative against
> + * `dir` (i.e. *not* necessarily the cwd).

I had to read the last sentence three times because my earlier two
attempts misread/misinterpreted as "this may be relative to cwd, but
not necessarily, because this may be relative to dir".  The correct
reading is "when this is relative, it is relative to dir.  It would
not be relative to cwd if you start with dir that is not cwd".

It would be nicer if all readers can get to that without re-reading
three times.

> -static const char *setup_git_directory_gently_1(int *nongit_ok)
> +static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> ...
> @@ -889,63 +888,104 @@ static const char *setup_git_directory_gently_1(int 
> *nongit_ok)
>*/
>   one_filesystem = !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0);
>   if (one_filesystem)
> - current_device = get_device_or_die(".", NULL, 0);
> + current_device = get_device_or_die(dir->buf, NULL, 0);
>   for (;;) {
> - gitfile = (char*)read_gitfile(DEFAULT_GIT_DIR_ENVIRONMENT);
> - if (gitfile)
> - gitdirenv = gitfile = xstrdup(gitfile);
> - else {
> - if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
> - gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
> + int offset = dir->len;
> +
> + if (offset > min_offset)
> + strbuf_addch(dir, '/');
> + strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
> + gitdirenv = read_gitfile(dir->buf);
> + if (!gitdirenv && is_git_directory(dir->buf))
> + gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
> + strbuf_setlen(dir, offset);
> + if (gitdirenv) {
> + strbuf_addstr(gitdir, gitdirenv);
> + return GIT_DIR_DISCOVERED;
>   }

I commented on this part more extensively in a later step of the
series after the read_gitfile() call is replaced with the non-dying
version.  I see that issues in corner cases are inherited from the
code even before this step.  

We'd either want to at least document the corner cases that are not
handled with /* NEEDSWORK: */ in-code comments , or address them in
a follow-up series before we forget.  They are not new problems, so
I am OK if we choose to leave them broken, as people haven't triggered
them in the current code.

Thanks.


[PATCH v5 04/11] setup_git_directory_1(): avoid changing global state

2017-03-09 Thread Johannes Schindelin
For historical reasons, Git searches for the .git/ directory (or the
.git file) by changing the working directory successively to the parent
directory of the current directory, until either anything was found or
until a ceiling or a mount point is hit.

Further global state may be changed in case a .git/ directory was found.

We do have a use case, though, where we would like to find the .git/
directory without having any global state touched, though: when we read
the early config e.g. for the pager or for alias expansion.

Let's just move all of code that changes any global state out of the
function `setup_git_directory_gently_1()` into
`setup_git_directory_gently()`.

In subsequent patches, we will use the _1() function in a new
`discover_git_directory()` function that we will then use for the early
config code.

Note: the new loop is a *little* tricky, as we have to handle the root
directory specially: we cannot simply strip away the last component
including the slash, as the root directory only has that slash. To remedy
that, we introduce the `min_offset` variable that holds the minimal length
of an absolute path, and using that to special-case the root directory,
including an early exit before trying to find the parent of the root
directory.

Signed-off-by: Johannes Schindelin 
---
 setup.c | 193 +++-
 1 file changed, 118 insertions(+), 75 deletions(-)

diff --git a/setup.c b/setup.c
index 20a1f0f870e..a7bb09608c0 100644
--- a/setup.c
+++ b/setup.c
@@ -818,50 +818,49 @@ static int canonicalize_ceiling_entry(struct 
string_list_item *item,
}
 }
 
+enum discovery_result {
+   GIT_DIR_NONE = 0,
+   GIT_DIR_EXPLICIT,
+   GIT_DIR_DISCOVERED,
+   GIT_DIR_BARE,
+   /* these are errors */
+   GIT_DIR_HIT_CEILING = -1,
+   GIT_DIR_HIT_MOUNT_POINT = -2
+};
+
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
+ *
+ * Also, we avoid changing any global state (such as the current working
+ * directory) to allow early callers.
+ *
+ * The directory where the search should start needs to be passed in via the
+ * `dir` parameter; upon return, the `dir` buffer will contain the path of
+ * the directory where the search ended, and `gitdir` will contain the path of
+ * the discovered .git/ directory, if any. This path may be relative against
+ * `dir` (i.e. *not* necessarily the cwd).
  */
-static const char *setup_git_directory_gently_1(int *nongit_ok)
+static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
+ struct strbuf *gitdir)
 {
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
-   static struct strbuf cwd = STRBUF_INIT;
-   const char *gitdirenv, *ret;
-   char *gitfile;
-   int offset, offset_parent, ceil_offset = -1;
+   const char *gitdirenv;
+   int ceil_offset = -1, min_offset = has_dos_drive_prefix(dir->buf) ? 3 : 
1;
dev_t current_device = 0;
int one_filesystem = 1;
 
/*
-* We may have read an incomplete configuration before
-* setting-up the git directory. If so, clear the cache so
-* that the next queries to the configuration reload complete
-* configuration (including the per-repo config file that we
-* ignored previously).
-*/
-   git_config_clear();
-
-   /*
-* Let's assume that we are in a git repository.
-* If it turns out later that we are somewhere else, the value will be
-* updated accordingly.
-*/
-   if (nongit_ok)
-   *nongit_ok = 0;
-
-   if (strbuf_getcwd(&cwd))
-   die_errno(_("Unable to read current working directory"));
-   offset = cwd.len;
-
-   /*
 * If GIT_DIR is set explicitly, we're not going
 * to do any discovery, but we still do repository
 * validation.
 */
gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
-   if (gitdirenv)
-   return setup_explicit_git_dir(gitdirenv, &cwd, nongit_ok);
+   if (gitdirenv) {
+   strbuf_addstr(gitdir, gitdirenv);
+   return GIT_DIR_EXPLICIT;
+   }
 
if (env_ceiling_dirs) {
int empty_entry_found = 0;
@@ -869,15 +868,15 @@ static const char *setup_git_directory_gently_1(int 
*nongit_ok)
string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, 
-1);
filter_string_list(&ceiling_dirs, 0,
   canonicalize_ceiling_entry, 
&empty_entry_found);
-   ceil_offset = longest_ancestor_length(cwd.buf, &ceiling_dirs);
+   ceil_offset = longest_ancestor_length(dir->buf, &ceiling_dirs);
string_list_clear(&ceiling_dirs, 0)