Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory

[Brandon Williams <bmwill@google.com>]

On 06/08, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Wed, 7 Jun 2017, Brandon Williams wrote:
> 
> > On 06/07, Johannes Schindelin wrote:
> > > @@ -1668,7 +1668,7 @@ void read_early_config(config_fn_t cb, void *data)
> > >  	 * notably, the current working directory is still the same after the
> > >  	 * call).
> > >  	 */
> > > -	else if (discover_git_directory(&buf))
> > > +	else if (discover_git_directory(&buf, worktree_dir))
> > >  		opts.git_dir = buf.buf;
> > 
> > It feels kind of weird to get back worktree info after calling
> > read_early_config but I understand why you need to get it.
> 
> Yeah. It is awkward. Required for backwards-compatibility, though (and it
> is hard to explain *when* it is needed, and even harder under what
> circumstances it is *not* needed even if there is a worktre).
> 
> > One thing to consider is the 'if' statement not shown here since you
> > aren't adding any worktree information if that branch is taken.
> 
> Right. For lurkers, that `if` statement reads thusly:
> 
> 	if (have_git_dir())
> 		opts.git_dir = get_git_dir();
> 
> > Maybe we can drop the first if statement all together as
> > 'read_early_config' is used before setup is run and it should really
> > only be triggered when setup has been run.
> 
> The `read_early_config()` function is *sometimes* used *after* setup has
> run. Look at `run_builtin()` in git.c:
> 
> 	if (p->option & RUN_SETUP)
> 		prefix = setup_git_directory();
> 	else if (p->option & RUN_SETUP_GENTLY) {
> 		int nongit_ok;
> 		prefix = setup_git_directory_gently(&nongit_ok);
> 	}
> 
> 	if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
> 		use_pager = check_pager_config(p->cmd);
> 

Ah, ok so my comment was incorrect, thanks for pointing this out.

> For builtins having either the RUN_SETUP or the RUN_SETUP_GENTLY flag, we
> do not need to re-discover the .git/ directory at all when checking the
> pager config.
> 
> Back to the worktree_dir variable.
> 
> I think part of the confusion here is that it may be left alone even when
> there is a worktree. For example, if we are already in the top-level
> directory. Or if the worktree somehow points to a different directory than
> the one containing the .git/ directory.
> 
> Therefore, I renamed this variable to `cdup_dir` to reflect the fact that
> it is only touched if Git determines that it is in a subdirectory of the
> directory containing the .git/ directory.

Ok, maybe I'm just not following but just from reading the variable name I can't
really understand what 'cdup_dir' means.

> 
> Ciao,
> Dscho

-- 
Brandon Williams