git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Blake <eblake-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
To: bug-patch-mXXj517/zsQ@public.gmane.org,
	Git Mailing List <git-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: rejecting patches that have an offset
Date: Mon, 15 Aug 2011 17:16:58 -0600	[thread overview]
Message-ID: <4E49A8EA.5020507@redhat.com> (raw)

I ran into a case that cost me several hours today, while building an 
rpm file for libvirt.  I have a context patch that only adds lines (no 
deletions), and which had multiple places in the destination file where 
the patch would match context and still apply, although only one of 
those places will compile as correct.  However, the patch file was 
inadvertently generated by git against the wrong version of the 
destination, so the line numbers in the patch did not match the version 
of the file that I was trying to apply it to, and 'patch -p1 --fuzz=0 
-s' ended up triggering patch's sliding algorithm where it applied the 
patch with an offset of 11 lines.  Meanwhile, running the same patch 
through git applied the patch in a different offset: git found the 
offset that matched the function name in the @@ line, which was more 
than 11 lines away, but actually matched the intent of the patch better.

The problem is that the difference in choice between patch and git 
resulted in a patch series that works or fails according to which tool 
you pass it through.  But the whole point of an rpm file is that if the 
patches were generated correctly, none of them should ever have any 
offset - an rpm should be tool-independent.

It would have saved me a lot of time if both 'patch' and 'git apply' 
could be taught a mode of operation where they explicitly reject a patch 
that cannot be applied without relying on an offset.  That is, 'patch 
--fuzz=0' is too weak, and the fact that 'patch -s' squelched the error 
message meant that I had nothing to alert me to the fact that an offset 
even took place.  And no, I don't want to filterdiff from patchutils to 
convert the patch from context-diff over to ed-script-diff just to 
benefit from the fact that patch does not do offset detection on 
ed-script-patches.

If it were possible to optionally reject patches with offsets, then 
building rpm files could use this mode to insist that all patches apply 
offset-free, making for a more robust patch chain (of course, the 
default should remain that the offset algorithm is still applied, and 
only suppressed by explicit request, as the use of offsets is normally a 
very useful feature - my point is that rpm patch chains are an exception 
for the rule where offsets normally make life easier).

It might also be nice if patch could learn the algorithm that appears to 
match the git behavior, where when there are multiple points with 
identical context (viewing just the context in isolation), but where 
those locations differ in function location (as learned by the @@ header 
line in the patch file), then the preferred offset is the one in the 
named function, even if that is not the closes context match to the line 
number given in the patch file.

-- 
Eric Blake   eblake-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

             reply	other threads:[~2011-08-15 23:17 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-08-15 23:16 Eric Blake [this message]
     [not found] ` <4E49A8EA.5020507-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2011-08-16 22:48   ` rejecting patches that have an offset Andreas Gruenbacher
2011-08-16 23:10     ` [bug-patch] " Eric Blake
2011-08-16 23:22 ` Junio C Hamano
     [not found]   ` <7vobzpeybh.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
2011-08-16 23:41     ` Eric Blake

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4E49A8EA.5020507@redhat.com \
    --to=eblake-h+wxahxf7alqt0dzr+alfa@public.gmane.org \
    --cc=bug-patch-mXXj517/zsQ@public.gmane.org \
    --cc=git-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).