patch, 6 partsMinor prompt, completion cleanups

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

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

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.

On Mon, Jun 03, 2013 at 03:17:27PM +0530, Ramkumar Ramachandra 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:

Jeff King wrote:

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?

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

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

Jeff King <peff@peff.net> writes:

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.

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

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.
 

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(+)

Ramkumar Ramachandra wrote:

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

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(+)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

[...]

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

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

On Mon, Jun 03, 2013 at 03:02:00PM +0530, Ramkumar Ramachandra wrote:

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

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

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.

Ramkumar Ramachandra:

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?

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

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

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

On Sun, Jun 2, 2013 at 9:03 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

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

Oh, ouch.

Never mind then.  Drop this patch.

'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(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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?

Junio C Hamano wrote:

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.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

On Sun, Jun 02, 2013 at 07:33:42PM +0530, Ramkumar Ramachandra 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.

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

SZEDER Gábor wrote:

What is the advantage of using __git_complete_file() over __git_complete_revlist_file()? Isn't it just a misleading alias?

But __git_complete_revlist_file() provides all this and more, no?

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.

It just accepts any revspec with pathspec filtering, like many many other commands.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

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

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

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.

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

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

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

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

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

Gábor

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

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