Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin

Johannes Schindelin [off-list ref] writes:

Seriously, do you really think it is a good idea to have
git_config_get_value() *ignore* any value in .git/config?

When we do not know ".git/" directory we see is the repository we
were told to work on, then we should ignore ".git/config" file.  So
allowing git_config_get_value() to _ignore_ ".git/config" before the
program calls setup_git_directory() does have its uses.

But I agree that "difftool.useBuiltin" that flips between two
implementations is a different use case than the above.  Both
implementations eventually want to work on ".git/" and would
want to read from ".git/config".

We need to fix this.

I have a feeling that "environment modifications that cannot be
undone" we used as the rationale in 73c2779f42 ("builtin-am:
implement skeletal builtin am", 2015-08-04) might be overly
pessimistic and in order to implement undo_setup_git_directory(),
all we need to do may just be matter of doing a chdir(2) back to
prefix and unsetting GIT_PREFIX environment, but I haven't looked
into details of the setup sequence recently.

Hi Junio,

On Mon, 5 Dec 2016, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

Seriously, do you really think it is a good idea to have
git_config_get_value() *ignore* any value in .git/config?

When we do not know ".git/" directory we see is the repository we were
told to work on, then we should ignore ".git/config" file.  So allowing
git_config_get_value() to _ignore_ ".git/config" before the program
calls setup_git_directory() does have its uses.

I think you are wrong. This is yet another inconsistent behavior that
violates the Law of Least Surprises.

quoted

We need to fix this.

I have a feeling that "environment modifications that cannot be undone"
we used as the rationale in 73c2779f42 ("builtin-am: implement skeletal
builtin am", 2015-08-04) might be overly pessimistic and in order to
implement undo_setup_git_directory(), all we need to do may just be
matter of doing a chdir(2) back to prefix and unsetting GIT_PREFIX
environment, but I haven't looked into details of the setup sequence
recently.

The problem is that we may not know enough anymore to "undo
setup_git_directory()", as some environment variables may have been set
before calling Git. If we simply unset the environment variables, we do it
incorrectly. On the other hand, the environment variables *may* have been
set by setup_git_directory(). In which case we *do* have to unset them.

The entire setup_git_directory() logic is a bit of a historically-grown
garden.

Ciao,
Dscho

On Tue, Dec 06, 2016 at 02:16:35PM +0100, Johannes Schindelin wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

Seriously, do you really think it is a good idea to have
git_config_get_value() *ignore* any value in .git/config?

When we do not know ".git/" directory we see is the repository we were
told to work on, then we should ignore ".git/config" file.  So allowing
git_config_get_value() to _ignore_ ".git/config" before the program
calls setup_git_directory() does have its uses.

I think you are wrong. This is yet another inconsistent behavior that
violates the Law of Least Surprises.

There are surprises in this code any way you turn.  If we have not
called setup_git_directory(), then how does git_config_get_value() know
if we are in a repository or not?

Should it blindly look at ".git/config"? Now your program behaves
differently depending on whether you are in the top-level of the working
tree.

Should it speculatively do repo discovery, and use any discovered config
file? Now some commands respect config that they shouldn't (e.g.,
running "git init foo.git" from inside another repository will
accidentally pick up the value of core.sharedrepository from wherever
you happen to run it).

So I think the caller of the config code has to provide some kind of
context about how it is expecting to run and how the value will be used.

Right now if setup_git_directory() or similar hasn't been called, the
config code does not look. Ideally there would be a way for a caller to
say "I am running early and not even sure yet if we are in a repo;
please speculatively try to find repo config for me".

The pager code does this manually, and without great accuracy; see the
hack in pager.c's read_early_config(). I think the way forward is:

  1. Make that an optional behavior in git_config_with_options() so
     other spots can reuse it (probably alias lookup, and something like
     your difftool config).

  2. Make it more accurate. Right now it blindly looks in .git/config,
     but it should be able to do the usual repo-detection (_without_
     actually entering the repo) to try to find a possible config file.

-Peff

Hi Peff,

On Tue, 6 Dec 2016, Jeff King wrote:

On Tue, Dec 06, 2016 at 02:16:35PM +0100, Johannes Schindelin wrote:

quoted

quoted

Johannes Schindelin [off-list ref] writes:

quoted

Seriously, do you really think it is a good idea to have
git_config_get_value() *ignore* any value in .git/config?

When we do not know ".git/" directory we see is the repository we
were told to work on, then we should ignore ".git/config" file.  So
allowing git_config_get_value() to _ignore_ ".git/config" before the
program calls setup_git_directory() does have its uses.

I think you are wrong. This is yet another inconsistent behavior that
violates the Law of Least Surprises.

