[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH] Add shell completion for git remote rm
From:       Keith Smiley <k () keith ! so>
Date:       2017-12-31 20:44:26
Message-ID: 20171231204426.GA30674 () bryant ! local
[Download RAW message or body]

I'm definitely happy to update this patch for now to just complete the 
remote names, and not add rm to the list of subcommand completions if 
we're all ok with that!

--
Keith Smiley

On 12/30, Ævar Arnfjörð Bjarmason wrote:
>
>On Sat, Dec 30 2017, Todd Zullinger jotted:
>
>> Ævar Arnfjörð Bjarmason wrote:
>>>> I think adding 'rm' to completion definitely counts as advertisement.
>>>> It doesn't have much practical use, after all: typing 'rm' with
>>>> completion is actually one more keystroke than without (r<TAB>m vs. rm).
>>>
>>> This is only one use of the completion interface, maybe you only use it
>>> like that, but not everyone does.
>>>
>>> The completion interface has two uses, one is to actually save you
>>> typing, the other is subcommand discovery, and maybe someone who has a
>>> use neither you or I have thought of is about to point out a third.
>>>
>>> I'll type "git $whatever $subcommand<TAB>" as *validation* that I've
>>> found the right command, not to complete it for me. This is a thing
>>> that's in my muscle memory for everything.
>>
>> Is that meant to be in favor of including rm in the
>> completion results or against? :)
>
>For.
>
>>> Since I've been typing "git remote rm<TAB>" for a while (started before
>>> this deprecation happened) I've actually been meaning to submit
>>> completion for "rm" since it works, not knowing about Duy's patch until
>>> now.
>>>
>>> Now, even if someone disagrees that we should have "rm" at all I think
>>> that in general we should not conflate two different things, one is
>>> whether:
>>>
>>>     git remote <TAB>
>>>
>>> shows both "rm" and "remove" in the list, and the other is whether:
>>>
>>>     git remote rm<TAB>
>>>
>>> Should yield:
>>>
>>>     git remote rm<SPACE>
>>>
>>> Or, as now happens:
>>>
>>>     git remote rm<NOTHING AND ÆVAR THINKS IT'S BROKEN>
>>>
>>> I can see why we'd, in general, we'd like to not advertise certain
>>> options for completion (due to deprecation, or just to avoid verbosity),
>>> but complete them once they're unambiguously typed out.
>>>
>>> I don't know whether the bash completion interface supports making that
>>> distinction, but it would be useful.
>>
>> It can be done, though I think that it's probably better to
>> subtly lead people to use 'git remote remove' going forward,
>> to keep things consistent.  I don't have a strong preference
>> for or against the rm -> remove change, but since that's
>> been done I think there's a benefit to keeping things
>> consistent in the UI.
>
>We changed it in the past, we can always change it again, it's never too
>late to fix the UI.
>
>Now whether we *should* change/fix this particular thing is another
>matter. I'm just pointing out that we shouldn't fall into the trap of
>thinking that git's UI is an established platform that can't be changed.
>
>The vast majority of people who'll ever use git will most likely start
>using a version that we're going to release many years into the future.
>
>I'm reminded of the story about the guy who decided makefiles must have
>tabs, who didn't want to change it because he already had some dozens of
>users.
>
>> And I think that should also apply to
>> not offering completion for commands/subcommands/options
>> which are only kept for backward compatibility.
>
>Yeah I think it makes sense to at some point stop completing things if
>we're going to remove stuff, if we decide to remove it.
>
>> Here's one way to make 'git remote rm <TAB>' work without
>> including it in the output of 'git remote <TAB>':
>>
>> diff --git i/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash
>> index 3683c772c5..aa63f028ab 100644
>> --- i/contrib/completion/git-completion.bash
>> +++ w/contrib/completion/git-completion.bash
>> @@ -2668,7 +2668,9 @@ _git_remote ()
>>  		add rename remove set-head set-branches
>>  		get-url set-url show prune update
>>  		"
>> -	local subcommand="$(__git_find_on_cmdline "$subcommands")"
>> +	# Don't advertise rm by including it in subcommands, but complete
>> +	# remotes if it is used.
>> +	local subcommand="$(__git_find_on_cmdline "$subcommands rm")"
>>  	if [ -z "$subcommand" ]; then
>>  		case "$cur" in
>>  		--*)
>
>Neat!
>
>> Keeping 'git remote rm' working to avoid breaking scripts is
>> one thing, but having it in the completion code makes it
>> more likely that it will continue to be seen as a
>> recommended subcommand.
>>
>> This leads to patches like this one, where it's presumed
>> that the lack of completion is simply an oversight or a bug.
>> Of course, the lack of completion hasn't caused everyone to
>> forget that 'remote rm' was changed to 'remote remove', so
>> that reasoning may be full of hot air (or worse). ;)
>>
>> The current result of 'git remote rm <TAB>' isn't so great.
>> It's arguably worse to have it pretend that no subcommand
>> was given than to list the remotes.
>>
>> $ git remote rm <TAB>
>> add            remove         set-head       update
>> get-url        rename         set-url
>> prune          set-branches   show
>
>Although that's a bug that has nothing to do with remove/rm, because you
>also get:
>
>    $ git remote blahblah <TAB>
>    $ git rebase doesntexist <TAB>
>
>etc. showing you valid subcommands, when perhaps we should show
>"warning: no such subcommand `blahblah`/`doesntexist`!" instead.
>
>> I think completing nothing or completing the remotes
>> (without offering rm in the subcommand list) would be
>> better, after looking at it a bit.
>>
>> I don't know how to disable file completion, but I'm not
>> intimately familiar with the git completion script (thanks
>> to it working so damn well).  I'm guessing there's a way to
>> do that, if there's a strong desire to not complete the
>> remotes at all.
>>
>> I don't think we should include rm in 'git remote <TAB>'
>> completion, but I don't care much either way what 'git
>> remote rm <TAB>' includes.  But it should be better than
>> including the other subcommands.
[prev in list] [next in list] [prev in thread] [next in thread]