[PATCH] enable elaped time with incappendhistory

[PATCH] enable elaped time with incappendhistory

[Han Pingtian]

Hello,

Could you please reveiw this patch? Thanks in advance.



If incappendhistory being set, the command line will be written before
it has finished. So elaped time doesn't work. Looks like this patch
lets it working again. And sharehistory will still cause the command
line written before it has finished.
---
 Src/hist.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Src/hist.c b/Src/hist.c
index f78c97d..33a8357 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -871,6 +871,8 @@ unlinkcurline(void)
 mod_export void
 hbegin(int dohist)
 {
+    char *hf;
+
     isfirstln = isfirstch = 1;
     errflag = histdone = 0;
     if (!dohist)
@@ -924,6 +926,10 @@ hbegin(int dohist)
 	defev = addhistnum(curhist, -1, HIST_FOREIGN);
     } else
 	histactive = HA_ACTIVE | HA_NOINC;
+
+    hf = getsparam("HISTFILE");
+    if (isset(INCAPPENDHISTORY))
+	savehistfile(hf, 0, HFILE_USE_OPTIONS | HFILE_FAST);
 }
 
 /**/
@@ -1328,7 +1334,7 @@ hend(Eprog prog)
     chline = hptr = NULL;
     chwords = NULL;
     histactive = 0;
