threads / patch / 58707

patch, 2 partshelp.c: don't call git_default_config in git_unknown_cmd_config

Subject: [PATCH 2/2] help.c: don't call git_default_config in git_unknown_cmd_config

## tl;dr

9 messages between Oct 28, 2022 and Dec 13, 2022. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

Simon Gerber via GitGitGadget· Oct 28, 2022, 15:24 UTC · lore

[PATCH 0/2] Fix autocorrect in work tree for bare repository

Currently, auto correction doesn't work reliably for commands which must run in a work tree (e.g. git status) in Git work trees which are created from a bare repository.

This patch series adds a test case illustrating the issue and proposes a fix which adjusts the usage of read_early_config() in help_unknown_cmd() to match other usages of read_early_config(). In particular the patch removes the call to git_default_config() in the read config callback.

Simon Gerber (2):
  tests: add test case for autocorrect in work tree for bare clone
  help.c: don't call git_default_config in git_unknown_cmd_config
 help.c                               |  2 +-
 t/t9005-help-autocorrect-worktree.sh | 29 ++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100755 t/t9005-help-autocorrect-worktree.sh
base-commit: e7e5c6f715b2de7bea0d39c7d2ba887335b40aa0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1373%2Fsimu%2Ffix%2Fautocorrect-bare-worktree-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1373/simu/fix/autocorrect-bare-worktree-v1
Pull-Request: https://github.com/git/git/pull/1373
-- 
gitgitgadget
Simon Gerber via GitGitGadget· Oct 28, 2022, 15:24 UTC · re: Simon Gerber via GitGitGadget · lore
From: Simon Gerber <gesimu@gmail.com>

Currently, auto correction doesn't work reliably for commands which must run in a work tree (e.g. `git status`) in Git work trees which are created from a bare repository.

As far as I'm able to determine, this has been broken since commit 659fef199f (help: use early config when autocorrecting aliases, 2017-06-14), where the call to `git_config()` in `help_unknown_cmd()` was replaced with a call to `read_early_config()`. From what I can tell, the actual cause for the unexpected error is that we call `git_default_config()` in the `git_unknown_cmd_config` callback instead of simply returning `0` for config entries which we aren't interested in.

Calling `git_default_config()` in this callback to `read_early_config()` seems like a bad idea since those calls will initialize a bunch of state in `environment.c` (among other things `is_bare_repository_cfg`) before we've properly detected that we're running in a work tree.

All other callbacks provided to `read_early_config()` appear to only extract their configurations while simply returning `0` for all other config keys.

This commit changes the `git_unknown_cmd_config` callback to not call `git_default_config()`. Instead we also simply return `0` for config keys which we're not interested in.

Signed-off-by: Simon Gerber <gesimu@gmail.com>
---
 help.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to help.c +1 −1
diff --git a/help.c b/help.c
index d04542d8261..ae534ff0bae 100644
--- a/help.c
+++ b/help.c
@@ -563,7 +563,7 @@ static int git_unknown_cmd_config(const char *var, const char *value, void *cb)
 	if (skip_prefix(var, "alias.", &p))
 		add_cmdname(&aliases, p, strlen(p));
 
-	return git_default_config(var, value, cb);
+	return 0;
 }
 
 static int levenshtein_compare(const void *p1, const void *p2)

-- 
gitgitgadget
Simon Gerber via GitGitGadget· Oct 28, 2022, 15:24 UTC · re: Simon Gerber via GitGitGadget · lore

[PATCH 1/2] tests: add test case for autocorrect in work tree for bare clone

From: Simon Gerber <gesimu@gmail.com>

Currently, auto correction doesn't work reliably for commands which must run in a work tree (e.g. `git status`) in Git work trees which are created from a bare repository.

This commit adds a test which shows the broken autocompletion by calling `git stauts` which gets autocorrected to `git status`. This works fine in a work tree created with `git worktree add` from a regular clone, but doesn't work in a work tree created with `git worktree add` from a bare clone.

Signed-off-by: Simon Gerber <gesimu@gmail.com>
---
 t/t9005-help-autocorrect-worktree.sh | 29 ++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100755 t/t9005-help-autocorrect-worktree.sh
