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

[Junio C Hamano]

Jeff King  writes:

>> > > Yes, exactly.  It would have been less confusing if I picked something
>> > > that passed nongit_ok. Like hash-object:
>> 
>> ... or like testing the early config directly?
>
> I was trying to demonstrate that the problem existed already without
> your patch series.

Yup, they are not new breakages introduced by Dscho's change.

>> From: Johannes Schindelin 
>> Subject: [PATCH] t1309: document cases where we would want early config not 
>> to
>>  die()
>> 
>> Jeff King came up with a couple examples that demonstrate how the new
>> read_early_config() that looks harder for the current .git/ directory
>> could die() in an undesirable way.
>> 
>> Let's add those cases to the test script, to document what we would like
>> to happen when early config encounters problems.
>
> Yep, these all look fine.

OK.  Thanks, both.

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

[Jeff King]

On Thu, Mar 09, 2017 at 12:51:06PM +0100, Johannes Schindelin wrote:

> On Wed, 8 Mar 2017, Junio C Hamano wrote:
> 
> > Jeff King  writes:
> > 
> > >> Or are you discussing a more general issue, iow, anything that can
> > >> work without repository (i.e. those who do _gently version of the
> > >> setup and act on *nongit_ok) should pretend as if there were no
> > >> (broken) repository and take the "no we are not in a repository"
> > >> codepath?
> > >
> > > Yes, exactly.  It would have been less confusing if I picked something
> > > that passed nongit_ok. Like hash-object:
> 
> ... or like testing the early config directly?

I was trying to demonstrate that the problem existed already without
your patch series.

> -- snipsnap --
> From: Johannes Schindelin 
> Subject: [PATCH] t1309: document cases where we would want early config not to
>  die()
> 
> Jeff King came up with a couple examples that demonstrate how the new
> read_early_config() that looks harder for the current .git/ directory
> could die() in an undesirable way.
> 
> Let's add those cases to the test script, to document what we would like
> to happen when early config encounters problems.

Yep, these all look fine.

-Peff

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

[Johannes Schindelin]

Hi,

On Wed, 8 Mar 2017, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> Or are you discussing a more general issue, iow, anything that can
> >> work without repository (i.e. those who do _gently version of the
> >> setup and act on *nongit_ok) should pretend as if there were no
> >> (broken) repository and take the "no we are not in a repository"
> >> codepath?
> >
> > Yes, exactly.  It would have been less confusing if I picked something
> > that passed nongit_ok. Like hash-object:

... or like testing the early config directly?

> >   $ git init
> >   $ echo content >file
> >   $ git hash-object file
> >   d95f3ad14dee633a758d2e331151e950dd13e4ed
> >
> >   $ echo '[core]repositoryformatversion = 10' >.git/config
> >   $ git hash-object file
> >   warning: Expected git repo version <= 1, found 10
> >   d95f3ad14dee633a758d2e331151e950dd13e4ed
> >
> > The warning is fine and reasonable here. But then:
> >
> >   $ echo '[core]repositoryformatversion = foobar' >.git/config
> >   $ git hash-object file
> >   fatal: bad numeric config value 'foobar' for 
> > 'core.repositoryformatversion' in file .git/config: invalid unit
> >
> > That's wrong. We're supposed to be gentle. And ditto:
> >
> >   $ echo '[co' >.git/config
> >   $ git hash-object file
> >   fatal: bad config line 1 in file .git/config
> >
> > Those last two should issue a warning at most, and then let the command
> > continue.
> 
> Yeah, I agree with that as one of the worthy goals.  IIUC, we
> decided to leave that outside of this series and later fix on top,
> which is fine by me, too.

How about this on top, then:

-- snipsnap --
From: Johannes Schindelin 
Subject: [PATCH] t1309: document cases where we would want early config not to
 die()

Jeff King came up with a couple examples that demonstrate how the new
read_early_config() that looks harder for the current .git/ directory
could die() in an undesirable way.

Let's add those cases to the test script, to document what we would like
to happen when early config encounters problems.

Signed-off-by: Johannes Schindelin 
---
 t/t1309-early-config.sh | 25 +
 1 file changed, 25 insertions(+)

diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
index 0c55dee514c..027eca63a3c 100755
--- a/t/t1309-early-config.sh
+++ b/t/t1309-early-config.sh
@@ -47,4 +47,29 @@ test_expect_success 'ceiling #2' '
test xdg = "$(cat output)"
 '
 
+test_with_config ()
+{
+   rm -rf throwaway &&
+   git init throwaway &&
+   (
+   cd throwaway &&
+   echo "$*" >.git/config &&
+   test-config read_early_config early.config
+   )
+}
+
+test_expect_success 'ignore .git/ with incompatible repository version' '
+   test_with_config "[core]repositoryformatversion = 99" 2>err &&
+   grep "warning:.* Expected git repo version <= [1-9]" err
+'
+
+test_expect_failure 'ignore .git/ with invalid repository version' '
+   test_with_config "[core]repositoryformatversion = invalid"
+'
+
+
+test_expect_failure 'ignore .git/ with invalid config' '
+   test_with_config "["
+'
+
 test_done
-- 
2.12.0.windows.1.7.g94dafc3b124

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

[Junio C Hamano]

Jeff King  writes:

>> Or are you discussing a more general issue, iow, anything that can
>> work without repository (i.e. those who do _gently version of the
>> setup and act on *nongit_ok) should pretend as if there were no
>> (broken) repository and take the "no we are not in a repository"
>> codepath?
>
> Yes, exactly.  It would have been less confusing if I picked something
> that passed nongit_ok. Like hash-object:
>
>   $ git init
>   $ echo content >file
>   $ git hash-object file
>   d95f3ad14dee633a758d2e331151e950dd13e4ed
>
>   $ echo '[core]repositoryformatversion = 10' >.git/config
>   $ git hash-object file
>   warning: Expected git repo version <= 1, found 10
>   d95f3ad14dee633a758d2e331151e950dd13e4ed
>
> The warning is fine and reasonable here. But then:
>
>   $ echo '[core]repositoryformatversion = foobar' >.git/config
>   $ git hash-object file
>   fatal: bad numeric config value 'foobar' for 'core.repositoryformatversion' 
> in file .git/config: invalid unit
>
> That's wrong. We're supposed to be gentle. And ditto:
>
>   $ echo '[co' >.git/config
>   $ git hash-object file
>   fatal: bad config line 1 in file .git/config
>
> Those last two should issue a warning at most, and then let the command
> continue.

Yeah, I agree with that as one of the worthy goals.  IIUC, we
decided to leave that outside of this series and later fix on top,
which is fine by me, too.

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

[Jeff King]

On Wed, Mar 08, 2017 at 05:18:46PM +0100, Johannes Schindelin wrote:

> On Wed, 8 Mar 2017, Jeff King wrote:
> 
> > Another "non-gentle" thing I noticed here while looking at
> > another thread: the repository-format version check uses the config
> > parser, which will die() in certain circumstances. [...]
> 
> Yes, that is part of the reason why I was not eager to add that check to
> discover_git_directory(). The config code is die()-happy.
> 
> This is a much bigger problem, of course, and related to a constant gripe
> of mine: I *always* get the quoting wrong in my aliases. Always. And when
> I want to fix it, `git config -e` simply errors out because of an invalid
> config. Yes, Git, I know, that is the exact reason why I want to edit the
> config in the first place.
> 
> I am certain you will agree that this is a different topic, therefore
> subject to a separate patch series.

I agree that in general it is a separate issue. Technically it could
cause a regression because with your series we are more eager to call
discover_git_directory() even when the command would not otherwise need
to even look at the current git directory.

But I don't think that is the case in practice. Your new function is
only called when we would try to read the config anyway. So it would
only affect cases which were previously so broken they did not properly
read the config (like running a command without RUN_SETUP from a
subdirectory of the working tree, which was _supposed_ to respect the
config but failed to do so).

So yeah, I'm happy to leave it for now.

-Peff

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

[Jeff King]

On Wed, Mar 08, 2017 at 09:09:31AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Good catch. Another "non-gentle" thing I noticed here while looking at
> > another thread: the repository-format version check uses the config
> > parser, which will die() in certain circumstances. So for instance:
> >
> >   $ git init
> >   $ git rev-parse && echo ok
> >   ok
> >
> >   $ echo '[core]repositoryformatversion = 10' >.git/config
> >   $ git rev-parse && echo ok
> >   fatal: Expected git repo version <= 1, found 10
> 
> Just to set my expectation straight.  Do you expect/wish this not to
> fail because of this in cmd_rev_parse()?

No, I was just using "rev-parse" as a sample command that tried to do
repo setup. I meant the above snippet that you quoted to both be fine
and expected outputs. The problem is the _other_ two cases where the
config code dies before we even get to the version-number check.

