Community
Participate
Working Groups
The diff viewer for Git patches drops the rest of a file path if it encounters a space, e.g. here: https://bugs.eclipse.org/bugs/attachment.cgi?id=226058&action=diff When you click "Collapse All", you can see 3 files called "a/org.eclipse.jdt.ui/core". The corresponding patch source is: diff --git a/org.eclipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/fix/FixMessages.java b/org.eclipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/fix/FixMessages.java index a87c8bc..66b3db3 100644 --- a/org.eclipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/fix/FixMessages.java +++ b/org.eclipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/fix/FixMessages.java @@ -7,6 +7,7 @@
I just remembered that I once played around a bit with this problem, but never followed up on it. See attachment 221720 [details] and attachment 221720 [details]. The problem seems to be that the patch viewer just scans the "diff --git..." line for the first space, and then stops there. This bug doesn't occur for CVS patches, see e.g. attachment 124322 [details].
I think the bug is in http://cpansearch.perl.org/src/TMANNERM/PatchReader-0.9.6/lib/PatchReader/Raw.pm in "sub next_line": The /^---\s*([\S ]+)\s*\t([^\t\r\n]*)\s*(\S*)/ doesn't match this line in a Git patch: --- a/bundles/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java In the end, /^diff\s*(-\S+\s*)*(\S+)\s*(\S*)/ matches the first part of line diff --git a/bundles/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java b/bundles/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java , but the $2 cuts off everything after the first space. I don't know how I could test this locally, but I think a good fix would be to replace line if ($line =~ /^---\s*([\S ]+)\s*\t([^\t\r\n]*)\s*(\S*)/) { with if ($line =~ /^---\s*([\S ]+)\s*?(?:\t([^\t\r\n]*)\s*(\S*))?/) { Changes: make \s* avoid \t: ? non-capturing group: (?: ) make old_date_str and old_revision optional ?
Webmaster, would it be possible to test this change to perl's PatchReader/Raw.pm on https://bugs.eclipse.org/bugstest/ or https://bugs.eclipse.org/ ?
--- ../../../bugzilla-prod/lib/PatchReader/Raw.pm 2011-04-11 02:06:40.000000000 -0400 +++ Raw.pm 2014-03-24 11:36:51.000000000 -0400 @@ -41,7 +41,8 @@ return if $line =~ /^\?/; # patch header parsing - if ($line =~ /^---\s*([\S ]+)\s*\t([^\t\r\n]*)\s*(\S*)/) { + # if ($line =~ /^---\s*([\S ]+)\s*\t([^\t\r\n]*)\s*(\S*)/) { + if ($line =~ /^---\s*([\S ]+)\s*?(?:\t([^\t\r\n]*)\s*(\S*))?/) { $this->_maybe_end_file(); if ($1 eq "/dev/null") { You can test it on /bugstest, but the bug in question doesn't exist. You'd have to attach the patch to an existing bug.
Works great: https://bugs.eclipse.org/bugstest/attachment.cgi?id=184857&action=diff Same attachment in production Bugzilla: https://bugs.eclipse.org/bugs/attachment.cgi?id=226058&action=diff
Great. I've filed a bug upstream: https://rt.cpan.org/Public/Bug/Report.html?Queue=PatchReader And I've patched the production version. Thanks for the fix! I'll leave this open while we wait to hear from upstream.
https://rt.cpan.org/Public/Bug/Display.html?id=94181
File additions are still broken, e.g. in attachment 242120 [details], click "Collapse All" to see the short line for this file: diff --git a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/IntroduceIndirectionTests18.java b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/IntroduceIndirectionTests18.java new file mode 100644 index 0000000..26df5b3 --- /dev/null +++ b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/IntroduceIndirectionTests18.java Fix in PatchReader/Raw.pm (same change as comment 2, just for the +++ case): Replace line } elsif ($line =~ /^\+\+\+\s*([\S ]+)\s*\t([^\t\r\n]*)(\S*)/) { with } elsif ($line =~ /^\+\+\+\s*([\S ]+)\s*?(?:\t([^\t\r\n]*)(\S*))?/) {
> Replace line > > } elsif ($line =~ /^\+\+\+\s*([\S ]+)\s*\t([^\t\r\n]*)(\S*)/) { > > with > > } elsif ($line =~ /^\+\+\+\s*([\S ]+)\s*?(?:\t([^\t\r\n]*)(\S*))?/) { I've applied this, but it doesn't seem to work. Here's the relevant part of Raw.pm: sub next_line { my $this = shift; my ($line) = @_; return if $line =~ /^\?/; # patch header parsing # if ($line =~ /^---\s*([\S ]+)\s*\t([^\t\r\n]*)\s*(\S*)/) { if ($line =~ /^---\s*([\S ]+)\s*?(?:\t([^\t\r\n]*)\s*(\S*))?/) { $this->_maybe_end_file(); if ($1 eq "/dev/null") { $this->{FILE_STATE}{is_add} = 1; } else { $this->{FILE_STATE}{filename} = $1; } $this->{FILE_STATE}{old_date_str} = $2; $this->{FILE_STATE}{old_revision} = $3 if $3; $this->{IN_HEADER} = 1; # } elsif ($line =~ /^\+\+\+\s*([\S ]+)\s*\t([^\t\r\n]*)(\S*)/) { } elsif ($line =~ /^\+\+\+\s*([\S ]+)\s*?(?:\t([^\t\r\n]*)(\S*))?/) {
(In reply to Denis Roy from comment #9) > I've applied this, but it doesn't seem to work. Rats! The problem seems to be a few lines further down in the code. In the } elsif ($line =~ /^\+\+\+... it doesn't store the file name at all. Could you please turn } elsif ($line =~ /^\+\+\+\s*([\S ]+)\s*\t([^\t\r\n]*)(\S*)/) { if ($1 eq "/dev/null") { $this->{FILE_STATE}{is_remove} = 1; } into this: } elsif ($line =~ /^\+\+\+\s*([\S ]+)\s*?(?:\t([^\t\r\n]*)(\S*))?/) { if ($1 eq "/dev/null") { $this->{FILE_STATE}{is_remove} = 1; } else { $this->{FILE_STATE}{filename} = $1; } This will make it use the +++ file name in case both are not "/dev/null", but that's an equally good choice as using the --- file name.
> File additions are still broken, e.g. in attachment 242120 [details], click > "Collapse All" to see the short line for this file: I've applied your latest fix. How does it look?
(In reply to Denis Roy from comment #11) > I've applied your latest fix. How does it look? Pretty good. On https://bugs.eclipse.org/bugstest/attachment.cgi?id=184858&action=diff after Collapse All, you can see: b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/IntroduceIndirectionTests18.java (+56 lines) On https://bugs.eclipse.org/bugs/attachment.cgi?id=242120&action=diff is was cut inside /test cases/: a/org.eclipse.jdt.ui.tests.refactoring/test (+56 lines) One remaining problem: (In reply to Markus Keller from comment #10) > This will make it use the +++ file name in case both are not "/dev/null", > but that's an equally good choice as using the --- file name. It's just cosmetics, but seeing all lines start with "b/" now is a bit strange. I don't speak enough Perl to turn the inserted "else" in } else { $this->{FILE_STATE}{filename} = $1; } into "else if $this->{FILE_STATE}{filename} has not been set before". If someone of you knows some Perl, then I'd appreciate a fix. I won't have time to learn Perl in the next few weeks.
Created attachment 254395 [details] patch for PatchReader/Raw.pm Here's a patch that fixes comment 12 as well. Tested on a local Bugzilla 5.0 install. Summary of the changes: - makes the date and revision information in --- and +++ lines optional - as a side-effect, this fixes file paths that were cut at the first whitespace - for modified files, renders the --- file path, not the +++ path Denis, could you please apply this to the https://bugs.eclipse.org/bugs/ install again? It looks like the previous modifications got lost, so you can just apply the patch to lib/PatchReader/Raw.pm.
Created attachment 254396 [details] example patch that shows file additions/removals/modifications
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
Is this bug still relevant?
(In reply to Denis Roy from comment #16) > Is this bug still relevant? Yes. Steps are in comment 0.
https://github.com/eclipsewebmaster/eclipse-bugzilla/commit/fc490ca213964fa67fe2eeefc69020061c50cb2f
