patch, 4 partsgit apply vs. renamed files index mismatch (was: Re: [PATCH 1/4] POWERPC: Rename dma_64.c to dma.c)

> Becky Bruce wrote:
>> This is in preparation for the merge of the 32 and 64-bit
>> dma code in arch/powerpc.
>>
>> Signed-off-by: Becky Bruce <becky.bruce@freescale.com>
>> ---
>>  arch/powerpc/kernel/Makefile |    2 +-
>>  arch/powerpc/kernel/dma.c    |  200 ++++++++++++++++++++++++++++++++++++++++++
>>  arch/powerpc/kernel/dma_64.c |  200 ------------------------------------------
>>  3 files changed, 201 insertions(+), 201 deletions(-)
>>  create mode 100644 arch/powerpc/kernel/dma.c
>>  delete mode 100644 arch/powerpc/kernel/dma_64.c
>
> Passing -M to git format-patch makes it much easier

> diff --git a/arch/powerpc/kernel/dma.c b/arch/powerpc/kernel/dma.c
> new file mode 100644
> index 0000000..ae5708e
> [...]
> diff --git a/arch/powerpc/kernel/dma_64.c b/arch/powerpc/kernel/dma_64.c
> deleted file mode 100644
> index ae5708e..0000000
> 
> That is, if hashes match then it was pure rename.

> Anton Vorontsov wrote:
>> I always thought that posting "-M" patches to the public lists is
>> discouraged since it is quite difficult to apply them via patch(1).
>> Also think of non-git users...
>
> I think the substantially enhanced reviewability trumps non-git-users  
> who can follow the rename instructions manually (or fix up their patch  
> utility) if they insist on shunning tools that would make their life 
> easier.

>> diff --git a/arch/powerpc/kernel/dma.c b/arch/powerpc/kernel/dma.c
>> new file mode 100644
>> index 0000000..ae5708e
>> [...]
>> diff --git a/arch/powerpc/kernel/dma_64.c b/arch/powerpc/kernel/dma_64.c
>> deleted file mode 100644
>> index ae5708e..0000000
>>
>> That is, if hashes match then it was pure rename.
>
> I suppose, though it's not as easy to spot, and won't help in showing  
> what the differences are if there are any.

> On Mon, Sep 08, 2008 at 04:38:57PM -0500, Scott Wood wrote:
> > Anton Vorontsov wrote:
> >> I always thought that posting "-M" patches to the public lists is
> >> discouraged since it is quite difficult to apply them via patch(1).
> >> Also think of non-git users...
> >
> > I think the substantially enhanced reviewability trumps non-git-users  
> > who can follow the rename instructions manually (or fix up their patch  
> > utility) if they insist on shunning tools that would make their life 
> > easier.
> 
> Can't disagree, I myself use git. ;-) But I'm also using standard
> patch(1) to test occasional patches... And sometimes linux-X tree
> isn't tracked by git (e.g. -mm otm snapshots).
> 
> >> diff --git a/arch/powerpc/kernel/dma.c b/arch/powerpc/kernel/dma.c
> >> new file mode 100644
> >> index 0000000..ae5708e
> >> [...]
> >> diff --git a/arch/powerpc/kernel/dma_64.c b/arch/powerpc/kernel/dma_64.c
> >> deleted file mode 100644
> >> index ae5708e..0000000
> >>
> >> That is, if hashes match then it was pure rename.
> >
> > I suppose, though it's not as easy to spot, and won't help in showing  
> > what the differences are if there are any.
> 
> ..rename and changes ideally go in separate patches.

>>>  3 files changed, 201 insertions(+), 201 deletions(-)
>>>  create mode 100644 arch/powerpc/kernel/dma.c
>>>  delete mode 100644 arch/powerpc/kernel/dma_64.c
>>
>> Passing -M to git format-patch makes it much easier
>
> I always thought that posting "-M" patches to the public lists is
> discouraged since it is quite difficult to apply them via patch(1).
> Also think of non-git users...

> This is still possible by comparing the hashes:
> ...
> That is, if hashes match then it was pure rename.
>
> Though, too bad git {apply,am} does not produce any warnings if there
> are any hidden changes...

> Anton Vorontsov <avorontsov@ru.mvista.com> writes:
> 
> >>>  3 files changed, 201 insertions(+), 201 deletions(-)
> >>>  create mode 100644 arch/powerpc/kernel/dma.c
> >>>  delete mode 100644 arch/powerpc/kernel/dma_64.c
> >>
> >> Passing -M to git format-patch makes it much easier
> >
> > I always thought that posting "-M" patches to the public lists is
> > discouraged since it is quite difficult to apply them via patch(1).
> > Also think of non-git users...
> 
> My understanding has been that it is encouraged on the kernel mailing
> list, because the rename format is far easier to review by showing the
> differences that matter to reviewers, than showing a big chunk of text
> deleted and another big chunk of text that is similar added elsewhere.
> 
> I won't comment on this any further; the use of it is strictly a list and
> community policy issue.
> 
> > This is still possible by comparing the hashes:
> > ...
> > That is, if hashes match then it was pure rename.
> >
> > Though, too bad git {apply,am} does not produce any warnings if there
> > are any hidden changes...
> 
> But I _do_ want to know what you mean by this comment.  Your statement
> makes it sounds as if apply/am happily and silently accept "hidden
> changes" and it is a bad thing.
> 
> Now what do you exactly mean by "any hidden changes"?  Do you mean "the
> sender did not use renaming format, the patch you fed was a one that
> removes a huge chunk of text from one file, and adds a similarly huge
> chunk of text to another file.  The changes to these files looked similar
> but was not quite the same"?  It is all there for you to review, and
> especially if you prefer non-renaming format, then that is what you get.

> Now consider the following patch (modified by hand: it should say
> +foo, but I changed it to +bar).
> ...
> The "index ..." stuff says that there are no changes and it is
> pure rename, but obviously there is a change.

> Anton Vorontsov <avorontsov@ru.mvista.com> writes:
> 
> > Now consider the following patch (modified by hand: it should say
> > +foo, but I changed it to +bar).
> > ...
> > The "index ..." stuff says that there are no changes and it is
> > pure rename, but obviously there is a change.
> 
> Ah, I see what you mean.  But in general, it is not obvious at all.
> 
> If you have the identical preimage (recorded on the LHS of the index line)
> or the patch reproduces the postimage in full (i.e. "create a new file"),
> you *could* notice.  It's an interesting idea from git person's point of
> view (i.e. "would be fun to implement"), but I doubt it would be useful in
> practice, because:
> 
>  (1) You often do not have the identically matching preimage;
> 
>  (2) More importantly, it is not unusual for people to *edit* the patch in
>      their MUA (think of typofixes), after getting it out of git.

> On Tue, Sep 09, 2008 at 07:45:19AM -0700, Junio C Hamano wrote:
> ...
>> ...  It's an interesting idea from git person's point of
>> view (i.e. "would be fun to implement"), but I doubt it would be useful in
>> practice, because:
>> 
>>  (1) You often do not have the identically matching preimage;
>> 
>>  (2) More importantly, it is not unusual for people to *edit* the patch in
>>      their MUA (think of typofixes), after getting it out of git.
>
> Not for rename patches...

> As for implementing, isn't this as simple as this pseudo code:
>
> if (index_deleted_file == index_new_file)
> 	if (deleted_file != new_file)
> 		printk("warning\n");
>
> In the git-apply?