Re: [PATCH/WIP] completion: complete git diff with only changed files.

From: Paul Ebermann <paul-ebermann@gmx.de>
Date: May 18, 2011, 13:22 UTC
Message-ID: <4DD3C814.8000100@gmx.de>
In-Reply-To: <7v8vu4efvj.fsf@alter.siamese.dyndns.org>

Junio C Hamano skribis:

Show 18 quoted lines

> Paul Ebermann <Paul-Ebermann@gmx.de> writes:
> 
>> I ported this idea to git. Now
>>    git diff -- <tab>
>> will complete any changed files. It also works for the other ways
>> of calling git diff, except the .. and ... notations (as I'm
>> not sure how to do this).
> 
> Very interesting.
> 
> I would be a bit dissapointed if this change makes
> 
> 	git diff maint master -- builtin/lo<TAB>
> 
> not complete for me when builtin/log.c did not change between these two
> branches, as running between different maintenance tracks and the master
> branch to see how far things have diverged is my first quick sanity check
> before deciding where in the history a new patch should be queued.

In fact, it does complete. (Even if there is no maint branch like in my clone.) There seems to be a fallback to filename completion if the completion list is empty. It takes quite longer than before, though. And it does not give a list of all files if there are changed files starting with the prefix typed.

Show 15 quoted lines

> Spending cycles to keep me waiting while running "diff" before giving me
> the control back, and implicitly telling me by not telling me that lo...
> is a completion candidate, would surely make me go "Huh? Is it being slow?
> Hello? <TAB> <TAAAB> <TAAAAAAAB> ... hmm, did I mistype the directory
> name?  B-U-I-L-T-I-N ... that's correct.... what's going on?  ah wait a
> minute, we recently applied that stupid change that tells me what I really
> want to know by not telling me, which is backwards."
>
> That's already 12 seconds wasted, and worse yet, even after I learn the
> quirk of the new behaviour that tells what I want to know by not telling
> me, the need to make sure that I didn't mistype the directory name would
> not disappear.
>
> I would rather not to be retrained to wire my brain backwards in the first
> place.

Okay, this seems to depend on the usage pattern.

I'm normally using (for differences to head) git status first, and then have a look at the files I really want to see. Then completion of only the changed files seems useful.

I must confess that I seldomly use
    git diff <commit> <commit> -- <path>
at all, while it seems to be an important tool for maintainers of projects
with lots of contributors (like you).

What about having this completion only apply for the no-commit-given case
(i.e. `git diff -- <path>`), and using the normal file-based completion
in the case of a given <commit>?
(But shouldn't we complete for files existent in one of the comparison ends
 instead of in the working tree?)

I also thought about somehow caching the results to make it faster, but the problem with this is that this will change after each commit, and also between the commits with every edit to a file.

Show 17 quoted lines

>> +    local -a args=()
>> +    local finish=false
>> +
>> +    for (( i=1 ; i < cword ; i++)) do
>> +    local current_arg=${words[$i]}
>> +    #  echo checking $current_arg >&2
>> +       case $current_arg in
>> +           --cached)
> 
> case arms align with case and esac in our codebase, i.e.
> 
>         case $current_arg in
>         --cached)
>         	...
>                 ;;
>                 ...
>         esac

Okay. (I simply used what my Emacs auto-indented.) I will change this if I submit another patch.

Show 11 quoted lines

>> +           *)
>> +               if git cat-file -e $current_arg 2> /dev/null
>> +               then
>> +                   case $( git cat-file -t $current_arg ) in
> 
> I do not see the need for the outer if/then/fi here. Wouldn't this
> sufficient?
> 
> 		case "$(git cat-file -t $current_arg 2>/dev/null)" in
> 		commit|tag)
>                 	...

Seems like it. Looks like I was reading the documentation of -e, which said "Suppress all output; instead exit with zero status if <object> exists and is a valid object.", and thus I thought I could avoid using the redirection at all - I then added it when it showed that the redirect still was necessary.

Yes, your version works, too.

Show 11 quoted lines

> If you are interested in dealing with ../... notation, you could instead
> use "git rev-parse --revs-only --no-flags", e.g.
> 
> 	$ git rev-parse --revs-only --no-flags maint..master
>         b602ed7dea968d72c5b3f61ca016de7f285d80ef
> 	^ea1ab4b280ed3b041da53e161e32e38930569f3e
> 
>         $ git rev-parse --revs-only --no-flags jch...pu
>         11b715c624d3766546a52cc333bc2ea2e426f631
>         14f92e20522bae26faa841374bbbe6c0d08770de
>         ^14f92e20522bae26faa841374bbbe6c0d08770de

The reason for using cat-file instead of rev-parse was the ability to distinguish between commits and blobs (for example).

	$ git rev-parse --revs-only --no-flags c4d58da4e
	c4d58da4e9050c6330ff145914cc379f0600f703

(This is zlib.c in the current master, and the exit code is still 0. Using the git 1.7.1 binaries on OpenSUSE, I did not compile the current master code.)

I'm not sure this is really necessary, as I wrote before:

Show 6 quoted lines

>> I'm checking the non-option arguments on being commits (or tags), and pass
>> only the matching ones to the nested `git diff` call.
>> It might be easier to ommit this check and pass everything that does not
>> start with a `-`. Then it would also easily work for the .. and ... syntax,
>> I think.
>> Opinions?

Thus I could simply write here

	case $current_arg in
	...
	*)
		args+=( $current_arg )
	esac

instead. If someone supplies something non-commitish, I don't have to care about it. It would also catch any non-option arguments, but it looks like there are none of these (apart from the commits).

> But I tend to think this change itself is not such a great idea to begin
> with, so....

Yeah, I understood this.

If this is not accepted, I'll publish it separately for the ones who like it (not as a patch, but as a separate bash file which you could source after the main comments.) Still thanks for your comments.

Paŭlo