> Or are you discussing a more general issue, iow, anything that can
> work without repository (i.e. those who do _gently version of the
> setup and act on *nongit_ok) should pretend as if there were no
> (broken) repository and take the "no we are not in a repository"
> codepath?

Yes, exactly.  It would have been less confusing if I picked something
that passed nongit_ok. Like hash-object:

  $ git init
  $ echo content >file
  $ git hash-object file
  d95f3ad14dee633a758d2e331151e950dd13e4ed

  $ echo '[core]repositoryformatversion = 10' >.git/config
  $ git hash-object file
  warning: Expected git repo version <= 1, found 10
  d95f3ad14dee633a758d2e331151e950dd13e4ed

The warning is fine and reasonable here. But then:

  $ echo '[core]repositoryformatversion = foobar' >.git/config
  $ git hash-object file
  fatal: bad numeric config value 'foobar' for 'core.repositoryformatversion' 
in file .git/config: invalid unit

That's wrong. We're supposed to be gentle. And ditto:

  $ echo '[co' >.git/config
  $ git hash-object file
  fatal: bad config line 1 in file .git/config

Those last two should issue a warning at most, and then let the command
continue.

-Peff

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

[Junio C Hamano]

Jeff King  writes:

> Good catch. Another "non-gentle" thing I noticed here while looking at
> another thread: the repository-format version check uses the config
> parser, which will die() in certain circumstances. So for instance:
>
>   $ git init
>   $ git rev-parse && echo ok
>   ok
>
>   $ echo '[core]repositoryformatversion = 10' >.git/config
>   $ git rev-parse && echo ok
>   fatal: Expected git repo version <= 1, found 10

Just to set my expectation straight.  Do you expect/wish this not to
fail because of this in cmd_rev_parse()?

/* No options; just report on whether we're in a git repo or not. */
if (argc == 1) {
setup_git_directory();
git_config(git_default_config, NULL);
return 0;
}

