Improved bash tab completion for 'git restore' - adds support for auto-completing filenames

[DrHyde]

This adds tab-completion of filenames to the bash completions for git restore.

[gitgitgadget-git]

Welcome to GitGitGadget

Hi @DrHyde, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax:

CC: Revi Ewer <revi.ewer@example.com>, Ill Takalook <ill.takalook@example.net>

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[gitgitgadget-git]

There are issues in commit 4658605:
tab completion of filenames for 'git restore'
Commit checks stopped - the message is too short
Commit not signed off

[Ikke]

/allow

[gitgitgadget-git]

User DrHyde is now allowed to use GitGitGadget.

WARNING: DrHyde has no public email address set on GitHub;
GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback
on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public
to let GitGitGadget know which email address to use.}

[DrHyde]

/preview

[gitgitgadget-git]

There are issues in commit 4658605:
tab completion of filenames for 'git restore'
Commit checks stopped - the message is too short
Commit not signed off

[DrHyde]

/preview

[gitgitgadget-git]

There are issues in commit ce75fd6:
tab completion of filenames for 'git restore'
Commit not signed off

[DrHyde]

/preview

[gitgitgadget-git]

Preview email sent as pull.1227.git.git.1647027711536.gitgitgadget@gmail.com

[DrHyde]

/submit

[gitgitgadget-git]

Submitted as pull.1227.git.git.1647032857097.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1227/DrHyde/filename-completion-for-git-restore-v1

To fetch this version to local tag pr-git-1227/DrHyde/filename-completion-for-git-restore-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1227/DrHyde/filename-completion-for-git-restore-v1

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"David Cantrell via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: David Cantrell <david@cantrell.org.uk>
>
> If no --args are present after 'git restore' it assumes that you want
> to tab-complete one of the files with uncommitted changes
>
> Signed-off-by: David Cantrell <david@cantrell.org.uk>
> ---
>     Improved bash tab completion for 'git restore' - adds support for
>     auto-completing filenames
>     
>     This adds tab-completion of filenames to the bash completions for git
>     restore.

Two questions

 - "restore" is a castrated half "checkout"; shouldn't the latter
   also be getting the same feature?

 - is "complete_index_file --committable" the right thing to use?

   It boils down to running "diff-index HEAD", which means path with
   differences from the HEAD commit is listed.  By default "restore"
   checks out the contents of the given path from the index to the
   working tree, so after "edit F && git add F", "diff-index HEAD"
   may show F in its output (i.e. F is "committable"), but "restore
   F" would be a no-op.  Which feels a bit iffy.

   Modelling it after "git add" completion, where we look for paths
   that are different between the index and the working tree, feels
   more appropriate, but I haven't thought things through.

> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1227%2FDrHyde%2Ffilename-completion-for-git-restore-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1227/DrHyde/filename-completion-for-git-restore-v1
> Pull-Request: https://github.com/git/git/pull/1227
>
>  contrib/completion/git-completion.bash | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 49a328aa8a4..7ccad8ff4b1 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2883,14 +2883,21 @@ _git_restore ()
>  	case "$cur" in
>  	--conflict=*)
>  		__gitcomp "diff3 merge zdiff3" "" "${cur##--conflict=}"
> +		return
>  		;;
>  	--source=*)
>  		__git_complete_refs --cur="${cur##--source=}"
> +		return
>  		;;
>  	--*)
>  		__gitcomp_builtin restore
> +		return
>  		;;
>  	esac
> +
> +	if __git rev-parse --verify --quiet HEAD >/dev/null; then
> +		__git_complete_index_file "--committable"
> +	fi
>  }

Do you need to sprinkle return's?  Instead you could just add
another case arm, like

	case "$cur" in
	--conflict=*)
		... all the existing code ...
	--*)
		__gitcomp_builtin restore
		;;
+	*)
+		... whatever you want to do when
+		... $cur is not a --dashed-option
+		;;
	esac

[DrHyde]

/preview

[gitgitgadget-git]

There are issues in commit 48d53d0:
if a file has been staged we don't want to list it
Commit checks stopped - the message is too short
Commit not signed off

[gitgitgadget-git]

On the Git mailing list, David Cantrell wrote (reply to this):

On 13/03/2022 06:45, Junio C Hamano wrote:
> "David Cantrell via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>      Improved bash tab completion for 'git restore' - adds support for
>>      auto-completing filenames
>>      >>      This adds tab-completion of filenames to the bash completions for git
>>      restore.
> Two questions
> >   - "restore" is a castrated half "checkout"; shouldn't the latter
>     also be getting the same feature?

`git checkout <tab>` already completes to a list of branches and tags, which is I think more useful in that case.

>   - is "complete_index_file --committable" the right thing to use?
> >     It boils down to running "diff-index HEAD", which means path with
>     differences from the HEAD commit is listed.  By default "restore"
>     checks out the contents of the given path from the index to the
>     working tree, so after "edit F && git add F", "diff-index HEAD"
>     may show F in its output (i.e. F is "committable"), but "restore
>     F" would be a no-op.  Which feels a bit iffy.

I'd not thought of that. --modified is better.

>> @@ -2883,14 +2883,21 @@ _git_restore ()
>>   	case "$cur" in
>>   	--conflict=*)
>>   		__gitcomp "diff3 merge zdiff3" "" "${cur##--conflict=}"
>> +		return
>>   		;;
>>   	--source=*)
>>   		__git_complete_refs --cur="${cur##--source=}"
>> +		return
>>   		;;
>> ...
> Do you need to sprinkle return's?  Instead you could just add
> another case arm, like
> > +	*)
> +		... whatever you want to do when
> +		... $cur is not a --dashed-option
> +		;;

Liberal sprinkling of return like that seems to be the norm for the rest of the file so I stuck with it.

-- 
David Cantrell

[DrHyde]

/preview

[gitgitgadget-git]

Preview email sent as pull.1227.v2.git.git.1647305287.gitgitgadget@gmail.com

[DrHyde]

/submit

[gitgitgadget-git]

Submitted as pull.1227.v2.git.git.1647305547.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1227/DrHyde/filename-completion-for-git-restore-v2

To fetch this version to local tag pr-git-1227/DrHyde/filename-completion-for-git-restore-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1227/DrHyde/filename-completion-for-git-restore-v2

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

David Cantrell <david@cantrell.org.uk> writes:

>>> @@ -2883,14 +2883,21 @@ _git_restore ()
>>>   	case "$cur" in
>>>   	--conflict=*)
>>>   		__gitcomp "diff3 merge zdiff3" "" "${cur##--conflict=}"
>>> +		return
>>>   		;;
>>>   	--source=*)
>>>   		__git_complete_refs --cur="${cur##--source=}"
>>> +		return
>>>   		;;
>>> ...
>> Do you need to sprinkle return's?  Instead you could just add
>> another case arm, like
>> +	*)
>> +		... whatever you want to do when
>> +		... $cur is not a --dashed-option
>> +		;;
>
> Liberal sprinkling of return like that seems to be the norm for the
> rest of the file so I stuck with it.

An existing bad practice is not a good excuse to spread it even
more, though.

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"David Cantrell via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This adds tab-completion of filenames to the bash completions for git
> restore.
>
> David Cantrell (2):
>   tab completion of filenames for 'git restore'
>   if a file has been staged we don't want to list it

Why two patches?  The second separate patch makes the topic look as
if "oops, the first step designed a wrong behaviour and here is a
brown paper bag fix-up".

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> "David Cantrell via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> This adds tab-completion of filenames to the bash completions for git
>> restore.
>>
>> David Cantrell (2):
>>   tab completion of filenames for 'git restore'
>>   if a file has been staged we don't want to list it
>
> Why two patches?  The second separate patch makes the topic look as
> if "oops, the first step designed a wrong behaviour and here is a
> brown paper bag fix-up".

Sorry, I forgot the obligatory clarification for new contributors.

This project gives all contributors a chance to pretend to be a
"perfect human".  When sending an updated patch (or patch series),
contributors are encouraged to hide^W correct their earlier mistakes
and present a perfect logical progression that they (would have, if
they were perfect) followed to arrive at a perfect end result.

So, instead of having step 1 that uses --committable without
justifying why it was chosen, and then change mind in step 2 to
replace it with --modified, have a single patch that uses
--modified, and explain in the proposed log message that
--committable and --modified may be possibilities, and why the patch
chose to use the latter.  The resulting history without a flip-flop
in the middle is easier to use by future developers to understand
the reasoning behind each change.

Thanks.

[DrHyde]

/preview

[gitgitgadget-git]

Preview email sent as pull.1227.v3.git.git.1647381626407.gitgitgadget@gmail.com

[DrHyde]

/submit

[gitgitgadget-git]

Submitted as pull.1227.v3.git.git.1647382437475.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1227/DrHyde/filename-completion-for-git-restore-v3

To fetch this version to local tag pr-git-1227/DrHyde/filename-completion-for-git-restore-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1227/DrHyde/filename-completion-for-git-restore-v3

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"David Cantrell via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: David Cantrell <david@cantrell.org.uk>
> Subject: Re: [PATCH v3] tab completion of filenames for 'git restore'

Perhaps

    Subject: [PATCH v3] completion: complete modified files for 'git restore'

cf. Documentation/SubmittingPatches::[[summary-section]]


> If no --args are present after 'git restore' it assumes that you want
> to tab-complete one of the files with unstaged uncommitted changes.

Since it is our convention that the first paragraph talks about the
current behaviour, i.e. without the proposed changes applied, in the
present tense, I'd assume the above is what the current code does.

What do you mean by "--args" in this sentence?  Dashed-options?
I am getting the same set of files whose name begins with 'a' from
these two:

 $ git restore a<TAB>
 $ git restore --staged a<TAB>

so, with or without --dashed-options, we complete one of the files,
whether they have any modifications.

