[PATCH 0/6] Minor prompt, completion cleanups

[PATCH 0/6] Minor prompt, completion cleanups

[Ramkumar Ramachandra]

Another lazy Sunday afternoon.  All of these are trivial.

Thanks.

Ramkumar Ramachandra (6):
  prompt: don't scream continuation state
  completion: add common options for rev-parse
  completion: add common options for blame
  completion: correct completion for format-patch
  completion: clarify difftool completion
  completion: clarify ls-tree, archive, show completion

 contrib/completion/git-completion.bash | 37 +++++++++++++++++++++++++---------
 contrib/completion/git-prompt.sh       | 18 ++++++++---------
 2 files changed, 36 insertions(+), 19 deletions(-)

-- 
1.8.3.457.g2410d5e

[PATCH 1/6] prompt: don't scream continuation state

[Ramkumar Ramachandra]

Currently, when performing any operation that saves the state and
expects the user the continue (like rebase, bisect, am), the prompt
screams:

  artagnon|completion|REBASE-i 2/2:~/src/git$

Lowercase the words, so we get a more pleasant

  artagnon|completion|rebase-i 2/2:~/src/git$

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-prompt.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index eaf5c36..f99d1f2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -270,29 +270,29 @@ __git_ps1 ()
 			step=$(cat "$g/rebase-merge/msgnum")
 			total=$(cat "$g/rebase-merge/end")
 			if [ -f "$g/rebase-merge/interactive" ]; then
-				r="|REBASE-i"
+				r="|rebase-i"
 			else
-				r="|REBASE-m"
+				r="|rebase-m"
 			fi
 		else
 			if [ -d "$g/rebase-apply" ]; then
 				step=$(cat "$g/rebase-apply/next")
 				total=$(cat "$g/rebase-apply/last")
 				if [ -f "$g/rebase-apply/rebasing" ]; then
-					r="|REBASE"
+					r="|rebase"
 				elif [ -f "$g/rebase-apply/applying" ]; then
-					r="|AM"
+					r="|am"
 				else
-					r="|AM/REBASE"
+					r="|am/rebase"
 				fi
 			elif [ -f "$g/MERGE_HEAD" ]; then
-				r="|MERGING"
+				r="|merge"
 			elif [ -f "$g/CHERRY_PICK_HEAD" ]; then
-				r="|CHERRY-PICKING"
+				r="|cherry-pick"
 			elif [ -f "$g/REVERT_HEAD" ]; then
-				r="|REVERTING"
+				r="|revert"
 			elif [ -f "$g/BISECT_LOG" ]; then
-				r="|BISECTING"
+				r="|bisect"
 			fi
 
 			b="$(git symbolic-ref HEAD 2>/dev/null)" || {
-- 
1.8.3.457.g2410d5e

Re: [PATCH 1/6] prompt: don't scream continuation state

[Thomas Rast]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Currently, when performing any operation that saves the state and
> expects the user the continue (like rebase, bisect, am), the prompt
> screams:
>
>   artagnon|completion|REBASE-i 2/2:~/src/git$
>
> Lowercase the words, so we get a more pleasant
>
>   artagnon|completion|rebase-i 2/2:~/src/git$

So I'm not sure whether this falls under bikeshedding or actual
features, but I like the screaming.  When I first saw these displays, I
thought that the point was that this display is more important than all
the others, so it should be louder and easy to distinguish from the
branch name (common ones, anyway).

Do you have other ways of distinguishing the branch and the state?
Colors?  I'm a bit too lazy to check.  Perhaps it could be made to only
use caps if not in colored mode?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 1/6] prompt: don't scream continuation state

[Ramkumar Ramachandra]

Thomas Rast wrote:
> Do you have other ways of distinguishing the branch and the state?
> Colors?  I'm a bit too lazy to check.  Perhaps it could be made to only
> use caps if not in colored mode?

Currently, no.   See git-prompt.sh:401, 403, 409; we don't have a
separate color for $r.  I didn't introduce a color in this series
because it would conflict with rr/zsh-color-prompt which is in pu: we
can introduce it after that graduates.  I was thinking yellow, since
that's not taken?