There are surprises in this code any way you turn.  If we have not
called setup_git_directory(), then how does git_config_get_value() know
if we are in a repository or not?

My biggest surprise, frankly, would be that `git init` and `git clone` are
not special-cased.

Should it blindly look at ".git/config"?

Absolutely not, of course. You did not need me to say that.

Now your program behaves differently depending on whether you are in the
top-level of the working tree.

Exactly. This, BTW, is already how the code would behave if anybody called
`git_path()` before `setup_git_directory()`, as the former function
implicitly calls `setup_git_env()` which does *not* call
`setup_git_directory()` but *does* set up `git_dir` which is then used by
`do_git_config_sequence()`..

We have a few of these nasty surprises in our code base, where code
silently assumes that global state is set up correctly, and succeeds in
sometimes surprising ways if it is not.

Should it speculatively do repo discovery, and use any discovered config
file?

Personally, I find the way we discover the repository most irritating. It
seems that we have multiple, mutually incompatible code paths
(`setup_git_directory()` and `setup_git_env()` come to mind already, and
it does not help that consecutive calls to `setup_git_directory()` will
yield a very unexpected outcome).

Just try to explain to a veteran software engineer why you cannot call
`setup_git_directory_gently()` multiple times and expect the very same
return value every time.

Another irritation is that some commands that clearly would like to use a
repository if there is one (such as `git diff`) are *not* marked with
RUN_SETUP_GENTLY, due to these unfortunate implementation details.

Now some commands respect config that they shouldn't (e.g., running "git
init foo.git" from inside another repository will accidentally pick up
the value of core.sharedrepository from wherever you happen to run it).

Right. That points to another problem with the way we do things: we leak
global state from discovering a git_dir, which means that we can neither
undo nor override it.

If we discovered our git_dir in a robust manner, `git init` could say:
hey, this git_dir is actually not what I wanted, here is what I want.

Likewise, `git submodule` would eventually be able to run in the very same
process as the calling `git`, as would a local fetch.

So I think the caller of the config code has to provide some kind of
context about how it is expecting to run and how the value will be used.

Yep.

Maybe even go a step further and say that the config code needs a context
"object".

Right now if setup_git_directory() or similar hasn't been called, the
config code does not look.

Correct.

Actually, half correct. If setup_git_directory() has not been called, but,
say, git_path() (and thereby implicitly setup_git_env()), the config code
*does* look. At a hard-coded .git/config.

Ideally there would be a way for a caller to say "I am running early and
not even sure yet if we are in a repo; please speculatively try to find
repo config for me".

And ideally, it would not roll *yet another* way to discover the git_dir,
but it would reuse the same function (fixing it not to chdir()
unilaterally).

Of course, not using `chdir()` means that we have to figure out symbolic
links somehow, in case somebody works from a symlinked subdirectory, e.g.:

	ln -s $PWD/t/ ~/test-directory
	cd ~/test-directory
	git log

The pager code does this manually, and without great accuracy; see the
hack in pager.c's read_early_config().

I saw it. And that is what triggered the mail you are responding to (you
probably saw my eye-rolling between the lines).

The real question is: can we fix this? Or is there simply too great
reluctance to change the current code?

I think the way forward is:

  1. Make that an optional behavior in git_config_with_options() so
     other spots can reuse it (probably alias lookup, and something like
     your difftool config).

Ideally: *any* early call to `git_config_get_value()`. Make things less
surprising.

  2. Make it more accurate. Right now it blindly looks in .git/config,
     but it should be able to do the usual repo-detection (_without_
     actually entering the repo) to try to find a possible config file.

The real trick will be to convince Junio to have a single function for
git_dir discovery, I guess, lest we have multiple, slightly incompatible
ways to discover it. I expect a lot of resistance here, because we would
have to change tried-and-tested (if POLA-violating) code.

Ciao,
Dscho

On Tue, Dec 06, 2016 at 03:48:38PM +0100, Johannes Schindelin wrote:

quoted

Should it blindly look at ".git/config"?

Absolutely not, of course. You did not need me to say that.

quoted

Now your program behaves differently depending on whether you are in the
top-level of the working tree.

Exactly. This, BTW, is already how the code would behave if anybody called
`git_path()` before `setup_git_directory()`, as the former function
implicitly calls `setup_git_env()` which does *not* call
`setup_git_directory()` but *does* set up `git_dir` which is then used by
`do_git_config_sequence()`..

We have a few of these nasty surprises in our code base, where code
silently assumes that global state is set up correctly, and succeeds in
sometimes surprising ways if it is not.