Because we do not have anything other than yes/no to the question
"Are we in Git repository?", I'd expect that the expected answer to
the question would be "no" (if we could say "Yes, we are in a Git
repository but its version and layout is unknown to us so we are not
supposed to look at or touch it", that is a different matter).

So "fatal:" may be bad, but I think not seeing "ok" is what we
want to happen in this case.

Having said that, I am not sure asking for default-config is what we
wanted to do in the above code.  Perhaps a more modern way to write
the above code would be to do the "gently" version of setup, without
calling git_config() ourselves, and return the resulting value
returned in *nongit_ok?  If we can do so without triggering "fatal:"
and still return "no, we are not in a Git repository we are supposed
to touch", that would be good.

Or are you discussing a more general issue, iow, anything that can
work without repository (i.e. those who do _gently version of the
setup and act on *nongit_ok) should pretend as if there were no
(broken) repository and take the "no we are not in a repository"
codepath?

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

[Johannes Schindelin]

Hi Peff,

On Wed, 8 Mar 2017, Jeff King wrote:

> Another "non-gentle" thing I noticed here while looking at
> another thread: the repository-format version check uses the config
> parser, which will die() in certain circumstances. [...]

Yes, that is part of the reason why I was not eager to add that check to
discover_git_directory(). The config code is die()-happy.

This is a much bigger problem, of course, and related to a constant gripe
of mine: I *always* get the quoting wrong in my aliases. Always. And when
I want to fix it, `git config -e` simply errors out because of an invalid
config. Yes, Git, I know, that is the exact reason why I want to edit the
config in the first place.

I am certain you will agree that this is a different topic, therefore
subject to a separate patch series.

In any case, I am fairly certain that the examples you showed demonstrate
that the config has to be rather broken for this patch series to have a
negative impact. And it still would report the broken config so that the
user is not blocked (she can fix the config and call the paginating
command again).

> On Tue, Mar 07, 2017 at 03:31:43PM +0100, Johannes Schindelin wrote:
> 
> > And another change: the GIT_DIR_NONE value was handled incorrectly in
> > discover_git_directory().
> 
> This is the "if (setup_git_directory_1() <= 0)" change from the
> interdiff? That's subtle.

Yes, it is subtle.

> The compiler could have noticed if we used a switch statement here. But
> then any new error conditions would have to be added to that switch
> statement.

We could still do that.

> > I am slightly disappointed that the these additional problems were not
> > spotted in any review but my own. And I had not even included a Duck.
> 
> Get used to being disappointed, I guess. A non-zero number of bugs will
> slip through when writing code _and_ when reviewing it.

I know that. I know that bugs are prone to come in through code
contributions. I don't go for 100%. But I would hope for a better rate
than we have right now: we pride ourselves in the OSS community to make bugs
shallow. I really would like to believe that we catch bugs rather than
discuss formatting (which should be automated) or white-space mangling
(which should not even be a problem if we were truly open for
contributions).

Maybe I am just a grumpy old guy. But I *hate* seeing how much buggy code
we get into Git, despite having a review process that does a very good job
of deterring seasoned developers from contributing.

I really wish it were different. I really wish that we did a better job at
catching bugs before they enter `master`. I really wish that I could be
proud of our code review process.

> > [ceil_offset]
> > Hopefully that clears up the picture?
> 
> Yes, it does. Thanks.

Perfect. Then the time I spent trying to figure all of this out was not
spent in vain.

Ciao,
Dscho

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

[Jeff King]

On Tue, Mar 07, 2017 at 03:31:43PM +0100, Johannes Schindelin wrote:

> >   /*
> >* 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.
> >*/
> 
> I changed it a little bit more. In particular, I changed the
> discover_git_directory() function to return the pointer to the path
> itself: it provides additional value, and if that is not what the caller
> wants, they can use git_dir->buf just as well.

Yes, that makes much more sense.

> There is one more thing I included in v4: when I (re-)implemented that
> pre-command/post-command hook I was hinting at earlier, the test suite
> identified a problem where an invalid .git file would prevent even `git
> init` from working (it was actually much more complicated than that, but
> the gist is that `git -p init` would fail, no matter how much sense it
> may make to you to paginate an `init` run, it should still not fail,
> right?). So I added a patch on top to fix that.

Good catch. Another "non-gentle" thing I noticed here while looking at
another thread: the repository-format version check uses the config
parser, which will die() in certain circumstances. So for instance:

  $ git init
  $ git rev-parse && echo ok
  ok

  $ echo '[core]repositoryformatversion = 10' >.git/config
  $ git rev-parse && echo ok
  fatal: Expected git repo version <= 1, found 10

  $ echo '[core]repositoryformatversion = foobar' >.git/config
  $ git rev-parse && echo ok
  fatal: bad numeric config value 'foobar' for 'core.repositoryformatversion' 
in file .git/config: invalid unit

  $ echo '[co' >.git/config
  $ git rev-parse && echo ok
  fatal: bad config line 1 in file .git/config

Your series correctly avoids the first failure by calling the
read/verify_repository_format functions correctly. But I think it would
get tripped up by the other two.

Fixing the first one is probably not too hard; check_repo_format()
should use a more forgiving parser than git_config_int().

The second one I thought would be tricky, but it looks like we added a
die_on_error flag in b2dc09455. That does what we want, but it needs to
be plumbed through to git_config_from_file().

> And another change: the GIT_DIR_NONE value was handled incorrectly in
> discover_git_directory().

This is the "if (setup_git_directory_1() <= 0)" change from the
interdiff? That's subtle. The compiler could have noticed if we used a
switch statement here. But then any new error conditions would have to
be added to that switch statement.

> I am slightly disappointed that the these additional problems were not
> spotted in any review but my own. And I had not even included a Duck.

Get used to being disappointed, I guess. A non-zero number of bugs will
slip through when writing code _and_ when reviewing it.

> [ceil_offset]
> Hopefully that clears up the picture?

Yes, it does. Thanks.

-Peff

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

[Johannes Schindelin]

Hi Junio,

On Fri, 3 Mar 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Notable notes:
> >
> > - In contrast to earlier versions, I no longer special-case init and
> >   clone. Peff pointed out that this adds technical debt, and that we can
> >   actually argue (for consistency's sake) that early config reads the
> >   current repository config (if any) even for init and clone.
> >
> > - The read_early_config() function does not cache Git directory
> >   discovery nor read values. If needed, this can be implemented later,
> >   in a separate patch series.
> >
> > - The alias handling in git.c could possibly benefit from this work, but
> >   again, this is a separate topic from the current patch series.
> 
> As Peff said in his review, I too find the result of this series a
> more pleasant read than than original.

As do I.

> 2/9 and corresponding 4/9 triggers "ERROR: trailing statements
> should be on next line" from ../linux/scripts/checkpatch.pl because
> of a line inherited from the original; I'll queue them with an
> obvious style fix to work it around.

Thank you. I'll try to pick it up for v3 (which is needed, as I found
another issue that needs to be fixed).

Ciao,
Johannes

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

[Johannes Schindelin]

Hi Peff,

On Sat, 4 Mar 2017, Jeff King wrote:

> 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()) {
> >  +  if (!have_git_dir() && discover_git_directory()) {
> 
> I think this "when we are not about to..." part of the comment is no
> longer true, given the second part of the hunk.

Yep, that was a stale part of that patch. Thanks for noticing!

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

Right. I also wanted to avoid that very, very much. My initial version
actually tried to detect whether cwd already has a trailing slash, but
then I figured that we can be much, much more precise here (and I am
really pleased how offset_1st_component() is *semantically* precise, i.e.
it describes very well what the code is supposed to do here).

> > +/* 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.
>*/

I changed it a little bit more. In particular, I changed the
discover_git_directory() function to return the pointer to the path
itself: it provides additional value, and if that is not what the caller
wants, they can use git_dir->buf just as well.

> This looks good to me aside from those few comment nits.

Thanks.

It is not obvious from the interdiff, but I had an incorrect fixup to 8/9
that actually wanted to go to 5/9: the code in
discover_git_repository() tests the repository version should be part of
the initial version of this function, of course.

There is one more thing I included in v4: when I (re-)implemented that
pre-command/post-command hook I was hinting at earlier, the test suite
identified a problem where an invalid .git file would prevent even `git
init` from working (it was actually much more complicated than that, but
the gist is that `git -p init` would fail, no matter how much sense it
may make to you to paginate an `init` run, it should still not fail,
right?). So I added a patch on top to fix that.

And another change: the GIT_DIR_NONE value was handled incorrectly in
discover_git_directory().

I am slightly disappointed that the these additional problems were not
spotted in any review but my own. And I had not even included a Duck.

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

Yes, that code is very confusing. It also does not help that the naming is
inconsistent in that it abbreviates "ceiling" but not "offset". What makes
it even worse is that the function name `longest_ancestor_length()` is
highly misleading: in Git's context, "ancestor" of sth pretty much always
refers to a commit reachable from sth, but in this context it refers to
the path of a directory containing sth.

So basically, we set ceil_offset to the offset of the last directory
separator in our path that corresponds to the most precise match in
GIT_CEILING_DIRECTORIES.

Example: given GIT_CEILING_DIRECTORIES /foo:/:/bar and a path of /foo/bar,
ceil_offset would be 4, pointing to the slash at the end of /foo/ because
that is the most precise match in GIT_CEILING_DIRECTORIES ("/" would also
match, but is less precise).

Later, setup_git_directory_gently_1() ensures that it does not go beyond
ceil_offset when looking for 

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

[Johannes Schindelin]

Hi Junio,

On Fri, 3 Mar 2017, Junio C Hamano wrote:

> 2/9 and corresponding 4/9 triggers "ERROR: trailing statements
> should be on next line" from ../linux/scripts/checkpatch.pl because
> of a line inherited from the original; I'll queue them with an
> obvious style fix to work it around.

Wow, it seems that script requires the entire Linux kernel repository to
be cloned, you cannot simply download just the script and run it.

In any case, as you pointed out, this style was inherited from the
original.

I squashed that style fix in, as you probably would have (your
js/early-config does not have anything beyond v2.12.0). But I have to
point out that it is conflating the purpose of this patch series (its goal
is *not* to fix any style). I am absolutely not a fan of that.

Ciao,
Johannes

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

[Junio C Hamano]

On Fri, Mar 3, 2017 at 11:39 PM, Jeff King  wrote:
>>  + * 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()) {
>>  +if (!have_git_dir() && discover_git_directory()) {
>
> I think this "when we are not about to..." part of the comment is no
> longer true, given the second part of the hunk.

Good point.

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

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

I'll wait to see if Dscho wants to clarify the code and comments further.

Thanks.

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

[Jeff King]

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()) {
>  +if (!have_git_dir() && discover_git_directory()) {

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

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

[Junio C Hamano]

Johannes Schindelin  writes:

> Notable notes:
>
> - In contrast to earlier versions, I no longer special-case init and
>   clone. Peff pointed out that this adds technical debt, and that we can
>   actually argue (for consistency's sake) that early config reads the
>   current repository config (if any) even for init and clone.
>
> - The read_early_config() function does not cache Git directory
>   discovery nor read values. If needed, this can be implemented later,
>   in a separate patch series.
>
> - The alias handling in git.c could possibly benefit from this work, but
>   again, this is a separate topic from the current patch series.

As Peff said in his review, I too find the result of this series a
more pleasant read than than original.

2/9 and corresponding 4/9 triggers "ERROR: trailing statements
should be on next line" from ../linux/scripts/checkpatch.pl because
of a line inherited from the original; I'll queue them with an
obvious style fix to work it around.

Thanks.

[PATCH v3 0/9] Fix the early config

[Johannes Schindelin]

These patches are an attempt to make Git's startup sequence a bit less
surprising.

The idea here is to discover the .git/ directory gently (i.e. without
changing the current working directory, or global variables), and to use
it to read the .git/config file early, before we actually called
setup_git_directory() (if we ever do that).

This also allows us to fix the early config e.g. to determine the pager
or to resolve aliases in a non-surprising manner.

My dirty little secret is that I need to discover the Git directory
early, without changing global state, for usage statistics gathering in
the GVFS Git project, so I actually do not care all that much about the
early config, although it is a welcome fallout (and a good reason for
accepting these patches and thereby releasing me of more maintenance
burden :-)).

Notable notes:

- In contrast to earlier versions, I no longer special-case init and
  clone. Peff pointed out that this adds technical debt, and that we can
  actually argue (for consistency's sake) that early config reads the
  current repository config (if any) even for init and clone.

- The read_early_config() function does not cache Git directory
  discovery nor read values. If needed, this can be implemented later,
  in a separate patch series.

- The alias handling in git.c could possibly benefit from this work, but
  again, this is a separate topic from the current patch series.

Changes since v2:

- replaced `test -e` by `test_path_is_file`

- fixed premature "cwd -> dir" in 2/9

- the setup_git_directory_gently_1() function is no longer renamed
  because it is not exported directly, anyway

- fixed the way setup_discovered_git_dir() expected the offset parameter
  to exclude the trailing slash (which is not true for root
  directories); also verified that setup_bare_git_dir() does not require
  a corresponding patch

- switched to using size_t instead of int to save the length of the
  strbuf in discover_git_directory()

- ensured that discover_git_directory() turns a relative gitdir into an
  absolute one even if there is already some text in the strbuf

- clarified under which circumstances we turn a relative gitdir into an
  absolute one

- avoided absolute gitdir with trailing "/." to be returned

- the commit that fixes the "really dirty hack" now rewords that comment
  to reflect that it is no longer a really dirty hack

- dropped the special-casing of init and clone

- the discover_git_directory() function now correctly checks the
  repository version, warning (and returning NULL) in case of a problem


Johannes Schindelin (9):
  t7006: replace dubious test
  setup_git_directory(): use is_dir_sep() helper
  Prepare setup_discovered_git_directory() the root directory
  setup_git_directory_1(): avoid changing global state
  Export the discover_git_directory() function
  Make read_early_config() reusable
  read_early_config(): avoid .git/config hack when unneeded
  read_early_config(): really discover .git/
  Test read_early_config()

 cache.h |   3 +
 config.c|  27 ++
 pager.c |  31 ---
 setup.c | 237 
 t/helper/test-config.c  |  15 +++
 t/t1309-early-config.sh |  50 ++
 t/t7006-pager.sh|  18 +++-
 7 files changed, 269 insertions(+), 112 deletions(-)
 create mode 100755 t/t1309-early-config.sh


base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858
Published-As: https://github.com/dscho/git/releases/tag/early-config-v3
Fetch-It-Via: git fetch https://github.com/dscho/git early-config-v3

Interdiff vs v2:

 diff --git a/cache.h b/cache.h
 index 0af7141242f..8a4580f921d 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);
 +/* Find GIT_DIR without changing the working directory or other global state 
*/
  extern const char *discover_git_directory(struct strbuf *gitdir);
  extern const char *setup_git_directory_gently(int *);
  extern const char *setup_git_directory(void);
 @@ -2070,7 +2071,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
  
  /* setup.c */
  struct startup_info {
 -  int have_repository, creating_repository;
 +  int have_repository;
const char *prefix;
  };
  extern struct startup_info *startup_info;
 diff --git a/config.c b/config.c
 index bcda397d42e..749623a9649 100644
 --- a/config.c
 +++ b/config.c
 @@ -1419,25 +1419,16 @@ void read_early_config(config_fn_t cb, void *data)
git_config_with_options(cb, data, NULL, 1);
  
/*
 -   * Note that this is a really dirty hack that does the wrong thing in
 -   * many cases. The crux of the problem is that we cannot run
 -   * setup_git_directory() early on in git's setup, so we have no idea if
 -   * we are in a repository or not, and therefore are not sure whether
 -