Re: [PATCH v2 7/7] completion: recognize more long-options
From: SZEDER Gábor <hidden>
Date: 2017-02-02 02:00:14
On Wed, Feb 1, 2017 at 5:49 PM, Cornelius Weig [off-list ref] wrote:
Hi Gabor, thanks for taking a look at these commits. On 01/31/2017 11:17 PM, SZEDER Gábor wrote:quoted
On Fri, Jan 27, 2017 at 10:17 PM, [off-list ref] wrote:quoted
From: Cornelius Weig <redacted> Recognize several new long-options for bash completion in the following commands:Adding more long options that git commands learn along the way is always an improvement. However, seeing "_several_ new long options" (or "some long options" in one of the other patches in the series) makes the reader wonder: are these the only new long options missing or are there more? If there are more, why only these are added? If there aren't any more missing long options left, then please say so, e.g. "Add all missing long options, except the potentially desctructive ones, for the following commands: ...."Personally, I agree with you thatquoted
Adding more long options that git commands learn along the way is always an improvement.However, people may start complaining that their terminal becomes too cluttered when doing a double-Tab. In my cover letter, I go to length about this. My assumption was that all options that are mentioned in the introduction of the command man-page should be important enough to have them in the completion list.
But that doesn't mean that the ones not mentioned in the synopsis section are not worth completing. The list of options listed by the completion script for several of these commands fits on a single line. The command with the most options among these is 'git pull', and even its options don't fill more than half of a 80x25 screen. I see no danger of people coming complaining.
quoted hunk ↗ jump to hunk
I'll change my commit message accordingly.quoted
quoted
- rm: --force'--force' is a potentially destructive option, too.Thanks for spotting this. Btw, I haven't found that non-destructive options should not be eligible for completion. To avoid confusion about this in the future, I suggest to also change the documentation: index 933bb6e..96f1c7f 100644--- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash@@ -13,7 +13,7 @@ # *) git email aliases for git-send-email # *) tree paths within 'ref:path/to/file' expressions # *) file paths within current working directory and index -# *) common --long-options +# *) common non-destructive --long-options
I don't mind such a change, but I don't think that list was ever meant to be comprehensive or decisive. It is definitely not the former, as it's missing several things that the completion script does support. OTOH, it talks about .git/remotes, which has been considered legacy for quite some years (though it's right, because the completion script still supports it).
I take it you have also looked at the code itself? Then I would gladly mention you as reviewer in my sign-off.
Yeah, most of the changes was rather straightforward, except the completion of 'git remote's subcommands' options, but that looks good, too. Gábor