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