Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skip installing .revision-hash if not running make install from a git checkout #723

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Skip installing .revision-hash if not running make install from a git checkout #723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,9 @@ install: all
cp .version zsh-syntax-highlighting.zsh $(SHARE_DIR)
cp COPYING.md README.md changelog.md $(DOC_DIR)
sed -e '1s/ .*//' -e '/^\[build-status-[a-z]*\]: /d' < README.md > $(DOC_DIR)/README.md
if [ x"true" = x"`git rev-parse --is-inside-work-tree 2>/dev/null`" ]; then \
git rev-parse HEAD; \
else \
cat .revision-hash; \
fi > $(SHARE_DIR)/.revision-hash
if prefix=`git rev-parse --show-prefix 2>/dev/null` && [ x"$$prefix" = x ]; then \
danielshahaf marked this conversation as resolved.
Show resolved Hide resolved
git rev-parse HEAD > $(SHARE_DIR)/.revision-hash; \
fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The incumbent code always creates .revision-hash; the PR does not. Should it?

This would manifest when running make install in a release tarball, for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The only place that .revision-hash is used is when defining ZSH_HIGHLIGHT_REVISION modified else-patch. There are three cases.

  1. Running from a git checkout. (ZSH_HIGHLIGHT_REVISION=HEAD)
  2. Running from a make install'd from a git checkout copy (ZSH_HIGHLIGHT_REVISION=revision_id)
  3. Running from a make install'd from a non-git checkout copy (with PR ZSH_HIGHLIGHT_REVISION is not set; previously HEAD)

ZSH_HIGHLIGHT_REVISION=HEAD doesn't give any extra information; however, it's an extra file to install and does cost a bit of time to do the pattern match in zsh-syntax-highlighting.zsh. Further since the third case should only happen from a release tarball, ZSH_HIGHLIGHT_VERSION should be sufficient to know which version of the code is in use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running from a make install'd from a non-git checkout copy (with PR ZSH_HIGHLIGHT_REVISION is not set; previously HEAD)

Factually wrong:

$ zsh -f 
% (cd ~/src/z-sy-h && git archive --format=tar 0.7.1) | tar xf -  
% make install PREFIX=$PWD/d
% source d/share/zsh-syntax-highlighting/zsh-syntax-highlighting.zsh 
% typeset -pm \*SION               
typeset ZSH_HIGHLIGHT_VERSION=0.7.1
typeset ZSH_HIGHLIGHT_REVISION=932e29a0c75411cb618f02995b66c0a4a25699bc
%

ZSH_HIGHLIGHT_REVISION=HEAD doesn't give any extra information; however, it's an extra file to install and does cost a bit of time to do the pattern match in zsh-syntax-highlighting.zsh.

Wait, are you saying that the test [[ $ZSH_HIGHLIGHT_REVISION == \$Format:* ]] accounts for a measurable fraction of the start-up time? That'd be quite surprising. Perhaps it's the glob operator that slows it down? Is either [[ ${ZSH_HIGHLIGHT_REVISION[1,8]} == \$Format: ]] or [[ ${ZSH_HIGHLIGHT_REVISION[1,8]} =~ '\$Format:' ]] faster the incumbent code?

We can't just use that substring trick without further ado because it breaks under KSH_ARRAYS. (That's another form of #688. I have a pending fix for that issue, but that fix should be merged after redrawhook in order to not confuse git merge.)

Further since the third case should only happen from a release tarball, ZSH_HIGHLIGHT_VERSION should be sufficient to know which version of the code is in use.

I see your point, but I'm not sold. Right now, there's an invariant that git show $ZSH_HIGHLIGHT_REVISION will always DTRT regardless of whether one is in case 1/2/3. That's also useful when doing support: I can ask someone to echo $ZSH_HIGHLIGHT_REVISION and it will always DTRT regardless of how they got z-sy-h. One, short command. Making that variable undefined in one case would break these.

Circling back to your original issue $ZSH_HIGHLIGHT_REVISION having a wrong value after make install from within another git repository, how about another approach:

diff --git a/.gitattributes b/.gitattributes
index 715e624..65b8d75 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1 +1,2 @@
+# If you change the contents of .revision-hash, update the 'install' target in Makefile.
 .revision-hash export-subst
