| Message ID | 20190621223107.8022-15-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | completion: a bunch of updates | expand |
Felipe Contreras <felipe.contreras@gmail.com> writes: > In case the command fails. It is unclear what you wanted to say with this. What command? After "git merge" fails? > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > contrib/completion/git-completion.bash | 4 +++- > t/t9902-completion.sh | 2 +- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 922ba5f925..91b87eb558 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -936,6 +936,7 @@ __git_list_merge_strategies () > }' > } > > +__git_merge_strategies_default='octopus ours recursive resolve subtree' > __git_merge_strategies= > # 'git merge -s help' (and thus detection of the merge strategy > # list) fails, unfortunately, if run outside of any git working > @@ -945,7 +946,8 @@ __git_merge_strategies= > __git_compute_merge_strategies () > { > test -n "$__git_merge_strategies" || > - __git_merge_strategies=$(__git_list_merge_strategies) > + { __git_merge_strategies=$(__git_list_merge_strategies); > + __git_merge_strategies="${__git_merge_strategies:-__git_merge_strategies_default}"; } > } > > __git_merge_strategy_options="ours theirs subtree subtree= patience > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index 14598bfbec..f4453ce70d 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -1722,7 +1722,7 @@ test_expect_success 'sourcing the completion script clears cached commands' ' > verbose test -z "$__git_all_commands" > ' > > -test_expect_failure 'sourcing the completion script clears cached merge strategies' ' > +test_expect_success 'sourcing the completion script clears cached merge strategies' ' > offgit && > GIT_TEST_GETTEXT_POISON= && > __git_compute_merge_strategies &&
On Mon, Jun 24, 2019 at 12:24 PM Junio C Hamano <gitster@pobox.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > In case the command fails. > > It is unclear what you wanted to say with this. What command? > After "git merge" fails? Yes. The command that __git_list_merge_strategies() uses. % cd /tmp % git merge -s help fatal: not a git repository (or any parent up to mount point /) Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
Felipe Contreras <felipe.contreras@gmail.com> writes: > On Mon, Jun 24, 2019 at 12:24 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> > In case the command fails. >> >> It is unclear what you wanted to say with this. What command? >> After "git merge" fails? > > Yes. The command that __git_list_merge_strategies() uses. Next round, write that in the proposed log message, please. An issue in the proposed commit log message that puzzles reviewers is something we expect future readers of "git log" to also stumble on. > % cd /tmp > % git merge -s help > fatal: not a git repository (or any parent up to mount point /) > Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set). I think the recent <20190612085606.12144-1-pclouds@gmail.com> established a good pattern we should follow; when a command we run to get list of things to use in completion fails, we refrain from caching that broken output, and arrange so that we will try again. It looks to me that "git merge -s help" barfing outside a repository is a good candidate to follow that pattern. Outside a repository, the user will not be able to perform a merge with any strategy, so not completing the command line when the user say "git merge -s <TAB>" outside a repository is not the end of the world, as long as we follow the right codepath to grab the available strategies once the user goes into a repository where "git merge -s help" works, no? Thanks.
On Mon, Jun 24, 2019 at 08:11:40PM -0500, Felipe Contreras wrote: > On Mon, Jun 24, 2019 at 12:24 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > > > In case the command fails. > > > > It is unclear what you wanted to say with this. What command? > > After "git merge" fails? > > Yes. The command that __git_list_merge_strategies() uses. > > % cd /tmp > % git merge -s help > fatal: not a git repository (or any parent up to mount point /) > Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set). If that command behind __git_list_merge_strategies() fails, then 'git merge -s <TAB>' won't simply list any merge strategies. However, that's not a big deal, because the command won't work without a repository anyway, so I don't see the point in adding a hard-coded list of merge strategies. And in this case $__git_merge_strategies will remain empty, so the next time the user attempts to complete a strategies while in a repository, then it will Just Work (unlike the undesired caching of options without a repository that is fixed in 69702523af (completion: do not cache if --git-completion-helper fails, 2019-06-12).
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 922ba5f925..91b87eb558 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -936,6 +936,7 @@ __git_list_merge_strategies () }' } +__git_merge_strategies_default='octopus ours recursive resolve subtree' __git_merge_strategies= # 'git merge -s help' (and thus detection of the merge strategy # list) fails, unfortunately, if run outside of any git working @@ -945,7 +946,8 @@ __git_merge_strategies= __git_compute_merge_strategies () { test -n "$__git_merge_strategies" || - __git_merge_strategies=$(__git_list_merge_strategies) + { __git_merge_strategies=$(__git_list_merge_strategies); + __git_merge_strategies="${__git_merge_strategies:-__git_merge_strategies_default}"; } } __git_merge_strategy_options="ours theirs subtree subtree= patience diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 14598bfbec..f4453ce70d 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1722,7 +1722,7 @@ test_expect_success 'sourcing the completion script clears cached commands' ' verbose test -z "$__git_all_commands" ' -test_expect_failure 'sourcing the completion script clears cached merge strategies' ' +test_expect_success 'sourcing the completion script clears cached merge strategies' ' offgit && GIT_TEST_GETTEXT_POISON= && __git_compute_merge_strategies &&
In case the command fails. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- contrib/completion/git-completion.bash | 4 +++- t/t9902-completion.sh | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-)