Ticket #90 (closed defect: fixed)

Opened 11 years ago

Last modified 6 years ago

savannah: Can't parse unified diff header, occurence of /^---/

Reported by: hrazdii Owned by: kdave
Priority: major Milestone: 4.7
Component: mc-vfs Version: 4.6.1
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: commited-master

Description

Original: http://savannah.gnu.org/bugs/?24906

Submitted by:Igor Hrazdil <hrazdii>Submitted on:Sun 23 Nov 2008 08:59:26 AM UTC
Category:NoneSeverity:3 - Normal
Status:NonePrivacy:Public
Assigned to:NoneOpen/Closed:Open
Release:NoneOperating System:None

Discussion:

Sun 23 Nov 2008 08:59:26 AM UTC, comment #5:

This item has been reassigned from the project Gnash - The GNU Flash
 player bugs tracker to your tracker.

The original report is still available at bugs #24885

Following are the information included in the original report:

[field #0] Item ID: 24885
[field #1] Group ID: 8191
[field #2] Open/Closed: Open
[field #3] Severity: 3 - Normal
[field #4] Privacy: Public
[field #8] : Unknown bugs Field Display Type
[field #9] Category: utilities
[field #10] Submitted by: hrazdii
[field #11] Assigned to: None
[field #12] Submitted on: Do 20 Nov 2008 06:17:52 CET
[field #13] Summary: Can't parse unified diff header, occurence of /^---/
[field #14] Original Submission: Problem: In my patch file is "---" 
on the beginning of line.

For better explanation i was changed my patchfs script:
# diff patchfs_ patchfs
142c142
< error "Can't parse unified diff header"
---
> error "Can't parse unified diff header >>$$buf<<"

The script output is:
$ /usr/share/mc/extfs/patchfs list adonis.patch.bz2
-rw-r--r-- 1 igor igor 566 02-09-2008 04:00 adonis/bin/adv_sugg_maintain.sh.diff
-rw-r--r-- 1 igor igor 1936 09-05-2008 23:52 adonis/bin/archiv.sh.diff
Can't parse unified diff header >>--- where ar.last_update >= "2008-09-25"
-group by 1,2
-order by 1,2;
<<

In one of original data file is line with "--" (a sql comment 
without blank space on the bebinning):
-- where ar.last_update >= "2008-09-25"

[field #16] Item Group: None
[field #17] Status: None
[field #18] Component Version: None
[field #19] Operating System: None
[field #20] Reproducibility: None
[field #21] Size (loc): None
[field #22] Fixed Release: None
[field #23] Planned Release: None
[field #24] Effort: 0.00
[field #28] Priority: 5 - Normal
[field #31] Percent Complete: 0%
[field #33] Release: None
[field #58] Custom Select Box #1: None
[field #59] Custom Select Box #2: None
[field #60] Custom Select Box #3: None
[field #61] Custom Select Box #4: None
[field #62] Custom Select Box #5: None
[field #63] Custom Select Box #6: None
[field #64] Custom Select Box #7: None
[field #65] Custom Select Box #8: None
[field #66] Custom Select Box #9: None
[field #67] Custom Select Box #10: None

	Benjamin Wolsey <bwy>
Sun 23 Nov 2008 08:59:23 AM UTC, comment #4:

Looks like one of your bugs has got lost, mc. Greetings from the 
Gnash team.
	Benjamin Wolsey <bwy>
Thu 20 Nov 2008 07:12:46 AM UTC, comment #3:

What is this about, please?
	Benjamin Wolsey <bwy>
Thu 20 Nov 2008 06:47:36 AM UTC, comment #2:

a clumsy single-purpose patch is:

--- patchfs_ 2008-11-20 06:07:51.000000000 +0100
+++ patchfs 2008-11-20 07:42:18.000000000 +0100
@@ -10,6 +10,7 @@
use strict;
use POSIX;
use File::Temp 'tempfile';
+use IO::Handle;

# standard binaries
my $bzip = 'bzip2';
@@ -149,6 +150,15 @@
}
}

+sub _b
+{
+ my ($buf, $c)=@_;
+ return if $$buf !~ /^--- /;
+ $c = IO::Handle::getc(*I);
+ IO::Handle::ungetc(*I, ord($c));
+ return 1 if ord($c) == 43; # ord("+")
+}
+
# list files affected by patch
sub list
{
@@ -168,7 +178,7 @@

# recognize diff type
if (!$unified && !$context) {
- $unified=1 if (/^--- /);
+ $unified=1 if b(\$);
$context=1 if (/^\\\* /);
if (!$unified && !$context) {
$len+=length;
@@ -176,7 +186,7 @@
}
}

- if (($unified && /^--- /) || ($context && /^\\\* [^\]$/)) {
+ if (($unified && b(\$)) || ($context && /^\\\* [^\]$/)) {
# start of new file
if ($state==1) {
printf "-rw-r--r-- 1 %s %s %d %s %s%s\n", $uid, $gid, $len, datetime($time), $prefix, $f
	Igor Hrazdil <hrazdii>
Thu 20 Nov 2008 05:23:21 AM UTC, comment #1:

I forget, the patchfs script version is 1.17:
http://cvs.savannah.gnu.org/viewvc/mc/vfs/extfs/patchfs.in?revision=1.17&root=mc&view=markup
	Igor Hrazdil <hrazdii>
Sun 23 Nov 2008 08:59:26 AM UTC, original submission:

Problem: In my patch file is "---" on the beginning of line.

For better explanation i was changed my patchfs script:
# diff patchfs_ patchfs
142c142
< error "Can't parse unified diff header"
---

> error "Can't parse unified diff header >>$$buf<<"


The script output is:
$ /usr/share/mc/extfs/patchfs list adonis.patch.bz2
-rw-r--r-- 1 igor igor 566 02-09-2008 04:00 adonis/bin/adv_sugg_maintain.sh.diff
-rw-r--r-- 1 igor igor 1936 09-05-2008 23:52 adonis/bin/archiv.sh.diff
Can't parse unified diff header >>--- where ar.last_update >= "2008-09-25"
-group by 1,2
-order by 1,2;
<<

In one of original data file is line with "--" (a sql comment 
without blank space on the bebinning):
-- where ar.last_update >= "2008-09-25" 

Attachments

patchfs-testcase.diff (2.2 KB) - added by kdave 11 years ago.
testfile: valid diff which is misparsed with current patchfs
patchfs.in (9.8 KB) - added by kdave 11 years ago.
rewritten parser of unified diff files, copyin is disabled

Change History

comment:1 Changed 11 years ago by kdave

  • Status changed from new to accepted
  • Owner set to kdave
  • Milestone set to 4.7

I'm taking this one. I've managed to create a valid diff which is misparsed by extfs/patchfs. Currently the unified diff parsing for 'list' command is done.

Changed 11 years ago by kdave

testfile: valid diff which is misparsed with current patchfs

Changed 11 years ago by kdave

rewritten parser of unified diff files, copyin is disabled

comment:2 Changed 11 years ago by kdave

So, here's rewritten parser of unified diffs with a testfile. You can test it by running

patchfs list patchfs-testcase.diff

It does more fine-grained parsing of hunks and is about 2 times slower, still fast enough when tested on 20MB diff.

The script does not allow the 'copyin' command. Why? What should copyin to diff file mean? There are a few options:

  • copyin of a non-diff file into archive -- just appends the data, but never displays it back (parsed as a diff comment and thrown away)
  • copyin of a diff file, which contains hunks of files already in the archive -- now comes the tricky part:
  1. delete all data from previous file and replace them with the copyin contents
  2. just append the new hunks -- this may produce invalid diff, hunks in reversed order or changing same lines from more hunks
  3. merge hunks of the files and produce a valid diff -- do we really want to rewrite patchutils?

Should user choose what to do for copyin? Does anybody rely on this feature? (Rely to that extent that it cannot be done in other way?)

Given that, I'd rather disable copyin.

comment:3 follow-up: ↓ 4 Changed 11 years ago by kdave

  • Keywords review added
  • Version set to 4.6.1
  • Component changed from mc-core to vfs

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 10 years ago by angel_il

Replying to kdave:
is still actual for current

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 7 Changed 10 years ago by kdave

Replying to angel_il:

is still actual for current

Sorry, do you ask if this is still actual for current git? Yes. AFAICS, nobody committed it to master or any branch.

comment:6 Changed 10 years ago by angel_il

Sorry very-very mach. :)
is still actual for current master?

comment:7 in reply to: ↑ 5 Changed 10 years ago by angel_il

Replying to kdave:

Replying to angel_il:Yes. AFAICS, nobody committed it to master or any branch.

ok, i'll create branch 90_

comment:8 Changed 10 years ago by angel_il

Branch: 90_patchfs
changeset: a821f1c9897e42439272a2fdf06ceced7e7ad7bf

comment:9 Changed 10 years ago by angel_il

  • Keywords vote-angel_il added; review removed

comment:10 Changed 10 years ago by slavazanko

  • Keywords vote-slavazanko approved added

good fix.

comment:11 Changed 10 years ago by slavazanko

  • Keywords rework added; vote-angel_il vote-slavazanko approved removed

ps, no vote

Branch will remove xz-stuff. Not good.

comment:12 Changed 10 years ago by angel_il

  • Keywords review added; rework removed

branch 90_patchfs
new changeset :b29e81556f67087955a9eceec45347d002c781f1

ps:sorry now i can't vote becouse i change this source.

comment:13 Changed 10 years ago by slavazanko

  • Keywords vote-slavazanko added

Now it's really look good :)

comment:14 Changed 10 years ago by andrew_b

  • Keywords vote-andrew_b approved added; review removed

Agree. My vote here.

comment:15 Changed 10 years ago by angel_il

  • Status changed from accepted to testing
  • Keywords commited-master added; vote-slavazanko vote-andrew_b approved removed
  • Resolution set to fixed

comment:16 Changed 10 years ago by styx

  • Status changed from testing to closed

comment:17 Changed 6 years ago by ossi

  • Branch state set to no branch
  • Reporter changed from slavazanko to hrazdii

comment:18 Changed 6 years ago by andrew_b

  • Keywords commited-master removed
  • Votes for changeset set to commited-master
  • Branch state changed from no branch to merged
Note: See TracTickets for help on using tickets.