Possible bug in includeIf / conditional includes on non git initialised directories

Possible bug in includeIf / conditional includes on non git initialised directories

[Raphael Stolt]

Hi there,

I might have stumbled this time over a real bug in includeIf / conditional includes or maybe it's just as intended.
1) Given I have a correct configured includeIf and I’m issuing `git config --show-origin --get user.email` against an directory which hasn’t been `git init`ed I get the user.email configured globally.
2) Given I have a correct configured includeIf and I’m issuing `git config --show-origin --get user.email` against an directory which has been `git init`ed I get the user.email configured conditionally.
For 1) I would probably expect to get the user.email configured conditionally even for a plain directory.

More details see this (http://stackoverflow.com/questions/43919191/git-2-13-conditional-config-on-windows/) Stack Overflow question.
Best regards,
Raphael Stolt

Re: Possible bug in includeIf / conditional includes on non git initialised directories

[Sebastian Schuberth]

On 2017-05-11 20:53, Raphael Stolt wrote:

> I might have stumbled this time over a real bug in includeIf / conditional includes or maybe it's just as intended.
> 1) Given I have a correct configured includeIf and I’m issuing `git config --show-origin --get user.email` against an directory which hasn’t been `git init`ed I get the user.email configured globally.

I don't think that's a bug surprise: The condition in the conditional include is "gitdir:". Before running "git init", it simply *is* no gitdir.

-- 
Sebastian Schuberth

Re: Possible bug in includeIf / conditional includes on non git initialised directories

[Jeff King]

On Thu, May 11, 2017 at 10:31:14PM +0200, Sebastian Schuberth wrote:

> On 2017-05-11 20:53, Raphael Stolt wrote:
> 
> > I might have stumbled this time over a real bug in includeIf / conditional includes or maybe it's just as intended.
> > 1) Given I have a correct configured includeIf and I’m issuing `git config --show-origin --get user.email` against an directory which hasn’t been `git init`ed I get the user.email configured globally.
> 
> I don't think that's a bug surprise: The condition in the conditional
> include is "gitdir:". Before running "git init", it simply *is* no
> gitdir.

Yeah, I think all is working as intended. A "cwd:" conditional seems
like it would be useful, but I think it would have a lot of corner
cases. It may change over the course of a program, and you have
weirdness with things like "git --git-dir=/some/other/path", where your
cwd and the git repository you're looking at are totally unrelated.

-Peff

Re: Possible bug in includeIf / conditional includes on non git initialised directories

[Ævar Arnfjörð Bjarmason]