-    if (isset(SHAREHISTORY)? histfileIsLocked() : isset(INCAPPENDHISTORY))
+    if (isset(SHAREHISTORY) && histfileIsLocked())
 	savehistfile(hf, 0, HFILE_USE_OPTIONS | HFILE_FAST);
     unlockhistfile(hf); /* It's OK to call this even if we aren't locked */
     /*
-- 
1.7.7.6

Re: [PATCH] enable elaped time with incappendhistory

[Peter Stephenson]

On Sat, 5 Oct 2013 20:50:10 +0800
Han Pingtian <hanpt@linux.vnet.ibm.com> wrote:
> Could you please reveiw this patch? Thanks in advance.
> 
> If incappendhistory being set, the command line will be written before
> it has finished. So elaped time doesn't work. Looks like this patch
> lets it working again. And sharehistory will still cause the command
> line written before it has finished.

It's probably not going to help much simply staring at it.  I use
INCAPPENDHISTORY, so I've committed this and I'll see how it works.

pws

Re: [PATCH] enable elaped time with incappendhistory

[Peter Stephenson]

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

On Sat, 05 Oct 2013 22:04:50 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Sat, 5 Oct 2013 20:50:10 +0800
> Han Pingtian <hanpt@linux.vnet.ibm.com> wrote:
> > Could you please reveiw this patch? Thanks in advance.
> > 
> > If incappendhistory being set, the command line will be written before
> > it has finished. So elaped time doesn't work. Looks like this patch
> > lets it working again. And sharehistory will still cause the command
> > line written before it has finished.
> 
> It's probably not going to help much simply staring at it.  I use
> INCAPPENDHISTORY, so I've committed this and I'll see how it works.

I'm doing that and something appears to be broken...

It's got into a state where it's calling savehistfile() and
readhistfile() recursively from a readhistfile() and
is consequently being very slow.  This is actually from an "fc -p
<filename>" in a ZLE script.  I don't think INCAPPENDHISTORY is relevant
in this case, so maybe it just needs more tests for when to apply the
modified code.  Note that HISTLEXWORDS is set; I think that's important
because that's how we get from histsplitwords() down into bufferwords().

Adding the test for "strin" in hbegin() in the patch below seems to
remove the unwanted behaviour.  Comparing with the code around, I think
we also don't want to do this in the case HA_NOINC is present in
histactive.  I'll commit this immediately to remove the pathology, but
there may be more tweaks.

For what it's worth, here's the backtrace, though the stuff towards the
end isn't likely to be interesting.  (I've slightly edited the make line
that's being searched for but that shouldn't be relevant.)  I'll attach
the script, which is the history-beginning-local I think I posted some
weeks ago, though probably the only interesting bit is the fc.

#0  0x08091bba in zfree (p=0x99a1898, sz=0) at mem.c:1323
#1  0x080927a9 in free (p=0x99a1898) at mem.c:1487
#2  0x08088097 in lexrestore () at lex.c:342
#3  0x0807cb70 in bufferwords (list=0xb338a010, buf=0x99d20bf "make -s", index=0x0, flags=4) at hist.c:3088
#4  0x0807d06b in histsplitwords (lineptr=0x99d20bf "make -s", wordsp=0xbfa9585c, nwordsp=0xbfa95854, nwordposp=0xbfa95858, uselex=1) at hist.c:3136
#5  0x0807d744 in readhistfile (fn=0x99b8b00 ".zsh_local_history", err=0, readflags=0) at hist.c:2397
#6  0x0807c4cc in savehistfile (fn=0x99b8b00 ".zsh_local_history", err=0, writeflags=32771) at hist.c:2635
#7  0x0807c749 in hbegin (dohist=0) at hist.c:932
#8  0x0807c8b8 in strinbeg (dohist=0) at hist.c:808
#9  0x0807c9ea in bufferwords (list=0xb3397010, buf=0x99c8f8f "unit make", index=0x0, flags=4) at hist.c:2951
#10 0x0807d06b in histsplitwords (lineptr=0x99c8f8f "unit make", wordsp=0xbfa95b3c, nwordsp=0xbfa95b34, nwordposp=0xbfa95b38, uselex=1) at hist.c:3136
#11 0x0807d744 in readhistfile (fn=0xb42d4008 ".zsh_local_history", err=1, readflags=32768) at hist.c:2397
#12 0x0805baf9 in bin_fc (nam=0xb42d3ff0 "fc", argv=<value optimized out>, ops=0xbfa95e24, func=16) at builtin.c:1385
#13 0x0805f2d1 in execbuiltin (args=0xb42d3fc0, bn=0x80d70ec) at builtin.c:450
#14 0x0806c361 in execcmd (state=0xbfa978f4, input=0, output=0, how=2, last1=2) at exec.c:3247
#15 0x0806c88f in execpline2 (state=0xbfa978f4, pcode=<value optimized out>, how=2, input=0, output=0, last1=0) at exec.c:1683
#16 0x0806cc25 in execpline (state=0xbfa978f4, slcode=<value optimized out>, how=2, last1=0) at exec.c:1470
#17 0x0806d994 in execlist (state=0xbfa978f4, dont_change_job=1, exiting=0) at exec.c:1253
#18 0x0808c8fc in execif (state=0xbfa978f4, do_exec=0) at loop.c:515
#19 0x0806b8ea in execcmd (state=0xbfa978f4, input=0, output=0, how=18, last1=2) at exec.c:3206
#20 0x0806c88f in execpline2 (state=0xbfa978f4, pcode=<value optimized out>, how=18, input=0, output=0, last1=0) at exec.c:1683
#21 0x0806cc25 in execpline (state=0xbfa978f4, slcode=<value optimized out>, how=18, last1=0) at exec.c:1470
#22 0x0806d994 in execlist (state=0xbfa978f4, dont_change_job=1, exiting=0) at exec.c:1253
#23 0x0808c8fc in execif (state=0xbfa978f4, do_exec=0) at loop.c:515
#24 0x0806b8ea in execcmd (state=0xbfa978f4, input=0, output=0, how=18, last1=2) at exec.c:3206
#25 0x0806c88f in execpline2 (state=0xbfa978f4, pcode=<value optimized out>, how=18, input=0, output=0, last1=0) at exec.c:1683
#26 0x0806cc25 in execpline (state=0xbfa978f4, slcode=<value optimized out>, how=18, last1=0) at exec.c:1470
#27 0x0806d994 in execlist (state=0xbfa978f4, dont_change_job=1, exiting=0) at exec.c:1253
#28 0x0808ba9b in execfor (state=0xbfa978f4, do_exec=0) at loop.c:159
#29 0x0806b8ea in execcmd (state=0xbfa978f4, input=0, output=0, how=2, last1=2) at exec.c:3206
#30 0x0806c88f in execpline2 (state=0xbfa978f4, pcode=<value optimized out>, how=2, input=0, output=0, last1=0) at exec.c:1683
#31 0x0806cc25 in execpline (state=0xbfa978f4, slcode=<value optimized out>, how=2, last1=0) at exec.c:1470
#32 0x0806d994 in execlist (state=0xbfa978f4, dont_change_job=1, exiting=0) at exec.c:1253
#33 0x0808cd6a in exectry (state=0xbfa978f4, do_exec=0) at loop.c:648
#34 0x0806b8ea in execcmd (state=0xbfa978f4, input=0, output=0, how=2, last1=2) at exec.c:3206
#35 0x0806c88f in execpline2 (state=0xbfa978f4, pcode=<value optimized out>, how=2, input=0, output=0, last1=0) at exec.c:1683
#36 0x0806cc25 in execpline (state=0xbfa978f4, slcode=<value optimized out>, how=2, last1=0) at exec.c:1470
#37 0x0806d994 in execlist (state=0xbfa978f4, dont_change_job=1, exiting=0) at exec.c:1253
#38 0x0806df36 in execode (p=0x99b5b08, dont_change_job=1, exiting=0, context=0x80cadd5 "shfunc") at exec.c:1065
#39 0x08067ffd in runshfunc (prog=0x99b5b08, wrap=0x0, name=0xb42d33f0 "history-beginning-local") at exec.c:4834
#40 0x08068622 in doshfunc (shfunc=0x98dc318, doshargs=0x0, noreturnval=1) at exec.c:4725
#41 0x0052c808 in execzlefunc (func=0x9908420, args=<value optimized out>, set_bindk=0) at zle_main.c:1380
#42 0x0052cd0a in zlecore () at zle_main.c:1063
#43 0x0052d895 in zleread (lp=0x80eb540, rp=0x0, flags=7, context=0, init=0x549615 "zle-line-init", finish=0x549605 "zle-line-finish") at zle_main.c:1247
#44 0x0052eb83 in zle_main_entry (cmd=1, ap=0xbfa97f54 "\001") at zle_main.c:1908
#45 0x08081232 in zleentry (cmd=1) at init.c:1464
#46 0x08081e5a in inputline () at input.c:283
#47 ingetc () at input.c:219
#48 0x0807b1ec in ihgetc () at hist.c:282
#49 0x0808a6ee in gettok () at lex.c:714
#50 zshlex () at lex.c:395
#51 0x080a6f89 in parse_event () at parse.c:451
#52 0x0807e6b9 in loop (toplevel=1, justonce=0) at init.c:132
#53 0x0808179e in zsh_main (argc=1, argv=0xbfa981e4) at init.c:1619
#54 0x080530bb in main (argc=1, argv=0xbfa981e4) at ./main.c:93

pws

diff --git a/Src/hist.c b/Src/hist.c
index 1961d83..c58ce8b 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -928,7 +928,7 @@ hbegin(int dohist)
 	histactive = HA_ACTIVE | HA_NOINC;
 
     hf = getsparam("HISTFILE");
-    if (isset(INCAPPENDHISTORY))
+    if (isset(INCAPPENDHISTORY) && !(histactive & HA_NOINC) && !strin)
 	savehistfile(hf, 0, HFILE_USE_OPTIONS | HFILE_FAST);
 }
 

[-- Attachment #2: history-beginning-local --]
[-- Type: application/octet-stream, Size: 4783 bytes --]

# history-beginning-local
#
#   Mostly a drop-in replacement for history-beginning-search-backward
#   or -forward, depending on the name.
#
#   Allows a local history file to be searched for certain commands to execute.
#   The style local-history-file should be set to a list of files that
#   can contain the local history, in the same format as $HISTFILE.
#   The files are searched in order until the first is find.  They
#   can contain a relative or absolute path; clearly an absolute
#   path to an existing file will always match.
#
#   local-history-commands should be set to a list of commands
#   (or patterns matching commands) that should use the local history
#   file; this can be "*" to handle all commands.  Both styles be set.
#
#   Alternatively (or in addition), local-history-pattern is a scalar
#   that gives a pattern that must match the command line exactly to
#   initiate use of the mechanism.  Typically this will end in a "*".
#
#   If the style local-history-only is not set the global history
#   will be searched if there is no match in the local history.
#   The global history is tried again from the most recent entry;
#   no ordering is implied between the two histories.  (Note
#   this style is also checked by zshaddhistory-local to decide
#   whether to save a history entry to the global history.)
#
#   If the style local-history-verbose is set a notice is printed
#   below the command line if the local history was searched.
#
#   Styles use the standard form of components separated by colons.
#   The components are
#    - The string "zhist": used rather than "zle" to make this consistent
#      across different functions handling history in different parts of
#      the shell.
#    - The widget name
#    - The current directory as given by $PWD.
#   There is a terminating colon.  It is recommended that a wildcard be
#   used at the end to protect against future enhancements.
#
#   For example,
#    zstyle ':zhist:*' local-history-file .zsh-local-history
#    zstyle ':zhist:*' local-history-commands make gcc gdb

emulate -L zsh
setopt extendedglob

local w f new lhp curline
local -a lhf lhc
integer lhv lho restore

typeset -gA __history_beginning_matches
if [[ $WIDGET != $LASTWIDGET ]]; then
  __history_beginning_matches=()
fi
integer oldcursor=CURSOR

zstyle -a ":zhist:${WIDGET}:${PWD}:" local-history-file lhf || return 1
zstyle -a ":zhist:${WIDGET}:${PWD}:" local-history-commands lhc || return 1
zstyle -s ":zhist:${WIDGET}:${PWD}:" local-history-pattern lhp
zstyle -t ":zhist:${WIDGET}:${PWD}:" local-history-verbose && lhv=1
zstyle -t ":zhist:${WIDGET}:${PWD}:" local-history-only && lho=1
# try / always block for restoring history
{
  for f in $lhf; do
    if [[ -f $f ]]; then
      integer iline
      local -a words found

      words=(${(z)BUFFER})
      if [[ ${(Q)words[1]} = (${(j.|.)~lhc}) || \
	    ( -n $lhp && ${BUFFER} = ${~lhp} ) ]]; then
	(( restore = 1 ))
	fc -p $f

	# Search history for pattern.
	# As $history is an associative array we can get all matches.
	if [[ $WIDGET = *forw* ]]; then
	  # Searching forward.  Look back through matches until we
	  # get back to the current history number.
	  found=(${(kOn)history[(R)${LBUFFER}*]})
	  for iline in $found; do
	    (( $iline <= HISTNO )) && break
	    # Skip duplicates.
	    curline=$history[$iline]
	    [[ $curline = $BUFFER ]] && continue
	    [[ -n ${__history_beginning_matches[$curline]} ]] && continue
	    new=$iline
	  done
	else
	  # Searching backward.  Look forward through matches until we
	  # reach the current history number.
	  found=(${(kon)history[(R)${LBUFFER}*]})
	  for iline in $found; do
	    (( $iline >= HISTNO )) && break
	    # Skip duplicates.
	    curline=$history[$iline]
	    [[ $curline = $BUFFER ]] && continue
	    [[ -n ${__history_beginning_matches[$curline]} ]] && continue
	    new=$iline
	  done
	fi
	[[ -n $new ]] && break
	fc -P
	(( restore = 0 ))
      fi
    fi
  done
  if [[ -n $new ]]; then
    # Match found.  Move to line.
    HISTNO=$new
    __history_beginning_matches[$history[$HISTNO]]=1
    if [[ $WIDGET = *-end* ]]; then
      zle end-of-line
    else
      (( CURSOR = oldcursor ))
    fi
    (( lhv )) && zle -M "Matched in local history"
    return 0
  elif (( lho )); then
    (( lhv )) && zle -M "No match in local history"
    return 1
  else
    (( lhv )) && zle -M "No match in local history; falling through"
  fi
} always {
  (( restore )) && fc -P
}

if [[ $WIDGET = *forw* ]]; then
  zle history-beginning-search-forward
else
  zle history-beginning-search-backward
fi
local stat=$?

if (( $stat == 0 )); then
  __history_beginning_matches[$history[$HISTNO]]=1
fi

if [[ $WIDGET = *-end* ]]; then
  zle end-of-line
else
  (( CURSOR = oldcursor ))
fi

return $stat
# end

Re: [PATCH] enable elaped time with incappendhistory

[Peter Stephenson]

On Mon, 07 Oct 2013 11:32:52 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> I'll commit this immediately to remove the pathology, but
> there may be more tweaks.

This is a somewhat non-standard setting, but we probably need to
continue to allow SHAREHISTORY to override INCAPPENDHISTORY: it always
did but now we need a separate test for the new case.

pws


diff --git a/Src/hist.c b/Src/hist.c
index c58ce8b..d1af30a 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -928,7 +928,15 @@ hbegin(int dohist)
 	histactive = HA_ACTIVE | HA_NOINC;
 
     hf = getsparam("HISTFILE");
-    if (isset(INCAPPENDHISTORY) && !(histactive & HA_NOINC) && !strin)
+    /*
+     * For INCAPPENDHISTORY, when interactive, save the history here
+     * as it gives a better estimate of the times of commands.
+     * If SHAREHISTORY is also set continue to do so in the
+     * standard place, because that's safer about reading and
+     * rewriting history atomically.
+     */
+    if (isset(INCAPPENDHISTORY) && !isset(SHAREHISTORY) &&
+	!(histactive & HA_NOINC) && !strin)
 	savehistfile(hf, 0, HFILE_USE_OPTIONS | HFILE_FAST);
 }
 

