BUG: failure in "vcs_info prompt" aborts precmd function

BUG: failure in "vcs_info prompt" aborts precmd function

[Daniel Hahler]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I am using a precmd hook to setup my prompt and call "vcs_info 'prompt'" from there.

When starting an interactive Git rebase, while the editor is still open, it will fail as follows:

    VCS_INFO_get_data_git:208: no such file or directory: .git/rebase-merge/done

While this bug can be fixed (I'll post a patch), I think it's still a bug that vcs_info aborts the precmd.

Why is that?

I have tried adding "set +e" and "|| true" before the call to vcs_info, but without success.


Regards,
Daniel.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iD8DBQFUcMQnfAK/hT/mPgARAg43AJ9e2H/SW5RlRu0HkN2xfrdJITLG7QCgtTIo
uWKxYeqH7EheJ0wMB30xsrg=
=BdkI
-----END PGP SIGNATURE-----

[PATCH] vcs_info: test -f .git/rebase-merge/done before reading it

[Daniel Hahler]

From: Daniel Hahler <git@thequod.de>

---
 Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
index 48d552f..fe1c356 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -204,11 +204,13 @@ then
     VCS_INFO_git_handle_patches
 elif [[ -d "${gitdir}/rebase-merge" ]]; then
     patchdir="${gitdir}/rebase-merge"
-    local p
-    for p in ${(f)"$(< "${patchdir}/done")"}; do
-        # remove action
-        git_patches_applied+=("${${(s: :)p}[2,-1]}")
-    done
+    if [[ -f ${patchdir}/done ]]; then
+        local p
+        for p in ${(f)"$(< "${patchdir}/done")"}; do
+            # remove action
+            git_patches_applied+=("${${(s: :)p}[2,-1]}")
+        done
+    fi
     git_patches_unapplied=(${(f)"$(grep -v '^$' "${patchdir}/git-rebase-todo" | grep -v '^#')"})
     VCS_INFO_git_handle_patches
 elif [[ -d "${gitdir}/rebase-apply" ]]; then
-- 
2.1.0

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Mikael Magnusson]

On Sat, Nov 22, 2014 at 6:13 PM, Daniel Hahler <dhahler@gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> I am using a precmd hook to setup my prompt and call "vcs_info 'prompt'" from there.
>
> When starting an interactive Git rebase, while the editor is still open, it will fail as follows:
>
>     VCS_INFO_get_data_git:208: no such file or directory: .git/rebase-merge/done
>
> While this bug can be fixed (I'll post a patch), I think it's still a bug that vcs_info aborts the precmd.
>
> Why is that?
>
> I have tried adding "set +e" and "|| true" before the call to vcs_info, but without success.

http://www.zsh.org/cgi-bin/mla/redirect?WORKERNUMBER=33671 should fix
this already.

-- 
Mikael Magnusson

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Daniel Hahler]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 22.11.2014 18:51, Mikael Magnusson wrote:
>> I am using a precmd hook to setup my prompt and call "vcs_info 'prompt'" from there.
>>
>> When starting an interactive Git rebase, while the editor is still open, it will fail as follows:
>>
>>     VCS_INFO_get_data_git:208: no such file or directory: .git/rebase-merge/done
>>
>> While this bug can be fixed (I'll post a patch), I think it's still a bug that vcs_info aborts the precmd.
>>
>> Why is that?
>>
>> I have tried adding "set +e" and "|| true" before the call to vcs_info, but without success.
> 
> http://www.zsh.org/cgi-bin/mla/redirect?WORKERNUMBER=33671 should fix
> this already.

Yes, it's likely to fix this, although it might not be necessary to replace that many occurrences.
Also, I don't know if "$(cat ..)" is as efficient as "$(< ..)". Does it use a subshell, or is it optimized?

Apart from that, I am still wondering why the error in vcs_info causes abortion of the precmd function?!


Thanks,
Daniel.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iD8DBQFUcM98fAK/hT/mPgARAtCqAJ0abLfgdxHjoAfYGYy/Lm1iTHbSjgCfZPWU
e0OaBNs5nttKfMpBinP+A9E=
=vmxC
-----END PGP SIGNATURE-----

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Mikael Magnusson]