On Thu, May 11, 2017 at 8:53 PM, Raphael Stolt <raphael.stolt@gmail.com> wrote:
> Hi there,
>
> I might have stumbled this time over a real bug in includeIf / conditional includes or maybe it's just as intended.
> 1) Given I have a correct configured includeIf and I’m issuing `git config --show-origin --get user.email` against an directory which hasn’t been `git init`ed I get the user.email configured globally.
> 2) Given I have a correct configured includeIf and I’m issuing `git config --show-origin --get user.email` against an directory which has been `git init`ed I get the user.email configured conditionally.
> For 1) I would probably expect to get the user.email configured conditionally even for a plain directory.
>
> More details see this (http://stackoverflow.com/questions/43919191/git-2-13-conditional-config-on-windows/) Stack Overflow question.
> Best regards,
> Raphael Stolt

The "this is how this works" has been covered already by others, but
can you elaborate on the use-case, why does it matter for you that
"git config" doesn't show your user.email when you have no git dir
there, I can't think of a reason for why this would matter since
there's no .git there, so there's no way anything can use the config
info at that location.

[Bug report] includeIf config is not displayed in normal directories

[Stuart MacDonald]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

Hello,

I've seen this thread:
https://public-inbox.org/git/F55DC360-9C1E-45B9-B8BA-39E1001BD620@gmail.com/t/#u

and respectfully disagree with the conclusion. Conditionally included
configuration can contain items like core.sshCommand that are required
for git clone while in a normal non-git directory. These should be
displayed properly so users know what configuration they are operating
with.

Also, conditionally included config is acted upon despite not being
displayed. This makes tracking down problems much more difficult.

Further, most complaints online are about user.name and user.email not
being displayed correctly. If those items are in ~/.gitconfig, then
they are displayed in a normal non-git directory by normal git config
commands. This makes conditionally included configuration display
inconsistent with regular configuration display. Inconsistency is bad
and should be fixed.

See 'git bugreport' attached for further information, reproduction steps, etc.

Thanks,
...Stu

[-- Attachment #2: git-bugreport-2020-12-15-0946.txt --]
[-- Type: text/plain, Size: 4404 bytes --]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

Set up a conditional include:
vvvvvvvvvv [ ~/.gitconfig ] vvvvvvvvvv
[includeIf "gitdir:~/work/"]
	path = ~/.gitconfig-work
^^^^^^^^^^ [ ~/.gitconfig ] ^^^^^^^^^^

vvvvvvvvvv [ ~/.gitconfig-work ] vvvvvvvvvv
[user]
	name = Stuart MacDonald
	email = stuartm.coding@gmail.com
[core]
	sshCommand = "ssh -i ~/.ssh/id_rsa.stuartm_github"
^^^^^^^^^^ [ ~/.gitconfig-work ] ^^^^^^^^^^

Make sure ssh is configured properly for github access:
vvvvvvvvvv [ ~/.ssh/config ] vvvvvvvvvv
Host github github.com
  Hostname github.com
  User git
  IdentitiesOnly yes
^^^^^^^^^^ [ ~/.ssh/config ] ^^^^^^^^^^

Run the following commands in a Git bash shell:
$ mkdir ~/work/git-bug-test
$ cd ~/work/git-bug-test
$ git config --show-origin --get-all core.sshCommand 
OBSERVE A: There is no output
$ git clone --recursive git@github.com:git/git.git
OBSERVE B: The clone succeeds
$ cd git
$ git config --show-origin --get-all core.sshCommand 
file:C:/Users/stuartm/.gitconfig-work        ssh -i ~/.ssh/id_rsa.stuartm_github
OBSERVE C: The configuration item is displayed
$ cd ..
$ git config --show-origin --get-all core.sshCommand 
OBSERVE D: There is no output

NOTE: In the above reproduction steps I've substituted my work email and
employer's private respository (which needs the approrpriate ssh key to be able
to clone) for my public email and a public repository. This means my
reproduction steps aren't an exact reproduction; for an exact reproduction
please substitute an email address and private repository that requires a
configured ssh key to clone.

What did you expect to happen? (Expected behavior)

I expected OBSERVE A and OBSERVE D to match OBSERVE C's output.

What happened instead? (Actual behavior)

There was no output.

What's different between what you expected and what actually happened?

Output vs no output.

Anything else you want to add:

I spent a lot of time yesterday tracking down a problem with my git
configuration that was preventing me from checking out private repositories. The
incorrect "no output" bug caused me to think I had some issue with my
conditional configuration setup and I was trapped in that dead end for too long.

It seems that includeIf is not widely discussed online. This makes it difficult
to find help. I did find
https://stackoverflow.com/questions/43919191/git-2-13-conditional-config-on-windows
which says in part "If its [sic] not git initialized then the includeIf gitdir
functionality will not work." which is actually inaccurate. It does function;
commenting out/in the sshCommand configuration line causes cloning to not work or
work from a normal directory.

Additionally, moving the sshCommand config item into ~/.gitconfig will cause
OBSERVE A and OBSERVE D to produce output. This clearly implicates includeIf
processing as the problem.

I've seen this thread
https://public-inbox.org/git/F55DC360-9C1E-45B9-B8BA-39E1001BD620@gmail.com/t/#u
and I understand that some config items don't make sense in a normal directory.
However, some do, critically core.sshCommand. Also, if user.name is in the top
level (non-conditional) .gitconfig, then it is reported by git config in a
normal directory whether that makes sense for a normal directory or not.

The minimum fix I'd like to see: conditional includes should be processed and
any configuration items for which use in a normal directory makes sense should
be present and reported properly. Again, critically, this includes
core.sshCommand.

The better fix is to have conditional include processing be at parity with
direction configuration; where a configuration item would be displayed if it was
in a top-level/direct .gitconfig file, then it should also be displayed if it's
conditionally-included. This would make the config system consistent, which is
important for ease of use.
I realise this goes against the sentiment in the git mailing list thread I
linked above. I believe that thread is wrong in its conclusion of "working as
designed".


Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.29.2.windows.2
cpu: x86_64
built from commit: 3464b98ce6803c98bf8fb34390cd150d66e4a0d3
sizeof-long: 4
sizeof-size_t: 8

Re: [Bug report] includeIf config is not displayed in normal directories

[Junio C Hamano]

Stuart MacDonald <stuartm.coding@gmail.com> writes:

> Hello,
>
> I've seen this thread:
> https://public-inbox.org/git/F55DC360-9C1E-45B9-B8BA-39E1001BD620@gmail.com/t/#u
>
> and respectfully disagree with the conclusion. Conditionally included
> configuration can contain items like core.sshCommand that are required
> for git clone while in a normal non-git directory. These should be
> displayed properly so users know what configuration they are operating
> with.

I have a feeling that this just repeats what was said in the quoted
thread.  It is a chicken-and-egg problem that cannot be solved by
use of [includeif "gitdir:*"] pattern, as it won't kick in until
your $(pwd) is associated with a git repository, and before "clone"
or "init", your $(pwd) that is a newly created directory won't be.

> Also, conditionally included config is acted upon despite not being
> displayed. This makes tracking down problems much more difficult.

> $ mkdir ~/work/git-bug-test
> $ cd ~/work/git-bug-test
> $ git config --show-origin --get-all core.sshCommand 
> OBSERVE A: There is no output

This is expected, as [includeif "gitdir:*"] is used and there is no
gitdir associated with the current location (yet).

It _might_ be possible to give "git config" a new option
"--pretend-as-if-cwd-is-the-root-of-a-git-working-tree" and teach it
to read ~/.gitconfig while pretending $(pwd)/.git exists and is a
gitdir, but it won't be as simple as exporting GIT_DIR=$(pwd)/.git,
I suspect.  A lot of other things would break by actually not having
a real repository there.

> $ git clone --recursive git@github.com:git/git.git
> OBSERVE B: The clone succeeds

This may be an unexpected but is an understandable behaviour.

First the command has to create an empty repository with an initial
configuration file, and at that point [includeif "gitdir:*"] match
would notice that there is a gitdir at $(pwd)/.git; by the time the
command actually starts talking to the other side, the repository
specific configuration can be picked up.

> $ cd git
> $ git config --show-origin --get-all core.sshCommand 
> file:C:/Users/stuartm/.gitconfig-work        ssh -i ~/.ssh/id_rsa.stuartm_github
> OBSERVE C: The configuration item is displayed

Of course.  At that point, you have a full fledged repository there.

> $ cd ..
> $ git config --show-origin --get-all core.sshCommand 
> OBSERVE D: There is no output

And you are outside any repository, so there shouldn't be any gitdir
in effect to influence [includeif "gitdir:*"] matching.

Another option, as Peff suggested in the old thread, would be to
introduce a separate [includeif "cwd:*"] match, but I do not know
how well it would fly.  with that, you may be able to

	[includeif "cwd:/home/stuart/work/*"]
		path = ...

I think that may be cleaner than the "pretend we have already a
git-dir here" hack I mentioned earlier, but I didn't think things
through.

THanks.

Re: [Bug report] includeIf config is not displayed in normal directories

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 15 2020, Stuart MacDonald wrote:

> Hello,
>
> I've seen this thread:
> https://public-inbox.org/git/F55DC360-9C1E-45B9-B8BA-39E1001BD620@gmail.com/t/#u

[I took the liberty of CC-ing the original thread participants & adding
the Message-Id's to the "References" header, hopefully the archive will
thread it and this together for future spelunking]

> and respectfully disagree with the conclusion. Conditionally included
> configuration can contain items like core.sshCommand that are required
> for git clone while in a normal non-git directory. These should be
> displayed properly so users know what configuration they are operating
> with.
>
> Also, conditionally included config is acted upon despite not being
> displayed. This makes tracking down problems much more difficult.
>
> Further, most complaints online are about user.name and user.email not
> being displayed correctly. If those items are in ~/.gitconfig, then
> they are displayed in a normal non-git directory by normal git config
> commands. This makes conditionally included configuration display
> inconsistent with regular configuration display. Inconsistency is bad
> and should be fixed.
>
> See 'git bugreport' attached for further information, reproduction steps, etc.

As the person with the last reply in that old thread & only a vague
recollection of it until I re-read it now: please don't take a "why
would you want this" question as dismissive of the use-case, but in
invite to tease out some of the nuances.

It's often easy to vaguely conceptualize a feature, but the hard work is
dealing with all the edge cases & emergent properties of something like
new IncludeIf semantics.

There's two interesting questions here: Is the current feature buggy (or
just has surprised but ultimately consistent & correct semantics), and
could we have some new IncludeIf use-case that covers use-case Y, where
now we can only do X?

