[PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

Hi,

Maybe it should be checked if FETCH_HEAD, ORIG_HEAD and
MERGE_HEAD exist before completing them, but I'm not sure how to
that.

Simon
---
 Completion/Unix/Command/_git |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index d7570cc..dbc3dae 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -3125,7 +3125,7 @@ __git_heads () {
   branch_names=(${${(f)"$(_call_program headrefs git for-each-ref --format='"%(refname)"' refs/heads refs/remotes 2>/dev/null)"}#refs/(heads|remotes)/})
   __git_command_successful || return

-  _wanted heads expl branch-name compadd $* - $branch_names HEAD
+  _wanted heads expl branch-name compadd $* - $branch_names HEAD FETCH_HEAD ORIG_HEAD MERGE_HEAD
 }

 (( $+functions[__git_tags] )) ||
-- 
1.7.0.2

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Benjamin R. Haskell]

On Sun, 21 Mar 2010, Simon Ruderich wrote:

> Hi,
> 
> Maybe it should be checked if FETCH_HEAD, ORIG_HEAD and
> MERGE_HEAD exist before completing them, but I'm not sure how to
> that.
> 
> Simon

This version checks that rev-parse returns something valid for each of 
those three 'symbolic ref names' (as git-rev-parse(1) calls them), plus 
'HEAD' itself (which isn't a valid name on a newly-init'ed repository).

It could probably be improved via the use of _call_program rather than 
calling 'git rev-parse' directly (the same way branch_names uses 
'headrefs' to allow for zstyle niceness, I think), but I don't fully 
grok that yet.

Best,
Ben

---
 Completion/Unix/Command/_git |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index d7570cc..730a952 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -3119,13 +3119,18 @@ __git_tag_ids () {
 
 (( $+functions[__git_heads] )) ||
 __git_heads () {
-  local expl
+  local expl head symbolic_heads
   declare -a branch_names
 
   branch_names=(${${(f)"$(_call_program headrefs git for-each-ref --format='"%(refname)"' refs/heads refs/remotes 2>/dev/null)"}#refs/(heads|remotes)/})
   __git_command_successful || return
 
-  _wanted heads expl branch-name compadd $* - $branch_names HEAD
+  symbolic_heads=()
+  for head in HEAD ORIG_HEAD FETCH_HEAD MERGE_HEAD ; do
+    git rev-parse $head &>/dev/null && symbolic_heads+=( $head )
+  done
+
+  _wanted heads expl branch-name compadd $* - $branch_names $symbolic_heads
 }
 
 (( $+functions[__git_tags] )) ||
-- 
1.7.0

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Nikolai Weibull]

On Sun, Mar 21, 2010 at 20:48, Benjamin R. Haskell <zsh@benizi.com> wrote:

> +  symbolic_heads=()
> +  for head in HEAD ORIG_HEAD FETCH_HEAD MERGE_HEAD ; do
> +    git rev-parse $head &>/dev/null && symbolic_heads+=( $head )
> +  done

That’s a lot of forking to make a simple check.  Is there no better
way?  Do we actually have to filter the heads?

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On Sun, Mar 21, 2010 at 09:17:25PM +0100, Nikolai Weibull wrote:
> On Sun, Mar 21, 2010 at 20:48, Benjamin R. Haskell wrote:
>
>> +  symbolic_heads=()
>> +  for head in HEAD ORIG_HEAD FETCH_HEAD MERGE_HEAD ; do
>> +    git rev-parse $head &>/dev/null && symbolic_heads+=( $head )
>> +  done
>
> That’s a lot of forking to make a simple check.  Is there no better
> way?  Do we actually have to filter the heads?

I don't know the _git completion code, but couldn't we just check
if .git/{ORIG_HEAD,FETCH_HEAD,MERGE_HEAD} exists? This should
work for most things.

Another possibility would be to just complete them all the time.

Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Benjamin R. Haskell]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2416 bytes --]

On Mon, 22 Mar 2010, Simon Ruderich wrote:

> On Sun, Mar 21, 2010 at 09:17:25PM +0100, Nikolai Weibull wrote:
> > On Sun, Mar 21, 2010 at 20:48, Benjamin R. Haskell wrote:
> >
> >> +  symbolic_heads=()
> >> +  for head in HEAD ORIG_HEAD FETCH_HEAD MERGE_HEAD ; do
> >> +    git rev-parse $head &>/dev/null && symbolic_heads+=( $head )
> >> +  done
> >
> > That’s a lot of forking to make a simple check.  Is there no better 
> > way?  Do we actually have to filter the heads?
> 
> I don't know the _git completion code, but couldn't we just check if 
> .git/{ORIG_HEAD,FETCH_HEAD,MERGE_HEAD} exists? This should work for 
> most things.
> 
> Another possibility would be to just complete them all the time.

Testing the existence of $gitdir/{refname} seems a fine compromise.  
(Really, I don't see the issue; this seems like a drop in the bucket of 
_git's performance issues...  I s'pose the forks are prohibitively 
expensive on Win32?  Revised patch below anyway.)

Also, this breaks the fact that my previous patch fixed 'HEAD' *not* 
completing in a newly init'ed repo.  (The file .git/HEAD contains the 
text "ref: refs/heads/master", even if HEAD doesn't exist.)  But, 
whatever; speed trumps correctness.

(In case it's unclear, I prefer my previous patch -- doesn't _git fork 
all over the place? -- but either one's preferable to no-change or 
always-adding-them.)

Best,
Ben

---
 Completion/Unix/Command/_git |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index d7570cc..ae6565f 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -3119,13 +3119,17 @@ __git_tag_ids () {
 
 (( $+functions[__git_heads] )) ||
 __git_heads () {
-  local expl
+  local expl gitdir head symbolic_heads
   declare -a branch_names
 
   branch_names=(${${(f)"$(_call_program headrefs git for-each-ref --format='"%(refname)"' refs/heads refs/remotes 2>/dev/null)"}#refs/(heads|remotes)/})
   __git_command_successful || return
 
-  _wanted heads expl branch-name compadd $* - $branch_names HEAD
+  symbolic_heads=()
+  gitdir=$(_call_program gitdir git rev-parse --git-dir 2>/dev/null)
+  __git_command_successful && symbolic_heads=( $gitdir/{,{ORIG,FETCH,MERGE}_}HEAD(N:t) )
+
+  _wanted heads expl branch-name compadd $* - $branch_names $symbolic_heads
 }
 
 (( $+functions[__git_tags] )) ||
-- 
1.7.0

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Nikolai Weibull]

On Mon, Mar 22, 2010 at 03:30, Benjamin R. Haskell <zsh@benizi.com> wrote:
> Testing the existence of $gitdir/{refname} seems a fine compromise.
> (Really, I don't see the issue; this seems like a drop in the bucket of
> _git's performance issues...  I s'pose the forks are prohibitively
> expensive on Win32?  Revised patch below anyway.)

> (In case it's unclear, I prefer my previous patch -- doesn't _git fork
> all over the place? -- but either one's preferable to no-change or
> always-adding-them.)

Yes, it does, and, as you mention, it’s horribly slow to fork on
Windows.  I mean, what where they thinking?  But I question the value
of forking for this particular test.  Still, it’s more correct to use
rev-parse.  As your patch uses rev-parse to find gitdir once already,
we’re not gaining much by globbing instead.

I just ran a benchmark on Cygwin and it takes about 0.5 seconds to run
four rev-parses.

Perhaps completing them all without checking if they’re valid
beforehand is the best solution?

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 427 bytes --]

On Mon, Mar 22, 2010 at 10:27:29AM +0100, Nikolai Weibull wrote:
> [snip]
>
> Perhaps completing them all without checking if they’re valid
> beforehand is the best solution?

I think so too, it's simple and works. And as all these names are
uppercase they shouldn't interfere with normal completion.

Thanks,
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

On Mon, Mar 29, 2010 at 02:07:15AM +0200, Simon Ruderich wrote:
> On Mon, Mar 22, 2010 at 10:27:29AM +0100, Nikolai Weibull wrote:
>> [snip]
>>
>> Perhaps completing them all without checking if they’re valid
>> beforehand is the best solution?
>
> I think so too, it's simple and works. And as all these names are
> uppercase they shouldn't interfere with normal completion.
>
> Thanks,
> Simon

Could please somebody commit one of the possible solutions (I
would prefer no forking if possible).

Thanks,
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Frank Terbeck]

Simon Ruderich wrote:
> On Mon, Mar 29, 2010 at 02:07:15AM +0200, Simon Ruderich wrote:
>> On Mon, Mar 22, 2010 at 10:27:29AM +0100, Nikolai Weibull wrote:
>>> Perhaps completing them all without checking if they’re valid
>>> beforehand is the best solution?
>>
>> I think so too, it's simple and works. And as all these names are
>> uppercase they shouldn't interfere with normal completion.
>
> Could please somebody commit one of the possible solutions (I
> would prefer no forking if possible).

Um, I didn't follow this thread closely; which one? Benjamin's patch
from workers-27817¹?

Regards, Frank

¹ <http://www.zsh.org/mla/workers/2010/msg00269.html>

-- 
In protocol design, perfection has been reached not when there is
nothing left to add, but when there is nothing left to take away.
                                                  -- RFC 1925

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On Sat, May 01, 2010 at 12:46:00PM +0200, Frank Terbeck wrote:
> Um, I didn't follow this thread closely; which one? Benjamin's patch
> from workers-27817¹?
>
> Regards, Frank
>
> ¹ <http://www.zsh.org/mla/workers/2010/msg00269.html>

I would prefer mine because of the speed penalty of Benjamin's
patch. Checking if the file exists in .git would be another
possibility.

Thanks,
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Frank Terbeck]

Simon Ruderich wrote:
> On Sat, May 01, 2010 at 12:46:00PM +0200, Frank Terbeck wrote:
>> Um, I didn't follow this thread closely; which one? Benjamin's patch
>> from workers-27817¹?
[...]
>> ¹ <http://www.zsh.org/mla/workers/2010/msg00269.html>
>
> I would prefer mine because of the speed penalty of Benjamin's
> patch. Checking if the file exists in .git would be another
> possibility.

And that would be the one from workers-27813¹?

Regards, Frank

¹ <http://www.zsh.org/mla/workers/2010/msg00265.html>

-- 
In protocol design, perfection has been reached not when there is
nothing left to add, but when there is nothing left to take away.
                                                  -- RFC 1925

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

On Sat, May 01, 2010 at 02:17:19PM +0200, Frank Terbeck wrote:
>> I would prefer mine because of the speed penalty of Benjamin's
>> patch. Checking if the file exists in .git would be another
>> possibility.
>
> And that would be the one from workers-27813¹?
>
> Regards, Frank
>
> ¹ <http://www.zsh.org/mla/workers/2010/msg00265.html>

Ah sorry. Yes, that's the one.

Thanks,
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Frank Terbeck]

