Error in zshaddhistory() should not prevent command from running

Error in zshaddhistory() should not prevent command from running

[Bart Schaefer]

[Fourth attempt sending this, primenet is blocking gmail again.]

Should this actually be in callhookfunc() itself?  I note that the doc
warns about errors in precmd killing off periodic, or else I'd say it
ought to be.

Possibly more controversial, why run this hook at all when nothing is
being added?

diff --git a/Src/hist.c b/Src/hist.c
index 97fd340..350688d 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1418,7 +1418,7 @@ hend(Eprog prog)
   DPUTS(hptr < chline, "History end pointer off start of line");
   *hptr = '\0';
     }
-    {
+    if (*chline) {
     LinkList hookargs = newlinklist();
     int save_errflag = errflag;
     errflag = 0;
@@ -1427,6 +1427,7 @@ hend(Eprog prog)
   addlinknode(hookargs, chline);
   callhookfunc("zshaddhistory", hookargs, 1, &hookret);

+	errflag &= ~ERRFLAG_ERROR;
	errflag |= save_errflag;
     }
     /* For history sharing, lock history file once for both read and write */

Re: Error in zshaddhistory() should not prevent command from running

[Daniel Shahaf]

Bart Schaefer wrote on Fri, Dec 30, 2016 at 22:13:27 -0800:
> [Fourth attempt sending this, primenet is blocking gmail again.]

Two came through.

> Should this actually be in callhookfunc() itself?  I note that the doc
> warns about errors in precmd killing off periodic, or else I'd say it
> ought to be.

I'd be wary of swallowing errors by default.  That said, we could change
callhookfunc()'s signature to remind callers that they may want to clear
errors; for example, an 'ignore_errors' boolean parameter that the
precmd() callsite can set to false and zshaddhistory() to true.

> Possibly more controversial, why run this hook at all when nothing is
> being added?

Playing devil's advocate, I see two reasons:

1) Compatibility.  https://xkcd.com/1172/

2) It's easier to call the user-provided hook function on one more case
that it can easily ignore, than not to do so and to leave the user with
no easy alternative.  (It does seem easier to handle multiline commands
with zshaddhistory() than with, say, wrapping accept-line(), since the
latter fires for each line, not just for the complete command.)

Cheers,

Daniel

Re: Error in zshaddhistory() should not prevent command from running

[Bart Schaefer]

On Jan 1,  3:10am, Daniel Shahaf wrote:
} Subject: Re: Error in zshaddhistory() should not prevent command from runn
}
} > Possibly more controversial, why run this hook at all when nothing is
} > being added?
} 
} 2) It's easier to call the user-provided hook function on one more case
} that it can easily ignore, than not to do so and to leave the user with
} no easy alternative.

As far as I can tell the only case where *chline == '\0' is when the EOF
character has been typed -- there's always at least a newline in there
in all other circumstances.  There are lots of other places to add to
the history on shell exit.  But this is why I asked -- other opinions?

Re: Error in zshaddhistory() should not prevent command from running

[Daniel Shahaf]

Bart Schaefer wrote on Sun, Jan 01, 2017 at 11:20:30 -0800:
> On Jan 1,  3:10am, Daniel Shahaf wrote:
> } Subject: Re: Error in zshaddhistory() should not prevent command from runn
> }
> } > Possibly more controversial, why run this hook at all when nothing is
> } > being added?
> } 
> } 2) It's easier to call the user-provided hook function on one more case
> } that it can easily ignore, than not to do so and to leave the user with
> } no easy alternative.
> 
> As far as I can tell the only case where *chline == '\0' is when the EOF
> character has been typed -- there's always at least a newline in there
> in all other circumstances.

I'd assumed we were talking about empty lines (i.e., <accept-line> when
BUFFER='').  My bad.  I don't have an opinion on whether zshaddhistory()
should be called during the EOF processing sequence.

> There are lots of other places to add to
> the history on shell exit.  But this is why I asked -- other opinions?
> 

Error in zshaddhistory() should not prevent command from running

[Bart Schaefer]

Should this actually be in callhookfunc() itself?  I note that the doc
warns about errors in precmd killing off periodic, or else I'd say it
ought to be.

Possibly more controversial, why run this hook at all when nothing is
being added?

diff --git a/Src/hist.c b/Src/hist.c
index 97fd340..350688d 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1418,7 +1418,7 @@ hend(Eprog prog)
 	DPUTS(hptr < chline, "History end pointer off start of line");
 	*hptr = '\0';
     }
-    {
+    if (*chline) {
 	LinkList hookargs = newlinklist();
 	int save_errflag = errflag;
 	errflag = 0;
@@ -1427,6 +1427,7 @@ hend(Eprog prog)
 	addlinknode(hookargs, chline);
 	callhookfunc("zshaddhistory", hookargs, 1, &hookret);
 
+	errflag &= ~ERRFLAG_ERROR;
 	errflag |= save_errflag;
     }
     /* For history sharing, lock history file once for both read and write */