nits with new _git

nits with new _git

[Mikael Magnusson]

Now that pws fixed the crash, I'm updating to the new _git, so far
I've noticed these two things.

git log --pret<tab> goes into correction, same for git show.
% git log --pretty=
---- option
--pretty  -- pretty print commit messages
---- corrections (errors: 2)
--grep    -- limit commits to those with log messages matching the given pattern
--pretty  -- pretty print commit messages

Usually this would hint at some missing ret=0 somewhere, but I
couldn't find any. Unconditionally returning 0 from _git does 'fix' it
though, so it must be somewhere.

The other thing is that git branch -d -r <tab> doesn't complete remote branches.

-- 
Mikael Magnusson

Re: nits with new _git

[Mikael Magnusson]

Also noticed now, some options that take numbers just show '-M' as the
description instead of the actual string. For example git tag -n <tab>
and git fast-export --progress=<tab>, while git config
gui.blamehistoryctx <tab> properly shows 'number of days'.

On 14 May 2011 03:28, Mikael Magnusson <mikachu@gmail.com> wrote:
> Now that pws fixed the crash, I'm updating to the new _git, so far
> I've noticed these two things.
>
> git log --pret<tab> goes into correction, same for git show.
> % git log --pretty=
> ---- option
> --pretty  -- pretty print commit messages
> ---- corrections (errors: 2)
> --grep    -- limit commits to those with log messages matching the given pattern
> --pretty  -- pretty print commit messages
>
> Usually this would hint at some missing ret=0 somewhere, but I
> couldn't find any. Unconditionally returning 0 from _git does 'fix' it
> though, so it must be somewhere.
>
> The other thing is that git branch -d -r <tab> doesn't complete remote branches.
>
> --
> Mikael Magnusson
>



-- 
Mikael Magnusson

build problem with patch level 1.5301

[S. Cowles]

i have tried building patchlevel 1.5301 from cvs pulls (on various 
platforms) and get the error shown in the following excerpt of the build:


make[4]: Leaving directory `xx/origdist/zsh/zsh/Src/Zle'
Updated `zleparameter.mdh'.
echo 'timestamp for zleparameter.mdh against zleparameter.mdd' > zleparameter.mdhs
gawk -f ../../Src/makepro.awk zleparameter.c Src/Zle > zleparameter.syms
(echo '/* Generated automatically */'; sed -n '/^E/{s/^E//;p;}' < zleparameter.syms) \
                 > zleparameter.epro
(echo '/* Generated automatically */'; sed -n '/^L/{s/^L//;p;}' < zleparameter.syms) \
                 > `echo zleparameter.epro | sed 's/\.epro$/.pro/'`
make[4]: Entering directory `xx/origdist/zsh/zsh/Src/Zle'
make[4]: Leaving directory `xx/origdist/zsh/zsh/Src/Zle'
Updated `zleparameter.mdh'.
make[3]: Leaving directory `xx/origdist/zsh/zsh/Src/Zle'
make[2]: Leaving directory `xx/origdist/zsh/zsh/Src'
rm -f stamp-modobjs.tmp
make[2]: Entering directory `xx/origdist/zsh/zsh/Src'
gcc -c -I. -I../Src -I../Src -I../Src/Zle -I.  -DHAVE_CONFIG_H -Wall -Wmissing-prototypes -O2  -o builtin.o builtin.c
In file included from zsh.mdh:49:0,
                  from builtin.c:33:
mem.epro:5:35: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'last_heap_id'
mem.epro:15:48: error: expected ')' before 'heap_id'
make[2]: *** [builtin.o] Error 1
make[2]: Leaving directory `xx/origdist/zsh/zsh/Src'
make[1]: *** [modobjs] Error 2
make[1]: Leaving directory `xx/origdist/zsh/zsh/Src'
make: *** [all] Error 1

Re: build problem with patch level 1.5301

[Bart Schaefer]

On May 13,  8:13pm, S. Cowles wrote:
}
} gcc -c -I. -I../Src -I../Src -I../Src/Zle -I.  -DHAVE_CONFIG_H -Wall -Wmissing-prototypes -O2  -o builtin.o builtin.c
} In file included from zsh.mdh:49:0,
}                   from builtin.c:33:
} mem.epro:5:35: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'last_heap_id'

Hmm, the script that generates zsh.mdh doesn't know about #ifdef,
so it tries to export a variable that's declared conditionally in
mem.c.

All that's really necessary, though, is to make the typedef for
"Heapid" visible regardless of whether ZSH_HEAP_DEBUG is defined.

Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.175
diff -u -r1.175 zsh.h
--- Src/zsh.h   14 May 2011 00:07:42 -0000      1.175
+++ Src/zsh.h   14 May 2011 03:59:32 -0000
@@ -2327,7 +2327,6 @@
  * Memory management *
  *********************/
 
