Re: [PATCH 6/6] completion: clarify ls-tree, archive, show completion
SZEDER Gábor sze...@ira.uka.de writes: Now, __git_complete_revlist_file() provides completion both for this master:DocTAB notation and for revision ranges, i.e. for master..nTAB and master...nTAB. However, since neither git ls-tree nor git archive accept revision ranges, calling __git_complete_revlist_file() in their completion function would be misleading. ohh, I missed this part, and you are right. git show is special, as it understands both the master:DocTAB notation and revision ranges, and even the combination of the two, so calling __git_complete_revlist_file() there would indeed be better. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] completion: clarify ls-tree, archive, show completion
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()? Isn't it just a misleading alias? 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:DocTAB notation. But that's only the name. Since both git ls-tree and git archive understand this notation, calling the helper for master:DocTAB in their completion functions is not misleading at all. But __git_complete_revlist_file() provides all this and more, no? Now, __git_complete_revlist_file() provides completion both for this master:DocTAB notation and for revision ranges, i.e. for master..nTAB and master...nTAB. 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? $ git ls-tree HEAD:Documentation What file? There's already a __git_complete_index_file(), which is properly named and used by the ls-files family. 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: we do spurious completions all over the place, and we haven't clamped down on any of that. $ git log HEAD:DocTAB Note how log doesn't even error out. git show is special, as it understands both the master:DocTAB 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] completion: clarify ls-tree, archive, show completion
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:DocTAB notation. But that's only the name. Since both git ls-tree and git archive understand this notation, calling the helper for master:DocTAB 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:DocTAB notation and for revision ranges, i.e. for master..nTAB and master...nTAB. 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:DocTAB 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:DocTAB 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] completion: clarify ls-tree, archive, show completion
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: I think this is the same as 5/6 and better explained in a single patch, as the rationale is the same: these commands can all take the usual revs and then paths, so using misnamed complete_FILE helper is wrong. Mind if I squashed them together? I'm okay with what you've queued in pu (sans some points raised by SZEDER; looking into that), but you can squash them together if you like. In any case, I think it is sensible not to remove the _file helper. We may want to fix it not to expand revname and do only the pathname, but that is a separate issue from using _revlist_file for the commands you updated completion for in these two patches. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] completion: clarify ls-tree, archive, show completion
Ramkumar Ramachandra artag...@gmail.com writes:
Currently, the 'git ls-tree', 'git archive', and 'git show' completions
use __git_complete_file (aliased to __git_complete_revlist_file).
In the case of 'git ls-tree' and 'git archive', they necessarily require
a tree-ish argument (and optionally a pathspec filter, or file
argument):
$ git ls-tree hot-branch git.c
$ git archive HEAD~4 git.c
So, __git_complete_file is a misleading name.
In the case of 'git show', it can take a pathspec and default the
revision to HEAD like:
$ git show git.c
(which is useful if git.c was modified in HEAD)
However, this usage is not idiomatic at all. The more common usage is
like:
$ git show HEAD~1
$ git show origin/pu:git.c
So, __git_complete_file is again a poor name.
Replace these three instances of __git_complete_file with
__git_complete_revlist_file, without making any functional changes.
Remove __git_complete_file, as it has no other callers.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
I think this is the same as 5/6 and better explained in a single
patch, as the rationale is the same: these commands can all take the
usual revs and then paths, so using misnamed complete_FILE helper is
wrong.
Mind if I squashed them together?
contrib/completion/git-completion.bash | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 8d70c30..84d1548 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -592,11 +592,6 @@ __git_complete_diff_index_file ()
esac
}
-__git_complete_file ()
-{
- __git_complete_revlist_file
-}
-
__git_complete_revlist ()
{
__git_complete_revlist_file
@@ -1007,7 +1002,7 @@ _git_archive ()
return
;;
esac
- __git_complete_file
+ __git_complete_revlist_file
}
_git_bisect ()
@@ -1476,7 +1471,7 @@ _git_ls_remote ()
_git_ls_tree ()
{
- __git_complete_file
+ __git_complete_revlist_file
}
# Options that go well for log, shortlog and gitk
@@ -2382,7 +2377,7 @@ _git_show ()
return
;;
esac
- __git_complete_file
+ __git_complete_revlist_file
}
_git_show_branch ()
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] completion: clarify ls-tree, archive, show completion
Ramkumar Ramachandra artag...@gmail.com writes: Currently, the 'git ls-tree', 'git archive', and 'git show' completions use __git_complete_file (aliased to __git_complete_revlist_file). In the case of 'git ls-tree' and 'git archive', they necessarily require a tree-ish argument (and optionally a pathspec filter, or file argument): $ git ls-tree hot-branch git.c $ git archive HEAD~4 git.c So, __git_complete_file is a misleading name. In the case of 'git show', it can take a pathspec and default the revision to HEAD like: $ git show git.c (which is useful if git.c was modified in HEAD) However, this usage is not idiomatic at all. More idiomatic is after seeing git show --stat doing git show dir/. But I do not think it really matters. Just like diff and difftool, show optionally takes revs and paths, and because the current completion code does not do anything fancy like we have seen enough revs and there will appear no revs hence we only complete paths from here on, complete_revlist_file is the right helper to use for all of them. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] completion: clarify ls-tree, archive, show completion
On Sun, Jun 02, 2013 at 07:33:42PM +0530, Ramkumar Ramachandra wrote: Currently, the 'git ls-tree', 'git archive', and 'git show' completions use __git_complete_file (aliased to __git_complete_revlist_file). In the case of 'git ls-tree' and 'git archive', they necessarily require a tree-ish argument (and optionally a pathspec filter, or file argument): $ git ls-tree hot-branch git.c $ git archive HEAD~4 git.c So, __git_complete_file is a misleading name. In the case of 'git show', it can take a pathspec and default the revision to HEAD like: $ git show git.c (which is useful if git.c was modified in HEAD) However, this usage is not idiomatic at all. The more common usage is like: $ git show HEAD~1 $ git show origin/pu:git.c So, __git_complete_file is again a poor name. Replace these three instances of __git_complete_file with __git_complete_revlist_file, without making any functional changes. Remove __git_complete_file, as it has no other callers. 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. 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:DocTAB notation. But that's only the name. Since both git ls-tree and git archive understand this notation, calling the helper for master:DocTAB in their completion functions is not misleading at all. Now, __git_complete_revlist_file() provides completion both for this master:DocTAB notation and for revision ranges, i.e. for master..nTAB and master...nTAB. However, since neither git ls-tree nor git archive accept revision ranges, calling __git_complete_revlist_file() in their completion function would be misleading. git show is special, as it understands both the master:DocTAB notation and revision ranges, and even the combination of the two, so calling __git_complete_revlist_file() there would indeed be better. Gábor -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
