From: SZEDER Gábor <szeder@ira.uka.de>
Date: Fri, 07 Jun 2013 18:45:27 GMT
Subject: Re: [PATCH 6/6] completion: clarify ls-tree, archive, show completion
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:
> 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.

> > 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.

> > 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.

> 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.

> > 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