Junio covered that in more detail in his reply.

Neither you nor anyone reading this from the archive later should read
that thread (or this one) as some refusal of a feature that does Y.

Whether we can have that ultimately depends on whether someone's going
to invest the time in coming up with patches for it, and in writing
those patches will either figure out all the expected & unexpected edge
cases involved & get past them, or not.

But it's not a "no" until that point (and not even then, maybe we can
keep the general idea of Y, but have Z which is mostly the same & works
for most people), it's just "nobody's really worked on it yet".

Re: [Bug report] includeIf config is not displayed in normal directories

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> But it's not a "no" until that point (and not even then, maybe we can
> keep the general idea of Y, but have Z which is mostly the same & works
> for most people), it's just "nobody's really worked on it yet".

Thanks for spelling these out.

Here is my snack-time hack to add the --pretend-gitdir option to "git
config".  It _might_ not be a bad idea to trigger this behaviour
(using $(pwd)/.git as the pretended directory path) automatically
when do not have a repository, but that would certainly be a
compatibility breaking change and would break existing workflow.

Only very lightly tested, and certainly not ready for inclusion (it
does not even have a doc update).


 builtin/config.c          |  4 ++++
 config.c                  |  2 ++
 config.h                  |  1 +
 t/t1305-config-include.sh | 21 +++++++++++++++++++++
 4 files changed, 28 insertions(+)