Perhaps you meant to say more like:

    When completing a non-option argument to 'git restore', the
    command line completion support offers names of the files
    present in the working tree as candidates.

to describe the status quo; to hint what the shortcoming of the
current behaviour is, we may want to add a bit more, perhaps
append the following at the end of that first paragraph:

    But many of these files may not have any changes, and running
    "restore" on them would be a no-op.  Listing only the files, to
    which doing "restore" is not a no-op, would reduce the clutter.
    
Then we'd continue with the solution, while explaining why the exact
choice between modified vs committable was made:

    Offer only the files that are different from the index, to match
    the default behaviour of "git restore" that checks out the
    contents last added to the index to the working tree.  We could
    instead show the files that are different between the index and
    HEAD, and that is more suittable if "git restore --staged" is
    being completed, but this should do for now.

or something.  The last part is written in such a way to explicitly
signal to future developers that we know we did not do a perfect job
and we do not mind if they extend the logic to use something other
than "--modified" when appropriate.  For example, they could build
on this solution to make it inspect the command line for "--staged"
and "--source" and drive "diff-index" differently to grab the paths
that are offered.  We just do not do that at least for now, but we
have no objection if other people do so in the future.

Thanks.  Will queue as-is for now.

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 49a328aa8a4..ba5c395d2d8 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2890,6 +2890,10 @@ _git_restore ()
>  	--*)
>  		__gitcomp_builtin restore
>  		;;
> +	*)
> +		if __git rev-parse --verify --quiet HEAD >/dev/null; then
> +			__git_complete_index_file "--modified"
> +		fi
>  	esac
>  }
>  
>
> base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b

[gitgitgadget-git]

This branch is now known as dc/complete-restore.

[gitgitgadget-git]

This patch series was integrated into seen via 4fa859a.

[gitgitgadget-git]

This patch series was integrated into seen via 5630825.

[gitgitgadget-git]

There was a status update in the "New Topics" section about the branch dc/complete-restore on the Git mailing list:

The command line completion support (in contrib/) learns to give
modified paths to the "git restore" command.

Will merge to 'next'?
source: <pull.1227.v3.git.git.1647382437475.gitgitgadget@gmail.com>

[gitgitgadget-git]

This patch series was integrated into seen via a4034df.

[gitgitgadget-git]

This patch series was integrated into seen via 022dacc.

[gitgitgadget-git]

This patch series was integrated into next via 34a4fd8.

[gitgitgadget-git]

On the Git mailing list, David Cantrell wrote (reply to this):

On 16/03/2022 00:44, Junio C Hamano wrote:
> "David Cantrell via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> If no --args are present after 'git restore' it assumes that you want
>> to tab-complete one of the files with unstaged uncommitted changes.
> What do you mean by "--args" in this sentence?  Dashed-options?

Yes.

> I am getting the same set of files whose name begins with 'a' from
> these two:
> >   $ git restore a<TAB>
>   $ git restore --staged a<TAB>
> > so, with or without --dashed-options, we complete one of the files,
> whether they have any modifications.

If --staged is present I think it falls back to the standard behaviour of attempting to complete to any file it can find

> Perhaps you meant to say more like:
> >      When completing a non-option argument to 'git restore', the
>      command line completion support offers names of the files
>      present in the working tree as candidates.
> > to describe the status quo; to hint what the shortcoming of the
> current behaviour is, we may want to add a bit more, perhaps
> append the following at the end of that first paragraph:
> >      But many of these files may not have any changes, and running
>      "restore" on them would be a no-op.  Listing only the files, to
>      which doing "restore" is not a no-op, would reduce the clutter.
>      > Then we'd continue with the solution, while explaining why the exact
> choice between modified vs committable was made:
> >      Offer only the files that are different from the index, to match
>      the default behaviour of "git restore" that checks out the
>      contents last added to the index to the working tree.  We could
>      instead show the files that are different between the index and
>      HEAD, and that is more suittable if "git restore --staged" is
>      being completed, but this should do for now.
> > or something.  The last part is written in such a way to explicitly
> signal to future developers that we know we did not do a perfect job
> and we do not mind if they extend the logic to use something other
> than "--modified" when appropriate.  For example, they could build
> on this solution to make it inspect the command line for "--staged"
> and "--source" and drive "diff-index" differently to grab the paths
> that are offered.  We just do not do that at least for now, but we
> have no objection if other people do so in the future.

That makes sense.

> Thanks.  Will queue as-is for now.

Thank you.

-- 
David Cantrell

[gitgitgadget-git]

This patch series was integrated into seen via 235ddff.

[gitgitgadget-git]

This patch series was integrated into seen via c25eae8.

[gitgitgadget-git]

This patch series was integrated into seen via 3a6509a.

[gitgitgadget-git]

This patch series was integrated into seen via 1f390f2.

[gitgitgadget-git]

This patch series was integrated into master via 1f390f2.

[gitgitgadget-git]

This patch series was integrated into next via 1f390f2.

[gitgitgadget-git]

Closed via 1f390f2.