Right. I have been working on fixing this. v2.11 has a ton of tweaks in
this area, and my patch to die() rather than default to ".git" is
cooking in next to catch any stragglers.

quoted

Should it speculatively do repo discovery, and use any discovered config
file?

Personally, I find the way we discover the repository most irritating. It
seems that we have multiple, mutually incompatible code paths
(`setup_git_directory()` and `setup_git_env()` come to mind already, and
it does not help that consecutive calls to `setup_git_directory()` will
yield a very unexpected outcome).

I agree. We should be killing off setup_git_env(), which is something
I've been slowly working towards over the years.

There are some annoyances with setup_git_directory(), too (like the fact
that you cannot ask "is there a git repository you can find" without
making un-recoverable changes to the process state). I think we should
fix those, too.

quoted

Now some commands respect config that they shouldn't (e.g., running "git
init foo.git" from inside another repository will accidentally pick up
the value of core.sharedrepository from wherever you happen to run it).

Right. That points to another problem with the way we do things: we leak
global state from discovering a git_dir, which means that we can neither
undo nor override it.

If we discovered our git_dir in a robust manner, `git init` could say:
hey, this git_dir is actually not what I wanted, here is what I want.

Likewise, `git submodule` would eventually be able to run in the very same
process as the calling `git`, as would a local fetch.

Yep, I agree with all that. I do think things _have_ been improving over
the years, and the code is way less tangled than it used to be. But
there are so many corner cases, and the code is so fundamental, that it
has been slow going. I'd be happy to review patches if you want to push
it along.

quoted

So I think the caller of the config code has to provide some kind of
context about how it is expecting to run and how the value will be used.

Yep.

Maybe even go a step further and say that the config code needs a context
"object".

If I were writing git from scratch, I'd consider making a "struct
repository" object. I'm not sure how painful it would be to retro-fit it
at this point.

quoted

Right now if setup_git_directory() or similar hasn't been called, the
config code does not look.

Correct.

Actually, half correct. If setup_git_directory() has not been called, but,
say, git_path() (and thereby implicitly setup_git_env()), the config code
*does* look. At a hard-coded .git/config.

Not since b9605bc4f (config: only read .git/config from configured
repos, 2016-09-12). That's why pager.c needs its little hack.

I guess you could see that as a step backwards, but I think it is
necessary one on the road to doing it right.

quoted

Ideally there would be a way for a caller to say "I am running early and
not even sure yet if we are in a repo; please speculatively try to find
repo config for me".

And ideally, it would not roll *yet another* way to discover the git_dir,
but it would reuse the same function (fixing it not to chdir()
unilaterally).

Yes, absolutely.

Of course, not using `chdir()` means that we have to figure out symbolic
links somehow, in case somebody works from a symlinked subdirectory, e.g.:

	ln -s $PWD/t/ ~/test-directory
	cd ~/test-directory
	git log

There's work happening elsewhere[1] on making real_path() work without
calling chdir(). Which necessarily involves resolving symlinks
ourselves. I wonder if we could leverage that work here (ideally by
using real_path() under the hood, and not reimplementing the same
readlink() logic ourselves).

[1] http://public-inbox.org/git/1480964316-99305-1-git-send-email-bmwill@google.com/

quoted

The pager code does this manually, and without great accuracy; see the
hack in pager.c's read_early_config().

I saw it. And that is what triggered the mail you are responding to (you
probably saw my eye-rolling between the lines).

The real question is: can we fix this? Or is there simply too great
reluctance to change the current code?

The code in pager.c is only a month or two old. Like I said, it's ugly,
but I think it's a necessary step on the way forward. So I don't think
there's reluctance at all. The next steps (which I outlined) just
haven't been taken yet.

quoted

I think the way forward is:

  1. Make that an optional behavior in git_config_with_options() so
     other spots can reuse it (probably alias lookup, and something like
     your difftool config).

Ideally: *any* early call to `git_config_get_value()`. Make things less
surprising.

I'm not convinced that's a good idea. The changes in b9605bc4f were
motivated by a real bug, which your suggestion would reintroduce (namely
low-level code run by git-init ending up with config variables from a
repo that _should_ be unrelated).

In my mental model, the cases are:

  1. We are "early" in the process, before we know if we have a repo or
     not. These early looks should speculatively look at repo config,
     which is confined to generic things like pager config, alias
     config, etc.

  2. We are in a repo. Obviously look at $GIT_DIR/config.

  3. We are in a program which has done setup and determined we are
     _not_ in a repo. Definitely do not look at .git/config or anything
     else.

My plan was for the config code to default to (3) when we are not in a
repo, but let some lookups ask specifically for (1).