-#ifdef ZSH_HEAP_DEBUG
 /*
  * A Heapid is a type for identifying, uniquely up to the point where
  * the count of new identifiers wraps. all heaps that are or
@@ -2341,6 +2340,8 @@
  */
 typedef unsigned int Heapid;
 
+#ifdef ZSH_HEAP_DEBUG
+
 /* printf format specifier corresponding to Heapid */
 #define HEAPID_FMT     "%x"
 

-- 

Re: nits with new _git

[Nikolai Weibull]

On Sat, May 14, 2011 at 03:46, Mikael Magnusson <mikachu@gmail.com> wrote:
> Also noticed now, some options that take numbers just show '-M' as the
> description instead of the actual string. For example git tag -n <tab>
> and git fast-export --progress=<tab>, while git config
> gui.blamehistoryctx <tab> properly shows 'number of days'.

I’ll look into these issues tomorrow.

Re: build problem with patch level 1.5301

[Peter Stephenson]

On Fri, 13 May 2011 21:00:00 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> All that's really necessary, though, is to make the typedef for
> "Heapid" visible regardless of whether ZSH_HEAP_DEBUG is defined.

No problem with that, but it's probably a bit safer also to hide the
prototypes of things we don't need to see.

Index: Src/mem.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/mem.c,v
retrieving revision 1.19
diff -p -u -r1.19 mem.c
--- Src/mem.c	14 May 2011 00:07:42 -0000	1.19
+++ Src/mem.c	14 May 2011 16:20:45 -0000
@@ -123,6 +123,7 @@ static Heap heaps;
 
 static Heap fheap;
 
+/**/
 #ifdef ZSH_HEAP_DEBUG
 /*
  * The heap ID we'll allocate next.
@@ -172,6 +173,8 @@ new_heap_id(void)
 {
     return next_heap_id++;
 }
+
+/**/
 #endif
 
 /* Use new heaps from now on. This returns the old heap-list. */
@@ -723,6 +726,7 @@ hrealloc(char *p, size_t old, size_t new
     }
 }
 
+/**/
 #ifdef ZSH_HEAP_DEBUG
 /*
  * Check if heap_id is the identifier of a currently valid heap,
@@ -768,6 +772,7 @@ memory_validate(Heapid heap_id)
 
     return 1;
 }
+/**/
 #endif
 
 /* allocate memory from the current memory pool and clear it */

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: nits with new _git

[Nikolai Weibull]

On Sat, May 14, 2011 at 03:28, Mikael Magnusson <mikachu@gmail.com> wrote:
> Now that pws fixed the crash, I'm updating to the new _git, so far
> I've noticed these two things.
>
> git log --pret<tab> goes into correction, same for git show.
> % git log --pretty=
> ---- option
> --pretty  -- pretty print commit messages
> ---- corrections (errors: 2)
> --grep    -- limit commits to those with log messages matching the given pattern
> --pretty  -- pretty print commit messages
>
> Usually this would hint at some missing ret=0 somewhere, but I
> couldn't find any. Unconditionally returning 0 from _git does 'fix' it
> though, so it must be somewhere.

I can’t reproduce this issue.

> The other thing is that git branch -d -r <tab> doesn't complete remote branches.

Yes, this is still on the TODO list.

Re: nits with new _git

[Nikolai Weibull]

On Sat, May 14, 2011 at 03:46, Mikael Magnusson <mikachu@gmail.com> wrote:
> Also noticed now, some options that take numbers just show '-M' as the
> description instead of the actual string. For example git tag -n <tab>
> and git fast-export --progress=<tab>, while git config
> gui.blamehistoryctx <tab> properly shows 'number of days'.

This is a problem with __git_guard_* not checking for compadd options.
 I’ll fix it tonight.

Is there any way that I could get commit access to

Completion/Unix/Command/_git

so that I can fix smaller issues like this one without first posting a
[PATCH] to zsh-workers?  I’m worried that the flood of smaller patches
drown out important ones and also may result in some going missed.

If so, my user name on SourceForge is pcppopper.

Thanks!

Re: nits with new _git

[Nikolai Weibull]

On Tue, May 17, 2011 at 14:56, Nikolai Weibull <now@bitwi.se> wrote:
> On Sat, May 14, 2011 at 03:28, Mikael Magnusson <mikachu@gmail.com> wrote:
>> Now that pws fixed the crash, I'm updating to the new _git, so far
>> I've noticed these two things.