On Sat, Nov 22, 2014 at 7:01 PM, Daniel Hahler <dhahler@gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 22.11.2014 18:51, Mikael Magnusson wrote:
>>> I am using a precmd hook to setup my prompt and call "vcs_info 'prompt'" from there.
>>>
>>> When starting an interactive Git rebase, while the editor is still open, it will fail as follows:
>>>
>>>     VCS_INFO_get_data_git:208: no such file or directory: .git/rebase-merge/done
>>>
>>> While this bug can be fixed (I'll post a patch), I think it's still a bug that vcs_info aborts the precmd.
>>>
>>> Why is that?
>>>
>>> I have tried adding "set +e" and "|| true" before the call to vcs_info, but without success.
>>
>> http://www.zsh.org/cgi-bin/mla/redirect?WORKERNUMBER=33671 should fix
>> this already.
>
> Yes, it's likely to fix this, although it might not be necessary to replace that many occurrences.
> Also, I don't know if "$(cat ..)" is as efficient as "$(< ..)". Does it use a subshell, or is it optimized?
>
> Apart from that, I am still wondering why the error in vcs_info causes abortion of the precmd function?!

Passing a non-existing file to $(< is a fatal error, or whichever term
is correct here. I'm not sure if that's a bug or not since it's
documented to not be the case. (in Command Substitution,
   The substitution `$(cat foo)' may be replaced
   by  the  equivalent but  faster  `$(<foo)'.
)

-- 
Mikael Magnusson

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Bart Schaefer]

On Nov 22, 11:20pm, Mikael Magnusson wrote:
} Subject: Re: BUG: failure in "vcs_info prompt" aborts precmd function
}
} Passing a non-existing file to $(< is a fatal error, or whichever term
} is correct here. I'm not sure if that's a bug or not since it's
} documented to not be the case. (in Command Substitution,
}    The substitution `$(cat foo)' may be replaced
}    by  the  equivalent but  faster  `$(<foo)'.
} )

I think you're reading way to much into the phrase "the equivalent".
It's not meant to imply precise equivalence on failure cases, only
that when it works it produces the same string.

On the other hand, I don't know WHY $(<notafile) was made a fatal error.

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Daniel Hahler]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 22.11.2014 23:20, Mikael Magnusson wrote:

>> Apart from that, I am still wondering why the error in vcs_info causes abortion of the precmd function?!
> 
> Passing a non-existing file to $(< is a fatal error, or whichever term
> is correct here.

I got that, but e.g. `false` does not abort the precmd function.

I am using vcs_info like this:

    if (( $ZSH_IS_SLOW_DIR )) || ! vcs_info 'prompt'; then
        # No vcs_info available, only set cwd
        prompt_vcs=""
    else
        prompt_vcs="${PR_RESET}$vcs_info_msg_0_"
        if ! zstyle -t ':vcs_info:*:prompt:*' 'check-for-changes'; then
            prompt_vcs+=' ?'
        fi
        rprompt_extra+=("${vcs_info_msg_1_}")
    fi


Regards,
Daniel.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iD8DBQFUcgQPfAK/hT/mPgARAii4AJ92A0NFg8Np8qSrNWfO/V9n0xonjwCfbyRS
VIO+Y1tkr0gZ2rm4QyYzELk=
=/FPk
-----END PGP SIGNATURE-----

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Mikael Magnusson]

On Sun, Nov 23, 2014 at 4:58 PM, Daniel Hahler <dhahler@gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 22.11.2014 23:20, Mikael Magnusson wrote:
>
>>> Apart from that, I am still wondering why the error in vcs_info causes abortion of the precmd function?!
>>
>> Passing a non-existing file to $(< is a fatal error, or whichever term
>> is correct here.
>
> I got that, but e.g. `false` does not abort the precmd function.

Yes, a command that returns false is not a fatal error. In the
meantime you can do this to work around the problem.
% { $(< foo) }; echo hello
zsh: no such file or directory: foo
% { $(< foo) } always { TRY_BLOCK_ERROR=0 }; echo hello
zsh: no such file or directory: foo
hello

eg, call vcs_info inside the first { }. However, I'm not sure how to
make the construct return false when it failed with a fatal error.

-- 
Mikael Magnusson

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Bart Schaefer]

On Nov 23,  4:58pm, Daniel Hahler wrote:
} Subject: Re: BUG: failure in "vcs_info prompt" aborts precmd function
}
} On 22.11.2014 23:20, Mikael Magnusson wrote:
} 
} >> Apart from that, I am still wondering why the error in vcs_info causes abortion of the precmd function?!
} > 
} > Passing a non-existing file to $(< is a fatal error, or whichever term
} > is correct here.
} 
} I got that, but e.g. `false` does not abort the precmd function.

"false" is just a normal command that returns nonzero.

$(<notafile) is treated similarly to a syntax error, it kills the whole
surrounding context.

It's pretty easy to make it behave like a failed external command; can
anyone recall why it wasn't, other than to avoid allocating an empty
linked list?  It has been a fatal error since before revision control
was in use.  It is possible something really depends on being stopped
by this error?


