Skip to content

Fix regex matching for diff output parsing.#9

Closed
RRethy wants to merge 2 commits into
wincent:masterfrom
RRethy:master
Closed

Fix regex matching for diff output parsing.#9
RRethy wants to merge 2 commits into
wincent:masterfrom
RRethy:master

Conversation

@RRethy
Copy link
Copy Markdown
Contributor

@RRethy RRethy commented Dec 21, 2020

Fix #8

The regex was failing for my diff output, I'm not sure the reason for the b/ in it but let me know if this breaks something I'm unaware of. The regex I'm using is the same used in the OG git jump command.

Copy link
Copy Markdown
Owner

@wincent wincent left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think it needs a small tweak though, noted inline, before it can be merged.

Comment thread bin/vcs-jump Outdated
Comment on lines +106 to +107
(line =~ %r{^\+\+\+ (.*)}) ? (file = root + $~[1]) : (next unless file)
(line =~ %r{^@@ .*?\+(\d+)}) ? (idx = $~[1].to_i) : (next unless idx)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

So here's the original Perl regexen:

	if (m{^\+\+\+ (.*)}) { $file = $1; next }
	defined($file) or next;
	if (m/^@@ .*?\+(\d+)/) { $line = $1; next }

Thoughts on the first line: this change is seems broken. Note that it only works because the upstream script invokes git diff with --no-prefix. ie. instead of producing a "standard" diff with headers that look like this:

diff --git a/aspects/dotfiles/files/.zsh/zsh-syntax-highlighting b/aspects/dotfiles/files/.zsh/zsh-syntax-highlighting
--- a/aspects/dotfiles/files/.zsh/zsh-syntax-highlighting
+++ b/aspects/dotfiles/files/.zsh/zsh-syntax-highlighting
@@ -1 +1 @@
-Subproject commit 82cf2527fc13fefee6e87f99171c1b368f0abdb9
+Subproject commit 82cf2527fc13fefee6e87f99171c1b368f0abdb9-dirty

It produces:

diff --git aspects/dotfiles/files/.zsh/zsh-syntax-highlighting aspects/dotfiles/files/.zsh/zsh-syntax-highlighting
--- aspects/dotfiles/files/.zsh/zsh-syntax-highlighting
+++ aspects/dotfiles/files/.zsh/zsh-syntax-highlighting
@@ -1 +1 @@
-Subproject commit 82cf2527fc13fefee6e87f99171c1b368f0abdb9
+Subproject commit 82cf2527fc13fefee6e87f99171c1b368f0abdb9-dirty

You'll note that vcs-jump instead invokes git diff with git -c diff.mnemonicPrefix=no diff. From man git-config:

       diff.mnemonicPrefix
           If set, git diff uses a prefix pair that is different from the standard "a/" and "b/" depending on what is being compared. When
           this configuration is in effect, reverse diff output also swaps the order of the prefixes:

Looks like they used to do a similar thing to vcs-jump but changed it in git/git@6108b04.

So that means that with this change, and depending on local config values, the jump list will be populated with entries like a/some/file.txt instead of the desired some/file.txt.

Relevant bits from man git-diff:

       --src-prefix=<prefix>
           Show the given source prefix instead of "a/".

       --dst-prefix=<prefix>
           Show the given destination prefix instead of "b/".

       --no-prefix
           Do not show any source or destination prefix.

Note also that this means that they used to jump to the post-image file-name (which I think is better, because it will land on the renamed file if rename detection is active), but now they jump to the pre-image filename.

I think this explains why it didn't work for you before but it works with out-of-the-box settings. There is likely something in you git config --list output that would account for the behavior difference.

So, I don't think we should apply this change. Instead, we should pass additional config options to force the use of prefixes.

git -c diff.noprefix=no -c diff.mnemonicPrefix=no

Thoughts on the second line: this is just turning it into a non-greedy pattern. They made the change here. I expect this is an exceedingly rare edge case and doesn't explain the problem you were having, but we should apply the change anyway.

@RRethy
Copy link
Copy Markdown
Contributor Author

RRethy commented Dec 21, 2020

Interesting, thanks for the detailed explanation (I did have noprefix=true in my config)! I made the change and went through https://git-scm.com/docs/diff-config to make sure there are no other config options that could cause an issue.

Copy link
Copy Markdown
Owner

@wincent wincent left a comment

Choose a reason for hiding this comment

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

Perfect. Thanks!

wincent pushed a commit that referenced this pull request Dec 21, 2020
wincent pushed a commit that referenced this pull request Dec 21, 2020
See discussion at: #9

TL;DR: diff.noprefix=yes will break our regex because it will produce
diff headers like:

    --- aspects/dotfiles/files/.zsh/zsh-syntax-highlighting
    +++ aspects/dotfiles/files/.zsh/zsh-syntax-highlighting

instead of these required for matching:

    --- a/aspects/dotfiles/files/.zsh/zsh-syntax-highlighting
    +++ b/aspects/dotfiles/files/.zsh/zsh-syntax-highlighting
@wincent wincent closed this in 8936ffc Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vcs-jump diff ... doesn't work

2 participants