On Fri, Mar 03, 2017 at 06:31:55PM +0100, Johannes Schindelin wrote:
> 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.
> @@ -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
