{"thread":{"id":"60989","subject":"[PATCH] completion: fix __git_complete_worktree_paths","startedAt":"2024-02-25T08:16:21Z","lastAt":"2024-02-27T21:06:53Z","messageCount":5,"participants":["Rubén Justo","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"489301","messageId":"a1561e02-40dc-4ac3-ae7a-666db0f810ed@gmail.com","threadId":"60989","inReplyTo":null,"subject":"[PATCH] completion: fix __git_complete_worktree_paths","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-25T08:16:15Z","receivedAt":"2024-02-25T08:16:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Use __git to invoke \"worktree list\" in __git_complete_worktree_paths, to\nrespect any \"-C\" and \"--git-dir\" options present on the command line.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nI stumbled upon this in a situation like:\n\n   $ git init /tmp/foo && cd /tmp/foo\n   $ git worktree add --orphan foo_wt\n\n   $ git init /tmp/bar && cd /tmp/bar\n   $ git worktree add --orphan bar_wt\n\n   $ cd /tmp/foo\n   $ git -C /tmp/bar worktree remove <TAB>\n   ... expecting /tmp/bar/wt, but ...\n   $ git -C /tmp/bar worktree remove /tmp/foo_wt\n\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 444b3efa63..86e55dc67f 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3571,7 +3571,7 @@ __git_complete_worktree_paths ()\n \t# Generate completion reply from worktree list skipping the first\n \t# entry: it's the path of the main worktree, which can't be moved,\n \t# removed, locked, etc.\n-\t__gitcomp_nl \"$(git worktree list --porcelain |\n+\t__gitcomp_nl \"$(__git worktree list --porcelain |\n \t\tsed -n -e '2,$ s/^worktree //p')\"\n }\n \n-- \n2.44.0\n"},{"id":"489338","messageId":"eaf3649e-30cf-4eba-befa-5be826c828a8@gmail.com","threadId":"60989","inReplyTo":"a1561e02-40dc-4ac3-ae7a-666db0f810ed@gmail.com","subject":"[PATCH v2] completion: fix __git_complete_worktree_paths","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-25T23:53:31Z","receivedAt":"2024-02-25T23:53:40Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Use __git to invoke \"worktree list\" in __git_complete_worktree_paths, to\nrespect any \"-C\" and \"--git-dir\" options present on the command line.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nI stumbled upon this in a situation like:\n\n   $ git init /tmp/foo && cd /tmp/foo\n   $ git worktree add --orphan foo_wt\n\n   $ git init /tmp/bar && cd /tmp/bar\n   $ git worktree add --orphan bar_wt\n\n   $ cd /tmp/foo\n   $ git -C /tmp/bar worktree remove <TAB>\n   ... expecting /tmp/bar/wt, but ...\n   $ git -C /tmp/bar worktree remove /tmp/foo_wt\n\nIn this iteration, v2, some tests are included.\n\nThe function __git was introduced in 1cd23e9e05 (completion: don't use\n__gitdir() for git commands, 2017-02-03).  It is a small function, so\nI'll include here to ease the review of this patch:\n\n\t# Runs git with all the options given as argument, respecting any\n\t# '--git-dir=<path>' and '-C <path>' options present on the command line\n\t__git ()\n\t{\n\t\tgit ${__git_C_args:+\"${__git_C_args[@]}\"} \\\n\t\t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n\t}\n\n\n contrib/completion/git-completion.bash |  2 +-\n t/t9902-completion.sh                  | 24 ++++++++++++++++++++++++\n 2 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 444b3efa63..86e55dc67f 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3571,7 +3571,7 @@ __git_complete_worktree_paths ()\n \t# Generate completion reply from worktree list skipping the first\n \t# entry: it's the path of the main worktree, which can't be moved,\n \t# removed, locked, etc.\n-\t__gitcomp_nl \"$(git worktree list --porcelain |\n+\t__gitcomp_nl \"$(__git worktree list --porcelain |\n \t\tsed -n -e '2,$ s/^worktree //p')\"\n }\n \ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex b16c284181..7c0f82f31a 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1263,6 +1263,30 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '\n \ttest_cmp expected out\n '\n \n+test_expect_success '__git_complete_worktree_paths' '\n+\ttest_when_finished \"git worktree remove other_wt\" &&\n+\tgit worktree add --orphan other_wt &&\n+\trun_completion \"git worktree remove \" &&\n+\tgrep other_wt out\n+'\n+\n+test_expect_success '__git_complete_worktree_paths - not a git repository' '\n+\t(\n+\t\tcd non-repo &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$ROOT\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\ttest_completion \"git worktree remove \" \"\" 2>err &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n+test_expect_success '__git_complete_worktree_paths with -C' '\n+\ttest_when_finished \"rm -rf to_delete\" &&\n+\tgit -C otherrepo worktree add --orphan otherrepo_wt &&\n+\trun_completion \"git -C otherrepo worktree remove \" &&\n+\tgrep otherrepo_wt out\n+'\n+\n test_expect_success 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' '\n \ttest_completion \"git switch \" <<-\\EOF\n \tbranch-in-other Z\n-- \n2.44.0.1.g0da3aa8f7f\n"},{"id":"489471","messageId":"Zd2bnJZtgW2tFMFn@tanuki","threadId":"60989","inReplyTo":"eaf3649e-30cf-4eba-befa-5be826c828a8@gmail.com","subject":"Re: [PATCH v2] completion: fix __git_complete_worktree_paths","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-27T08:21:48Z","receivedAt":"2024-02-27T08:21:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 26, 2024 at 12:53:31AM +0100, Rubén Justo wrote:\n> Use __git to invoke \"worktree list\" in __git_complete_worktree_paths, to\n> respect any \"-C\" and \"--git-dir\" options present on the command line.\n> \n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n> \n> I stumbled upon this in a situation like:\n> \n>    $ git init /tmp/foo && cd /tmp/foo\n>    $ git worktree add --orphan foo_wt\n> \n>    $ git init /tmp/bar && cd /tmp/bar\n>    $ git worktree add --orphan bar_wt\n> \n>    $ cd /tmp/foo\n>    $ git -C /tmp/bar worktree remove <TAB>\n>    ... expecting /tmp/bar/wt, but ...\n>    $ git -C /tmp/bar worktree remove /tmp/foo_wt\n> \n> In this iteration, v2, some tests are included.\n> \n> The function __git was introduced in 1cd23e9e05 (completion: don't use\n> __gitdir() for git commands, 2017-02-03).  It is a small function, so\n> I'll include here to ease the review of this patch:\n> \n> \t# Runs git with all the options given as argument, respecting any\n> \t# '--git-dir=<path>' and '-C <path>' options present on the command line\n> \t__git ()\n> \t{\n> \t\tgit ${__git_C_args:+\"${__git_C_args[@]}\"} \\\n> \t\t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n> \t}\n> \n> \n>  contrib/completion/git-completion.bash |  2 +-\n>  t/t9902-completion.sh                  | 24 ++++++++++++++++++++++++\n>  2 files changed, 25 insertions(+), 1 deletion(-)\n> \n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 444b3efa63..86e55dc67f 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -3571,7 +3571,7 @@ __git_complete_worktree_paths ()\n>  \t# Generate completion reply from worktree list skipping the first\n>  \t# entry: it's the path of the main worktree, which can't be moved,\n>  \t# removed, locked, etc.\n> -\t__gitcomp_nl \"$(git worktree list --porcelain |\n> +\t__gitcomp_nl \"$(__git worktree list --porcelain |\n>  \t\tsed -n -e '2,$ s/^worktree //p')\"\n>  }\n>  \n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index b16c284181..7c0f82f31a 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -1263,6 +1263,30 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '\n>  \ttest_cmp expected out\n>  '\n>  \n> +test_expect_success '__git_complete_worktree_paths' '\n> +\ttest_when_finished \"git worktree remove other_wt\" &&\n> +\tgit worktree add --orphan other_wt &&\n> +\trun_completion \"git worktree remove \" &&\n> +\tgrep other_wt out\n> +'\n> +\n> +test_expect_success '__git_complete_worktree_paths - not a git repository' '\n> +\t(\n> +\t\tcd non-repo &&\n> +\t\tGIT_CEILING_DIRECTORIES=\"$ROOT\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\ttest_completion \"git worktree remove \" \"\" 2>err &&\n> +\t\ttest_must_be_empty err\n> +\t)\n> +'\n\nIf I understand correctly, we assume that the repo isn't detected here,\nand thus we will fail to complete the command. We don't want an error\nmessage though, which we assert. But do we also want to assert that\nthere is no output on stdout?\n\n> +\n> +test_expect_success '__git_complete_worktree_paths with -C' '\n> +\ttest_when_finished \"rm -rf to_delete\" &&\n\nWhat does this delete? I don't see \"to_delete\" being created as part of\nthis test.\n\n> +\tgit -C otherrepo worktree add --orphan otherrepo_wt &&\n> +\trun_completion \"git -C otherrepo worktree remove \" &&\n> +\tgrep otherrepo_wt out\n\nAnd as far as I can see, we don't write to \"out\" in this test, either.\nSo I think we're accidentally relying on state by the first test here.\n\nPatrick\n\n> +'\n> +\n>  test_expect_success 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' '\n>  \ttest_completion \"git switch \" <<-\\EOF\n>  \tbranch-in-other Z\n> -- \n> 2.44.0.1.g0da3aa8f7f\n> \n"},{"id":"489555","messageId":"0928d29b-41df-40a4-9536-4a8d097faa83@gmail.com","threadId":"60989","inReplyTo":"Zd2bnJZtgW2tFMFn@tanuki","subject":"Re: [PATCH v2] completion: fix __git_complete_worktree_paths","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-27T18:09:39Z","receivedAt":"2024-02-27T18:09:44Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Feb 27, 2024 at 09:21:48AM +0100, Patrick Steinhardt wrote:\n\n> > +test_expect_success '__git_complete_worktree_paths - not a git repository' '\n> > +\t(\n> > +\t\tcd non-repo &&\n> > +\t\tGIT_CEILING_DIRECTORIES=\"$ROOT\" &&\n> > +\t\texport GIT_CEILING_DIRECTORIES &&\n> > +\t\ttest_completion \"git worktree remove \" \"\" 2>err &&\n> > +\t\ttest_must_be_empty err\n> > +\t)\n> > +'\n> \n> If I understand correctly, we assume that the repo isn't detected here,\n> and thus we will fail to complete the command. We don't want an error\n> message though, which we assert.\n\nCorrect.\n\n> But do we also want to assert that\n> there is no output on stdout?\n\nTo me, the check makes sense;  to notice if we leak a message in such a\ncircumstance, for instance.  I can drop it if you think it does not add\nvalue.\n\nThe test for stderr is my main goal here.\n\n> \n> > +\n> > +test_expect_success '__git_complete_worktree_paths with -C' '\n> > +\ttest_when_finished \"rm -rf to_delete\" &&\n> \n> What does this delete? I don't see \"to_delete\" being created as part of\n> this test.\n\nGood eyes.  It's noise.  I'll drop this line.  Thanks.\n\n> \n> > +\tgit -C otherrepo worktree add --orphan otherrepo_wt &&\n> > +\trun_completion \"git -C otherrepo worktree remove \" &&\n> > +\tgrep otherrepo_wt out\n> \n> And as far as I can see, we don't write to \"out\" in this test, either.\n> So I think we're accidentally relying on state by the first test here.\n\nThe function run_completion leaves the result of the completion in the\nfile \"out\".  So we're checking here if \"otherrepo_wt\" is present in what\n\"git -C otherrepo worktree remove <TAB>\" returns.\n\nMaybe a new function: grep_completion, similar to test_completion, could\nmake this clearer?\n\n> \n> Patrick\n\nThanks.\n\n> \n> > +'\n> > +\n> >  test_expect_success 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' ' ow\n> >  \ttest_completion \"git switch \" <<-\\EOF\n> >  \tbranch-in-other Z\n> > -- \n> > 2.44.0.1.g0da3aa8f7f\n> > \n\n\n"},{"id":"489576","messageId":"b8f09e20-d0d3-4e0b-afe2-31affeb61052@gmail.com","threadId":"60989","inReplyTo":"eaf3649e-30cf-4eba-befa-5be826c828a8@gmail.com","subject":"[PATCH v3] completion: fix __git_complete_worktree_paths","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-27T21:06:49Z","receivedAt":"2024-02-27T21:06:53Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Use __git to invoke \"worktree list\" in __git_complete_worktree_paths, to\nrespect any \"-C\" and \"--git-dir\" options present on the command line.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nI stumbled upon this in a situation like:\n\n   $ git init /tmp/foo && cd /tmp/foo\n   $ git worktree add --orphan foo_wt\n\n   $ git init /tmp/bar && cd /tmp/bar\n   $ git worktree add --orphan bar_wt\n\n   $ cd /tmp/foo\n   $ git -C /tmp/bar worktree remove <TAB>\n   ... expecting /tmp/bar/wt, but ...\n   $ git -C /tmp/bar worktree remove /tmp/foo_wt\n\nThe function __git was introduced in 1cd23e9e05 (completion: don't use\n__gitdir() for git commands, 2017-02-03).  It is a small function, so\nI'll include here to ease the review of this patch:\n\n        # Runs git with all the options given as argument, respecting any\n        # '--git-dir=<path>' and '-C <path>' options present on the command line\n        __git ()\n        {\n                git ${__git_C_args:+\"${__git_C_args[@]}\"} \\\n                        ${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n        }\n\nHere are the changes since v2:\n\n\"test_completion\" already asserts if something is printed on stderr.\nTherefore \"test_must_be_empty err\" does not make sense.  Removed.  This\nmakes the test_completion with an empty response, more valuable, IMO.\nBut I'm open to change it.\n\nThe second test_when_finished has been fixed.\n\nThanks in advance for your review.\n\nRange-diff against v2:\n1:  4cc071fb8e ! 1:  de5d2ca1d3 completion: fix __git_complete_worktree_paths\n    @@ t/t9902-completion.sh: test_expect_success '__git_complete_fetch_refspecs - full\n     +\t\tcd non-repo &&\n     +\t\tGIT_CEILING_DIRECTORIES=\"$ROOT\" &&\n     +\t\texport GIT_CEILING_DIRECTORIES &&\n    -+\t\ttest_completion \"git worktree remove \" \"\" 2>err &&\n    -+\t\ttest_must_be_empty err\n    ++\t\ttest_completion \"git worktree remove \" \"\"\n     +\t)\n     +'\n     +\n     +test_expect_success '__git_complete_worktree_paths with -C' '\n    -+\ttest_when_finished \"rm -rf to_delete\" &&\n    ++\ttest_when_finished \"git -C otherrepo worktree remove otherrepo_wt\" &&\n     +\tgit -C otherrepo worktree add --orphan otherrepo_wt &&\n     +\trun_completion \"git -C otherrepo worktree remove \" &&\n     +\tgrep otherrepo_wt out\n\n contrib/completion/git-completion.bash |  2 +-\n t/t9902-completion.sh                  | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 444b3efa63..86e55dc67f 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3571,7 +3571,7 @@ __git_complete_worktree_paths ()\n \t# Generate completion reply from worktree list skipping the first\n \t# entry: it's the path of the main worktree, which can't be moved,\n \t# removed, locked, etc.\n-\t__gitcomp_nl \"$(git worktree list --porcelain |\n+\t__gitcomp_nl \"$(__git worktree list --porcelain |\n \t\tsed -n -e '2,$ s/^worktree //p')\"\n }\n \ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex b16c284181..aae2cbeeea 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1263,6 +1263,29 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '\n \ttest_cmp expected out\n '\n \n+test_expect_success '__git_complete_worktree_paths' '\n+\ttest_when_finished \"git worktree remove other_wt\" &&\n+\tgit worktree add --orphan other_wt &&\n+\trun_completion \"git worktree remove \" &&\n+\tgrep other_wt out\n+'\n+\n+test_expect_success '__git_complete_worktree_paths - not a git repository' '\n+\t(\n+\t\tcd non-repo &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$ROOT\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\ttest_completion \"git worktree remove \" \"\"\n+\t)\n+'\n+\n+test_expect_success '__git_complete_worktree_paths with -C' '\n+\ttest_when_finished \"git -C otherrepo worktree remove otherrepo_wt\" &&\n+\tgit -C otherrepo worktree add --orphan otherrepo_wt &&\n+\trun_completion \"git -C otherrepo worktree remove \" &&\n+\tgrep otherrepo_wt out\n+'\n+\n test_expect_success 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' '\n \ttest_completion \"git switch \" <<-\\EOF\n \tbranch-in-other Z\n-- \n2.44.0.1.g0da3aa8f7f\n\n"}]}