zsh-workers
 help / color / mirror / code / Atom feed
* [PATCH] vcs_info git: Fix stagedstr for empty repos
@ 2014-06-01  2:26 Daniel Shahaf
  2014-06-01 12:33 ` Aaron Schrab
  2014-06-02  8:14 ` [PATCH] " Frank Terbeck
  0 siblings, 2 replies; 6+ messages in thread
From: Daniel Shahaf @ 2014-06-01  2:26 UTC (permalink / raw)
  To: zsh-workers

[-- 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re:  vcs_info git: Fix stagedstr for empty repos
  2014-06-01  2:26 [PATCH] vcs_info git: Fix stagedstr for empty repos Daniel Shahaf
@ 2014-06-01 12:33 ` Aaron Schrab
  2014-06-02  8:14 ` [PATCH] " Frank Terbeck
  1 sibling, 0 replies; 6+ messages in thread
From: Aaron Schrab @ 2014-06-01 12:33 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

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.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vcs_info git: Fix stagedstr for empty repos
  2014-06-01  2:26 [PATCH] vcs_info git: Fix stagedstr for empty repos Daniel Shahaf
  2014-06-01 12:33 ` Aaron Schrab
@ 2014-06-02  8:14 ` Frank Terbeck
  2014-06-02 13:00   ` Daniel Shahaf
  1 sibling, 1 reply; 6+ messages in thread
From: Frank Terbeck @ 2014-06-02  8:14 UTC (permalink / raw)
  To: zsh-workers

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vcs_info git: Fix stagedstr for empty repos
  2014-06-02  8:14 ` [PATCH] " Frank Terbeck
@ 2014-06-02 13:00   ` Daniel Shahaf
  2014-06-02 13:19     ` Frank Terbeck
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Shahaf @ 2014-06-02 13:00 UTC (permalink / raw)
  To: Frank Terbeck; +Cc: zsh-workers

[-- 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vcs_info git: Fix stagedstr for empty repos
  2014-06-02 13:00   ` Daniel Shahaf
@ 2014-06-02 13:19     ` Frank Terbeck
  2014-06-02 18:21       ` Daniel Shahaf
  0 siblings, 1 reply; 6+ messages in thread
From: Frank Terbeck @ 2014-06-02 13:19 UTC (permalink / raw)
  To: zsh-workers

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vcs_info git: Fix stagedstr for empty repos
  2014-06-02 13:19     ` Frank Terbeck
@ 2014-06-02 18:21       ` Daniel Shahaf
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Shahaf @ 2014-06-02 18:21 UTC (permalink / raw)
  To: Frank Terbeck; +Cc: zsh-workers

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.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2014-06-02 18:21 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-01  2:26 [PATCH] vcs_info git: Fix stagedstr for empty repos Daniel Shahaf
2014-06-01 12:33 ` Aaron Schrab
2014-06-02  8:14 ` [PATCH] " Frank Terbeck
2014-06-02 13:00   ` Daniel Shahaf
2014-06-02 13:19     ` Frank Terbeck
2014-06-02 18:21       ` Daniel Shahaf

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).