If you want to default to (1), you need some way for programs to say "I
am really case (3); do not look for a repo". And it needs to be global,
as the config lookup may be done by much lower-level code. That could be
by turning startup_info->have_repository into a tri-state. It just
wasn't the way I was planning on it.

quoted

  2. Make it more accurate. Right now it blindly looks in .git/config,
     but it should be able to do the usual repo-detection (_without_
     actually entering the repo) to try to find a possible config file.

The real trick will be to convince Junio to have a single function for
git_dir discovery, I guess, lest we have multiple, slightly incompatible
ways to discover it. I expect a lot of resistance here, because we would
have to change tried-and-tested (if POLA-violating) code.

Personally, I haven't seen any resistance from Junio on refactoring this
area. I'm sure he is concerned that we do not regress, but it's not like
the area has been unchanged over the years. It has been slow going
because we want to do it carefully, but I think we are actually at the
point now where the next step is making setup_git_directory() more sane.

-Peff

On Tue, Dec 6, 2016 at 7:09 AM, Jeff King [off-list ref] wrote:

quoted

Maybe even go a step further and say that the config code needs a context
"object".

If I were writing git from scratch, I'd consider making a "struct
repository" object. I'm not sure how painful it would be to retro-fit it
at this point.

Would it be possible to introduce "the repo" struct similar to "the index"
in cache.h?

From a submodule perspective I would very much welcome this
object oriented approach to repositories.

On Tue, Dec 06, 2016 at 10:22:21AM -0800, Stefan Beller wrote:

quoted

quoted

Maybe even go a step further and say that the config code needs a context
"object".

If I were writing git from scratch, I'd consider making a "struct
repository" object. I'm not sure how painful it would be to retro-fit it
at this point.

Would it be possible to introduce "the repo" struct similar to "the index"
in cache.h?

From a submodule perspective I would very much welcome this
object oriented approach to repositories.

I think it may be more complicated, because there's some implicit global
state in "the repo", like where files are relative to our cwd. All of
those low-level functions would have to start caring about which repo
we're talking about so they can prefix the appropriate working tree
path, etc.

For some operations that would be fine, but there are things that would
subtly fail for submodules. I'm thinking we'd end up with some code
state like:

  /* finding a repo does not modify global state; good */
  struct repository *repo = repo_discover(".");

  /* obvious repo-level operations like looking up refs can be done with
   * a repository object; good */
  repo_for_each_ref(repo, callback, NULL);

  /*
   * "enter" the repo so that we are at the top-level of the working
   * tree, etc. After this you can actually look at the index without
   * things breaking.
   */
  repo_enter(repo);

That would be enough to implement a lot of submodule-level stuff, but it
would break pretty subtly as soon as you asked the submodule about its
working tree. The solution is to make everything that accesses the
working tree aware of the idea of a working tree root besides the cwd.
But that's a pretty invasive change.

-Peff

On 12/06, Jeff King wrote:

On Tue, Dec 06, 2016 at 10:22:21AM -0800, Stefan Beller wrote:

quoted

quoted

quoted

Maybe even go a step further and say that the config code needs a context
"object".

If I were writing git from scratch, I'd consider making a "struct
repository" object. I'm not sure how painful it would be to retro-fit it
at this point.

Would it be possible to introduce "the repo" struct similar to "the index"
in cache.h?

From a submodule perspective I would very much welcome this
object oriented approach to repositories.

I think it may be more complicated, because there's some implicit global
state in "the repo", like where files are relative to our cwd. All of
those low-level functions would have to start caring about which repo
we're talking about so they can prefix the appropriate working tree
path, etc.

For some operations that would be fine, but there are things that would
subtly fail for submodules. I'm thinking we'd end up with some code
state like:

  /* finding a repo does not modify global state; good */
  struct repository *repo = repo_discover(".");

  /* obvious repo-level operations like looking up refs can be done with
   * a repository object; good */
  repo_for_each_ref(repo, callback, NULL);

  /*
   * "enter" the repo so that we are at the top-level of the working
   * tree, etc. After this you can actually look at the index without
   * things breaking.
   */
  repo_enter(repo);

That would be enough to implement a lot of submodule-level stuff, but it
would break pretty subtly as soon as you asked the submodule about its
working tree. The solution is to make everything that accesses the
working tree aware of the idea of a working tree root besides the cwd.
But that's a pretty invasive change.

-Peff

Some other challenges would be how to address people setting environment
variables like GIT_DIR that indicate the location of a repositories git
directory, which wouldn't work if you have multiple repos open.

I do agree that having a repo object of some sort would aid in
simplifying submodule operations but may require too many invasive
changes to basic low-level functions.

-- 
Brandon Williams