[PATCH] Fix hexdump command used for mercurial dirstate parsing

[PATCH] Fix hexdump command used for mercurial dirstate parsing

[Reed Riley]

Normally, the old command works.  But very rarely, it outputs a string
like the following instead:
❯ hexdump -n20 -e '1/1 "%02x"' .hg/dirstate
77bba665e970146bd2be0b2da40092e340*
8804

Changing the command resolves the problem:
❯ xxd -p -l20 .hg/dirstate
77bba665e970146bd2be0b2da40092e340408804
---
 Functions/VCS_Info/Backends/VCS_INFO_get_data_hg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg b/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
index f35ad59..abcd8bc 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
@@ -42,7 +42,7 @@ if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" get-revision ; then
     if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" use-simple \
             && ( VCS_INFO_check_com hexdump ) && [[ -r ${dirstatefile} ]] ; then
         # Calling hexdump is (much) faster than hg but doesn't get the local rev
-        r_csetid=$(hexdump -n 20 -e '1/1 "%02x"' ${dirstatefile})
+        r_csetid=$(xxd -p -l20 ${dirstatefile})
     else
         # Settling for a short (but unique!) hash because getting the full
         # 40-char hash in addition to all the other info we want isn't
-- 
2.10.2

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Bart Schaefer]

On Oct 29, 10:47pm, Reed Riley wrote:
} Subject: [PATCH] Fix hexdump command used for mercurial dirstate parsing
}
}      if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" use-simple \
}              && ( VCS_INFO_check_com hexdump ) && [[ -r ${dirstatefile} ]] ; then
}          # Calling hexdump is (much) faster than hg but doesn't get the local rev
} -        r_csetid=$(hexdump -n 20 -e '1/1 "%02x"' ${dirstatefile})
} +        r_csetid=$(xxd -p -l20 ${dirstatefile})


Other references to "hexdump" -- especially "VCS_INFO_check_com hexdump",
but also the comment -- ought to be changed if this use is changing.

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Mikael Magnusson]

On Sun, Oct 30, 2016 at 3:47 AM, Reed Riley <john.reed.riley@gmail.com> wrote:
> Normally, the old command works.  But very rarely, it outputs a string
> like the following instead:
> ❯ hexdump -n20 -e '1/1 "%02x"' .hg/dirstate
> 77bba665e970146bd2be0b2da40092e340*
> 8804
>
> Changing the command resolves the problem:
> ❯ xxd -p -l20 .hg/dirstate
> 77bba665e970146bd2be0b2da40092e340408804
> ---
>  Functions/VCS_Info/Backends/VCS_INFO_get_data_hg | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg b/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
> index f35ad59..abcd8bc 100644
> --- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
> +++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
> @@ -42,7 +42,7 @@ if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" get-revision ; then
>      if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" use-simple \
>              && ( VCS_INFO_check_com hexdump ) && [[ -r ${dirstatefile} ]] ; then
>          # Calling hexdump is (much) faster than hg but doesn't get the local rev
> -        r_csetid=$(hexdump -n 20 -e '1/1 "%02x"' ${dirstatefile})
> +        r_csetid=$(xxd -p -l20 ${dirstatefile})
>      else
>          # Settling for a short (but unique!) hash because getting the full
>          # 40-char hash in addition to all the other info we want isn't

I don't think it's a good idea to add a dependency on Vim here.

-- 
Mikael Magnusson

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Frank Terbeck]

Hi,

Reed Riley wrote:
> Normally, the old command works.  But very rarely, it outputs a string
> like the following instead:
> ❯ hexdump -n20 -e '1/1 "%02x"' .hg/dirstate
> 77bba665e970146bd2be0b2da40092e340*
> 8804
[...]
> -        r_csetid=$(hexdump -n 20 -e '1/1 "%02x"' ${dirstatefile})
> +        r_csetid=$(xxd -p -l20 ${dirstatefile})

I would much rather know, what causes hexdump to behave like that. Is
the way we are using it documented behavior? If so, that would suggest a
bug in hexdump, that should be resolved instead. If no, we're probably
using hexdump in a wrong way — maybe just in a non-portable way, and we
should fix the way we're using hexdump.

Are you able to provides example data, that causes the bug? Maybe even
formulate a supposition as to what it is exactly, that triggers this
behaviour.

