public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Manlio Perillo <manlio.perillo@gmail•com>
Cc: git@vger•kernel.org, szeder@ira•uka.de, felipe.contreras@gmail•com
Subject: Re: [PATCH v4] git-completion.bash: add support for path completion
Date: Fri, 21 Dec 2012 09:59:17 -0800	[thread overview]
Message-ID: <7vmwx71e2y.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <1356108872-5881-1-git-send-email-manlio.perillo@gmail.com> (Manlio Perillo's message of "Fri, 21 Dec 2012 17:54:32 +0100")

Manlio Perillo <manlio.perillo@gmail•com> writes:

> +		case "$path" in
> +		?*/*) echo "${path%%/*}/" ;;
> +		*) echo $path ;;

$path unquoted???

> +# __git_index_files accepts 1 or 2 arguments:
> +# 1: Options to pass to ls-files (required).
> +#    Supported options are --cached, --modified, --deleted, --others,
> +#    and --directory.
> +# 2: A directory path (optional).
> +#    If provided, only files within the specified directory are listed.
> +#    Sub directories are never recursed.  Path must have a trailing
> +#    slash.
> +__git_index_files ()
> +{
> +	local dir="$(__gitdir)"
> +
> +	if [ -d "$dir" ]; then
> +		# NOTE: $1 is not quoted in order to support multiple options

Good thinking to document this.  Thanks.

I take it that $1 never comes from the end user and it is known that
it is correct to split them at $IFS?  That is the way I read callers
of this function in this patch, but I am just double-checking.

> @@ -998,7 +1093,13 @@ _git_commit ()
>  			"
>  		return
>  	esac
> -	COMPREPLY=()
> +
> +	if git rev-parse --verify --quiet HEAD 1>/dev/null; then

s/1>/>/;

> +		__git_complete_diff_index_file "HEAD"

As this runs "git diff-index" without --cached, 

The completion will give only for paths that have difference between
the working tree and the HEAD.  If the user has a bogus contents
that was "git add"ed earlier, (i.e. the index is different from
HEAD), then realizes the mistake and fixes it in the working tree
with his editor to match "HEAD" (i.e. the working tree is the same
as HEAD):

	git commit the-prefix-to-that-file<TAB>

to complete the filename will not give that file.  I do not think it
is a show-stopper, but it may puzzle the users when they encounter
the situation.

I am wondering if reading from "git status --porcelain" might be a
better alternative, or if it is too much trouble and slow things
down to cover such a corner case.

> @@ -1362,7 +1464,14 @@ _git_mv ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +
> +	if [ $cword -gt 2 ]; then
> +		# We need to show both cached and untracked files (including
> +		# empty directories) since this may not be the last argument.
> +		__git_complete_index_file "--cached --others --directory"
> +	else
> +		__git_complete_index_file "--cached"
> +	fi

Is $cword affected by the presense of "-f" in "git mv [-f] foo bar"?
Just being curious.

Other than that, I do not see anything majorly wrong from the coding
and semantics point of view in the patch.  As to the interaction
with the rest of the completion machinery, I'll leave the review to
the area experts CC'ed and wait for their comments.

Thanks.

next prev parent reply	other threads:[~2012-12-21 17:59 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-21 16:54 [PATCH v4] git-completion.bash: add support for path completion Manlio Perillo
2012-12-21 17:59 ` Junio C Hamano [this message]
2012-12-21 19:02   ` Manlio Perillo
2013-01-04 21:25 ` Marc Khouzam
2013-01-04 23:20   ` Junio C Hamano
2013-01-05  6:27     ` Junio C Hamano
2013-01-05 20:23       ` Marc Khouzam
2013-01-05 21:27         ` Manlio Perillo
2013-01-06 18:39         ` Manlio Perillo
2013-01-07 13:43         ` Manlio Perillo
2013-01-07 15:26           ` Marc Khouzam
2013-01-08 17:54         ` Manlio Perillo
2013-01-08 18:05           ` John Keeping
2013-01-08 18:28             ` Manlio Perillo
2013-01-06 18:00       ` Manlio Perillo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7vmwx71e2y.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox$(echo .)com \
    --cc=felipe.contreras@gmail$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=manlio.perillo@gmail$(echo .)com \
    --cc=szeder@ira$(echo .)uka.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox