Re: bash completion of "addable" files for `git add`

[Jeff King <peff@peff.net>]

On Fri, Nov 09, 2012 at 02:22:27PM +0100, Andreas Zeidler wrote:

> so here's a patch adding bash completion on `git add` for modified,
> updated and untracked files.  i've also set up a pull request — before
> i found `Documentation/SubmittingPatches`.  fwiw, it's at
> https://github.com/git/git/pull/29

Please put cover letter material like this after the "---" divider, or
include a "-- >8 --" scissors line before your commit. Either helps "git
am" realize which content should go into the commit message.

> From cbac6caee7e788569562cb7138eb698cc46a1b8f Mon Sep 17 00:00:00 2001
> From: Andreas Zeidler <az@zitc.de>
> Date: Fri, 9 Nov 2012 13:05:43 +0100

Please omit these lines, as they are redundant with your email header.

> Subject: [PATCH] add bash completion for "addable" files

This one is useful, but it would be even better if it were the subject
of your email. :)

> +__git_addable ()
> +{
> +	local dir="$(__gitdir)"
> +	if [ -d "$dir" ]; then
> +		git --git-dir="$dir" status --short --untracked=all |\
> +			egrep '^.[UM?] ' | sed 's/^.. //'
> +		return
> +	fi
> +}

You would want to use the stable, scriptable "--porcelain" output, so
the completion is not broken by future changes to the "--short" format.
However, I do not think using "git status" is the best option. It
computes three things:

  1. The diff between HEAD and index.

  2. The diff between index and working tree.

  3. The list of untracked files.

But you only care about (2) and (3), so you are wasting time computing
(1).  I think the list you want could be generated with:

  git diff-files --name-only
  git ls-files --exclude-standard -o

and then you do not need to worry about grep or sed at all.

> @@ -810,6 +820,11 @@ _git_add ()
>  			--ignore-errors --intent-to-add
>  			"
>  		return
> +		;;
> +	*)
> +		__gitcomp "$(__git_addable)"
> +		return
> +		;;

I think you'd want to use __gitcomp_nl to handle filenames with spaces.

Speaking of which, the output of status (or of the commands I mentioned)
may have quoting applied to pathnames. I think that is not something we
handle very well right now in the completion, so it may not be worth
worrying about for this particular patch.

Other than those comments, I think the intent of your patch makes a lot
of sense.

-Peff