Show changes to t/t9005-help-autocorrect-worktree.sh +29 −0
diff --git a/t/t9005-help-autocorrect-worktree.sh b/t/t9005-help-autocorrect-worktree.sh
new file mode 100755
index 00000000000..4fecc8a8e01
--- /dev/null
+++ b/t/t9005-help-autocorrect-worktree.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='test autocorrect in work tree based on bare repository'
+. ./test-lib.sh
+
+test_expect_success 'setup non-bare' '
+	echo one >file &&
+	git add file &&
+	git commit -m one &&
+	echo two >file &&
+	git commit -a -m two
+'
+
+test_expect_success 'setup bare' '
+	git clone --bare . bare.git &&
+	cd bare.git
+'
+
+test_expect_success 'setup worktree from bare' '
+	git worktree add ../bare-wt &&
+	cd ../bare-wt
+'
+
+test_expect_success 'autocorrect works in work tree created from bare repo' '
+	git config help.autocorrect immediate &&
+	git stauts
+'
+
+test_done

-- 
gitgitgadget
Junio C Hamano· Oct 28, 2022, 19:28 UTC · re: Simon Gerber via GitGitGadget · lore

Re: [PATCH 1/2] tests: add test case for autocorrect in work tree for bare clone

"Simon Gerber via GitGitGadget" <gitgitgadget@gmail.com> writes:
>  t/t9005-help-autocorrect-worktree.sh | 29 ++++++++++++++++++++++++++++

Don't we have existing test script for auto-correction? Is it sensible to waste a new fail for this single test? I doubt it.

Perhaps appending after 9003 a new test instead?
Show 18 quoted lines
> diff --git a/t/t9005-help-autocorrect-worktree.sh b/t/t9005-help-autocorrect-worktree.sh
> new file mode 100755
> index 00000000000..4fecc8a8e01
> --- /dev/null
> +++ b/t/t9005-help-autocorrect-worktree.sh
> @@ -0,0 +1,29 @@
> +#!/bin/sh
> +
> +test_description='test autocorrect in work tree based on bare repository'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup non-bare' '
> +	echo one >file &&
> +	git add file &&
> +	git commit -m one &&
> +	echo two >file &&
> +	git commit -a -m two
> +'

Wouldn't "test_commit" be simpler to use than rolling our own here? If we piggy-back on set-up in an existing test script, there probably is already a history the single test to attempt running 'git staut' can use. After all, that test does not care an iota what is in the history.

> +
> +test_expect_success 'setup bare' '
> +	git clone --bare . bare.git &&
> +	cd bare.git
Do not chdir around in a test script, especially in separate steps.
Show 6 quoted lines
> +'
> +
> +test_expect_success 'setup worktree from bare' '
> +	git worktree add ../bare-wt &&
> +	cd ../bare-wt
> +'
Ditto.
Either do it in a subshell in a single step, or
	(
		git clone --bare . bare.git &&
		cd bare.git &&
		git worktree add ../worktree &&
		cd ../worktree &&
		git -c help.autocorrect=immediate staut
	)
use "git -C <over-there>" form, e.g.
	git clone --bare . bare.git &&
	git -C bare.git worktree add ../worktree &&
	git -C worktree -c help.autocorrect=immediate staut
> +test_expect_success 'autocorrect works in work tree created from bare repo' '

When patch 1/2 is applied without 2/2, this test_expect_success will not be satisfied, breaking future bisection.

For a small change like this, have the code change *and* test that verifies the new behaviour in a single step. That way

 * you do not break bisection.
 * if somebody wants to cherry-pick the fix to an older maintenance
   tracks, they can do so by picking a single unit, fix and
   verification combined together.
 * acceptance or review can be done by checking the end-result and
   then tentatively reverting only the code change with something
   like
	$ git show -- ':!t/' | git apply -R
   and see the test that expects success actually fails without the fix.
Simon Gerber· Oct 29, 2022, 08:07 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] tests: add test case for autocorrect in work tree for bare clone

On Fri, 28 Oct 2022 at 21:28, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
>
> "Simon Gerber via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >  t/t9005-help-autocorrect-worktree.sh | 29 ++++++++++++++++++++++++++++
>
> Don't we have existing test script for auto-correction?  Is it
> sensible to waste a new fail for this single test?  I doubt it.
>
> Perhaps appending after 9003 a new test instead?

I'll move the test to 9003. I was considering doing so in the beginning, but was hesitant to add an only vaguely related test in 9003 which has the description "help.autocorrect finding a match".

Show 58 quoted lines
>
> > diff --git a/t/t9005-help-autocorrect-worktree.sh b/t/t9005-help-autocorrect-worktree.sh
> > new file mode 100755
> > index 00000000000..4fecc8a8e01
> > --- /dev/null
> > +++ b/t/t9005-help-autocorrect-worktree.sh
> > @@ -0,0 +1,29 @@
> > +#!/bin/sh
> > +
> > +test_description='test autocorrect in work tree based on bare repository'
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'setup non-bare' '
> > +     echo one >file &&
> > +     git add file &&
> > +     git commit -m one &&
> > +     echo two >file &&
> > +     git commit -a -m two
> > +'
>
> Wouldn't "test_commit" be simpler to use than rolling our own here?
> If we piggy-back on set-up in an existing test script, there
> probably is already a history the single test to attempt running
> 'git staut' can use.  After all, that test does not care an iota
> what is in the history.
>
> > +
> > +test_expect_success 'setup bare' '
> > +     git clone --bare . bare.git &&
> > +     cd bare.git
>
> Do not chdir around in a test script, especially in separate steps.
>
> > +'
> > +
> > +test_expect_success 'setup worktree from bare' '
> > +     git worktree add ../bare-wt &&
> > +     cd ../bare-wt
> > +'
>
> Ditto.
>
> Either do it in a subshell in a single step, or
>
>         (
>                 git clone --bare . bare.git &&
>                 cd bare.git &&
>                 git worktree add ../worktree &&
>                 cd ../worktree &&
>                 git -c help.autocorrect=immediate staut
>         )
>
> use "git -C <over-there>" form, e.g.
>
>         git clone --bare . bare.git &&
>         git -C bare.git worktree add ../worktree &&
>         git -C worktree -c help.autocorrect=immediate staut
>

Thanks for taking the time to point out how I'm supposed to use the test framework, I was pretty lost trying to write my test and ended up copying some bits from 7103.

Show 21 quoted lines
> > +test_expect_success 'autocorrect works in work tree created from bare repo' '
>
> When patch 1/2 is applied without 2/2, this test_expect_success will
> not be satisfied, breaking future bisection.
>
> For a small change like this, have the code change *and* test that
> verifies the new behaviour in a single step.  That way
>
>  * you do not break bisection.
>
>  * if somebody wants to cherry-pick the fix to an older maintenance
>    tracks, they can do so by picking a single unit, fix and
>    verification combined together.
>
>  * acceptance or review can be done by checking the end-result and
>    then tentatively reverting only the code change with something
>    like
>
>         $ git show -- ':!t/' | git apply -R
>
>    and see the test that expects success actually fails without the fix.
That makes sense. I'll resubmit this as a single patch.
Simon Gerber via GitGitGadget· Oct 29, 2022, 19:56 UTC · re: Simon Gerber via GitGitGadget · lore

[PATCH v2] help.c: fix autocorrect in work tree for bare repository

From: Simon Gerber <gesimu@gmail.com>

Currently, auto correction doesn't work reliably for commands which must run in a work tree (e.g. `git status`) in Git work trees which are created from a bare repository.

As far as I'm able to determine, this has been broken since commit 659fef199f (help: use early config when autocorrecting aliases, 2017-06-14), where the call to `git_config()` in `help_unknown_cmd()` was replaced with a call to `read_early_config()`. From what I can tell, the actual cause for the unexpected error is that we call `git_default_config()` in the `git_unknown_cmd_config` callback instead of simply returning `0` for config entries which we aren't interested in.

Calling `git_default_config()` in this callback to `read_early_config()` seems like a bad idea since those calls will initialize a bunch of state in `environment.c` (among other things `is_bare_repository_cfg`) before we've properly detected that we're running in a work tree.

All other callbacks provided to `read_early_config()` appear to only extract their configurations while simply returning `0` for all other config keys.

This commit changes the `git_unknown_cmd_config` callback to not call `git_default_config()`. Instead we also simply return `0` for config keys which we're not interested in.

Additionally the commit adds a new test case covering `help.autocorrect` in a work tree created from a bare clone.