Re: [PATCH] enable elaped time with incappendhistory

[Peter Stephenson]

I think this change has broken use of "fc -p" to switch files
temporarily within an zshaddhistory hook, too, but I'll have to look
tomorrow.

pws

Re: [PATCH] enable elaped time with incappendhistory

[Peter Stephenson]

On Thu, 10 Oct 2013 18:30:35 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> I think this change has broken use of "fc -p" to switch files
> temporarily within an zshaddhistory hook, too, but I'll have to look
> tomorrow.

Yes, it did.

I think, after trying it out and pondering the result, this sorts it all
out, but I've added a tortured essay anyway.

There is some kind of law of conservation of spaghetti code for zsh.

I'm wondering if there's a neater way of doing this, but I think the
answer is "not much neater, anyway".

diff --git a/Src/hist.c b/Src/hist.c
index bd650e8..5531f47 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -937,12 +937,21 @@ hbegin(int dohist)
     /*
      * For INCAPPENDHISTORY, when interactive, save the history here
      * as it gives a better estimate of the times of commands.
+     *
      * If SHAREHISTORY is also set continue to do so in the
      * standard place, because that's safer about reading and
      * rewriting history atomically.
+     *
+     * The histsave_stack_pos test won't usually fail here.
+     * We need to test the opposite for the hend() case because we
+     * need to save in the history file we've switched to, but then
+     * we pop immediately after that so the variable becomes zero.
+     * We will already have saved the line and restored the history
+     * so that (correctly) nothing happens here.  But it shows
+     * I thought about it.
      */
     if (isset(INCAPPENDHISTORY) && !isset(SHAREHISTORY) &&
-	!(histactive & HA_NOINC) && !strin)
+	!(histactive & HA_NOINC) && !strin && histsave_stack_pos == 0)
 	savehistfile(hf, 0, HFILE_USE_OPTIONS | HFILE_FAST);
 }
 