You really should use colors.  I don't think it's worth the extra
ugliness to scream in the no-color case.

As such, the prompt is a fine bikeshedding target.  I even introduced
GIT_PS1_SEPARATOR, because some people were unhappy with me stripping
one whitespace (yes, one).  If we go down the configurability road,
we'll either end up with a ton of environment variables, or be made to
write a generalized custom formatter which splices together tons of
arguments using color (super painful).  While I do agree that it is a
matter of taste, we have to make a few compromises for the sake of
sanity.

Re: [PATCH 1/6] prompt: don't scream continuation state

[Jeff King]

On Mon, Jun 03, 2013 at 03:17:27PM +0530, Ramkumar Ramachandra wrote:

> Thomas Rast wrote:
> > Do you have other ways of distinguishing the branch and the state?
> > Colors?  I'm a bit too lazy to check.  Perhaps it could be made to only
> > use caps if not in colored mode?
> 
> Currently, no.   See git-prompt.sh:401, 403, 409; we don't have a
> separate color for $r.  I didn't introduce a color in this series
> because it would conflict with rr/zsh-color-prompt which is in pu: we
> can introduce it after that graduates.  I was thinking yellow, since
> that's not taken?
> 
> You really should use colors.  I don't think it's worth the extra
> ugliness to scream in the no-color case.
> 
> As such, the prompt is a fine bikeshedding target.  I even introduced
> GIT_PS1_SEPARATOR, because some people were unhappy with me stripping
> one whitespace (yes, one).  If we go down the configurability road,
> we'll either end up with a ton of environment variables, or be made to
> write a generalized custom formatter which splices together tons of
> arguments using color (super painful).  While I do agree that it is a
> matter of taste, we have to make a few compromises for the sake of
> sanity.

It seems silly to argue about output formats when we are writing a
prompt in a convenient Turing-complete scripting language already.
What about something like:

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index eaf5c36..1da3833 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -404,6 +404,8 @@ __git_ps1 ()
 			fi
 			gitstring=$(printf -- "$printf_format" "$gitstring")
 			PS1="$ps1pc_start$gitstring$ps1pc_end"
+		elif test "$(type -t __git_ps1_printer)" = "function"; then
+			__git_ps1_printer
 		else
 			# NO color option unless in PROMPT_COMMAND mode
 			printf -- "$printf_format" "$c${b##refs/heads/}${f:+ $f}$r$p"

and then you can define your own custom function if you like, which will
inherit all of the variables from __git_ps1. E.g.:

  __git_ps1_printer() {
          echo "${r,,}"
  }

yields a lower-case "rebase-i". Of course there are many other variables
you want to put in your prompt, too. The only problem I see is that the
current set of variables is not suitable as a user-visible interface.
The names are not good (the branch should be "$branch" rather than "$b",
the operation-in-progress should be "$operation" or similar rather than
"$r", etc), and the formats are not as convenient ("$r" contains the
full "|REBASE-i 6/10", whereas it should be broken down so the printing
function can easily use a case statement).

And of course the variables would need documentation.

-Peff

Re: [PATCH 1/6] prompt: don't scream continuation state

[Ramkumar Ramachandra]

Jeff King wrote:
> It seems silly to argue about output formats when we are writing a
> prompt in a convenient Turing-complete scripting language already.
> What about something like:

Could you have a look at __git_ps1_colorize_gitstring from
rr/zsh-color-prompt in pu?  In the general case, wouldn't the user
need to re-implement this entire function (with so many variables) in
her ~/.zshrc?  Isn't it horribly painful for too little gain?

Re: [PATCH 1/6] prompt: don't scream continuation state

[Jeff King]

On Tue, Jun 04, 2013 at 09:14:23AM +0530, Ramkumar Ramachandra wrote:

> Jeff King wrote:
> > It seems silly to argue about output formats when we are writing a
> > prompt in a convenient Turing-complete scripting language already.
> > What about something like:
> 
> Could you have a look at __git_ps1_colorize_gitstring from
> rr/zsh-color-prompt in pu?  In the general case, wouldn't the user
> need to re-implement this entire function (with so many variables) in
> her ~/.zshrc?  Isn't it horribly painful for too little gain?