Signed-off-by: Simon Gerber <gesimu@gmail.com>
---
    Fix autocorrect in work tree for bare repository
    
    Currently, auto correction doesn't work reliably for commands which must
    run in a work tree (e.g. git status) in Git work trees which are created
    from a bare repository.
    
    This patch adds a test case illustrating the issue and proposes a fix
    which adjusts the usage of read_early_config() in help_unknown_cmd() to
    match other usages of read_early_config(). In particular the patch
    removes the call to git_default_config() in the read config callback.
    
    Changes since v1 (both suggested by Junio):
    
     * Moved test to 9003
     * Squashed change and test into a single commit
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1373%2Fsimu%2Ffix%2Fautocorrect-bare-worktree-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1373/simu/fix/autocorrect-bare-worktree-v2
Pull-Request: https://github.com/git/git/pull/1373
Range-diff vs v1:
 1:  d0a6eea93b5 < -:  ----------- tests: add test case for autocorrect in work tree for bare clone
 2:  7a2bef32439 ! 1:  4c598dbe778 help.c: don't call git_default_config in git_unknown_cmd_config
     @@ Metadata
      Author: Simon Gerber <gesimu@gmail.com>
      
       ## Commit message ##
     -    help.c: don't call git_default_config in git_unknown_cmd_config
     +    help.c: fix autocorrect in work tree for bare repository
      
          Currently, auto correction doesn't work reliably for commands which must
          run in a work tree (e.g. `git status`) in Git work trees which are
     @@ Commit message
          `git_default_config()`. Instead we also simply return `0` for config
          keys which we're not interested in.
      
     +    Additionally the commit adds a new test case covering `help.autocorrect`
     +    in a work tree created from a bare clone.
     +
          Signed-off-by: Simon Gerber <gesimu@gmail.com>
      
       ## help.c ##
     @@ help.c: static int git_unknown_cmd_config(const char *var, const char *value, vo
       }
       
       static int levenshtein_compare(const void *p1, const void *p2)
     +
     + ## t/t9003-help-autocorrect.sh ##
     +@@ t/t9003-help-autocorrect.sh: test_expect_success 'autocorrect can be declined altogether' '
     + 	test_line_count = 1 actual
     + '
     + 
     ++test_expect_success 'autocorrect works in work tree created from bare repo' '
     ++	git clone --bare . bare.git &&
     ++	git -C bare.git worktree add ../worktree &&
     ++	git -C worktree -c help.autocorrect=immediate stauts
     ++'
     ++
     + test_done
 help.c                      | 2 +-
 t/t9003-help-autocorrect.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)
Show changes to 2 files +7 −1

help.c, t/t9003-help-autocorrect.sh

diff --git a/help.c b/help.c
index d04542d8261..ae534ff0bae 100644
--- a/help.c
+++ b/help.c
@@ -563,7 +563,7 @@ static int git_unknown_cmd_config(const char *var, const char *value, void *cb)
 	if (skip_prefix(var, "alias.", &p))
 		add_cmdname(&aliases, p, strlen(p));
 
-	return git_default_config(var, value, cb);
+	return 0;
 }
 
 static int levenshtein_compare(const void *p1, const void *p2)
diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
index f00deaf3815..f5b6b4f746b 100755
--- a/t/t9003-help-autocorrect.sh
+++ b/t/t9003-help-autocorrect.sh
@@ -60,4 +60,10 @@ test_expect_success 'autocorrect can be declined altogether' '
 	test_line_count = 1 actual
 '
 
+test_expect_success 'autocorrect works in work tree created from bare repo' '
+	git clone --bare . bare.git &&
+	git -C bare.git worktree add ../worktree &&
+	git -C worktree -c help.autocorrect=immediate stauts
+'
+
 test_done

base-commit: e7e5c6f715b2de7bea0d39c7d2ba887335b40aa0

-- 
gitgitgadget
Simon Gerber· Dec 12, 2022, 16:38 UTC · re: Simon Gerber via GitGitGadget · lore

Re: [PATCH v2] help.c: fix autocorrect in work tree for bare repository

On Sat, 29 Oct 2022 at 21:56, Simon Gerber via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 20 quoted lines
>
> [commit message omitted]
> ---
>     Fix autocorrect in work tree for bare repository
>
>     Currently, auto correction doesn't work reliably for commands which must
>     run in a work tree (e.g. git status) in Git work trees which are created
>     from a bare repository.
>
>     This patch adds a test case illustrating the issue and proposes a fix
>     which adjusts the usage of read_early_config() in help_unknown_cmd() to
>     match other usages of read_early_config(). In particular the patch
>     removes the call to git_default_config() in the read config callback.
>
>     Changes since v1 (both suggested by Junio):
>
>      * Moved test to 9003
>      * Squashed change and test into a single commit
>
> [diff omitted]
Hi all,

