Re: Bug#749969: history no longer syncs immediately, INC_APPEND_HISTORY broken

Re: Bug#749969: history no longer syncs immediately, INC_APPEND_HISTORY broken

[Frank Terbeck]

Hi list,

Debian's BTS received a regression report for zsh.

I'll include a slightly shortened account of the bug-reporter's
findings. The version in "stable" he references is 4.3.17; the version
that shows the broken behaviour is 5.0.5 (and I suspect that releases
starting at 5.0.3 will contain the regression):

Peter Palfrader wrote:
[...]
> since upgrading from stable, I noticed that zsh no longer adds commands
> to the history immediately when they are started.  They only get added
> when they have finished.
>
> That's a regression from stable, where things got added as soon as they
> started.
>
> This is as important feature for me, as I often want to run similar
> (long running) commands at the same time.  I.e., I issue a command, open
> a new shell in a new terminal emulator window, slightly modify it and
> send it off.  Now that's no longer possible.
>
> E.g.:
>  - Run "sleep 10" in a zsh in your favorite terminal emulator.
>  - Open a second window.  "sleep 10" is not yet in your history.
>  - <wait 10 seconds>
>  - Open a third window.  "sleep 10" is now there.
>
> Ideally the sleep 10 would be there even in the second window.
[...]
> All I ask is that INC_APPEND_HISTORY again work as documented:
>
> | This  options  works like APPEND_HISTORY except that new history
> | lines are added to the $HISTFILE incrementally (as soon as  they
>                                                   ^^^^^^^^^^^^^^^^
> | are  entered)
>   ^^^^^^^^^^^^

I've skimmed through our version control history, and I'm pretty sure
the reason for this is 16ff79a885f aka workers-31789 aka ¹. And I agree
that it's a regression, that breaks long standing behaviour, which
affects at least the use case that Peter describes.

I think to enable both ways to be possible, there should be two options:
INC_APPEND_HISTORY that enables the original behaviour and (if the name
is agreeable) DELAY_INC_APPEND_HISTORY, that amends the behaviour of the
original option in the way that is reflected by the current behaviour.

If your first instinct is to suggest enabling SHARE_HISTORY, then please
note, that that's a rather intrusive option that can't possibly be
forced onto users that don't want it (I wouldn't want it, and the bug
reporter doesn't either).

CC:ing Han, who is the original author. PWS, who did some refinements on
top of Han's original code, reads all mails on -workers anyway. ;)


Regards, Frank

¹ http://www.zsh.org/mla/workers/2013/msg00824.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: Bug#749969: history no longer syncs immediately, INC_APPEND_HISTORY broken

[Peter Stephenson]

On Sat, 31 May 2014 11:21:59 +0200
Frank Terbeck <ft@bewatermyfriend.org> wrote:
> I think to enable both ways to be possible, there should be two options:
> INC_APPEND_HISTORY that enables the original behaviour and (if the name
> is agreeable) DELAY_INC_APPEND_HISTORY, that amends the behaviour of the
> original option in the way that is reflected by the current behaviour.

The issue is that, without rewriting the history, either the time taken
for the command is wrong (that was the old case) or the command isn't
written out to the history until it's finished (that's the current
position).  Writing the history to get the best of both worlds probably
isn't beyond the bounds of 21st century science, but it's beyond my
understanding of the history code at the moment.  So at the moment
having two options looks like the only way to allow you to achieve any
one of the effects.  I think the new behaviour could be
INC_APPEND_HISTORY_TIME which puts it next to the original one and
indicates why it exists.

I haven't looked, but I don't think this ought to be too difficult ---
there are still some reasons why you might have to deal with history at
the original place anyway, so it should be possible to add an option
test at that point as well as one at the later point.

pws

Re: Bug#749969: history no longer syncs immediately, INC_APPEND_HISTORY broken

[Wayne Davison]

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

On Sat, May 31, 2014 at 2:21 AM, Frank Terbeck <ft@bewatermyfriend.org>
wrote:

> I think to enable both ways to be possible, there should be two options:
> INC_APPEND_HISTORY that enables the original behaviour and (if the name
> is agreeable) DELAY_INC_APPEND_HISTORY, that amends the behaviour of the
> original option in the way that is reflected by the current behaviour.
>