It is a lot of ugly code, but then, complexity and flexibility are a
tradeoff. You would not _have_ to use a custom function at all. But if
we make it an option, and leave the existing code as the default
function, then only those who want it have to use it.

Even better, we can hit a middle ground by abstracting away some of the
complexity.  You could probably write colorize() function to handle
colorizing a particular string (and handle bash/zsh magic), and then
pull the whole "which color is the branch" function into a
colorize_branch(), and so forth.

I haven't looked that closely, so there may be hidden troubles awaiting,
but I don't see why you couldn't ultimately let the user write something
like:

  __git_ps1_printer() {
          echo ' ('
          test "$bare" = t && echo "BARE:"; # or "bare:" :)
          color_branch "$(short_ref $branch)" "$detached"
          test "$wt_changes" = t && colorize red "*"
          ...
          echo ')'
  }

And if that is too much, you can abstract bits of it out. The point is
that if we can expose the lowest-level details, we can compose functions
around them at whatever level is appropriate, and callers can mix and
match for the balance of complexity and flexibility they want.

Perhaps it is overkill, but I also think it may make the code itself
more readable. For example, I put a "..." above because the next line
would handle this:

  if [ -n "$i" ]; then
          gitstring="$gitstring\[$ok_color\]$i"
  fi

I have no clue what that does, or what the global variable "$i"
contains. Reading through __git_ps1, it looks like it will be "+" for
index changes. Being able to write:

  test "$index_changes" = t && colorize green "+"

seems much more readable to me. And we could probably compose a:

  colorize_diff_state() {
          test "$wt_changes" = t && colorize red "*"
          test "$index_changes" = t && colorize green "+"
  }

for people who just want to take the default colors and characters.

-Peff

Re: [PATCH 1/6] prompt: don't scream continuation state

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Even better, we can hit a middle ground by abstracting away some of the
> complexity.

The latter half of __git_ps1 is already fairly nice; w/i/s/u/c/p and
friends can serve as the basis of such an abstraction, even though r
does want to be separated further.

I tried the GIT_PS1_SHOW* variables several times, but every time I
did so, I eventually had to give up because I simply couldn't read
and/or remember what */+#/$/% line noises meant.  If we had a custom
"collect these pieces and form the final presentation" mechanism, I
could easily replace these with mnemonic letters that can be more
easily memorable.

Re: [PATCH 1/6] prompt: don't scream continuation state

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Currently, when performing any operation that saves the state and
>> expects the user the continue (like rebase, bisect, am), the prompt
>> screams:
>>
>>   artagnon|completion|REBASE-i 2/2:~/src/git$
>>
>> Lowercase the words, so we get a more pleasant
>>
>>   artagnon|completion|rebase-i 2/2:~/src/git$
>
> So I'm not sure whether this falls under bikeshedding or actual
> features, but I like the screaming.

Sounds like bikeshedding.

My personal opinion is that "detached in the middle of something" is
not a normal state, and it deserves to be in bold as a reminder.
Let's leave it as-is.
 

[PATCH 2/6] completion: add common options for rev-parse

[Ramkumar Ramachandra]

Add support for completing 'git rev-parse'.  List only the options that
are likely to be used on the command-line, as opposed to scripts.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-completion.bash | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1c35eef..139f48e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2299,6 +2299,17 @@ _git_reset ()
 	__gitcomp_nl "$(__git_refs)"
 }
 
+_git_rev_parse ()
+{
+	case "$cur" in
+	--*)
+		__gitcomp "--show-toplevel --is-inside-work-tree --symbolic-full-name --verify"
+		return
+		;;
+	esac
+	__gitcomp_nl "$(__git_refs)"
+}
+
 _git_revert ()
 {
 	case "$cur" in
-- 
1.8.3.457.g2410d5e

Re: [PATCH 2/6] completion: add common options for rev-parse

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Add support for completing 'git rev-parse'.  List only the options that
> are likely to be used on the command-line, as opposed to scripts.

Can we get this patch?  I use git rev-list quite a lot, and want completion.

[PATCH 3/6] completion: add common options for blame

[Ramkumar Ramachandra]

Add support for completing 'git blame'.  List only the common short
options.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-completion.bash | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 139f48e..a003b81 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1034,6 +1034,17 @@ _git_bisect ()
 	esac
 }
 
