Re: [PATCH v3 0/9] Fix the early config

From: Jeff King <peff@peff.net>
Date: Mar 4, 2017, 07:39 UTC
Message-ID: <20170304073952.drfgy7jacnlm6tum@sigill.intra.peff.net>
In-Reply-To: <cover.1488562287.git.johannes.schindelin@gmx.de>

On Fri, Mar 03, 2017 at 06:31:55PM +0100, Johannes Schindelin wrote:

Show 14 quoted lines

> Interdiff vs v2:
> [...]
>  +	 * When we are not about to create a repository ourselves (init or
>  +	 * clone) and when no .git/ directory was set up yet (in which case
>  +	 * git_config_with_options() would already have picked up the
>  +	 * repository config), we ask discover_git_directory() to figure out
>  +	 * whether there is any repository config we should use (but unlike
>  +	 * setup_git_directory_gently(), no global state is changed, most
>  +	 * notably, the current working directory is still the same after
>  +	 * the call).
>   	 */
>  -	if (!startup_info->creating_repository && !have_git_dir() &&
>  -	    discover_git_directory(&buf)) {
>  +	if (!have_git_dir() && discover_git_directory(&buf)) {

I think this "when we are not about to..." part of the comment is no longer true, given the second part of the hunk.

Show 9 quoted lines

>  @@ -721,8 +721,10 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>   	if (offset == cwd->len)
>   		return NULL;
>   
>  -	/* Make "offset" point to past the '/', and add a '/' at the end */
>  -	offset++;
>  +	/* Make "offset" point past the '/' (already the case for root dirs) */
>  +	if (offset != offset_1st_component(cwd->buf))
>  +		offset++;

Nice. I was worried we would have to have a hacky "well, sometimes we don't add one here..." code, but using offset_1st_component says exactly what we mean.

> +/* Find GIT_DIR without changing the working directory or other global state */
>  extern const char *discover_git_directory(struct strbuf *gitdir);

The parts that actually confused me were the parameters (mostly whether gitdir was a directory to start looking in, or an output parameter). So maybe:

  /*
   * Find GIT_DIR of the repository that contains the current working
   * directory, without changing the working directory or other global
   * state. The result is appended to gitdir. The return value is NULL
   * if no repository was found, or gitdir->buf otherwise.
   */

This looks good to me aside from those few comment nits. I'm still not sure I understand how ceil_offset works in setup_git_directory_gently_1(), but I don't think your patch actually changed it. I can live with my confusion.

-Peff