[Patch 0/3] includeIf series for worktrees

[Patch 0/3] includeIf series for worktrees

[randall.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

Add support for includeIf with a worktree: prefix. This permits
conditional includes that are specific to individual worktrees.
The set of patches differ slightly from gitdir: as worktrees to not
have .git/ directories, so matching repo/ is somewhat problematic -
suggestings on dealing with that case are more than welcome.

Also added worktree test condition to verify that onbranch: works
in a worktree scenario as different from the main repository.

Randall S. Becker (3):
  config.c: add conditional include based on worktree.
  Documentation/config.txt: add worktree includeIf conditionals.
  t1305: add tests for includeIf:worktree.

 Documentation/config.txt  | 11 +++++-
 config.c                  | 63 ++++++++++++++++++++++++++++++
 t/t1305-config-include.sh | 81 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+), 1 deletion(-)

-- 
2.32.0

[Patch 1/3] config.c: add conditional include based on worktree.

[randall.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

This enhancement extends the [includeIf] semantics to include conditional
inclusion based on whether the conditional is within a specific worktree
or case-insensitive worktree. The [includeIf "worktree:path"] and
[includeIf "worktree/i:path"] and analogous to the gitdir: and gitdir/i:
conditions, respectively.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 config.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/config.c b/config.c
index f9c400ad30..e2b2364579 100644
--- a/config.c
+++ b/config.c
@@ -272,6 +272,64 @@ static int include_by_gitdir(const struct config_options *opts,
 	return ret;
 }
 
+static int include_by_worktree(const struct config_options *opts,
+			       const char *cond, size_t cond_len, int icase)
+{
+	struct strbuf text = STRBUF_INIT;
+	struct strbuf pattern = STRBUF_INIT;
+	int ret = 0, prefix;
+	const char *worktree;
+	int already_tried_absolute = 0;
+
+	if (the_repository->worktree)
+		worktree = the_repository->worktree;
+	else
+		goto done;
+
+	strbuf_realpath(&text, worktree, 1);
+	strbuf_add(&pattern, cond, cond_len);
+	prefix = prepare_include_condition_pattern(&pattern);
+
+again:
+	if (prefix < 0)
+		goto done;
+
+	if (prefix > 0) {
+		/*
+		 * perform literal matching on the prefix part so that
+		 * any wildcard character in it can't create side effects.
+		 */
+		if (text.len < prefix)
+			goto done;
+		if (!icase && strncmp(pattern.buf, text.buf, prefix))
+			goto done;
+		if (icase && strncasecmp(pattern.buf, text.buf, prefix))
+			goto done;
+	}
+
+	ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
+			 WM_PATHNAME | (icase ? WM_CASEFOLD : 0));
+
+	if (!ret && !already_tried_absolute) {
+		/*
+		 * We've tried e.g. matching worktree:~/work, but if
+		 * ~/work is a symlink to /mnt/storage/work
+		 * strbuf_realpath() will expand it, so the rule won't
+		 * match. Let's match against a
+		 * strbuf_add_absolute_path() version of the path,
+		 * which'll do the right thing
+		 */
+		strbuf_reset(&text);
+		strbuf_add_absolute_path(&text, worktree);
+		already_tried_absolute = 1;
+		goto again;
+	}
+done:
+	strbuf_release(&pattern);
+	strbuf_release(&text);
+	return ret;
+}
+
 static int include_by_branch(const char *cond, size_t cond_len)
 {
 	int flags;
@@ -300,6 +358,11 @@ static int include_condition_is_true(const struct config_options *opts,
 		return include_by_gitdir(opts, cond, cond_len, 0);
 	else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
 		return include_by_gitdir(opts, cond, cond_len, 1);
+	else if (skip_prefix_mem(cond, cond_len, "worktree:", &cond, &cond_len))
+		return include_by_worktree(opts, cond, cond_len, 0);
+	else if (skip_prefix_mem(cond, cond_len, "worktree/i:", &cond,
+				 &cond_len))
+		return include_by_worktree(opts, cond, cond_len, 1);
 	else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len))
 		return include_by_branch(cond, cond_len);
 
-- 
2.32.0

[Patch 2/3] Documentation/config.txt: add worktree includeIf conditionals.