diff --git a/Src/exec.c b/Src/exec.c
index 042215d..02a8fe3 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3840,8 +3840,8 @@ getoutput(char *cmd, int qt)
 	    return NULL;
 	untokenize(s);
 	if ((stream = open(unmeta(s), O_RDONLY | O_NOCTTY)) == -1) {
-	    zerr("%e: %s", errno, s);
-	    return NULL;
+	    zwarn("%e: %s", errno, s);
+	    return newlinklist();
 	}
 	return readoutput(stream, qt);
     }

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Mikael Magnusson]

On Sun, Nov 23, 2014 at 7:13 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Nov 23,  4:58pm, Daniel Hahler wrote:
> } Subject: Re: BUG: failure in "vcs_info prompt" aborts precmd function
> }
> } On 22.11.2014 23:20, Mikael Magnusson wrote:
> }
> } >> Apart from that, I am still wondering why the error in vcs_info causes abortion of the precmd function?!
> } >
> } > Passing a non-existing file to $(< is a fatal error, or whichever term
> } > is correct here.
> }
> } I got that, but e.g. `false` does not abort the precmd function.
>
> "false" is just a normal command that returns nonzero.
>
> $(<notafile) is treated similarly to a syntax error, it kills the whole
> surrounding context.
>
> It's pretty easy to make it behave like a failed external command; can
> anyone recall why it wasn't, other than to avoid allocating an empty
> linked list?  It has been a fatal error since before revision control
> was in use.  It is possible something really depends on being stopped
> by this error?

Maybe it predates the UNSET option and nobody ever caught it? compare eg,
% ( setopt nounset; echo $a; echo hello )
zsh: a: parameter not set
% ( echo $(< foo); echo hello )
zsh: no such file or directory: foo

It is sort of a parameter expansion, after all.

-- 
Mikael Magnusson

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Peter Stephenson]

On Sat, 22 Nov 2014 14:44:00 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On the other hand, I don't know WHY $(<notafile) was made a fatal error.

Looks like most redirection errors are fatal, so from that point of view
this would be consistent.  However, from the other point of view,
$(<...) might be thought to be faking up a subshell, in which case it
probably shouldn't be.  I suppose that just never got discussed before.

The principle of least surprise perhaps says it doesn't look like it should
be fatal, therefore it shouldn't be.

pws

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Bart Schaefer]

On Nov 23,  7:02pm, Peter Stephenson wrote:
} Subject: Re: BUG: failure in "vcs_info prompt" aborts precmd function
}
} On Sat, 22 Nov 2014 14:44:00 -0800
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > On the other hand, I don't know WHY $(<notafile) was made a fatal error.
} 
} Looks like most redirection errors are fatal, so from that point of view

Hmm.

torch% () { cat < notafile ; echo got here }
(anon): no such file or directory: notafile
got here

What was fatal about that?  Even with a builtin:

torch% () { read -E < notafile ; echo got here }
(anon): no such file or directory: notafile
got here

Even this:

torch% print =(<notafile)
zsh: no such file or directory: notafile
/tmp/zshd0zlSR

(Aside:

    cat =(<file) >| file

reads the contents into the temp file for =(...) before clobbering it
for the >| redirection.  Probably handy for something.)

} this would be consistent.  However, from the other point of view,
} $(<...) might be thought to be faking up a subshell, in which case it
} probably shouldn't be.  I suppose that just never got discussed before.
} 
} The principle of least surprise perhaps says it doesn't look like it should
} be fatal, therefore it shouldn't be.

Should I commit/push 33775 then?  Anybody object?

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Bart Schaefer]

On Nov 23,  7:52pm, Mikael Magnusson wrote:
}
} Maybe it predates the UNSET option and nobody ever caught it? compare eg,
} % ( setopt nounset; echo $a; echo hello )
} zsh: a: parameter not set
} % ( echo $(< foo); echo hello )
} zsh: no such file or directory: foo
} 
} It is sort of a parameter expansion, after all.

Command substitution and parameter expansion are only loosely related,
but I could see an argument for making this behavior depend on NOUNSET.

It defintely predates the NOUNSET option (by decades), but the former
behavior of parameter expansion was as if UNSET were always in effect,
whereas $(<notafile) has always been an error.

TRY_BLOCK_ERROR and exit status

[Bart Schaefer]

On Nov 23,  7:08pm, Mikael Magnusson wrote:
}
} % { $(< foo) } always { TRY_BLOCK_ERROR=0 }; echo hello
} zsh: no such file or directory: foo
} hello
} 
} However, I'm not sure how to
} make the construct return false when it failed with a fatal error.