+_git_blame ()
+{
+	__git_has_doubledash && return
+
+	case "$cur" in
+	-*)
+		__gitcomp "-M -C -L -s -w"
+	esac
+	__git_complete_revlist_file
+}
+
 _git_branch ()
 {
 	local i c=1 only_local_ref="n" has_r="n"
-- 
1.8.3.457.g2410d5e

Re: [PATCH 3/6] completion: add common options for blame

[Thomas Rast]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Add support for completing 'git blame'.  List only the common short
> options.
[...]
> +_git_blame ()
> +{
> +	__git_has_doubledash && return
> +
> +	case "$cur" in
> +	-*)
> +		__gitcomp "-M -C -L -s -w"
> +	esac
> +	__git_complete_revlist_file
> +}

Is this the first time we introduce completion (I guess you could call
it "help") for short options?  I only did a quick search for /-. -/ but
it certainly seems that way.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 3/6] completion: add common options for blame

[Ramkumar Ramachandra]

Thomas Rast wrote:
> Is this the first time we introduce completion (I guess you could call
> it "help") for short options?  I only did a quick search for /-. -/ but
> it certainly seems that way.

Yeah.  We generally prefer the long-form equivalents while doing
completions, but these blame options do not have equivalent
long-forms.

Re: [PATCH 3/6] completion: add common options for blame

[SZEDER Gábor]

On Mon, Jun 03, 2013 at 03:02:00PM +0530, Ramkumar Ramachandra wrote:
> Thomas Rast wrote:
> > Is this the first time we introduce completion (I guess you could call
> > it "help") for short options?  I only did a quick search for /-. -/ but
> > it certainly seems that way.
> 
> Yeah.  We generally prefer the long-form equivalents while doing
> completions, but these blame options do not have equivalent
> long-forms.

But providing short options for completion is pointless.  Those who
know git blame's short options will just type them right away, because
it requires less key presses than using completion, and those who
don't know these options will only see -C, -M, etc. and won't have any
clues what it stands for.


Best,
Gábor

Re: [PATCH 3/6] completion: add common options for blame

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> But providing short options for completion is pointless.  Those who
> know git blame's short options will just type them right away, because
> it requires less key presses than using completion, and those who
> don't know these options will only see -C, -M, etc. and won't have any
> clues what it stands for.


It is not necessarily true that "they won't have any clue"; one
could argue that "-C -M etc" can serve as reminder to people who
once learned but forgot momentarily.

My conclusion is the same, though; "git blame -h" is for those
people.

Re: [PATCH 3/6] completion: add common options for blame

[Peter Krefting]

Ramkumar Ramachandra:

> Yeah.  We generally prefer the long-form equivalents while doing 
> completions, but these blame options do not have equivalent 
> long-forms.

Perhaps that is the real bug, then. -M and -C already have long names 
for diff (and its friends), perhaps blame should have the same long 
option names for them?

-- 
\\// Peter - http://www.softwolves.pp.se/

Re: [PATCH 3/6] completion: add common options for blame

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Add support for completing 'git blame'.  List only the common short
>> options.
> [...]
>> +_git_blame ()
>> +{
>> +	__git_has_doubledash && return
>> +
>> +	case "$cur" in
>> +	-*)
>> +		__gitcomp "-M -C -L -s -w"
>> +	esac
>> +	__git_complete_revlist_file
>> +}
>
> Is this the first time we introduce completion (I guess you could call
> it "help") for short options?  I only did a quick search for /-. -/ but
> it certainly seems that way.

Yeah, I do not think this is a good idea.

[PATCH 4/6] completion: correct completion for format-patch

[Ramkumar Ramachandra]

Currently, the completion for 'git format-patch' uses
__git_complete_revlist.  Although this is technically correct, and you
can

  $ git format-patch master contrib