[randall.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

Documentation of the worktree and worktree/i conditionals is add based on
gitdir rules except that the trailing / form of the path is not supported.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 Documentation/config.txt | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bf82766a6a..7e951937ae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -143,7 +143,16 @@ refer to linkgit:gitignore[5] for details. For convenience:
 
 `gitdir/i`::
 	This is the same as `gitdir` except that matching is done
-	case-insensitively (e.g. on case-insensitive file systems)
+	case-insensitively (e.g. on case-insensitive file systems).
+
+`worktree`::
+	This is similar to `gitdir` except that matching is done with
+	the path of a worktree instead of the main repository. Unlike
+	`gitdir`, the trailing / form of the worktree path is not supported.
+
+`worktree/i`::
+	This is the same as `worktree` except that matching is done
+	case-insensitively (e.g. on case-insensitive file systems).
 
 `onbranch`::
 	The data that follows the keyword `onbranch:` is taken to be a
-- 
2.32.0

[Patch 3/3] t1305: add tests for includeIf:worktree.

[randall.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

The tests are a subset of those for gitdir:, taking into account that
the worktree: form does not support the trailing / at this time in
pattern matches. Some resets of the .git/config file are done to restrict
the set of includeIf paths being evaluated that conflict with prior
subtests.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 t/t1305-config-include.sh | 81 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index ccbb116c01..fe1ad106c3 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -309,6 +309,69 @@ test_expect_success SYMLINKS 'conditional include, gitdir matching symlink, icas
 	)
 '
 
+test_expect_success 'conditional worktree include, unanchored' '
+	(
+		cd foo &&
+		# Must add a commit for worktree add
+		git commit --allow-empty --allow-empty-message &&
+		sed -i "/includeIf/,\$d" .git/config &&
+		git worktree add ../foo.wt &&
+		echo "[includeIf \"worktree:foo.wt\"]path=bar" >>.git/config &&
+		echo "[test]one=1" >.git/bar &&
+		cd ../foo.wt &&
+		echo 1 >expect &&
+		git config test.one >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional worktree include, $HOME expansion' '
+	(
+		cd foo &&
+		echo "[includeIf \"worktree:~/foo.wt\"]path=bar2" >>.git/config &&
+		echo "[test]two=2" >.git/bar2 &&
+		cd ../foo.wt &&
+		echo 2 >expect &&
+		git config test.two >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional worktree include, full pattern' '
+	(
+		cd foo &&
+		echo "[includeIf \"worktree:**/foo.wt\"]path=bar3" >>.git/config &&
+		echo "[test]three=3" >.git/bar3 &&
+		cd ../foo.wt &&
+		echo 3 >expect &&
+		git config test.three >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional worktree include, relative path' '
+	echo "[includeIf \"worktree:./foo.wt\"]path=bar4" >>.gitconfig &&
+	echo "[test]four=4" >bar4 &&
+	(
+		cd foo.wt &&
+		echo 4 >expect &&
+		git config test.four >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional worktree include, both unanchored, icase' '
+	(
+		cd foo &&
+		echo "[includeIf \"worktree/i:FOO.WT\"]path=bar5" >>.git/config &&
+		echo "[test]five=5" >.git/bar5 &&
+		cd ../foo.wt &&
+		echo 5 >expect &&
+		git config test.five >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'conditional include, onbranch' '
 	echo "[includeIf \"onbranch:foo-branch\"]path=bar9" >>.git/config &&
 	echo "[test]nine=9" >.git/bar9 &&
@@ -348,6 +411,24 @@ test_expect_success 'conditional include, onbranch, implicit /** for /' '
 	test_cmp expect actual
 '
 
+test_expect_success 'conditional worktree include, onbranch' '
+	(
+		cd foo &&
+		sed -i "/includeIf/,\$d" .git/config &&
+		echo "[includeIf \"onbranch:foo.wt2\"]path=bar12" >>.git/config &&
+		echo "[test]twelve=12" >.git/bar12
+	) &&
+	(
+		cd foo.wt &&
+		git checkout -b main &&
+		test_must_fail git config test.twelve &&
+		git checkout -b foo.wt2 &&
+		echo 12 >expect &&
+		git config test.twelve >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'include cycles are detected' '
 	git init --bare cycle &&
 	git -C cycle config include.path cycle &&
-- 
2.32.0

Re: [Patch 1/3] config.c: add conditional include based on worktree.

[Johannes Schindelin]

Hi Randall,

On Mon, 12 Jul 2021, randall.becker@nexbridge.ca wrote:

> @@ -300,6 +358,11 @@ static int include_condition_is_true(const struct config_options *opts,
>  		return include_by_gitdir(opts, cond, cond_len, 0);
>  	else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
>  		return include_by_gitdir(opts, cond, cond_len, 1);
> +	else if (skip_prefix_mem(cond, cond_len, "worktree:", &cond, &cond_len))
> +		return include_by_worktree(opts, cond, cond_len, 0);
> +	else if (skip_prefix_mem(cond, cond_len, "worktree/i:", &cond,
> +				 &cond_len))
> +		return include_by_worktree(opts, cond, cond_len, 1);

Thank you for not forgetting the `/i` case.

Ciao,
Dscho

>  	else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len))
>  		return include_by_branch(cond, cond_len);
>
> --
> 2.32.0
>
>

Re: [Patch 2/3] Documentation/config.txt: add worktree includeIf conditionals.

[Junio C Hamano]

randall.becker@nexbridge.ca writes:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> Documentation of the worktree and worktree/i conditionals is add based on
> gitdir rules except that the trailing / form of the path is not supported.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  Documentation/config.txt | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index bf82766a6a..7e951937ae 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -143,7 +143,16 @@ refer to linkgit:gitignore[5] for details. For convenience:
>  
>  `gitdir/i`::
>  	This is the same as `gitdir` except that matching is done
> -	case-insensitively (e.g. on case-insensitive file systems)
> +	case-insensitively (e.g. on case-insensitive file systems).
> +
> +`worktree`::
> +	This is similar to `gitdir` except that matching is done with
> +	the path of a worktree instead of the main repository. Unlike
> +	`gitdir`, the trailing / form of the worktree path is not supported.

It is not immediately obvious what "the trailing / form" means.

Does it refer to the 4th item in the 4-bullet list in the
description just above the patch context (I am trying to make a
guess here)?

The problem I perceive in this description is that there is no
phrase "trailing" in the vicinity of what readers have read so far;
readers who are not exactly familiar with the system may need a bit
more assurance that they guessed correctly.

    Unlike `gitdir`, `**` will not be automatically added to a
    pattern that ends with `/`

would be easier to give that assurance, albeit more verbosely.

Assuming that I guessed correctly, is this a deliberate design
decision not to "automatically add ** after a pattern that ends with
a slash", and if so why?  I would have thought that "in the
worktrees that I create inside /var/tmp/, please enable these
configuration variables" would be a fairly natural thing to ask, and
I do not immediately see a reason why we want to apply different
syntax rules between "gitdir" and "worktree".

Thanks.

RE: [Patch 2/3] Documentation/config.txt: add worktree includeIf conditionals.

[Randall S. Becker]

On July 13, 2021 9:05 PM. Junio C Hamano wrote:
>randall.becker@nexbridge.ca writes:
>
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>
>> Documentation of the worktree and worktree/i conditionals is add based
>> on gitdir rules except that the trailing / form of the path is not supported.
>>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>>  Documentation/config.txt | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt index
>> bf82766a6a..7e951937ae 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -143,7 +143,16 @@ refer to linkgit:gitignore[5] for details. For convenience:
>>
>>  `gitdir/i`::
>>  	This is the same as `gitdir` except that matching is done
>> -	case-insensitively (e.g. on case-insensitive file systems)
>> +	case-insensitively (e.g. on case-insensitive file systems).
>> +
>> +`worktree`::
>> +	This is similar to `gitdir` except that matching is done with
>> +	the path of a worktree instead of the main repository. Unlike
>> +	`gitdir`, the trailing / form of the worktree path is not supported.
>
>It is not immediately obvious what "the trailing / form" means.
>
>Does it refer to the 4th item in the 4-bullet list in the description just above the patch context (I am trying to make a guess
here)?
>
>The problem I perceive in this description is that there is no phrase "trailing" in the vicinity of what readers have read so far;
readers who
>are not exactly familiar with the system may need a bit more assurance that they guessed correctly.
>
>    Unlike `gitdir`, `**` will not be automatically added to a
>    pattern that ends with `/`
>
>would be easier to give that assurance, albeit more verbosely.
>
>Assuming that I guessed correctly, is this a deliberate design decision not to "automatically add ** after a pattern that ends with
a slash",
>and if so why?  I would have thought that "in the worktrees that I create inside /var/tmp/, please enable these configuration
variables"
>would be a fairly natural thing to ask, and I do not immediately see a reason why we want to apply different syntax rules between
"gitdir"
>and "worktree".

The reason for this comes down to what is in *the_repository. Essentially, the_repository->gitdir always has a /path/to/.git
directory with full qualification. the_repository->worktree does not have /.git added for obvious reasons, so the /path/to is bare
of the trailing /. This causes a trailing pattern /to/path/** match to fail. I could copy the value into a working buffer but that
seemed a bit clunky. So using the available information, the syntax rules need to be different between the two, unless the value of
worktree is augmented. I was unsure which way the team wanted to go on this.

Re: [Patch 2/3] Documentation/config.txt: add worktree includeIf conditionals.

[Junio C Hamano]

"Randall S. Becker" <rsbecker@nexbridge.com> writes:

>>Assuming that I guessed correctly, is this a deliberate design
>>decision not to "automatically add ** after a pattern that ends
>>with a slash", and if so why?  I would have thought that "in the
>>worktrees that I create inside /var/tmp/, please enable these
>>configuration variables" would be a fairly natural thing to ask,
>>and I do not immediately see a reason why we want to apply
>>different syntax rules between "gitdir" and "worktree".

> The reason for this comes down to what is in
>*the_repository.

Sorry, but I still do not understand.

> Essentially, the_repository->gitdir always has a /path/to/.git
> directory with full qualification.

Yes.

> the_repository->worktree does not have /.git added
> for obvious reasons, so the /path/to is bare of the trailing
>/.

It may be the case, but /path/to/.git does not have trailing slash,
either, so I do not see the relevance.

When you say [includeIf "gitdir:/path/"], the "behave as if ** is
added after the slash at the end" rule kicks in, and the pattern
"/path/**" is used to see if it matches "/path/to/.git" and it does,
right?  When you say [includeIf "worktree:/path/"], wouldn't the
resulting "/path/**" match "/path/to"?

By the way, I think [PATCH 1/3] should turn the body of
include_by_gitdir() to a common helper function that

 - accepts a path to a directory and a pattern
 - turns it into a relpath
 - prepares the pattern with prepare_include_condition_pattern()
 - do the match include_by_gitdir() does.

and make include_by_gitdir() a very thin wrapper that passes
opts->git_dir to that common helper.  Then you do not have to copy
the entire function to create your new include_by_worktree(); it can
be another very thin wrapper that passes the_repository->worktree
instead of opts->git_dir to the common helper, as there is no other
difference in these two functions.

Thanks.

RE: [Patch 2/3] Documentation/config.txt: add worktree includeIf conditionals.

[Randall S. Becker]

On July 14, 2021 1:10 PM, Junio C Hamano
>"Randall S. Becker" <rsbecker@nexbridge.com> writes:
>
>>>Assuming that I guessed correctly, is this a deliberate design
>>>decision not to "automatically add ** after a pattern that ends with a
>>>slash", and if so why?  I would have thought that "in the worktrees
>>>that I create inside /var/tmp/, please enable these configuration
>>>variables" would be a fairly natural thing to ask, and I do not
>>>immediately see a reason why we want to apply different syntax rules
>>>between "gitdir" and "worktree".
>
>> The reason for this comes down to what is in *the_repository.
>
>Sorry, but I still do not understand.
>
>> Essentially, the_repository->gitdir always has a /path/to/.git
>> directory with full qualification.
>
>Yes.
>
>> the_repository->worktree does not have /.git added  for obvious
>>reasons, so the /path/to is bare of the trailing /.
>
>It may be the case, but /path/to/.git does not have trailing slash, either, so I do not see the relevance.
>
>When you say [includeIf "gitdir:/path/"], the "behave as if ** is added after the slash at the end" rule kicks in, and the pattern
"/path/**" is
>used to see if it matches "/path/to/.git" and it does, right?  When you say [includeIf "worktree:/path/"], wouldn't the resulting
"/path/**"
>match "/path/to"?

I think I over-complicated the first test case and got myself into a mess. Will fix that.

>By the way, I think [PATCH 1/3] should turn the body of
>include_by_gitdir() to a common helper function that
>
> - accepts a path to a directory and a pattern
> - turns it into a relpath
> - prepares the pattern with prepare_include_condition_pattern()
> - do the match include_by_gitdir() does.
>
>and make include_by_gitdir() a very thin wrapper that passes
>opts->git_dir to that common helper.  Then you do not have to copy
>the entire function to create your new include_by_worktree(); it can be another very thin wrapper that passes
the_repository->worktree
>instead of opts->git_dir to the common helper, as there is no other difference in these two functions.

That sounds like a plan. Will go for it in V2.

-Randall