@@ -1348,7 +1357,11 @@ hend(Eprog prog)
     chline = hptr = NULL;
     chwords = NULL;
     histactive = 0;
-    if (isset(SHAREHISTORY) && histfileIsLocked())
+    /*
+     * For normal INCAPPENDHISTORY case and reasoning, see hbegin().
+     */
+    if (isset(SHAREHISTORY) ? histfileIsLocked() :
+	(isset(INCAPPENDHISTORY) && 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 */
     /*

-- 
Peter Stephenson <p.stephenson@samsung.com>       Principal Software Engineer
Tel: +44 (0)1223 434724              Samsung Cambridge Solution Centre
St John's House, St John's Innovation Park,
Cowley Road, Cambridge, CB4 0DS, UK

Re: [PATCH] enable elaped time with incappendhistory

[Peter Stephenson]

On Fri, 11 Oct 2013 11:05:33 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> I'm wondering if there's a neater way of doing this, but I think the
> answer is "not much neater, anyway".

May as well mention it before I go back to sleep...

Actually, I think we could split hend() in two.  The early part needs to
return a status indicating if it's OK to execute the line that's been
retrieved (remember the history mechanism sits on top of the input
mechanism, an unusual case of the shell obeying the standard rules of
hierarchy).  I don't think it actually needs to perform the save (or
sharehistory rewrite) at that point.  It would probably be safe to do
that later in loop().

The one thing this might get wrong is if you were using share_history
and expected to be able to retrieve a line you were still executing in
another window, i.e. corresponding to a command that takes a significant
time.  This sounds a plausible requirement.  Obviously it's entirely
incompatible with the requirement of getting the execution time for the
command correct.  (Well, unless you save the line twice using
the sharehistory editing mechanism, but this is getting silly.)

I don't actually intend to look further at this unless the current
situation proves impossible to handle.

pws