Arguably a fatal error should set both errflag and lastval nonzero, but
that might require a lot of tiny changes all over the place ... on the
other hand I think always+TRY_BLOCK_ERROR is the only way lastval can
still be examined after errflag has been set, so maybe it would suffice
to handle this there?

What should the value of lastval ($?) be in this circumstance?

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Peter Stephenson]

Bart Schaefer wrote:
> On Nov 23,  7:02pm, Peter Stephenson wrote:
> } Subject: Re: BUG: failure in "vcs_info prompt" aborts precmd function
> }
> } On Sat, 22 Nov 2014 14:44:00 -0800
> } Bart Schaefer <schaefer@brasslantern.com> wrote:
> } > On the other hand, I don't know WHY $(<notafile) was made a fatal error.
> } 
> } Looks like most redirection errors are fatal, so from that point of view
> 
> Hmm.
> 
> torch% () { cat < notafile ; echo got here }
> (anon): no such file or directory: notafile
> got here
> 
> What was fatal about that?  Even with a builtin:
> 
> torch% () { read -E < notafile ; echo got here }
> (anon): no such file or directory: notafile
> got here

addfd()'s stuffed with zerr()'s, so I've no idea what's going on.
 
> Should I commit/push 33775 then?  Anybody object?

No, that looks fine.

pws

Re: BUG: failure in "vcs_info prompt" aborts precmd function

[Oliver Kiddle]

Bart wrote:
> It's pretty easy to make it behave like a failed external command; can
> anyone recall why it wasn't, other than to avoid allocating an empty
> linked list?  It has been a fatal error since before revision control
> was in use.  It is possible something really depends on being stopped
> by this error?

I'd think it was unlikely.
Neither bash or ksh (88 or 93) treat it as a fatal error.

I'd vote for applying the patch.

Oliver

Re: TRY_BLOCK_ERROR and exit status

[Peter Stephenson]

On Sun, 23 Nov 2014 11:57:55 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Nov 23,  7:08pm, Mikael Magnusson wrote:
> }
> } % { $(< foo) } always { TRY_BLOCK_ERROR=0 }; echo hello
> } zsh: no such file or directory: foo
> } hello
> } 
> } However, I'm not sure how to
> } make the construct return false when it failed with a fatal error.
> 
> Arguably a fatal error should set both errflag and lastval nonzero, but
> that might require a lot of tiny changes all over the place ... on the
> other hand I think always+TRY_BLOCK_ERROR is the only way lastval can
> still be examined after errflag has been set, so maybe it would suffice
> to handle this there?

Yes, probably.

> What should the value of lastval ($?) be in this circumstance?

The easiest way out is probably to leave it if it's non-zero, else set
it to 1.

Given that "always" isn't all that widely used, changing it to return
the last status of the always block might also work.  However, it
doesn't look necessary to go that far.

pws

Re: TRY_BLOCK_ERROR and exit status

[Bart Schaefer]

On Nov 24, 10:11am, Peter Stephenson wrote:
}
} > other hand I think always+TRY_BLOCK_ERROR is the only way lastval can
} > still be examined after errflag has been set, so maybe it would suffice
} > to handle this there?
} 
} Yes, probably.
} 
} > What should the value of lastval ($?) be in this circumstance?
} 
} The easiest way out is probably to leave it if it's non-zero, else set
} it to 1.
} 
} Given that "always" isn't all that widely used, changing it to return
} the last status of the always block might also work.  However, it
} doesn't look necessary to go that far.

Do you mean return the last status of the always block only in the case
where errflag is nonzero at the end of the try block?  If that's not
what you mean, wouldn't that break the current normal use case where
always is for cleaning up whether or not there was an error?

I agree it isn't necessary to go that far.  This?


diff --git a/Src/loop.c b/Src/loop.c
index 2f639fd..82d2fe3 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -659,8 +659,9 @@ exectry(Estate state, int do_exec)
 
     try_tryflag = save_try_tryflag;
 
-    /* Don't record errflag here, may be reset. */
-    endval = lastval;
+    /* Don't record errflag here, may be reset.  However, */
+    /* endval should show failure when there is an error. */
+    endval = lastval ? lastval : errflag;
 
     freeheap();
 

Re: TRY_BLOCK_ERROR and exit status

[Peter Stephenson]

On Tue, 25 Nov 2014 00:01:23 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> } Given that "always" isn't all that widely used, changing it to return
> } the last status of the always block might also work.  However, it
> } doesn't look necessary to go that far.
> 
> Do you mean return the last status of the always block only in the case
> where errflag is nonzero at the end of the try block?

Yes, we wouldn't want to affect non-error cases.

> I agree it isn't necessary to go that far.  This?

That seems reasonable.

pws