| Message ID | 20190607093034.816-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | completion: do not cache if --git-completion-helper fails | expand |
On Fri, Jun 07, 2019 at 04:30:34PM +0700, Nguyễn Thái Ngọc Duy wrote: > "git <cmd> --git-completion-helper" could fail if the command checks for > a repo before parse_options(). If the result is cached, later on when > the user moves to a worktree with repo, tab completion will still fail. > > Avoid this by detecting errors and not cache the completion output. We > can try again and hopefully succeed next time (e.g. when a repo is > found). > > Of course if --git-completion-helper fails permanently because of other > reasons (*), this will slow down completion. But I don't see any better > option to handle that case. I think a permanently failing 'git cmd --git-completion-helper' shouldn't really happen, unless there is a bug in the completion script or the git installation or similar exceptional situation. And then that issue should be fixed, but I don't think we should worry about an extra subshell and git process in those situations. > (*) one of those cases is if __gitcomp_builtin is called on a command > that does not support --git-completion-helper. And we do have a > generic call > > __git_complete_common "$command" > > but this case is protected with __git_support_parseopt_helper so we're > good. > > Reported-by: Felipe Contreras <felipe.contreras@gmail.com> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > contrib/completion/git-completion.bash | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 9f71bcde96..a515de8501 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -398,13 +398,15 @@ __gitcomp_builtin () > eval "options=\$$var" > > if [ -z "$options" ]; then > + local nocache= > # leading and trailing spaces are significant to make > # option removal work correctly. > - options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " > + options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " || nocache=t Why not just 'return' here? If we return, then it won't list any options, and the shell should fall back to regular file completion. With this patch it will still list extra options, if there are any, and I don't really see the point in doing that. > + > for i in $excl; do > options="${options/ $i / }" > done > - eval "$var=\"$options\"" > + test -n "$nocache" || eval "$var=\"$options\"" > fi > > __gitcomp "$options" > -- > 2.22.0.rc0.322.g2b0371e29a >
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > if [ -z "$options" ]; then > + local nocache= > # leading and trailing spaces are significant to make > # option removal work correctly. > - options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " > + options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " || nocache=t > + > for i in $excl; do > options="${options/ $i / }" > done Is there a point in doing this loop if we are not going to eval after all? IOW... > - eval "$var=\"$options\"" > + test -n "$nocache" || eval "$var=\"$options\"" > fi ... I am wondering why it is not more like this if options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " then for i in $excl do options=... done eval "$var=..." fi or just return from the function instead of introducing and setting a new variable, as only remaining thing the function does is to call __gitcomp with $options, but we know that we are giving up on completing this round.
On Fri, Jun 7, 2019 at 5:02 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Fri, Jun 07, 2019 at 04:30:34PM +0700, Nguyễn Thái Ngọc Duy wrote: > > "git <cmd> --git-completion-helper" could fail if the command checks for > > a repo before parse_options(). If the result is cached, later on when > > the user moves to a worktree with repo, tab completion will still fail. > > > > Avoid this by detecting errors and not cache the completion output. We > > can try again and hopefully succeed next time (e.g. when a repo is > > found). > > > > Of course if --git-completion-helper fails permanently because of other > > reasons (*), this will slow down completion. But I don't see any better > > option to handle that case. > > I think a permanently failing 'git cmd --git-completion-helper' > shouldn't really happen, unless there is a bug in the completion > script or the git installation or similar exceptional situation. And > then that issue should be fixed, but I don't think we should worry > about an extra subshell and git process in those situations. Indeed. In think there's only sane option to make this work in all situation; a reorganization. Something like this should work: struct command checkout_command = { .name = "checkout", .function = cmd_checkout, .run_options = RUN_SETUP | NEED_WORK_TREE, .help = N_("Switch branches or restore working tree files"), .options = { OPT__QUIET(&opts.quiet, N_("suppress progress reporting")), ... }, } This way we could run parse_options_show_gitcomp() from git.c and not worry about whatever cmd_checkout() needs. This has the added advantage that it gathers information about this command that is stray in multiple sources (git.c, command-list.h), and it makes builtin.h cleaner too. Plus, we could rework the way -h works too.
On Sat, Jun 8, 2019 at 12:33 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Fri, Jun 7, 2019 at 5:02 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > On Fri, Jun 07, 2019 at 04:30:34PM +0700, Nguyễn Thái Ngọc Duy wrote: > > > "git <cmd> --git-completion-helper" could fail if the command checks for > > > a repo before parse_options(). If the result is cached, later on when > > > the user moves to a worktree with repo, tab completion will still fail. > > > > > > Avoid this by detecting errors and not cache the completion output. We > > > can try again and hopefully succeed next time (e.g. when a repo is > > > found). > > > > > > Of course if --git-completion-helper fails permanently because of other > > > reasons (*), this will slow down completion. But I don't see any better > > > option to handle that case. > > > > I think a permanently failing 'git cmd --git-completion-helper' > > shouldn't really happen, unless there is a bug in the completion > > script or the git installation or similar exceptional situation. And > > then that issue should be fixed, but I don't think we should worry > > about an extra subshell and git process in those situations. > > Indeed. In think there's only sane option to make this work in all > situation; a reorganization. > > Something like this should work: > > struct command checkout_command = { > .name = "checkout", > .function = cmd_checkout, > .run_options = RUN_SETUP | NEED_WORK_TREE, > .help = N_("Switch branches or restore working tree files"), > .options = { > OPT__QUIET(&opts.quiet, N_("suppress progress reporting")), > ... > }, > } > > This way we could run parse_options_show_gitcomp() from git.c and not > worry about whatever cmd_checkout() needs. This only works for a few commands. Those with subcommands already have struct option[] array scattered in different places. And some new ones also have struct option array dynamically created. It's not impossible to do. But I feel there's a lot of reorganizing for little gain. Maybe when we pass 'struct repository *' to all commands, which means we hit all commmands at once anyway, we can reconsider this (and having config parser in a more declarative form like cmd option parser). > This has the added advantage that it gathers information about this > command that is stray in multiple sources (git.c, command-list.h), and > it makes builtin.h cleaner too. > > Plus, we could rework the way -h works too.
On Wed, Jun 12, 2019 at 3:52 AM Duy Nguyen <pclouds@gmail.com> wrote: > > On Sat, Jun 8, 2019 at 12:33 AM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > Something like this should work: > > > > struct command checkout_command = { > > .name = "checkout", > > .function = cmd_checkout, > > .run_options = RUN_SETUP | NEED_WORK_TREE, > > .help = N_("Switch branches or restore working tree files"), > > .options = { > > OPT__QUIET(&opts.quiet, N_("suppress progress reporting")), > > ... > > }, > > } > > > > This way we could run parse_options_show_gitcomp() from git.c and not > > worry about whatever cmd_checkout() needs. > > This only works for a few commands. Those with subcommands already > have struct option[] array scattered in different places. And some new > ones also have struct option array dynamically created. > > It's not impossible to do. But I feel there's a lot of reorganizing > for little gain. Maybe when we pass 'struct repository *' to all > commands, which means we hit all commmands at once anyway, we can > reconsider this (and having config parser in a more declarative form > like cmd option parser). Well yes, there is little *functional* gain at the moment, but this (or some version of this) must be done eventually. For the moment we still have an issue, but I see there's already a hack present for '-h', maybe we can re-utilize it. Something like this: --- a/git.c +++ b/git.c @@ -408,6 +408,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) prefix = NULL; help = argc == 2 && !strcmp(argv[1], "-h"); + if (!help) + help = argc == 2 && !strcmp(argv[1], "--git-completion-helper"); if (!help) { if (p->option & RUN_SETUP) prefix = setup_git_directory(); One way or the other, shouldn't my tests be merged? The issue is still there, and it's nice to have tests for that.
On Fri, Jun 14, 2019 at 7:30 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Wed, Jun 12, 2019 at 3:52 AM Duy Nguyen <pclouds@gmail.com> wrote: > > > > On Sat, Jun 8, 2019 at 12:33 AM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > > > > Something like this should work: > > > > > > struct command checkout_command = { > > > .name = "checkout", > > > .function = cmd_checkout, > > > .run_options = RUN_SETUP | NEED_WORK_TREE, > > > .help = N_("Switch branches or restore working tree files"), > > > .options = { > > > OPT__QUIET(&opts.quiet, N_("suppress progress reporting")), > > > ... > > > }, > > > } > > > > > > This way we could run parse_options_show_gitcomp() from git.c and not > > > worry about whatever cmd_checkout() needs. > > > > This only works for a few commands. Those with subcommands already > > have struct option[] array scattered in different places. And some new > > ones also have struct option array dynamically created. > > > > It's not impossible to do. But I feel there's a lot of reorganizing > > for little gain. Maybe when we pass 'struct repository *' to all > > commands, which means we hit all commmands at once anyway, we can > > reconsider this (and having config parser in a more declarative form > > like cmd option parser). > > Well yes, there is little *functional* gain at the moment, but this > (or some version of this) must be done eventually. > > For the moment we still have an issue, but I see there's already a > hack present for '-h', maybe we can re-utilize it. Something like > this: > > --- a/git.c > +++ b/git.c > @@ -408,6 +408,8 @@ static int run_builtin(struct cmd_struct *p, int > argc, const char **argv) > > prefix = NULL; > help = argc == 2 && !strcmp(argv[1], "-h"); > + if (!help) > + help = argc == 2 && !strcmp(argv[1], "--git-completion-helper"); > if (!help) { > if (p->option & RUN_SETUP) > prefix = setup_git_directory(); > > One way or the other, shouldn't my tests be merged? The issue is still > there, and it's nice to have tests for that. Is there any good reason to complete options when they are not going to work anyway (e.g. like checkout which needs $GIT_DIR)? Besides "it used to work before --git-completion-helper" which I don't consider a good reason given the maintenance tradeoff of --git-completion-helper.
On Thu, Jun 13, 2019 at 9:53 PM Duy Nguyen <pclouds@gmail.com> wrote: > > On Fri, Jun 14, 2019 at 7:30 AM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > One way or the other, shouldn't my tests be merged? The issue is still > > there, and it's nice to have tests for that. > > Is there any good reason to complete options when they are not going > to work anyway (e.g. like checkout which needs $GIT_DIR)? Besides "it > used to work before --git-completion-helper" which I don't consider a > good reason given the maintenance tradeoff of --git-completion-helper. No, there is no good reason that I can think of, except checking the arguments, which is precisely how I found out the issue; not something I usually do. But a newcomer might not know what commands don't work outside a git directory. But more importantly; is there a good enough reason not to? I seem to recall to be annoyed by the fact that 'git command -h' failed on some command with a fatal error. Similarly, I don't see any good reason why 'git help clone' should ever fail. These are not dealbreakers by any means, just some kind of weird corner cases. Such things never happen in in Mercurial BTW. And of course --git-completion-helper is the way to go, I recall I wanted to implement such a thing myself, this has the potential to increase the power of the zsh completion incredibly, although not yet. But that doesn't mean it's perfect and there are no regressions; there are. I just think they should be documented in the testing framework. They are not big enough to warrant going back from --git-completion-helper though. The only real issue I think has not been raised is that the completion scripts are in "contrib", they are not considered part of the main deliverables. So it's conceivable that somebody running Git v1.6 tries the completion scripts of v1.20, and everything breaks. I'm still not exactly sure what should be the way to solve this conundrum, but again, I don't think going back from --git-completion-helper is a good move. I don't think I suggested that. Cheers.
On Fri, Jun 14, 2019 at 1:07 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Thu, Jun 13, 2019 at 9:53 PM Duy Nguyen <pclouds@gmail.com> wrote: > > > > On Fri, Jun 14, 2019 at 7:30 AM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > > > > One way or the other, shouldn't my tests be merged? The issue is still > > > there, and it's nice to have tests for that. > > > > Is there any good reason to complete options when they are not going > > to work anyway (e.g. like checkout which needs $GIT_DIR)? Besides "it > > used to work before --git-completion-helper" which I don't consider a > > good reason given the maintenance tradeoff of --git-completion-helper. > > No, there is no good reason that I can think of, except checking the > arguments, which is precisely how I found out the issue; not something > I usually do. But a newcomer might not know what commands don't work > outside a git directory. > > But more importantly; is there a good enough reason not to? For me, yes. No extra work for me. > I seem to > recall to be annoyed by the fact that 'git command -h' failed on some > command with a fatal error. Similarly, I don't see any good reason why > 'git help clone' should ever fail. You could give the '-h' hack a try. It probably works for some, but I don't think it also works for things like 'git worktree add --git-completion-helper'.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 9f71bcde96..a515de8501 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -398,13 +398,15 @@ __gitcomp_builtin () eval "options=\$$var" if [ -z "$options" ]; then + local nocache= # leading and trailing spaces are significant to make # option removal work correctly. - options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " + options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " || nocache=t + for i in $excl; do options="${options/ $i / }" done - eval "$var=\"$options\"" + test -n "$nocache" || eval "$var=\"$options\"" fi __gitcomp "$options"
"git <cmd> --git-completion-helper" could fail if the command checks for a repo before parse_options(). If the result is cached, later on when the user moves to a worktree with repo, tab completion will still fail. Avoid this by detecting errors and not cache the completion output. We can try again and hopefully succeed next time (e.g. when a repo is found). Of course if --git-completion-helper fails permanently because of other reasons (*), this will slow down completion. But I don't see any better option to handle that case. (*) one of those cases is if __gitcomp_builtin is called on a command that does not support --git-completion-helper. And we do have a generic call __git_complete_common "$command" but this case is protected with __git_support_parseopt_helper so we're good. Reported-by: Felipe Contreras <felipe.contreras@gmail.com> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- contrib/completion/git-completion.bash | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)