"xxd" is a command shipped with vim. Adding such a dependency is out of
the question for me. It would be conceivable to make the command that is
called in these places configurable via styles. But that is a separate
issue to the one that fixes our use of hexdump.


Regards, Frank

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Reed Riley]

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

Damn - I had no idea `xxd` was a part of Vim, sorry!  The original version
of my patch was `hexdump -n 20 -e '20/1 "%02x"' ${dirstatefile}` - I only
switched to xxd at Daniel Shahaf's suggestion.

I sadly can't provide a full dirstate file for a repro, but I can provide
the relevant part as a hexdump:
❯ hexdump dirstate | head -n3

                0000000 77 bb a6 65 e9 70 14 6b d2 be 0b 2d a4 00 92 e3
0000010 40 40 88 04 00 00 00 00 00 00 00 00 00 00 00 00
0000020 00 00 00 00 00 00 00 00 6e 00 00 81 a4 00 00 04
❯ hexdump -n20 -e '1/1 "%02x"' dirstate

          77bba665e970146bd2be0b2da40092e340*
8804%

>From the limited research I've done, the issue in the output is that '*' is
used to represent repetitions:
 -
http://superuser.com/questions/494245/what-does-an-asterisk-mean-in-hexdump-output
 - also described (partially?) in `man hexdump` in the description of the
`-v` option.

In other words, if you have repeated octets in your commit hash, the old
command doesn't work properly.

So, based on that, there are two versions of the command that work
correctly on this particular case for me:
hexdump -n 20 -e '20/1 "%02x"' ${dirstatefile}
hexdump -n 20 -v -e '1/1 "%02x"' ${dirstatefile}

Do you have any preference between them?

Thanks,
Reed

On Sun, Oct 30, 2016 at 7:11 AM, Frank Terbeck <ft@bewatermyfriend.org>
wrote:

> Hi,
>
> Reed Riley wrote:
> > Normally, the old command works.  But very rarely, it outputs a string
> > like the following instead:
> > ❯ hexdump -n20 -e '1/1 "%02x"' .hg/dirstate
> > 77bba665e970146bd2be0b2da40092e340*
> > 8804
> [...]
> > -        r_csetid=$(hexdump -n 20 -e '1/1 "%02x"' ${dirstatefile})
> > +        r_csetid=$(xxd -p -l20 ${dirstatefile})
>
> I would much rather know, what causes hexdump to behave like that. Is
> the way we are using it documented behavior? If so, that would suggest a
> bug in hexdump, that should be resolved instead. If no, we're probably
> using hexdump in a wrong way — maybe just in a non-portable way, and we
> should fix the way we're using hexdump.
>
> Are you able to provides example data, that causes the bug? Maybe even
> formulate a supposition as to what it is exactly, that triggers this
> behaviour.
>
> "xxd" is a command shipped with vim. Adding such a dependency is out of
> the question for me. It would be conceivable to make the command that is
> called in these places configurable via styles. But that is a separate
> issue to the one that fixes our use of hexdump.
>
>
> Regards, Frank
>



-- 
Reed Riley <john.reed.riley@gmail.com>

--------------------------------------------
Q: Why is this email five sentences or less?
A: http://five.sentenc.es

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Daniel Shahaf]

Frank Terbeck wrote on Sun, Oct 30, 2016 at 12:11:01 +0100:
> "xxd" is a command shipped with vim. Adding such a dependency is out of
> the question for me.

That's my fault.  Reed's original patch used hexdump; I suggested to
just use xxd if hexdump gives trouble.

> It would be conceivable to make the command that is called in these
> places configurable via styles. But that is a separate issue to the
> one that fixes our use of hexdump.

Couldn't we replace hexdump by a pure zsh solution?  We simply need to
read 20 octets and print their hexadecimal values.

Cheers,

Daniel

[PATCH] Fix hexdump command used for mercurial dirstate parsing

[Reed Riley]

Normally, the old command works.  But very rarely, it outputs a string
like the following instead:
❯ hexdump -n20 -e '1/1 "%02x"' .hg/dirstate
77bba665e970146bd2be0b2da40092e340*
8804

