Re: rejecting patches that have an offset

[Eric Blake <eblake-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On 08/16/2011 05:22 PM, Junio C Hamano wrote:
> Eric Blake<eblake-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>  writes:
>
>> 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.
>
> I am not sure about this. I somehow doubt you would want to make sure that
> the preimage your patch is to be applied must be bit-for-bit identical to
> what you prepared your patch for, IOW, you are using a patchfile merely as
> a mean to "compress" the replacement file. You would want your RPM change
> to tolerate some changes in the upstream and keep your patch applicable to
> the next version of the upstream, no?

When the RPM file is generated by git->patchfile list conversion, and I 
am trying to recreate a git repository from patchfile list->git, then 
yes, I _do_ want that patchfile list to apply bit-for-bit identical to 
anyone else starting from the same point, whether they use git or patch, 
so that anyone else can regenerate the end sources that were compiled 
into the rpm release.

Remember, the rpm file format includes both the starting point (it 
documents the upstream tarball) and the changes to that starting point 
(a patch stream) that were used to create a given released binary, in a 
format that is independent of git.  The idea is that managing an rpm 
patch series in git is much nicer for day-to-day work (and daily work 
within that git repository greatly benefits from the default of being 
able to assume patch offsets, such as rebasing a patch series to apply 
on top of newer upstream versions), but once converting from git out to 
rpm, the conversion from rpm back to git should give a bit-for-bit 
replay.  If heuristics for how to apply patch offsets change, and an rpm 
file includes an ambiguous patch that requires an offset, then you risk 
the rpm file being broken the moment you upgrade to a newer tool chain 
with a slightly different heuristic for where to resolve offsets; but if 
all patches in the series are 0-offset, then you have isolated the rpm 
from any implicit dependency on the version of the tool used to 
reconstruct the final software from the patch series.  So the question 
is now how to identify whether a patch series meets that 0-offset rule, 
and that's where a new option would be handy.

Hence, I'm requesting an option to reject patches with non-zero offsets, 
but not making it default, as there are only a few limited places (such 
as rebuilding a git repo starting from an rpm patch list) where 
bit-for-bit rebuild is more desirable than accounting for offsets due to 
changes in the starting point.

>
> Given a patch that is not precise and can apply to multiple places,
> "patch" and/or "git apply" can apply it to a place you may not have
> intended. It may feel like a bug if that happens to a preimage that is
> bit-for-bit identical to the version you prepared your patch is against,
> but I would rather think, instead of blaming "patch" and/or "git apply",
> it would be more productive to prepare a patch with larger context when
> you know that the preimage file you are patching has many similar looking
> lines, to make it _impossible_ for it to apply to places different from
> what you intended.

Yes, I know that as well - the particular patch that sparked this thread 
was ambiguous with three lines of context, but unambiguous with 6 lines, 
even when an offset still had to be applied.

So maybe you raise yet another feature proposal: What would it take for 
git to generate unambiguous patches - that is, when generating a patch 
with context, to then ensure that given the file it is being applied to, 
then context is auto-widened until there are no other offsets where the 
patch can possibly be applied?  In other words, if I say 'git diff HEAD^ 
--auto-context', then the resulting patch would have automatically have 
6 context lines for my problematic hunk, while sticking to the default 3 
context lines for other hunks that were already unambiguous.  Of course, 
this only protects you if starting from the same version of the file 
(since any other patch can introduce an ambiguity not present at the 
time you computed the minimal context needed for non-ambiguity in your 
version of the pre-patch file).

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