[PATCH] vcs_info git: Fix stagedstr for empty repos

[PATCH] vcs_info git: Fix stagedstr for empty repos

[Daniel Shahaf]

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

Good morning Frank, *,

vcs_info git false-negatives to detect staged changes in a repository that has
no commits.  Attached a patch for that.

The patch includes two variants for computing the "has staged changes?" bit in
empty repositories: one via 'git ls-files' and one via git's empty tree.

- The output of ls-files isn't O(1), but I tested with a 3000-file tree and
  didn't notice a slowdown, so I suppose it's acceptable.

- 'diff-index --cached --quiet $empty_tree' is an O(1) operation, but I am not
  sure whether it is forwards compatible with future versions of Git, i.e.,
  whether it's a hack that happens to work or a legitimate use of git's API.

WDYT?

Daniel

[-- Attachment #2: 0002-vcs_info-git-Fix-stagedstr-for-empty-repos.patch --]
[-- Type: text/x-patch, Size: 2379 bytes --]

>From 9994132f33c5439fbb5d3c4ef91ba122b942fd7c Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 30 May 2014 23:50:55 +0000
Subject: [PATCH 2/2] vcs_info git: Fix stagedstr for empty repos

In empty repositories, HEAD is an unresolvable symbolic ref.  Start computing
stagedstr/unstagedstr in that case; for the former, use a different method
than the non-empty-repository case.
---
 Functions/VCS_Info/Backends/VCS_INFO_get_data_git |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
index a48dc39..caf938b 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -128,16 +128,25 @@ elif zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" "check-for-staged-change
     querystaged=1
 fi
 if (( querystaged || queryunstaged )) && \
-   [[ "$(${vcs_comm[cmd]} rev-parse --is-inside-git-dir 2> /dev/null)" != 'true' ]] && \
-   ${vcs_comm[cmd]} rev-parse --quiet --verify HEAD &> /dev/null ; then
+   [[ "$(${vcs_comm[cmd]} rev-parse --is-inside-work-tree 2> /dev/null)" == 'true' ]] ; then
     # Default: off - these are potentially expensive on big repositories
     if (( queryunstaged )) ; then
         ${vcs_comm[cmd]} diff --no-ext-diff --ignore-submodules --quiet --exit-code ||
             gitunstaged=1
     fi
     if (( querystaged )) ; then
-        ${vcs_comm[cmd]} diff-index --cached --quiet --ignore-submodules HEAD 2> /dev/null
-        (( $? && $? != 128 )) && gitstaged=1
+        if ${vcs_comm[cmd]} rev-parse --quiet --verify HEAD &> /dev/null ; then
+            ${vcs_comm[cmd]} diff-index --cached --quiet --ignore-submodules HEAD 2> /dev/null
+            (( $? && $? != 128 )) && gitstaged=1
+        else
+            # empty repository (no commits yet)
+            # TODO: ls-files' output isn't O(1); might be faster to use
+            #         empty_tree_sha1=$(git hash-object -t tree /dev/null)
+            #         git diff-index --cached --quiet $empty_tree_sha1
+            #         (( $? && $? != 128 ))
+            #       to efficiently get the "is the index not empty?" bit.
+            [[ -n "$(${vcs_comm[cmd]} ls-files --cached -z)" ]] && gitstaged=1
+        fi
     fi
 fi
 
-- 
1.7.10.4

Re: vcs_info git: Fix stagedstr for empty repos

[Aaron Schrab]

At 02:26 +0000 01 Jun 2014, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>'diff-index --cached --quiet $empty_tree' is an O(1) operation, but I 
>am not sure whether it is forwards compatible with future versions of 
>Git, i.e., whether it's a hack that happens to work or a legitimate use 
>of git's API.

git itself does that comparison in several places, including in the 
sample pre-commit hook that is shipped with it.  So I'd say that that 
behavior is highly unlikely to change.  Many of those places, including 
that sample hook actually hard-code the sha1 for the empty tree as 
4b825dc642cb6eb9a060e54bf8d69288fbee4904.

Re: [PATCH] vcs_info git: Fix stagedstr for empty repos

[Frank Terbeck]

Hello Daniel,

Daniel Shahaf wrote:
> vcs_info git false-negatives to detect staged changes in a repository that has
> no commits.  Attached a patch for that.

Great!

> The patch includes two variants for computing the "has staged changes?" bit in
> empty repositories: one via 'git ls-files' and one via git's empty tree.
>
> - The output of ls-files isn't O(1), but I tested with a 3000-file tree and
>   didn't notice a slowdown, so I suppose it's acceptable.
>
> - 'diff-index --cached --quiet $empty_tree' is an O(1) operation, but I am not
>   sure whether it is forwards compatible with future versions of Git, i.e.,
>   whether it's a hack that happens to work or a legitimate use of git's API.
>
> WDYT?

I'm with Aaron on this one. IIRC, git uses the diff-index command in
various places, so I don't think it'll be removed anytime soon. Unless
you got other insight (personally, I'm not following the development of
any version control system very closely anymore), I suppose we don't
need two variants of the same thing in the code.

If you don't want the second variant to be forgotten, you can always put
it into the change's commit message. But in the code I'd just use the
variant that you think is faster (if only theoretically).


Regards, Frank

-- 
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: [PATCH] vcs_info git: Fix stagedstr for empty repos

[Daniel Shahaf]

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

Frank Terbeck wrote on Mon, Jun 02, 2014 at 10:14:22 +0200:
> Hello Daniel,
> 
> Daniel Shahaf wrote:
> > vcs_info git false-negatives to detect staged changes in a repository that has
> > no commits.  Attached a patch for that.
> 
> Great!
> 
> > The patch includes two variants for computing the "has staged changes?" bit in
> > empty repositories: one via 'git ls-files' and one via git's empty tree.
> >
> > - The output of ls-files isn't O(1), but I tested with a 3000-file tree and
> >   didn't notice a slowdown, so I suppose it's acceptable.
> >
> > - 'diff-index --cached --quiet $empty_tree' is an O(1) operation, but I am not
> >   sure whether it is forwards compatible with future versions of Git, i.e.,
> >   whether it's a hack that happens to work or a legitimate use of git's API.
> >
> > WDYT?
> 
> I'm with Aaron on this one. IIRC, git uses the diff-index command in
> various places, so I don't think it'll be removed anytime soon.

My concern was not with using diff-index but with using the empty tree.
Since Aaron says using it would be robust, let's just use it.  Attached.

> If you don't want the second variant to be forgotten, you can always put
> it into the change's commit message. But in the code I'd just use the
> variant that you think is faster (if only theoretically).

It's mentioned in this thread, good enough for me.

Daniel

[-- Attachment #2: 0002-vcs_info-git-Fix-stagedstr-for-empty-repos.patch --]
[-- Type: text/x-patch, Size: 2215 bytes --]

>From 9797a5a46403ab7c60b4ee06aab27c65e7d4c75a Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 30 May 2014 23:50:55 +0000
Subject: [PATCH 2/2] vcs_info git: Fix stagedstr for empty repos

In empty repositories, HEAD is an unresolvable symbolic ref.  Start computing
stagedstr/unstagedstr in that case; for the former, use a different method
than the non-empty-repository case.
---
 Functions/VCS_Info/Backends/VCS_INFO_get_data_git |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
index a48dc39..76ab92f 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -128,16 +128,22 @@ elif zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" "check-for-staged-change
     querystaged=1
 fi
 if (( querystaged || queryunstaged )) && \
-   [[ "$(${vcs_comm[cmd]} rev-parse --is-inside-git-dir 2> /dev/null)" != 'true' ]] && \
-   ${vcs_comm[cmd]} rev-parse --quiet --verify HEAD &> /dev/null ; then
+   [[ "$(${vcs_comm[cmd]} rev-parse --is-inside-work-tree 2> /dev/null)" == 'true' ]] ; then
     # Default: off - these are potentially expensive on big repositories
     if (( queryunstaged )) ; then
         ${vcs_comm[cmd]} diff --no-ext-diff --ignore-submodules --quiet --exit-code ||
             gitunstaged=1
     fi
     if (( querystaged )) ; then
-        ${vcs_comm[cmd]} diff-index --cached --quiet --ignore-submodules HEAD 2> /dev/null
-        (( $? && $? != 128 )) && gitstaged=1
+        if ${vcs_comm[cmd]} rev-parse --quiet --verify HEAD &> /dev/null ; then
+            ${vcs_comm[cmd]} diff-index --cached --quiet --ignore-submodules HEAD 2> /dev/null
+            (( $? && $? != 128 )) && gitstaged=1
+        else
+            # empty repository (no commits yet)
+            # 4b825dc642cb6eb9a060e54bf8d69288fbee4904 is the git empty tree.
+            ${vcs_comm[cmd]} diff-index --cached --quiet --ignore-submodules 4b825dc642cb6eb9a060e54bf8d69288fbee4904 2>/dev/null
+            (( $? && $? != 128 )) && gitstaged=1
+        fi
     fi
 fi
 
-- 
1.7.10.4

Re: [PATCH] vcs_info git: Fix stagedstr for empty repos

[Frank Terbeck]

Daniel Shahaf wrote:
[...]
>> I'm with Aaron on this one. IIRC, git uses the diff-index command in
>> various places, so I don't think it'll be removed anytime soon.
>
> My concern was not with using diff-index but with using the empty tree.
> Since Aaron says using it would be robust, let's just use it.  Attached.

I see. I suppose this would only change, if git decides to change its
hashing algorithm. That's a problem for another day, I guess. :)

>> If you don't want the second variant to be forgotten, you can always put
>> it into the change's commit message. But in the code I'd just use the
>> variant that you think is faster (if only theoretically).
>
> It's mentioned in this thread, good enough for me.

Great, thanks! Pushed.


Regards, Frank

Re: [PATCH] vcs_info git: Fix stagedstr for empty repos

[Daniel Shahaf]

Frank Terbeck wrote on Mon, Jun 02, 2014 at 15:19:34 +0200:
> Daniel Shahaf wrote:
> [...]
> >> I'm with Aaron on this one. IIRC, git uses the diff-index command in
> >> various places, so I don't think it'll be removed anytime soon.
> >
> > My concern was not with using diff-index but with using the empty tree.
> > Since Aaron says using it would be robust, let's just use it.  Attached.
> 
> I see. I suppose this would only change, if git decides to change its
> hashing algorithm. That's a problem for another day, I guess. :)

Well, the people from the future who run into this problem can try my
original patch --- either the ls-files solution it implements, or the
hash-object solution it describes in a comment, might work even if the
hashing algorithm changes backwards-incompatibly.