You should just be able to setopt SHARE_HISTORY to have zsh always append
the entry early.  Of course, that also has the effect of the shell
importing the shared history from other shells, so it's nice to have things
like up/down arrow set to only browse local history (while things like
search look in the shared history).  There was a fairly recent discussion
of up-line-or-local-history() functions and a discussion of making
everything local except for isearch functions.

..wayne..

Re: Bug#749969: history no longer syncs immediately, INC_APPEND_HISTORY broken

[Peter Stephenson]

On Sat, 31 May 2014 20:22:11 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Sat, 31 May 2014 11:21:59 +0200
> Frank Terbeck <ft@bewatermyfriend.org> wrote:
> > I think to enable both ways to be possible, there should be two options:
> > INC_APPEND_HISTORY that enables the original behaviour and (if the name
> > is agreeable) DELAY_INC_APPEND_HISTORY, that amends the behaviour of the
> > original option in the way that is reflected by the current behaviour.
> 
> The issue is that, without rewriting the history, either the time taken
> for the command is wrong (that was the old case) or the command isn't
> written out to the history until it's finished (that's the current
> position).  Writing the history to get the best of both worlds probably
> isn't beyond the bounds of 21st century science, but it's beyond my
> understanding of the history code at the moment.  So at the moment
> having two options looks like the only way to allow you to achieve any
> one of the effects.  I think the new behaviour could be
> INC_APPEND_HISTORY_TIME which puts it next to the original one and
> indicates why it exists.
> 
> I haven't looked, but I don't think this ought to be too difficult ---
> there are still some reasons why you might have to deal with history at
> the original place anyway, so it should be possible to add an option
> test at that point as well as one at the later point.

I think this works, but it's getting pretty silly.

diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
index 349946d..7e46048 100644
--- a/Doc/Zsh/options.yo
+++ b/Doc/Zsh/options.yo
@@ -971,6 +971,19 @@ The file will still be periodically re-written to trim it when the
 number of lines grows 20% beyond the value specified by
 tt($SAVEHIST) (see also the HIST_SAVE_BY_COPY option).
 )
+pindex(INC_APPEND_HISTORY_TIME)
+pindex(NO_INC_APPEND_HISTORY_TIME)
+pindex(INCAPPENDHISTORYTIME)
+pindex(NOINCAPPENDHISTORYTIME)
+cindex(history, incremental appending to a file with time)
+item(tt(INC_APPEND_HISTORY_TIME))(
+This option is a variant of tt(INC_APPEND_HISTORY) in which, where
+possible, the history entry is written out to the file after the
+command is finished, so that the time taken by the command is recorded
+correctly in the history file in tt(EXTENDED_HISTORY) format.  This
+means that the history entry will not be available immediately from
+other instances of the shell that are using the same history file.
+)
 pindex(SHARE_HISTORY)
 pindex(NO_SHARE_HISTORY)
 pindex(SHAREHISTORY)
diff --git a/Src/hist.c b/Src/hist.c
index 1182994..64f88f5 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -935,9 +935,11 @@ hbegin(int dohist)
 
     hf = getsparam("HISTFILE");
     /*
-     * For INCAPPENDHISTORY, when interactive, save the history here
+     * For INCAPPENDHISTORYTIME, when interactive, save the history here
      * as it gives a better estimate of the times of commands.
      *
+     * If INCAPPENDHISTORY is also set we've already done it.
+     *
      * If SHAREHISTORY is also set continue to do so in the
      * standard place, because that's safer about reading and
      * rewriting history atomically.
@@ -950,7 +952,8 @@ hbegin(int dohist)
      * so that (correctly) nothing happens here.  But it shows
      * I thought about it.
      */
-    if (isset(INCAPPENDHISTORY) && !isset(SHAREHISTORY) &&
+    if (isset(INCAPPENDHISTORYTIME) && !isset(SHAREHISTORY) &&
+	!isset(INCAPPENDHISTORY) &&
 	!(histactive & HA_NOINC) && !strin && histsave_stack_pos == 0)
 	savehistfile(hf, 0, HFILE_USE_OPTIONS | HFILE_FAST);
 }