Simon Ruderich wrote:
> On Sat, May 01, 2010 at 02:17:19PM +0200, Frank Terbeck wrote:
>>> I would prefer mine because of the speed penalty of Benjamin's
>>> patch. Checking if the file exists in .git would be another
>>> possibility.
>>
>> And that would be the one from workers-27813¹?
>>
>> Regards, Frank
>>
>> ¹ <http://www.zsh.org/mla/workers/2010/msg00265.html>
>
> Ah sorry. Yes, that's the one.

Committed this one. Actually checking whether the head symbols are
available at any time probably requires something like this:

    gdir="$(git rev-parse --git-dir)"
    [[ -f ${gdir}/ORIG_HEAD ]] && ...
    [[ -f ${gdir}/FETCH_HEAD ]] && ...
    ...

I don't know if it's worth the additional fork.

Regards, Frank

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Benjamin R. Haskell]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2525 bytes --]

On Sun, 2 May 2010, Frank Terbeck wrote:

> Simon Ruderich wrote:
> > On Sat, May 01, 2010 at 02:17:19PM +0200, Frank Terbeck wrote:
> >>> I would prefer mine because of the speed penalty of Benjamin's 
> >>> patch. Checking if the file exists in .git would be another 
> >>> possibility.
> >>
> >> And that would be the one from workers-27813¹?
> >>
> >> Regards, Frank
> >>
> >> ¹ <http://www.zsh.org/mla/workers/2010/msg00265.html>
> >
> > Ah sorry. Yes, that's the one.
> 
> Committed this one. Actually checking whether the head symbols are 
> available at any time probably requires something like this:
> 
>     gdir="$(git rev-parse --git-dir)"
>     [[ -f ${gdir}/ORIG_HEAD ]] && ...
>     [[ -f ${gdir}/FETCH_HEAD ]] && ...
>     ...
> 
> I don't know if it's worth the additional fork.

