Re: [PATCH 6/6] completion: clarify ls-tree, archive, show completion

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Jun 7, 2013, 18:45 UTC
Message-ID: <20130607184527.GB25731@goldbirke>
In-Reply-To: <CALkWK0mbVgJHRoNXNkc6n7==-H+caNqvzrzUwspS_Eq2sMfJbg@mail.gmail.com>

On Fri, Jun 07, 2013 at 10:51:53PM +0530, Ramkumar Ramachandra wrote:

Show 7 quoted lines

> SZEDER Gábor wrote:
> > Well, people out there might have completion scriplets for their
> > aliases or custom git commands which use __git_complete_file().
> > Removing this function would break those scripts.
> 
> What is the advantage of using __git_complete_file() over
> __git_complete_revlist_file()?

That it doesn't imply that the command takes refs in the form of ref1..ref2.

> Isn't it just a misleading alias?

No. It's an implementation detail that __git_complete_file() became an alias to __git_complete_revlist_file() to avoid unnecessary code duplication.

And it was a concious decision to keep __git_complete_file() (and __git_complete_revlist()) around in order not to break completion scriplets for users' alieses and custom git commands which might call it.

Show 8 quoted lines

> > Arguably the name of __git_complete_file() could describe better what
> > the function does, or what it did, i.e. it used to provide completion
> > for the master:Doc<TAB> notation.  But that's only the name.  Since
> > both git ls-tree and git archive understand this notation, calling the
> > helper for master:Doc<TAB> in their completion functions is not
> > misleading at all.
> 
> But __git_complete_revlist_file() provides all this and more, no?

Indeed, and this "more" is exactly why it is misleading to call __git_complete_revlist_file() directly for git ls-tree and git archive.

Show 9 quoted lines

> > Now, __git_complete_revlist_file() provides completion both for this
> > master:Doc<TAB> notation and for revision ranges, i.e. for
> > master..n<TAB> and master...n<TAB>.  However, since neither git
> > ls-tree nor git archive accept revision ranges, calling
> > __git_complete_revlist_file() in their completion function would be
> > misleading.
> 
> Yeah, they accept tree-ish'es.  Isn't __git_complete_file() still a
> horrible name?

We can't go back in time to correct it, unfortunately.

Show 5 quoted lines

> If anything, we
> should write a new __git_complete_treeish() function that does what
> __git_complete_revlist_file() does, except that it doesn't complete
> revision ranges, right?  Frankly, I don't know if it's worth the
> additional trouble

I agree that it isn't worth it, and that is exactly why __git_complete_revlist() and __git_complete_file() were unified in __git_complete_revlist_file().

>   $ git log HEAD:Doc<TAB>
> 
> Note how log doesn't even error out.

But note how git log master..HEAD:Documentation/ errors out.

Show 6 quoted lines

> > git show is special, as it understands both the master:Doc<TAB>
> > notation and revision ranges, and even the combination of the two, so
> > calling __git_complete_revlist_file() there would indeed be better.
> 
> It just accepts any revspec with pathspec filtering, like many many
> other commands.

Which many many other commands do accept ref1..ref2:file?

Gábor