diff --git c/builtin/config.c w/builtin/config.c
index f71fa39b38..2603dc448c 100644
--- c/builtin/config.c
+++ w/builtin/config.c
@@ -30,6 +30,7 @@ static int use_worktree_config;
 static struct git_config_source given_config_source;
 static int actions, type;
 static char *default_value;
+static char *pretend_gitdir;
 static int end_nul;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -165,6 +166,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
 	OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")),
 	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
+	OPT_FILENAME(0, "pretend-gitdir", &pretend_gitdir, N_("when outside a repository, pretend this is the gitdir")),
 	OPT_END(),
 };
 
@@ -732,6 +734,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		config_options.commondir = get_git_common_dir();
 		config_options.git_dir = get_git_dir();
 	}
+	if (pretend_gitdir)
+		config_options.pretend_gitdir = pretend_gitdir;
 
 	if (end_nul) {
 		term = '\0';
diff --git c/config.c w/config.c
index 1137bd73af..d8d406ecda 100644
--- c/config.c
+++ w/config.c
@@ -224,6 +224,8 @@ static int include_by_gitdir(const struct config_options *opts,
 
 	if (opts->git_dir)
 		git_dir = opts->git_dir;
+	else if (opts->pretend_gitdir)
+		git_dir = opts->pretend_gitdir;
 	else
 		goto done;
 
diff --git c/config.h w/config.h
index c1449bb790..14e8d8c576 100644
--- c/config.h
+++ w/config.h
@@ -89,6 +89,7 @@ struct config_options {
 	unsigned int system_gently : 1;
 	const char *commondir;
 	const char *git_dir;
+	const char *pretend_gitdir;
 	config_parser_event_fn_t event_fn;
 	void *event_fn_data;
 	enum config_error_action {
diff --git c/t/t1305-config-include.sh w/t/t1305-config-include.sh
index f1e1b289f9..883762ad36 100755
--- c/t/t1305-config-include.sh
+++ w/t/t1305-config-include.sh
@@ -162,6 +162,27 @@ test_expect_success 'relative includes from stdin line fail' '
 	test_must_fail git config --file - test.one
 '
 
+test_expect_success 'conditional include, pretend gitdir' '
+	test_when_finished "git config --global --unset-all \"includeif.gitdir:*.path\"" &&
+	git config --global "includeif.gitdir:*.path" included &&
+
+	git config --file included "custom.variable" value &&
+	echo value >expect &&
+
+	# in the TRASH repository
+	git config --get custom.variable >actual &&
+	test_cmp expect actual &&
+
+	# nongit without pretend should not find stuff from included
+	nongit test_must_fail git config --get custom.variable >actual &&
+	test_must_be_empty actual &&
+
+	# nongit with pretend should find stuff from included
+	nongit git config --pretend-gitdir "$(pwd)/.git" \
+		--get custom.variable >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'conditional include, both unanchored' '
 	git init foo &&
 	(

Re: [Bug report] includeIf config is not displayed in normal directories

[Jeff King]

On Tue, Dec 15, 2020 at 02:23:17PM -0800, Junio C Hamano wrote:

> > $ git clone --recursive git@github.com:git/git.git
> > OBSERVE B: The clone succeeds
> 
> This may be an unexpected but is an understandable behaviour.
> 
> First the command has to create an empty repository with an initial
> configuration file, and at that point [includeif "gitdir:*"] match
> would notice that there is a gitdir at $(pwd)/.git; by the time the
> command actually starts talking to the other side, the repository
> specific configuration can be picked up.

I think this is not just unexpected, but something we've explicitly
tried to make work. After creating the repository, we re-read the config
in order to pick up any "clone -c" options. I'm not sure if anybody
thought about how includeIf would interact here, but I think it is quite
logical to say "now that we are in a repository, these config options
that are conditional on being in a particular repository will take
effect". I.e., expecting it not to work is making assumptions about
the timing of when Git locks in the value of config options.

> And you are outside any repository, so there shouldn't be any gitdir
> in effect to influence [includeif "gitdir:*"] matching.
> 
> Another option, as Peff suggested in the old thread, would be to
> introduce a separate [includeif "cwd:*"] match, but I do not know
> how well it would fly.  with that, you may be able to
> 
> 	[includeif "cwd:/home/stuart/work/*"]
> 		path = ...
> 
> I think that may be cleaner than the "pretend we have already a
> git-dir here" hack I mentioned earlier, but I didn't think things
> through.

Perhaps. I have no objection to adding an option like that. But I don't
think it solves the fundamental issue in this bug report, which is
asking "what will the config look like in a repository that I created at
this path via git-init or git-clone?".

Maybe Stuart really would prefer to change his config to "whenever I am
in this directory root, repo or no, use this config". It does have weird
corner cases around "git --git-dir=/path/to/repo", but doing that from
another directory is perhaps rare enough that people using a cwd:
conditional would be happy to ignore that.

But if we do want to solve it, then your --pretend-git-dir is doing that
directly. I don't think it's terribly useful in general, but it could be
a debugging aid.

-Peff

Re: [Bug report] includeIf config is not displayed in normal directories

[Stuart MacDonald]

On Wed, Dec 16, 2020 at 2:24 PM Jeff King <peff@peff.net> wrote:
> Perhaps. I have no objection to adding an option like that. But I don't
> think it solves the fundamental issue in this bug report, which is
> asking "what will the config look like in a repository that I created at
> this path via git-init or git-clone?".

My fundamental issue was "git is using a conditionally included config
option core.sshCommand while outside of a git repository, but git config
is not displaying that config option when I ask it to". The good news is
given the discussion in this thread I can now say that I was labouring
under a misconception about how git clone works, and what I thought
was a bug is normal expected behaviour.

My old understanding of git clone:
- create sub-directory
- create subdir/.git and whatever initial states are needed
- clone the repo from origin into the directory
- all never having left $CWD

It's actually:
- create sub-directory
- cd subdir
- create ./.git and initial states
- clone the repo _while inside_ a gitdir
- cd ..

I would like to suggest that the documentation for either or both
git clone and includeIf could be improved to talk about this case so
that others having trouble with a conditionally included sshComand
during cloning will not have to relearn the wheel.

I believe my misunderstanding was partially misled by the fact that
despite the documentation for includeIf talking about it being a gitdir
the part of that path being matched is not itself within a git directory;
~/work/git/git/.git was the end result, ~/work being the matched on
part of the path and not in a git directory.

> Maybe Stuart really would prefer to change his config to "whenever I am
> in this directory root, repo or no, use this config". It does have weird
> corner cases around "git --git-dir=/path/to/repo", but doing that from
> another directory is perhaps rare enough that people using a cwd:
> conditional would be happy to ignore that.

I do not. Hopefully this thread will also assist future includeIf
trouble-havers.

Thanks all, very much appreciated. I consider the matter settled
as 'working as designed'.

Re: [Bug report] includeIf config is not displayed in normal directories

[Junio C Hamano]

Stuart MacDonald <stuartm.coding@gmail.com> writes:

> My old understanding of git clone:
> - create sub-directory
> - create subdir/.git and whatever initial states are needed
> - clone the repo from origin into the directory
> - all never having left $CWD
>
> It's actually:
> - create sub-directory
> - cd subdir
> - create ./.git and initial states
> - clone the repo _while inside_ a gitdir
> - cd ..

I am not sure how the above two should make any difference with the
[includeif "gitdir:<pattern>"] matching.  Regardless of where your
$CWD is, the inclusion is decided on the location of the .git/
directory we are dealing with, so as long as "subdir/.git" matches
the pattern given to "gitdir:<pattern>" the inclusion should work
the same way even if the internal implementation of "git clone"
changes between the above two.

IIRC "includeif" is a Peff's brainchild, so I'd ask him to correct
any mistakes in the above paragraph, but I am a bit puzzled as to
what the true misunderstanding is, where the misunderstanding came
from, and which documentation if any we should look into improving.

Thanks.

Re: [Bug report] includeIf config is not displayed in normal directories

[Jeff King]

On Wed, Dec 16, 2020 at 03:24:28PM -0800, Junio C Hamano wrote:

> Stuart MacDonald <stuartm.coding@gmail.com> writes:
> 
> > My old understanding of git clone:
> > - create sub-directory
> > - create subdir/.git and whatever initial states are needed
> > - clone the repo from origin into the directory
> > - all never having left $CWD
> >
> > It's actually:
> > - create sub-directory
> > - cd subdir
> > - create ./.git and initial states
> > - clone the repo _while inside_ a gitdir
> > - cd ..
> 
> I am not sure how the above two should make any difference with the
> [includeif "gitdir:<pattern>"] matching.  Regardless of where your
> $CWD is, the inclusion is decided on the location of the .git/
> directory we are dealing with, so as long as "subdir/.git" matches
> the pattern given to "gitdir:<pattern>" the inclusion should work
> the same way even if the internal implementation of "git clone"
> changes between the above two.

I think the question is just "are we in a repository as part of clone or
not". Stuart's original world-view was that we were not, as we were
creating a clone but not "entering" it. But that is not how it is
implemented (nor the conceptual model I think we would want to give the
user).

> IIRC "includeif" is a Peff's brainchild, so I'd ask him to correct
> any mistakes in the above paragraph,

The notion of conditional includes was my thing, but gitdir: was Duy's.
That said...

> but I am a bit puzzled as to
> what the true misunderstanding is, where the misunderstanding came
> from, and which documentation if any we should look into improving.

I think it might just be a case of mistaken assumptions. But I am
certainly open to hearing from Stuart if there are places in the
documentation that misled him (or could have led him better).

-Peff