Just wanted to check what the state on this patch is (side-note: I just noticed that I didn't cc Junio in the v2 submission, my apologies). As described in the quote (and the parent email), the patch fixes how auto correct initializes the config. Version 2 addresses the points raised on v1, in particular, I've moved the test to 9003, and have squashed the change into a single commit. Please let me know if further changes are necessary.

-- Simon
Junio C Hamano· Dec 13, 2022, 01:37 UTC · re: Simon Gerber via GitGitGadget · lore

Re: [PATCH v2] help.c: fix autocorrect in work tree for bare repository

"Simon Gerber via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 8 quoted lines
> Calling `git_default_config()` in this callback to `read_early_config()`
> seems like a bad idea since those calls will initialize a bunch of state
> in `environment.c` (among other things `is_bare_repository_cfg`) before
> we've properly detected that we're running in a work tree.
>
> All other callbacks provided to `read_early_config()` appear to only
> extract their configurations while simply returning `0` for all other
> config keys.
Very good observation and justification for this change.
Show 15 quoted lines
>  help.c                      | 2 +-
>  t/t9003-help-autocorrect.sh | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/help.c b/help.c
> index d04542d8261..ae534ff0bae 100644
> --- a/help.c
> +++ b/help.c
> @@ -563,7 +563,7 @@ static int git_unknown_cmd_config(const char *var, const char *value, void *cb)
>  	if (skip_prefix(var, "alias.", &p))
>  		add_cmdname(&aliases, p, strlen(p));
>  
> -	return git_default_config(var, value, cb);
> +	return 0;
>  }
And this is exactly what the proposed log message promises to do.
Show 13 quoted lines
>  static int levenshtein_compare(const void *p1, const void *p2)
> diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
> index f00deaf3815..f5b6b4f746b 100755
> --- a/t/t9003-help-autocorrect.sh
> +++ b/t/t9003-help-autocorrect.sh
> @@ -60,4 +60,10 @@ test_expect_success 'autocorrect can be declined altogether' '
>  	test_line_count = 1 actual
>  '
>  
> +test_expect_success 'autocorrect works in work tree created from bare repo' '
> +	git clone --bare . bare.git &&
> +	git -C bare.git worktree add ../worktree &&
> +	git -C worktree -c help.autocorrect=immediate stauts
The reason why this third line is a sufficient test is...?

If "status" is invoked successfully, it would not exit with non-zero status as long as it correctly notices that it was invoked in a worktree (as opposed to the current code without your fix, which would say "nah, where you are running there is no worktree", that is incorrect), but one scenario I am a bit worried about is what if the tester has an entry on $PATH that has "git-static" or whatever that is similar enough to "status", to cause autocorrect work differently from the case where "git status" would be the only plausible case.

But then we can tell such a tester "don't do that, then" ;-)
Let's queue the patch as-is and see what others think.
Thanks.
> +'
> +
>  test_done
Simon Gerber· Dec 13, 2022, 09:48 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] help.c: fix autocorrect in work tree for bare repository

On Tue, 13 Dec 2022 at 02:37, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
>
> "Simon Gerber via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > --- a/t/t9003-help-autocorrect.sh
> > +++ b/t/t9003-help-autocorrect.sh
> > @@ -60,4 +60,10 @@ test_expect_success 'autocorrect can be declined altogether' '
> >       test_line_count = 1 actual
> >  '
> >
> > +test_expect_success 'autocorrect works in work tree created from bare repo' '
> > +     git clone --bare . bare.git &&
> > +     git -C bare.git worktree add ../worktree &&
> > +     git -C worktree -c help.autocorrect=immediate stauts
>
> The reason why this third line is a sufficient test is...?
>
> If "status" is invoked successfully, it would not exit with non-zero
> status as long as it correctly notices that it was invoked in a
> worktree (as opposed to the current code without your fix, which
> would say "nah, where you are running there is no worktree", that is
> incorrect), but one scenario I am a bit worried about is what if the
> tester has an entry on $PATH that has "git-static" or whatever that
> is similar enough to "status", to cause autocorrect work differently
> from the case where "git status" would be the only plausible case.

Thanks for pointing out that autocorrect could work differently for a tester depending on what's in their path. I didn't consider that case at all.

>
> But then we can tell such a tester "don't do that, then" ;-)
I think that's fine in this case.
>
> Let's queue the patch as-is and see what others think.
Awesome, thanks
Show 7 quoted lines
>
> Thanks.
>
> > +'
> > +
> >  test_done
>

← back to recent threads