{"thread":{"id":"28108","subject":"rejecting patches that have an offset","startedAt":"2011-08-15T23:16:58Z","lastAt":"2011-08-16T23:41:30Z","messageCount":5,"participants":["Eric Blake","Andreas Gruenbacher","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"173560","messageId":"4E49A8EA.5020507@redhat.com","threadId":"28108","inReplyTo":null,"subject":"rejecting patches that have an offset","fromName":"Eric Blake","fromEmail":"eblake-h+wxahxf7alqt0dzr+alfa@public.gmane.org","sentAt":"2011-08-15T23:16:58Z","receivedAt":"2011-08-15T23:16:58Z","isPatch":false,"sender":{"key":"eblake-h+wxahxf7alqt0dzr+alfa@public.gmane.org","avatar":null},"body":"I ran into a case that cost me several hours today, while building an \nrpm file for libvirt. I have a context patch that only adds lines (no \ndeletions), and which had multiple places in the destination file where \nthe patch would match context and still apply, although only one of \nthose places will compile as correct. However, the patch file was \ninadvertently generated by git against the wrong version of the \ndestination, so the line numbers in the patch did not match the version \nof the file that I was trying to apply it to, and 'patch -p1 --fuzz=0 \n-s' ended up triggering patch's sliding algorithm where it applied the \npatch with an offset of 11 lines. Meanwhile, running the same patch \nthrough git applied the patch in a different offset: git found the \noffset that matched the function name in the @@ line, which was more \nthan 11 lines away, but actually matched the intent of the patch better.\n\nThe problem is that the difference in choice between patch and git \nresulted in a patch series that works or fails according to which tool \nyou pass it through. But the whole point of an rpm file is that if the \npatches were generated correctly, none of them should ever have any \noffset - an rpm should be tool-independent.\n\nIt would have saved me a lot of time if both 'patch' and 'git apply' \ncould be taught a mode of operation where they explicitly reject a patch \nthat cannot be applied without relying on an offset. That is, 'patch \n--fuzz=0' is too weak, and the fact that 'patch -s' squelched the error \nmessage meant that I had nothing to alert me to the fact that an offset \neven took place. And no, I don't want to filterdiff from patchutils to \nconvert the patch from context-diff over to ed-script-diff just to \nbenefit from the fact that patch does not do offset detection on \ned-script-patches.\n\nIf it were possible to optionally reject patches with offsets, then \nbuilding rpm files could use this mode to insist that all patches apply \noffset-free, making for a more robust patch chain (of course, the \ndefault should remain that the offset algorithm is still applied, and \nonly suppressed by explicit request, as the use of offsets is normally a \nvery useful feature - my point is that rpm patch chains are an exception \nfor the rule where offsets normally make life easier).\n\nIt might also be nice if patch could learn the algorithm that appears to \nmatch the git behavior, where when there are multiple points with \nidentical context (viewing just the context in isolation), but where \nthose locations differ in function location (as learned by the @@ header \nline in the patch file), then the preferred offset is the one in the \nnamed function, even if that is not the closes context match to the line \nnumber given in the patch file.\n\n-- \nEric Blake eblake-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org +1-801-349-2682\nLibvirt virtualization library http://libvirt.org\n"},{"id":"173643","messageId":"1313534889.5598.21.camel@schurl.linbit","threadId":"28108","inReplyTo":"4E49A8EA.5020507-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org","subject":"Re: rejecting patches that have an offset","fromName":"Andreas Gruenbacher","fromEmail":"andreas.gruenbacher-re5jqeeqqe8avxtiumwx3w@public.gmane.org","sentAt":"2011-08-16T22:48:07Z","receivedAt":"2011-08-16T22:48:07Z","isPatch":false,"sender":{"key":"andreas.gruenbacher-re5jqeeqqe8avxtiumwx3w@public.gmane.org","avatar":null},"body":"Eric,\n\nOn Mon, 2011-08-15 at 17:16 -0600, Eric Blake wrote:\n> It would have saved me a lot of time if both 'patch' and 'git apply' \n> could be taught a mode of operation where they explicitly reject a patch \n> that cannot be applied without relying on an offset.\n\nthat sounds reasonable. Can you send a patch or at least add a bug on\nSavannah?\n\n> It might also be nice if patch could learn the algorithm that appears to \n> match the git behavior, where when there are multiple points with \n> identical context (viewing just the context in isolation), but where \n> those locations differ in function location (as learned by the @@ header \n> line in the patch file), then the preferred offset is the one in the \n> named function, even if that is not the closes context match to the line \n> number given in the patch file.\n\nSounds interesting; a patch for that would be great as well.\n\nThanks,\nAndreas\n"},{"id":"173647","messageId":"4E4AF8F4.60709@redhat.com","threadId":"28108","inReplyTo":"1313534889.5598.21.camel@schurl.linbit","subject":"Re: [bug-patch] rejecting patches that have an offset","fromName":"Eric Blake","fromEmail":"eblake@redhat.com","sentAt":"2011-08-16T23:10:44Z","receivedAt":"2011-08-16T23:10:44Z","isPatch":true,"sender":{"key":"eblake@redhat.com","avatar":"https://avatars.githubusercontent.com/u/32933908?v=4"},"body":"On 08/16/2011 04:48 PM, Andreas Gruenbacher wrote:\n> Eric,\n>\n> On Mon, 2011-08-15 at 17:16 -0600, Eric Blake wrote:\n>> It would have saved me a lot of time if both 'patch' and 'git apply'\n>> could be taught a mode of operation where they explicitly reject a patch\n>> that cannot be applied without relying on an offset.\n>\n> that sounds reasonable. Can you send a patch or at least add a bug on\n> Savannah?\n\nBug opened: https://savannah.gnu.org/bugs/index.php?34031\n\n>\n>> It might also be nice if patch could learn the algorithm that appears to\n>> match the git behavior, where when there are multiple points with\n>> identical context (viewing just the context in isolation), but where\n>> those locations differ in function location (as learned by the @@ header\n>> line in the patch file), then the preferred offset is the one in the\n>> named function, even if that is not the closes context match to the line\n>> number given in the patch file.\n>\n> Sounds interesting; a patch for that would be great as well.\n\nBug opened: https://savannah.gnu.org/bugs/index.php?34032\n\n-- \nEric Blake eblake@redhat.com +1-801-349-2682\nLibvirt virtualization library http://libvirt.org\n"},{"id":"173648","messageId":"7vobzpeybh.fsf@alter.siamese.dyndns.org","threadId":"28108","inReplyTo":"4E49A8EA.5020507@redhat.com","subject":"Re: rejecting patches that have an offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-16T23:22:42Z","receivedAt":"2011-08-16T23:22:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Blake writes:\n\n> It would have saved me a lot of time if both 'patch' and 'git apply'\n> could be taught a mode of operation where they explicitly reject a\n> patch that cannot be applied without relying on an offset.\n\nI am not sure about this. I somehow doubt you would want to make sure that\nthe preimage your patch is to be applied must be bit-for-bit identical to\nwhat you prepared your patch for, IOW, you are using a patchfile merely as\na mean to \"compress\" the replacement file. You would want your RPM change\nto tolerate some changes in the upstream and keep your patch applicable to\nthe next version of the upstream, no?\n\nGiven a patch that is not precise and can apply to multiple places,\n\"patch\" and/or \"git apply\" can apply it to a place you may not have\nintended. It may feel like a bug if that happens to a preimage that is\nbit-for-bit identical to the version you prepared your patch is against,\nbut I would rather think, instead of blaming \"patch\" and/or \"git apply\",\nit would be more productive to prepare a patch with larger context when\nyou know that the preimage file you are patching has many similar looking\nlines, to make it _impossible_ for it to apply to places different from\nwhat you intended.\n"},{"id":"173649","messageId":"4E4B002A.8020207@redhat.com","threadId":"28108","inReplyTo":"7vobzpeybh.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org","subject":"Re: rejecting patches that have an offset","fromName":"Eric Blake","fromEmail":"eblake-h+wxahxf7alqt0dzr+alfa@public.gmane.org","sentAt":"2011-08-16T23:41:30Z","receivedAt":"2011-08-16T23:41:30Z","isPatch":false,"sender":{"key":"eblake-h+wxahxf7alqt0dzr+alfa@public.gmane.org","avatar":null},"body":"On 08/16/2011 05:22 PM, Junio C Hamano wrote:\n> Eric Blake writes:\n>\n>> It would have saved me a lot of time if both 'patch' and 'git apply'\n>> could be taught a mode of operation where they explicitly reject a\n>> patch that cannot be applied without relying on an offset.\n>\n> I am not sure about this. I somehow doubt you would want to make sure that\n> the preimage your patch is to be applied must be bit-for-bit identical to\n> what you prepared your patch for, IOW, you are using a patchfile merely as\n> a mean to \"compress\" the replacement file. You would want your RPM change\n> to tolerate some changes in the upstream and keep your patch applicable to\n> the next version of the upstream, no?\n\nWhen the RPM file is generated by git->patchfile list conversion, and I \nam trying to recreate a git repository from patchfile list->git, then \nyes, I _do_ want that patchfile list to apply bit-for-bit identical to \nanyone else starting from the same point, whether they use git or patch, \nso that anyone else can regenerate the end sources that were compiled \ninto the rpm release.\n\nRemember, the rpm file format includes both the starting point (it \ndocuments the upstream tarball) and the changes to that starting point \n(a patch stream) that were used to create a given released binary, in a \nformat that is independent of git. The idea is that managing an rpm \npatch series in git is much nicer for day-to-day work (and daily work \nwithin that git repository greatly benefits from the default of being \nable to assume patch offsets, such as rebasing a patch series to apply \non top of newer upstream versions), but once converting from git out to \nrpm, the conversion from rpm back to git should give a bit-for-bit \nreplay. If heuristics for how to apply patch offsets change, and an rpm \nfile includes an ambiguous patch that requires an offset, then you risk \nthe rpm file being broken the moment you upgrade to a newer tool chain \nwith a slightly different heuristic for where to resolve offsets; but if \nall patches in the series are 0-offset, then you have isolated the rpm \nfrom any implicit dependency on the version of the tool used to \nreconstruct the final software from the patch series. So the question \nis now how to identify whether a patch series meets that 0-offset rule, \nand that's where a new option would be handy.\n\nHence, I'm requesting an option to reject patches with non-zero offsets, \nbut not making it default, as there are only a few limited places (such \nas rebuilding a git repo starting from an rpm patch list) where \nbit-for-bit rebuild is more desirable than accounting for offsets due to \nchanges in the starting point.\n\n>\n> Given a patch that is not precise and can apply to multiple places,\n> \"patch\" and/or \"git apply\" can apply it to a place you may not have\n> intended. It may feel like a bug if that happens to a preimage that is\n> bit-for-bit identical to the version you prepared your patch is against,\n> but I would rather think, instead of blaming \"patch\" and/or \"git apply\",\n> it would be more productive to prepare a patch with larger context when\n> you know that the preimage file you are patching has many similar looking\n> lines, to make it _impossible_ for it to apply to places different from\n> what you intended.\n\nYes, I know that as well - the particular patch that sparked this thread \nwas ambiguous with three lines of context, but unambiguous with 6 lines, \neven when an offset still had to be applied.\n\nSo maybe you raise yet another feature proposal: What would it take for \ngit to generate unambiguous patches - that is, when generating a patch \nwith context, to then ensure that given the file it is being applied to, \nthen context is auto-widened until there are no other offsets where the \npatch can possibly be applied? In other words, if I say 'git diff HEAD^ \n--auto-context', then the resulting patch would have automatically have \n6 context lines for my problematic hunk, while sticking to the default 3 \ncontext lines for other hunks that were already unambiguous. Of course, \nthis only protects you if starting from the same version of the file \n(since any other patch can introduce an ambiguity not present at the \ntime you computed the minimal context needed for non-ambiguity in your \nversion of the pre-patch file).\n\n-- \nEric Blake eblake-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org +1-801-349-2682\nLibvirt virtualization library http://libvirt.org\n"}]}