Re: [PATCH v2 4/9] Export the discover_git_directory() function

[Jeff King <peff@peff.net>]

On Fri, Mar 03, 2017 at 03:04:15AM +0100, Johannes Schindelin wrote:

> The function we introduced earlier needs to return both the path to the
> .git/ directory as well as the "cd-up" path to allow
> setup_git_directory() to retain its previous behavior as if it changed
> the current working directory on its quest for the .git/ directory.
> 
> Let's rename it and export a function with an easier signature that
> *just* discovers the .git/ directory.
> 
> We will use it in a subsequent patch to support early config reading
> better.
> 

Seems like an obviously good next step.

> diff --git a/cache.h b/cache.h
> index 80b6372cf76..a104b76c02e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -518,6 +518,7 @@ extern void set_git_work_tree(const char *tree);
>  #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
>  
>  extern void setup_work_tree(void);
> +extern const char *discover_git_directory(struct strbuf *gitdir);

Perhaps it's worth adding a short docstring describing the function. I
know that would make it unlike all of its neighbors, but it is not
immediately obvious to me what the return value is (or whether gitdir is
an input or output parameter).

> +const char *discover_git_directory(struct strbuf *gitdir)
> +{
> +	struct strbuf dir = STRBUF_INIT;
> +	int len;

Nit: please use size_t for storing strbuf lengths.

> +	if (strbuf_getcwd(&dir))
> +		return NULL;
> +
> +	len = dir.len;
> +	if (discover_git_directory_1(&dir, gitdir) < 0) {
> +		strbuf_release(&dir);
> +		return NULL;
> +	}
> +
> +	if (dir.len < len && !is_absolute_path(gitdir->buf)) {
> +		strbuf_addch(&dir, '/');
> +		strbuf_insert(gitdir, 0, dir.buf, dir.len);
> +	}
> +	strbuf_release(&dir);

I was confused by two things here.

One is that because I was wondering whether "gitdir" was supposed to be
passed empty or not, it wasn't clear that this insert is doing the right
thing.  If there _is_ something in it, then discover_git_directory_1()
would just append to it, which makes sense. But then this insert blindly
sticks the absolute-path bit at the front of the string, leaving
whatever was originally there spliced into the middle of the path.
Doing:

  size_t base = gitdir->len;
  ...
  strbuf_insert(gitdir, base, dir.buf, dir.len);

would solve that. It's probably not that likely for somebody to do:

  strbuf_addstr(&buf, "my git dir is ");
  discover_git_directory(&buf);

but since it's not much effort, it might be worth making it work.

The second is that I don't quite understand why we only make the result
absolute when we walked upwards. In git.git, the result of the function
in various directories is:

  - ".git", when we're at the root of the worktree
  - "/home/peff/git/.git, when we're in "t/"
  - "." when we're already in ".git"
  - "/home/peff/git/.git/." when we're in ".git/objects"

I'm not sure if some of those cases are not behaving as intended, or
there's some reason for the differences that I don't quite understand.

-Peff