@@ -1378,7 +1381,8 @@ hend(Eprog prog)
      * For normal INCAPPENDHISTORY case and reasoning, see hbegin().
      */
     if (isset(SHAREHISTORY) ? histfileIsLocked() :
-	(isset(INCAPPENDHISTORY) && histsave_stack_pos != 0))
+	(isset(INCAPPENDHISTORY) || (isset(INCAPPENDHISTORYTIME) &&
+				     histsave_stack_pos != 0)))
 	savehistfile(hf, 0, HFILE_USE_OPTIONS | HFILE_FAST);
     unlockhistfile(hf); /* It's OK to call this even if we aren't locked */
     /*
@@ -2542,7 +2546,7 @@ savehistfile(char *fn, int err, int writeflags)
     }
     if (writeflags & HFILE_USE_OPTIONS) {
 	if (isset(APPENDHISTORY) || isset(INCAPPENDHISTORY)
-	 || isset(SHAREHISTORY))
+	 || isset(INCAPPENDHISTORYTIME) || isset(SHAREHISTORY))
 	    writeflags |= HFILE_APPEND | HFILE_SKIPOLD;
 	else
 	    histfile_linect = 0;
@@ -2578,7 +2582,7 @@ savehistfile(char *fn, int err, int writeflags)
 		tmpfile = NULL;
 		if (err) {
 		    if (isset(APPENDHISTORY) || isset(INCAPPENDHISTORY)
-		     || isset(SHAREHISTORY))
+		     || isset(INCAPPENDHISTORYTIME) || isset(SHAREHISTORY))
 			zerr("rewriting %s would change its ownership -- skipped", fn);
 		    else
 			zerr("rewriting %s would change its ownership -- history not saved", fn);
diff --git a/Src/options.c b/Src/options.c
index e83dc58..2163bff 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -165,6 +165,7 @@ static struct optname optns[] = {
 {{NULL, "ignoreclosebraces",  OPT_EMULATE},		 IGNORECLOSEBRACES},
 {{NULL, "ignoreeof",	      0},			 IGNOREEOF},
 {{NULL, "incappendhistory",   0},			 INCAPPENDHISTORY},
+{{NULL, "incappendhistorytime",   0},			 INCAPPENDHISTORYTIME},
 {{NULL, "interactive",	      OPT_SPECIAL},		 INTERACTIVE},
 {{NULL, "interactivecomments",OPT_BOURNE},		 INTERACTIVECOMMENTS},
 {{NULL, "ksharrays",	      OPT_EMULATE|OPT_BOURNE},	 KSHARRAYS},
diff --git a/Src/zsh.h b/Src/zsh.h
index 620883b..05d582c 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2115,6 +2115,7 @@ enum {
     IGNORECLOSEBRACES,
     IGNOREEOF,
     INCAPPENDHISTORY,
+    INCAPPENDHISTORYTIME,
     INTERACTIVE,
     INTERACTIVECOMMENTS,
     KSHARRAYS,

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

Re: Bug#749969: history no longer syncs immediately, INC_APPEND_HISTORY broken

[Han Pingtian]

I have just a quick idea for it :)

How about in this case, write the command into history first in the
hend(), then rewrite the history in hbegin() after the commnad finished?

Thanks.

On Tue, Jun 03, 2014 at 08:56:07PM +0100, Peter Stephenson wrote:
> On Sat, 31 May 2014 20:22:11 +0100
> Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> > On Sat, 31 May 2014 11:21:59 +0200
> > Frank Terbeck <ft@bewatermyfriend.org> wrote:
> > > I think to enable both ways to be possible, there should be two options:
> > > INC_APPEND_HISTORY that enables the original behaviour and (if the name
> > > is agreeable) DELAY_INC_APPEND_HISTORY, that amends the behaviour of the
> > > original option in the way that is reflected by the current behaviour.
> > 
> > The issue is that, without rewriting the history, either the time taken
> > for the command is wrong (that was the old case) or the command isn't
> > written out to the history until it's finished (that's the current
> > position).  Writing the history to get the best of both worlds probably
> > isn't beyond the bounds of 21st century science, but it's beyond my
> > understanding of the history code at the moment.  So at the moment
> > having two options looks like the only way to allow you to achieve any
> > one of the effects.  I think the new behaviour could be
> > INC_APPEND_HISTORY_TIME which puts it next to the original one and
> > indicates why it exists.
> > 
> > I haven't looked, but I don't think this ought to be too difficult ---
> > there are still some reasons why you might have to deal with history at
> > the original place anyway, so it should be possible to add an option
> > test at that point as well as one at the later point.
> 
> I think this works, but it's getting pretty silly.
> 
> diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
> index 349946d..7e46048 100644
> --- a/Doc/Zsh/options.yo
> +++ b/Doc/Zsh/options.yo
> @@ -971,6 +971,19 @@ The file will still be periodically re-written to trim it when the
>  number of lines grows 20% beyond the value specified by
>  tt($SAVEHIST) (see also the HIST_SAVE_BY_COPY option).
>  )
> +pindex(INC_APPEND_HISTORY_TIME)
> +pindex(NO_INC_APPEND_HISTORY_TIME)
> +pindex(INCAPPENDHISTORYTIME)
> +pindex(NOINCAPPENDHISTORYTIME)
> +cindex(history, incremental appending to a file with time)
> +item(tt(INC_APPEND_HISTORY_TIME))(
> +This option is a variant of tt(INC_APPEND_HISTORY) in which, where
> +possible, the history entry is written out to the file after the
> +command is finished, so that the time taken by the command is recorded
> +correctly in the history file in tt(EXTENDED_HISTORY) format.  This
> +means that the history entry will not be available immediately from
> +other instances of the shell that are using the same history file.
> +)
>  pindex(SHARE_HISTORY)
>  pindex(NO_SHARE_HISTORY)
>  pindex(SHAREHISTORY)
> diff --git a/Src/hist.c b/Src/hist.c
> index 1182994..64f88f5 100644
> --- a/Src/hist.c
> +++ b/Src/hist.c
> @@ -935,9 +935,11 @@ hbegin(int dohist)
> 
>      hf = getsparam("HISTFILE");
>      /*
> -     * For INCAPPENDHISTORY, when interactive, save the history here
> +     * For INCAPPENDHISTORYTIME, when interactive, save the history here
>       * as it gives a better estimate of the times of commands.
>       *
> +     * If INCAPPENDHISTORY is also set we've already done it.
> +     *
>       * If SHAREHISTORY is also set continue to do so in the
>       * standard place, because that's safer about reading and
>       * rewriting history atomically.
> @@ -950,7 +952,8 @@ hbegin(int dohist)
>       * so that (correctly) nothing happens here.  But it shows
>       * I thought about it.
>       */
> -    if (isset(INCAPPENDHISTORY) && !isset(SHAREHISTORY) &&
> +    if (isset(INCAPPENDHISTORYTIME) && !isset(SHAREHISTORY) &&
> +	!isset(INCAPPENDHISTORY) &&
>  	!(histactive & HA_NOINC) && !strin && histsave_stack_pos == 0)
>  	savehistfile(hf, 0, HFILE_USE_OPTIONS | HFILE_FAST);
>  }
> @@ -1378,7 +1381,8 @@ hend(Eprog prog)
>       * For normal INCAPPENDHISTORY case and reasoning, see hbegin().
>       */
>      if (isset(SHAREHISTORY) ? histfileIsLocked() :
> -	(isset(INCAPPENDHISTORY) && histsave_stack_pos != 0))
> +	(isset(INCAPPENDHISTORY) || (isset(INCAPPENDHISTORYTIME) &&
> +				     histsave_stack_pos != 0)))
>  	savehistfile(hf, 0, HFILE_USE_OPTIONS | HFILE_FAST);
>      unlockhistfile(hf); /* It's OK to call this even if we aren't locked */
>      /*
> @@ -2542,7 +2546,7 @@ savehistfile(char *fn, int err, int writeflags)
>      }
>      if (writeflags & HFILE_USE_OPTIONS) {
>  	if (isset(APPENDHISTORY) || isset(INCAPPENDHISTORY)
> -	 || isset(SHAREHISTORY))
> +	 || isset(INCAPPENDHISTORYTIME) || isset(SHAREHISTORY))
>  	    writeflags |= HFILE_APPEND | HFILE_SKIPOLD;
>  	else
>  	    histfile_linect = 0;
> @@ -2578,7 +2582,7 @@ savehistfile(char *fn, int err, int writeflags)
>  		tmpfile = NULL;
>  		if (err) {
>  		    if (isset(APPENDHISTORY) || isset(INCAPPENDHISTORY)
> -		     || isset(SHAREHISTORY))
> +		     || isset(INCAPPENDHISTORYTIME) || isset(SHAREHISTORY))
>  			zerr("rewriting %s would change its ownership -- skipped", fn);
>  		    else
>  			zerr("rewriting %s would change its ownership -- history not saved", fn);
> diff --git a/Src/options.c b/Src/options.c
> index e83dc58..2163bff 100644
> --- a/Src/options.c
> +++ b/Src/options.c
> @@ -165,6 +165,7 @@ static struct optname optns[] = {
>  {{NULL, "ignoreclosebraces",  OPT_EMULATE},		 IGNORECLOSEBRACES},
>  {{NULL, "ignoreeof",	      0},			 IGNOREEOF},
>  {{NULL, "incappendhistory",   0},			 INCAPPENDHISTORY},
> +{{NULL, "incappendhistorytime",   0},			 INCAPPENDHISTORYTIME},
>  {{NULL, "interactive",	      OPT_SPECIAL},		 INTERACTIVE},
>  {{NULL, "interactivecomments",OPT_BOURNE},		 INTERACTIVECOMMENTS},
>  {{NULL, "ksharrays",	      OPT_EMULATE|OPT_BOURNE},	 KSHARRAYS},
> diff --git a/Src/zsh.h b/Src/zsh.h
> index 620883b..05d582c 100644
> --- a/Src/zsh.h
> +++ b/Src/zsh.h
> @@ -2115,6 +2115,7 @@ enum {
>      IGNORECLOSEBRACES,
>      IGNOREEOF,
>      INCAPPENDHISTORY,
> +    INCAPPENDHISTORYTIME,
>      INTERACTIVE,
>      INTERACTIVECOMMENTS,
>      KSHARRAYS,
> 
> -- 
> Peter Stephenson <p.w.stephenson@ntlworld.com>
> Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: Bug#749969: history no longer syncs immediately, INC_APPEND_HISTORY broken

[Bart Schaefer]

On Jun 4,  3:57pm, Han Pingtian wrote:
} Subject: Re: Bug#749969: history no longer syncs immediately, INC_APPEND_H
}
} I have just a quick idea for it :)
} 
} How about in this case, write the command into history first in the
} hend(), then rewrite the history in hbegin() after the commnad finished?

This would work only if we were guaranteed that no other zsh session
had manipulated the history file in the meantime.  Given that the only
reason to care how soon the command is written is because there *is*
another zsh session manipulating the file, this approach unfortunately
is probably not practical.

It'd be more practical if history was maintained as an actual database,
and maybe nobody cares any more that the history be readable as text or by
other shells ... but that's a pretty large change.

Re: Bug#749969: history no longer syncs immediately, INC_APPEND_HISTORY broken

[Bart Schaefer]

On Jun 3,  8:56pm, Peter Stephenson wrote:
}
} I think this works, but it's getting pretty silly.

There are a number of cases where binary on/off options don't really lend
themselves to describing complex behavior.

 
} +     * For INCAPPENDHISTORYTIME, when interactive, save the history here
}       * as it gives a better estimate of the times of commands.
}       *
} +     * If INCAPPENDHISTORY is also set we've already done it.

I was somewhat concerned that this implies that given

    % setopt INCAPPENDHISTORY INCAPPENDHISTORYTIME
    % unsetopt INCAPPENDHISTORY

the unsetopt command would be appended to the history more than once,
but some other part of the tests apparently prevents that.

The doc for INC_APPEND_HISTORY_TIME should probably point out that the
option has no effect when INC_APPEND_HISTORY or SHARE_HISTORY is set,
and the "you want more control" paragraph under SHARE_HISTORY might
suggest INC_APPEND_HISTORY_TIME instead of INC_APPEND_HISTORY.