where master is a ref and contrib is a pathspec, just like in 'git log',
the usage is unidiomatic and undocumented.  'git format-patch' is used
without pathspec filtering most of the time, and it makes sense to
provide sensible completions using __git_refs.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a003b81..f46964d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1347,7 +1347,7 @@ _git_format_patch ()
 		return
 		;;
 	esac
-	__git_complete_revlist
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_fsck ()
-- 
1.8.3.457.g2410d5e

Re: [PATCH 4/6] completion: correct completion for format-patch

[Felipe Contreras]

On Sun, Jun 2, 2013 at 9:03 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Currently, the completion for 'git format-patch' uses
> __git_complete_revlist.  Although this is technically correct, and you
> can
>
>   $ git format-patch master contrib
>
> where master is a ref and contrib is a pathspec, just like in 'git log',
> the usage is unidiomatic and undocumented.  'git format-patch' is used
> without pathspec filtering most of the time, and it makes sense to
> provide sensible completions using __git_refs.

This breaks 'git format-patch master..<TAB>'.

Moreover, this is a perfectly fine usage of 'git format-patch':

% git format-patch --full-diff master..fc/remote/hg-next --
contrib/remote-helpers/git-remote-bzr

Plus, even even with your patch 'contrib' will be completed regardless
(by default completion), wouldn't it?

NAK.

-- 
Felipe Contreras

Re: [PATCH 4/6] completion: correct completion for format-patch

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> This breaks 'git format-patch master..<TAB>'.

Oh, ouch.

> Moreover, this is a perfectly fine usage of 'git format-patch':
>
> % git format-patch --full-diff master..fc/remote/hg-next --
> contrib/remote-helpers/git-remote-bzr

Never mind then.  Drop this patch.

[PATCH 5/6] completion: clarify difftool completion

[Ramkumar Ramachandra]

'git difftool' is clearly a frontend to 'git diff' and is used in
exactly the same way, but it uses a misleading completion function name
__git_complete_file (aliased to to __git_complete_revlist_file).  Change
it to use __git_complete_revlist_file, just like 'git diff'.  No
functional changes.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f46964d..8d70c30 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1305,7 +1305,7 @@ _git_difftool ()
 		return
 		;;
 	esac
-	__git_complete_file
+	__git_complete_revlist_file
 }
 
 __git_fetch_options="
-- 
1.8.3.457.g2410d5e

Re: [PATCH 5/6] completion: clarify difftool completion

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 'git difftool' is clearly a frontend to 'git diff' and is used in
> exactly the same way, but it uses a misleading completion function name
> __git_complete_file (aliased to to __git_complete_revlist_file).  Change
> it to use __git_complete_revlist_file, just like 'git diff'.  No
> functional changes.

Makes sense.  The completion helper for both revname and pathname
should not be called __git_complete_file, and this is a step in the
right direction.

If we have a situation where we _know_ we only want to complete
pathname and never revname, we may want to keep __git_complete_file
function.  For example, after seeing "--" on the command line, we
may want to use __git_complete_file (that does not look at revs,
instead of __git_complete_revlist_file function.

Will apply.

Thanks.

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index f46964d..8d70c30 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1305,7 +1305,7 @@ _git_difftool ()
>  		return
>  		;;
>  	esac
> -	__git_complete_file
> +	__git_complete_revlist_file
>  }
>  
>  __git_fetch_options="

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

[Ramkumar Ramachandra]

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 <artagnon@gmail.com>
---
 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 ()
-- 
1.8.3.457.g2410d5e

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@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 <artagnon@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 ()

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

[Ramkumar Ramachandra]

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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@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.

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

[SZEDER Gábor]

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

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.

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.


Gábor

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

[Ramkumar Ramachandra]

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

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

  $ 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:Doc<TAB>

Note how log doesn't even error 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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

None.

But "Those who have been using it can update their script" is not an
answer you give when dealing with a regression.

I think the right thing is to add __git_complete_file() back in;
perhaps strip support of A..B/A...B range notations from it, but
that is an independent change.

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

[SZEDER Gábor]

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

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

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

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

ohh, I missed this part, and you are right.

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