diff --git a/INSTALL.md b/INSTALL.md
index 23acdf5..70bc538 100644
--- a/INSTALL.md
+++ b/INSTALL.md
@@ -125,3 +125,13 @@ source /usr/local/share/zsh-syntax-highlighting/zsh-syntax-highlighting.zsh
 ```
 
 to their `.zshrc`s.
+
+### For packagers
+
+Packagers are asked to ensure that the values reported by `typeset -p
+ZSH_HIGHLIGHT_VERSION ZSH_HIGHLIGHT_REVISION` in the installed packages
+are accurate.  In particular, packagers who apply patches to the upstream
+releases are asked to munge the values of those two variables accordingly.
+(For example, Debian sets the values to `0.7.1-1_debian` and `debian/0.7.1-1`
+respectively, the latter being the name of a tag in the downstream package's
+git repository.)
diff --git a/Makefile b/Makefile
index bbc1d43..83cbeba 100644
--- a/Makefile
+++ b/Makefile
@@ -18,8 +18,14 @@ install: all
 	cp .version zsh-syntax-highlighting.zsh $(SHARE_DIR)
 	cp COPYING.md README.md changelog.md $(DOC_DIR)
 	sed -e '1s/ .*//' -e '/^\[build-status-[a-z]*\]: /d' < README.md > $(DOC_DIR)/README.md
-	if [ x"true" = x"`git rev-parse --is-inside-work-tree 2>/dev/null`" ]; then \
-		git rev-parse HEAD; \
+# Fabricate $(SHARE_DIR)/.revision-hash, if needed.  The contents of ./.revision-hash depend
+# on whether we're running from git or from a release tarball.
+	if grep -q '[$$]Format:.*[$$]' <.revision-hash; then \
+		if prefix=`git rev-parse --show-prefix 2>/dev/null` && [ x"$$prefix" = x ]; then \
+			git rev-parse HEAD; \
+		else \
+			echo "(unknown; report this as a bug to your distro packager)" \
+		fi \
 	else \
 		cat .revision-hash; \
 	fi > $(SHARE_DIR)/.revision-hash
diff --git a/changelog.md b/changelog.md
index 496b4a8..76c9b86 100644
--- a/changelog.md
+++ b/changelog.md
@@ -73,6 +73,9 @@
 
 - Recognize `env` as a precommand (e.g., `env FOO=bar ls`)
 
+- Correct the value of `$ZSH_HIGHLIGHT_REVISION` in an edge case
+  [#723]
+
 # Changes in version 0.7.1
 
 - Remove out-of-date information from the 0.7.0 changelog.

It's based on your fix, but I edited the changelog entry to use $ZSH_HIGHLIGHT_REVISION, which is the public API term.

That leaves the pattern match in the driver, though. I'd be surprised if it's actually a performance hot spot, but if it is, let's handle it.

Hope this doesn't come across as bikeshedding — that's not my intention.

:
# The [ -e ] check below is to because sh evaluates this with (the moral
# equivalent of) NONOMATCH in effect, and highlighters/*.zsh has no matches.
Expand Down
3 changes: 3 additions & 0 deletions changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@

- Recognize `env` as a precommand (e.g., `env FOO=bar ls`)

- Skip installing .revision-hash if not running `make install` from a git checkout.
[#723]

# Changes in version 0.7.1

- Remove out-of-date information from the 0.7.0 changelog.
Expand Down
14 changes: 8 additions & 6 deletions zsh-syntax-highlighting.zsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,14 @@ builtin unalias -m '[^+]*'
if true; then
# $0 is reliable
typeset -g ZSH_HIGHLIGHT_VERSION=$(<"${0:A:h}"/.version)
typeset -g ZSH_HIGHLIGHT_REVISION=$(<"${0:A:h}"/.revision-hash)
if [[ $ZSH_HIGHLIGHT_REVISION == \$Format:* ]]; then
# When running from a source tree without 'make install', $ZSH_HIGHLIGHT_REVISION
# would be set to '$Format:%H$' literally. That's an invalid value, and obtaining
# the valid value (via `git rev-parse HEAD`, as Makefile does) might be costly, so:
ZSH_HIGHLIGHT_REVISION=HEAD
if [[ -f "${0:A:h}"/.revision-hash ]]; then
typeset -g ZSH_HIGHLIGHT_REVISION=$(<"${0:A:h}"/.revision-hash)
if [[ $ZSH_HIGHLIGHT_REVISION == \$Format:* ]]; then
# When running from a source tree without 'make install', $ZSH_HIGHLIGHT_REVISION
# would be set to '$Format:%H$' literally. That's an invalid value, and obtaining
# the valid value (via `git rev-parse HEAD`, as Makefile does) might be costly, so:
ZSH_HIGHLIGHT_REVISION=HEAD
fi
fi
fi

Expand Down