That's essentially what my 'compromise' patch does.  The one from 
workers-27817 [links below].

==== this portion: ====
+  symbolic_heads=()
+  gitdir=$(_call_program gitdir git rev-parse --git-dir 2>/dev/null)
+  __git_command_successful && symbolic_heads=( $gitdir/{,{ORIG,FETCH,MERGE}_}HEAD(N:t) )
=======================

As stated before, I think the fork is worth it.  The three options were:

[27813]. always-complete:  usually-incorrect, 0 extra forks
[27817]. --git-dir + filesystem tests: possibly-incorrect, 1 extra fork
[27814]. rev-parse each name: always correct, 4 extra forks

Personally, I think the 4 extra forks are worth it (The particular 
execution path that this alters already does two _call_program git 
forks).  But, I'm not stuck on a system where forking is expensive (100 
git forks takes ~1sec)[2], so I see the need for the compromise [27817].

[27813] is usually incorrect (depending on your workflow) because 
FETCH_HEAD, ORIG_HEAD, and MERGE_HEAD come and go depending on what 
actions you take on your repository.

The reason [27817] is possibly-incorrect is that the .git/*HEAD files 
can exist even if the branch hasn't been born.  (e.g. try 'git rev-parse 
HEAD' in a newly-init'ed repo, even though .git/HEAD exists)
But, it's rare enough that the correctness vs. speed tradeoff seems 
reasonable.

-- 
Best,
Ben

[27813] http://www.zsh.org/mla/workers/2010/msg00265.html
[27814] http://www.zsh.org/mla/workers/2010/msg00266.html
[27817] http://www.zsh.org/mla/workers/2010/msg00269.html

[2]
$ date ; r=( HEAD ORIG_HEAD --git-dir ) ; for l in {1..100} ; git rev-parse $r[$(( 1 + $RANDOM % $#r ))] 2>/dev/null >/dev/null ; date
Mon May  3 10:50:06 EDT 2010
Mon May  3 10:50:07 EDT 2010

Re: [PATCH] _git: Also complete FETCH_HEAD, ORIG_HEAD and MERGE_HEAD.

[Benjamin R. Haskell]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2101 bytes --]

[oops, sat in my drafts]

On Mon, 22 Mar 2010, Nikolai Weibull wrote:

> On Mon, Mar 22, 2010 at 03:30, Benjamin R. Haskell <zsh@benizi.com> wrote:
> > Testing the existence of $gitdir/{refname} seems a fine compromise.  
> > (Really, I don't see the issue; this seems like a drop in the bucket 
> > of _git's performance issues...  I s'pose the forks are 
> > prohibitively expensive on Win32?  Revised patch below anyway.)
> 
> > (In case it's unclear, I prefer my previous patch -- doesn't _git 
> > fork all over the place? -- but either one's preferable to no-change 
> > or always-adding-them.)
> 
> Yes, it does, and, as you mention, it’s horribly slow to fork on 
> Windows.  I mean, what where they thinking?  But I question the value 
> of forking for this particular test.  Still, it’s more correct to use 
> rev-parse.  As your patch uses rev-parse to find gitdir once already, 
> we’re not gaining much by globbing instead.
> 
> I just ran a benchmark on Cygwin and it takes about 0.5 seconds to run 
> four rev-parses.
> 
> Perhaps completing them all without checking if they’re valid 
> beforehand is the best solution?

I'd still rather see some checking than none at all.  (MERGE_HEAD isn't 
often applicable, for example.)

In the context I was using to test:

$ git log <Tab>

It appears to be completing both tags and branches, and it already forks 
git once to find each of those (@lines 3117 and 3140).  Four more forks 
(the version calling rev-parse on each name) might be excessive, but one 
doesn't seem so bad (the version calling it once to get --git-dir) for 
the added filtering.

rev-parse each: 100% correct, only much slower on Win32 (~.5s)
rev-parse dir + glob: mostly correct, a-bit-slower on Win32 (~.125s)
always-complete: usually wrong, no slower

Really, I feel like the forking cost would prohibit _git from being 
useful on Win32 in the first place, so it seems an odd metric to use.

(Are there other systems where the fork is expensive?  AFAIK, anything 
Unix-like shouldn't incur too much cost for it, which leaves Win32 and 
...?)

-- 
Best,
Ben