HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Vincent Lefevre]

With zsh 5.9, HIST_IGNORE_DUPS is buggy: it also ignores command lines
that differ by a space between (single or double) quotes.

For instance, with

  setopt HIST_IGNORE_DUPS
  echo " "
  echo "  "

(the first echo with only 1 space character between the quotes,
the second echo with 2 space characters between the quotes), zsh
keeps only the second one in the history.

Such spaces are significant, so that these command lines are not dups.

However, if the spaces are quoted, this problem does not occur:

  echo \ .
  echo \ \ .

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Bart Schaefer]

On Wed, Mar 13, 2024 at 5:38 AM Vincent Lefevre <vincent@vinc17.net> wrote:
>
> With zsh 5.9, HIST_IGNORE_DUPS is buggy: it also ignores command lines
> that differ by a space between (single or double) quotes.

With zsh back to at least April 1, 2000 and probably sooner.
Searching history with the ZLE widgets works the same way.

Doing a full syntax analysis here would make it a lot more complicated
and a lot slower.

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Vincent Lefevre]

On 2024-03-13 22:13:09 -0700, Bart Schaefer wrote:
> On Wed, Mar 13, 2024 at 5:38 AM Vincent Lefevre <vincent@vinc17.net> wrote:
> >
> > With zsh 5.9, HIST_IGNORE_DUPS is buggy: it also ignores command lines
> > that differ by a space between (single or double) quotes.
> 
> With zsh back to at least April 1, 2000 and probably sooner.
> Searching history with the ZLE widgets works the same way.
> 
> Doing a full syntax analysis here would make it a lot more complicated
> and a lot slower.

I don't think you need to do a full syntax analysis. Isn't this just
related to tokenization (something like that)?

Note that spelling correction, which occurs *before* the line is put
in the history, detects quoted text and won't try to correct it. So
it seems that "quoted status" can be taken into account at that point.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Bart Schaefer]

On Fri, Mar 15, 2024 at 5:53 AM Vincent Lefevre <vincent@vinc17.net> wrote:
>
> On 2024-03-13 22:13:09 -0700, Bart Schaefer wrote:
> > Doing a full syntax analysis here would make it a lot more complicated
> > and a lot slower.
>
> I don't think you need to do a full syntax analysis. Isn't this just
> related to tokenization (something like that)?

Tokenization is performed by lexical analysis.

> Note that spelling correction, which occurs *before* the line is put
> in the history, detects quoted text and won't try to correct it.

Spelling correction is actually performed by the lexer, at the same
time as alias expansion.

> it seems that "quoted status" can be taken into account at that point.

Not without separately storing both the original and lexed state of
the text.  Which is in fact done internally, but for hopefully obvious
reasons is not done in the history file, which (during
reading/writing) is where most duplicate elimination has to occur.

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Vincent Lefevre]

On 2024-03-16 09:00:28 -0700, Bart Schaefer wrote:
> On Fri, Mar 15, 2024 at 5:53 AM Vincent Lefevre <vincent@vinc17.net> wrote:
> > Note that spelling correction, which occurs *before* the line is put
> > in the history, detects quoted text and won't try to correct it.
> 
> Spelling correction is actually performed by the lexer, at the same
> time as alias expansion.
> 
> > it seems that "quoted status" can be taken into account at that point.
> 
> Not without separately storing both the original and lexed state of
> the text.

I don't understand what you mean. The original text does *not*
seem to be used, as what is put in the history is the contents
*after* spelling correction. Moreover, the difference concerning
spaces between word splitting and quoted text is already taking
into account for HIST_REDUCE_BLANKS (which I'm using). I don't
see why it cannot be used for HIST_IGNORE_DUPS too.

qaa:~> echo fil   "foo   bar"
zsh: correct 'fil' to 'file' [nyae]? y
file foo   bar

Recalling the command from the history:

qaa:~> echo file "foo   bar"

As you can see, in the history, "fil" has changed to "file" as
corrected, and the 3 spaces after "fil"/"file" have been squashed
to a single one due to HIST_REDUCE_BLANKS.

> Which is in fact done internally, but for hopefully obvious
> reasons is not done in the history file, which (during
> reading/writing) is where most duplicate elimination has to occur.

The history file is not concerned here. This happens with "zsh -f",
where there is no history file:

qaa:~> zsh -f
qaa% setopt HIST_IGNORE_DUPS
qaa% echo "a b"
a b
qaa% echo "a  b"
a  b
qaa% history 
    1  setopt HIST_IGNORE_DUPS
    2  echo "a  b"

BTW, the zshoptions(1) man page correctly says "history list",
not "history file":

    HIST_IGNORE_DUPS (-h)
        Do not enter command lines into the history list if they are
        duplicates of the previous event.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Mikael Magnusson]