>> The other thing is that git branch -d -r <tab> doesn't complete remote branches.

> Yes, this is still on the TODO list.

This has now been fixed in CVS.

Re: nits with new _git

[Mikael Magnusson]

On 17 May 2011 14:56, Nikolai Weibull <now@bitwi.se> wrote:
> On Sat, May 14, 2011 at 03:28, Mikael Magnusson <mikachu@gmail.com> wrote:
>> Now that pws fixed the crash, I'm updating to the new _git, so far
>> I've noticed these two things.
>>
>> git log --pret<tab> goes into correction, same for git show.
>> % git log --pretty=
>> ---- option
>> --pretty  -- pretty print commit messages
>> ---- corrections (errors: 2)
>> --grep    -- limit commits to those with log messages matching the given pattern
>> --pretty  -- pretty print commit messages
>>
>> Usually this would hint at some missing ret=0 somewhere, but I
>> couldn't find any. Unconditionally returning 0 from _git does 'fix' it
>> though, so it must be somewhere.
>
> I can’t reproduce this issue.

This was sort of a mindfuck, but I figured it out :). It looks like
this is a systematic error in the whole _git file, but should be
easily fixed. _git() itself defines local ret=1, then does
_call_function ret _git-$words[1], then _git-log() in this case sets
ret=0 on success. Sounds good? Not really, because it doesn't return
ret, so then _call_function overwrites ret with something else. The
solution would seem to simply not use _call_function since none of the
helper functions return anything, they just set ret directly.

--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -6023,11 +6023,11 @@ _git() {
       (option-or-argument)
         curcontext=${curcontext%:*:*}:git-$words[1]:

-        _call_function ret _git-$words[1]
+        _git-$words[1]
         ;;
     esac
   else
-    _call_function ret _$service
+    _$service
   fi
   return ret
 }



-- 
Mikael Magnusson

Re: nits with new _git

[Nikolai Weibull]

On Fri, Jun 3, 2011 at 23:39, Mikael Magnusson <mikachu@gmail.com> wrote:
> It looks like
> this is a systematic error in the whole _git file, but should be
> easily fixed. _git() itself defines local ret=1, then does
> _call_function ret _git-$words[1], then _git-log() in this case sets
> ret=0 on success. Sounds good? Not really, because it doesn't return
> ret, so then _call_function overwrites ret with something else. The
> solution would seem to simply not use _call_function since none of the
> helper functions return anything, they just set ret directly.

I’d rather remove all superfluous ⟨&& ret=0⟩s and add ⟨integer ret=1⟩
where appropriate.

Opinions?

Re: nits with new _git

[Nikolai Weibull]

On Wed, Jul 20, 2011 at 13:28, Nikolai Weibull <now@bitwi.se> wrote:
> On Fri, Jun 3, 2011 at 23:39, Mikael Magnusson <mikachu@gmail.com> wrote:
>> It looks like
>> this is a systematic error in the whole _git file, but should be
>> easily fixed. _git() itself defines local ret=1, then does
>> _call_function ret _git-$words[1], then _git-log() in this case sets
>> ret=0 on success. Sounds good? Not really, because it doesn't return
>> ret, so then _call_function overwrites ret with something else. The
>> solution would seem to simply not use _call_function since none of the
>> helper functions return anything, they just set ret directly.
>
> I’d rather remove all superfluous ⟨&& ret=0⟩s and add ⟨integer ret=1⟩
> where appropriate.
>
> Opinions?

I have now updated all functions to use the correct semantics.

Re: nits with new _git

[Mikael Magnusson]

On 21 July 2011 11:06, Nikolai Weibull <now@bitwi.se> wrote:
> On Wed, Jul 20, 2011 at 13:28, Nikolai Weibull <now@bitwi.se> wrote:
>> On Fri, Jun 3, 2011 at 23:39, Mikael Magnusson <mikachu@gmail.com> wrote:
>>> It looks like
>>> this is a systematic error in the whole _git file, but should be
>>> easily fixed. _git() itself defines local ret=1, then does
>>> _call_function ret _git-$words[1], then _git-log() in this case sets
>>> ret=0 on success. Sounds good? Not really, because it doesn't return
>>> ret, so then _call_function overwrites ret with something else. The
>>> solution would seem to simply not use _call_function since none of the
>>> helper functions return anything, they just set ret directly.
>>
>> I’d rather remove all superfluous ⟨&& ret=0⟩s and add ⟨integer ret=1⟩
>> where appropriate.
>>
>> Opinions?
>
> I have now updated all functions to use the correct semantics.

Ah, not much time to react, it seems to still work so I'm happy with
this way too. :)

-- 
Mikael Magnusson