Changing the command resolves the problem:
❯ hexdump -n20 -v -e '1/1 "%02x"' .hg/dirstate
77bba665e970146bd2be0b2da40092e340408804
---
 Functions/VCS_Info/Backends/VCS_INFO_get_data_hg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg b/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
index f35ad59..cd714ab 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
@@ -42,7 +42,7 @@ if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" get-revision ; then
     if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" use-simple \
             && ( VCS_INFO_check_com hexdump ) && [[ -r ${dirstatefile} ]] ; then
         # Calling hexdump is (much) faster than hg but doesn't get the local rev
-        r_csetid=$(hexdump -n 20 -e '1/1 "%02x"' ${dirstatefile})
+        r_csetid=$(hexdump -n 20 -v -e '1/1 "%02x"' ${dirstatefile})
     else
         # Settling for a short (but unique!) hash because getting the full
         # 40-char hash in addition to all the other info we want isn't
-- 
2.10.2

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Daniel Shahaf]

Daniel Shahaf wrote on Sun, Oct 30, 2016 at 15:55:30 +0000:
> Frank Terbeck wrote on Sun, Oct 30, 2016 at 12:11:01 +0100:
> > It would be conceivable to make the command that is called in these
> > places configurable via styles. But that is a separate issue to the
> > one that fixes our use of hexdump.
>
> Couldn't we replace hexdump by a pure zsh solution?  We simply need to
> read 20 octets and print their hexadecimal values.

This seems to do the job:

% f() {
    local fname=$1 bytecount=$2
    # Read 20 bytes
    setopt localoptions nomultibyte
    integer fd; local val
    exec {fd}<$fname
    read -k $bytecount -r -u $fd val
    exec {fd}<&-
    # Convert them to hex
    setopt localoptions extendedglob
    local -r -A pad=( $'\0' 0 $'\1' 0 $'\2' 0 $'\3' 0 $'\4' 0 $'\5' 0 $'\6' 0 $'\7' 0 $'\10' 0 $'\11' 0 $'\12' 0 $'\13' 0 $'\14' 0 $'\15' 0 $'\16' 0 $'\17' 0 )
    REPLY=${(L)val//(#b)(?)/$pad[$match[1]]$(( [##16] ##${match[1]} ))}
}
% f dirstate 20 
% typeset -p REPLY
typeset REPLY=77bba665e970146bd2be0b2da40092e340408804
% xxd -l20 -p < dirstate
77bba665e970146bd2be0b2da40092e340408804
% 

Would something along these lines be preferable?

Thanks to Mikael for the inner loop.

Cheers,

Daniel

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Bart Schaefer]