in hist.c:
    if ((isset(HISTIGNOREDUPS) || isset(HISTIGNOREALLDUPS)) && save > 0
     && hist_ring && histstrcmp(chline, hist_ring->node.nam) == 0) {
        /* This history entry compares the same as the previous.
         * In case minor changes were made, we overwrite the
         * previous one with the current one.  This also gets the
         * timestamp right.  Perhaps, preserve the HIST_OLD flag.
         */
        he = hist_ring;
        newflags |= he->node.flags & HIST_OLD; /* Avoid re-saving */
        freehistdata(he, 0);
        curline.histnum = curhist;

and in hashtable.c (weird place for it):
/* Compare two strings with normalized white-space */

/**/
int
histstrcmp(const char *str1, const char *str2)
{
    while (inblank(*str1)) str1++;
    while (inblank(*str2)) str2++;
    while (*str1 && *str2) {
    if (inblank(*str1)) {
        if (!inblank(*str2))
        break;
        do str1++; while (inblank(*str1));
        do str2++; while (inblank(*str2));
    }
    else {
        if (*str1 != *str2)
        break;
        str1++;
        str2++;
    }
    }
    return *str1 - *str2;
}

seems you could simply replace histstrcmp with strcmp and be happy.

On Tue, Mar 19, 2024 at 11:58 AM Vincent Lefevre <vincent@vinc17.net> wrote:
>
> On 2024-03-16 09:00:28 -0700, Bart Schaefer wrote:
> > On Fri, Mar 15, 2024 at 5:53 AM Vincent Lefevre <vincent@vinc17.net> wrote:
> > > Note that spelling correction, which occurs *before* the line is put
> > > in the history, detects quoted text and won't try to correct it.
> >
> > Spelling correction is actually performed by the lexer, at the same
> > time as alias expansion.
> >
> > > it seems that "quoted status" can be taken into account at that point.
> >
> > Not without separately storing both the original and lexed state of
> > the text.
>
> I don't understand what you mean. The original text does *not*
> seem to be used, as what is put in the history is the contents
> *after* spelling correction. Moreover, the difference concerning
> spaces between word splitting and quoted text is already taking
> into account for HIST_REDUCE_BLANKS (which I'm using). I don't
> see why it cannot be used for HIST_IGNORE_DUPS too.
>
> qaa:~> echo fil   "foo   bar"
> zsh: correct 'fil' to 'file' [nyae]? y
> file foo   bar
>
> Recalling the command from the history:
>
> qaa:~> echo file "foo   bar"
>
> As you can see, in the history, "fil" has changed to "file" as
> corrected, and the 3 spaces after "fil"/"file" have been squashed
> to a single one due to HIST_REDUCE_BLANKS.
>
> > Which is in fact done internally, but for hopefully obvious
> > reasons is not done in the history file, which (during
> > reading/writing) is where most duplicate elimination has to occur.
>
> The history file is not concerned here. This happens with "zsh -f",
> where there is no history file:
>
> qaa:~> zsh -f
> qaa% setopt HIST_IGNORE_DUPS
> qaa% echo "a b"
> a b
> qaa% echo "a  b"
> a  b
> qaa% history
>     1  setopt HIST_IGNORE_DUPS
>     2  echo "a  b"
>
> BTW, the zshoptions(1) man page correctly says "history list",
> not "history file":
>
>     HIST_IGNORE_DUPS (-h)
>         Do not enter command lines into the history list if they are
>         duplicates of the previous event.
>
> --
> Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
> 100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
>


-- 
Mikael Magnusson

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Vincent Lefevre]

On 2024-03-19 12:08:26 +0100, Mikael Magnusson wrote:
> in hist.c:
>     if ((isset(HISTIGNOREDUPS) || isset(HISTIGNOREALLDUPS)) && save > 0
>      && hist_ring && histstrcmp(chline, hist_ring->node.nam) == 0) {
>         /* This history entry compares the same as the previous.
>          * In case minor changes were made, we overwrite the
>          * previous one with the current one.  This also gets the
>          * timestamp right.  Perhaps, preserve the HIST_OLD flag.
>          */
>         he = hist_ring;
>         newflags |= he->node.flags & HIST_OLD; /* Avoid re-saving */
>         freehistdata(he, 0);
>         curline.histnum = curhist;
> 
> and in hashtable.c (weird place for it):
> /* Compare two strings with normalized white-space */

It is placed in hashtable.c perhaps because of its use for cmpnodes
in this file:

    histtab->cmpnodes    = histstrcmp;

But is this correct?

> /**/
> int
> histstrcmp(const char *str1, const char *str2)
[...]

> seems you could simply replace histstrcmp with strcmp and be happy.

Thanks. This now works as expected.

BTW, without this change, the following commands are regarded as
the same for HIST_IGNORE_DUPS, while they are very different:

qaa:~> printf "%s\n" a\ b
a b
qaa:~> printf "%s\n" a\  b
a 
b

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Bart Schaefer]

On Tue, Mar 19, 2024 at 5:34 AM Vincent Lefevre <vincent@vinc17.net> wrote:
>
> On 2024-03-19 12:08:26 +0100, Mikael Magnusson wrote:
> > and in hashtable.c (weird place for it):
> > /* Compare two strings with normalized white-space */
>
> It is placed in hashtable.c perhaps because of its use for cmpnodes

Yes.  That's connected to what I said about history search also
working the way ignoredups does.

That placement is not necessary given that it isn't a static function
... if it ever was, that's buried somewhere in the pre-version-control
zsh-workers archives.

> > seems you could simply replace histstrcmp with strcmp and be happy.
>
> Thanks. This now works as expected.
>
> BTW, without this change, the following commands are regarded as
> the same for HIST_IGNORE_DUPS, while they are very different:

Although this statement is correct, I'm reluctant to discard a quarter
century of practice about which there has not previously been a
complaint.  It could lead to significantly larger history files and/or
the "expiration" of commands with more obvious distinctions -- not
because those distinctions are more "important" but because they're
more likely to be noticed and their loss complained about.

So how about this as a compromise:

diff --git a/Src/hashtable.c b/Src/hashtable.c
index 75b06c4ad..e1f575a52 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -1397,6 +1397,14 @@ histstrcmp(const char *str1, const char *str2)
 {
     while (inblank(*str1)) str1++;
     while (inblank(*str2)) str2++;
+
+    /* If significant whitespace has already been eliminated,
+     * there is no reason to expend similar effort here.  Also,
+     * this is more accurate in cases of quoted whitespace.
+     */
+    if (isset(HISTREDUCEBLANKS))
+       return strcmp(str1, str2);
+
     while (*str1 && *str2) {
        if (inblank(*str1)) {
            if (!inblank(*str2))

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Bart Schaefer]

On Wed, Mar 20, 2024 at 10:46 AM Bart Schaefer
<schaefer@brasslantern.com> wrote:
>
> +    /* If significant whitespace has already been eliminated,

Except that should say "insignificant" ...

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Vincent Lefevre]

On 2024-03-20 10:46:44 -0700, Bart Schaefer wrote:
> So how about this as a compromise:
> 
> diff --git a/Src/hashtable.c b/Src/hashtable.c
> index 75b06c4ad..e1f575a52 100644
> --- a/Src/hashtable.c
> +++ b/Src/hashtable.c
> @@ -1397,6 +1397,14 @@ histstrcmp(const char *str1, const char *str2)
>  {
>      while (inblank(*str1)) str1++;
>      while (inblank(*str2)) str2++;
> +
> +    /* If significant whitespace has already been eliminated,
(insignificant)
> +     * there is no reason to expend similar effort here.  Also,
> +     * this is more accurate in cases of quoted whitespace.
> +     */
> +    if (isset(HISTREDUCEBLANKS))
> +       return strcmp(str1, str2);

That's OK for me.

The documentation of HIST_IGNORE_DUPS in the man page should also
be corrected / completed.

FYI, I had noticed this issue while I was doing tests on various
inputs (provided by "echo ... | command"), where I had to test
with different numbers of spaces, as they really matter. And I got
surprised that I didn't find some of my inputs in the history.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Jun T]

I think HIST_IGNORE_DUPS would work as expected even without
HIST_REDUCCE_BLANKS if we use the information in chwords and
hist_ring->words, as in the patch below, for example.

we can't simply replace histstrcmp() with the one that uses
the words[] array in struct histent. But I think Bart's patch
is enough for HIST_IGNORE_ALL_DUPS and history searching
(and recommend users to set HIST_REDUCCE_BLANKS).



diff --git a/Src/hist.c b/Src/hist.c
index 1a00c30ed..3a52ee038 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1467,6 +1467,36 @@ should_ignore_line(Eprog prog)
     return 0;
 }
 
+/*
+ * compare the history entry in {chline,chwords} with the previous
+ * one in hist_ring->{node.nam,words}. Returns 0 if the two are equal
+ * (except for insignificant spaces), nonzero otherwise.
+ */
+
+/**/
+static int
+hist_compare(void)
+{
+    int i;
+
+    if (isset(HISTREDUCEBLANKS))
+	return strcmp(chline, hist_ring->node.nam);
+    if (chwordpos/2 != hist_ring->nwords)
+	return 1;
+    for (i = 0; i < chwordpos; i += 2) {
+	const char *p = &chline[chwords[i]];
+	const char *q = &hist_ring->node.nam[hist_ring->words[i]];
+	int n = chwords[i+1] - chwords[i], j;
+
+	if (n != hist_ring->words[i+1] - hist_ring->words[i])
+	    return 1;
+	for (j=0; j < n; ++j)
+	    if (p[j] != q[j])
+		return 1;
+    }
+    return 0;
+}
+
 /* say we're done using the history mechanism */
 
 /**/
@@ -1600,7 +1630,7 @@ hend(Eprog prog)
 	else
 	    newflags = 0;
 	if ((isset(HISTIGNOREDUPS) || isset(HISTIGNOREALLDUPS)) && save > 0
-	 && hist_ring && histstrcmp(chline, hist_ring->node.nam) == 0) {
+	 && hist_ring && hist_compare() == 0) {
 	    /* This history entry compares the same as the previous.
 	     * In case minor changes were made, we overwrite the
 	     * previous one with the current one.  This also gets the

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Bart Schaefer]

On Wed, Mar 20, 2024 at 10:23 PM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote:
>
> I think HIST_IGNORE_DUPS would work as expected even without
> HIST_REDUCCE_BLANKS if we use the information in chwords and
> hist_ring->words, as in the patch below, for example.

Doesn't that require HIST_LEX_WORDS to be set to poplulate ->words
when reading histfiles?  And further, would it not behave differently
for "print -s" vs. "print -S"?

This is more a concern when someone is using something similar to
per-directory history files or other fooling with "fc -p" or
zshaddhistory.  Maybe not much of a worry.

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Jun T]

> 2024/03/21 14:36、Bart Schaefer <schaefer@brasslantern.com>のメール:
> 
> On Wed, Mar 20, 2024 at 10:23 PM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote:
>> 
>> I think HIST_IGNORE_DUPS would work as expected even without
>> HIST_REDUCCE_BLANKS if we use the information in chwords and
>> hist_ring->words, as in the patch below, for example.
> 
> Doesn't that require HIST_LEX_WORDS to be set to poplulate ->words
> when reading histfiles?

I think ->words is always populated; without HIST_LEX_WORDS it may
contain wrong info, but for HIST_IGNORE_DUPS only the last event in the
history file matters.
# If this is a problem, we may set uselex to true for the last event
# from the hist file, near line 2821 in hist.c, in function readhistfile().

> And further, would it not behave differently
> for "print -s" vs. "print -S"?

Sorry I can't understand this, but:

print -S ': "a b"'
and
print -s : '"a b"'
are equivalent.

print -s : "a b"
is different, but 

% print -s : "a b"
% history -1
 1025  : a b         # 'a b' is a single word
% echo !1025:1
echo a b
a b

This is quite confusing. With my patch, even with HIST_IGNORE_DUP,

% print -s : "a b"
% : a b
% history -2
 1028  : a b	# one arg
 1029  : a b	# two args

This may make already confusing situation more confusing.
Without my patch the event 1028 is overwritten by the
correct one (with two args); somewhat better than with my patch?
But anyway use of 'print -s' with args containing spaces is
best avoided, I think.

BUT:
Do we really need the option HIST_REDUCE_BLANKS? I mean, how about
making zsh behave always as if HIST_REDUCE_BLANKS is ON?
Are there any user who want to keep unnecessary spaces in history?
Or calling histreduceblanks() may slow down the interactive zsh?

Re: HIST_IGNORE_DUPS also ignores command lines that differ by a space between quotes

[Vincent Lefevre]

On 2024-03-21 18:41:08 +0900, Jun T wrote:
> BUT:
> Do we really need the option HIST_REDUCE_BLANKS? I mean, how about
> making zsh behave always as if HIST_REDUCE_BLANKS is ON?
> Are there any user who want to keep unnecessary spaces in history?

One possibility is that they might have "formatted" long commands,
e.g. using the backslash to write it on several lines, e.g.

  echo "some string" \
    "another string"

With HIST_REDUCE_BLANKS, this is simplified to

  echo "some string" "another string"

> Or calling histreduceblanks() may slow down the interactive zsh?

I doubt very much about it. With HIST_REDUCE_BLANKS, history handling
always seems immediate for me.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)