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

From: Jeff King <peff@peff.net>
Date: Mar 3, 2017, 04:45 UTC
Message-ID: <20170303044518.rdtyvgs7kqe7fkpf@sigill.intra.peff.net>
In-Reply-To: <0ca4cce042cc0b0f13f87363b70a3689057ae9b0.1488506615.git.johannes.schindelin@gmx.de>

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

Show 11 quoted lines

> 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.

Show 9 quoted lines

> 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.

Show 14 quoted lines

> +	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