On Oct 30,  5:06pm, Daniel Shahaf wrote:
}
}     setopt localoptions extendedglob
}     local -r -A pad=( $'\0' 0 $'\1' 0 $'\2' 0 $'\3' 0 $'\4' 0 $'\5' 0 $'\6' 0 $'\7' 0 $'\10' 0 $'\11' 0 $'\12' 0 $'\13' 0 $'\14' 0 $'\15' 0 $'\16' 0 $'\17' 0 )
}     REPLY=${(L)val//(#b)(?)/$pad[$match[1]]$(( [##16] ##${match[1]} ))}

I can't help thinking here must be an easier way to do that.

f() {                                              
  setopt localoptions nomultibyte extendedglob
  local val
  read -k $2 -u 0 val <$1
  print -r -- ${(Lj::)${(l:2::0:)${(@s//)val}//(#m)*/$(( [##16] ##$MATCH ))}}
}


-- 
Barton E. Schaefer

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Frank Terbeck]

Hey again,

Reed Riley wrote:
> -        r_csetid=$(hexdump -n 20 -e '1/1 "%02x"' ${dirstatefile})
> +        r_csetid=$(hexdump -n 20 -v -e '1/1 "%02x"' ${dirstatefile})

I like this one, though the entirely native implementation, mentioned in
another sub-thread with Bart and Daniel has a certain charm about it as
well. We should probably pick either this one, or something they come up
with.


Regards, Frank

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Daniel Shahaf]

Bart Schaefer wrote on Sun, Oct 30, 2016 at 11:28:32 -0700:
> On Oct 30,  5:06pm, Daniel Shahaf wrote:
> }
> }     setopt localoptions extendedglob
> }     local -r -A pad=( $'\0' 0 $'\1' 0 $'\2' 0 $'\3' 0 $'\4' 0 $'\5' 0 $'\6' 0 $'\7' 0 $'\10' 0 $'\11' 0 $'\12' 0 $'\13' 0 $'\14' 0 $'\15' 0 $'\16' 0 $'\17' 0 )
> }     REPLY=${(L)val//(#b)(?)/$pad[$match[1]]$(( [##16] ##${match[1]} ))}
> 
> I can't help thinking here must be an easier way to do that.
> 
> f() {                                              
>   setopt localoptions nomultibyte extendedglob
>   local val
>   read -k $2 -u 0 val <$1
>   print -r -- ${(Lj::)${(l:2::0:)${(@s//)val}//(#m)*/$(( [##16] ##$MATCH ))}}
> }

Thanks; that's now VCS_INFO_hexdump:

diff --git a/Functions/VCS_Info/VCS_INFO_hexdump b/Functions/VCS_Info/VCS_INFO_hexdump
index e69de29..11f1c1a 100644
--- a/Functions/VCS_Info/VCS_INFO_hexdump
+++ b/Functions/VCS_Info/VCS_INFO_hexdump
@@ -0,0 +1,16 @@
+## vim:ft=zsh
+
+# VCS_INFO_hexdump FILENAME BYTECOUNT
+#
+# Return in $REPLY a hexadecimal representation (lowercase, no whitespace)
+# of the first BYTECOUNT bytes of FILENAME.
+
+if [[ -r $1 ]]; then
+  setopt localoptions nomultibyte extendedglob
+  local val
+  read -k $2 -u 0 val <$1
+  REPLY=${(Lj::)${(l:2::0:)${(@s//)val}//(#m)*/$(( [##16] ##$MATCH ))}}
+else
+  return 1
+fi
+
diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg b/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
index f35ad59..4ad4089 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
@@ -40,9 +40,9 @@ VCS_INFO_adjust
 # Disabled by default anyway, so no harm done.
 if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" get-revision ; then
     if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" use-simple \
-            && ( VCS_INFO_check_com hexdump ) && [[ -r ${dirstatefile} ]] ; then
+            && VCS_INFO_hexdump ${dirstatefile} 20 ; then
         # Calling hexdump is (much) faster than hg but doesn't get the local rev
-        r_csetid=$(hexdump -n 20 -e '1/1 "%02x"' ${dirstatefile})
+        r_csetid=$REPLY
     else
         # Settling for a short (but unique!) hash because getting the full
         # 40-char hash in addition to all the other info we want isn't
diff --git a/Functions/VCS_Info/vcs_info b/Functions/VCS_Info/vcs_info
index f13f6b5..c0c1182 100644
--- a/Functions/VCS_Info/vcs_info
+++ b/Functions/VCS_Info/vcs_info
@@ -19,6 +19,7 @@ static_functions=(
     VCS_INFO_check_com
     VCS_INFO_formats
     VCS_INFO_get_cmd
+    VCS_INFO_hexdump
     VCS_INFO_hook
     VCS_INFO_maxexports
     VCS_INFO_nvcsformats

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Frank Terbeck]

Hi,

Daniel Shahaf wrote:
> -            && ( VCS_INFO_check_com hexdump ) && [[ -r ${dirstatefile} ]] ; then
> +            && VCS_INFO_hexdump ${dirstatefile} 20 ; then
>          # Calling hexdump is (much) faster than hg but doesn't get the local rev

While we're at it, that comment should probably be adjusted as well,
since we don't really *call* hexdump any more.


Regards, Frank

Re: [PATCH] Fix hexdump command used for mercurial dirstate parsing

[Daniel Shahaf]

Frank Terbeck wrote on Mon, Oct 31, 2016 at 10:04:03 +0100:
> Daniel Shahaf wrote:
> > -            && ( VCS_INFO_check_com hexdump ) && [[ -r ${dirstatefile} ]] ; then
> > +            && VCS_INFO_hexdump ${dirstatefile} 20 ; then
> >          # Calling hexdump is (much) faster than hg but doesn't get the local rev
> 
> While we're at it, that comment should probably be adjusted as well,
> since we don't really *call* hexdump any more.

Pushed with that change.  (I'd noticed it before posting the patch, but
thought the language was clear as it was.)  Thanks for the review.