[PATCH] git-p4: format-patch to diff-tree change breaks binary patches

[PATCH] git-p4: format-patch to diff-tree change breaks binary patches

[Tolga Ceylan]

When applying binary patches a full index is required. format-patch
already handles this, but diff-tree needs '--full-index' argument
to always output full index.

Signed-off-by: Tolga Ceylan <tolga.ceylan@gmail.com>
---
 git-p4.py |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index cdfa2df..4ee6739 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1311,7 +1311,7 @@ class P4Submit(Command, P4UserMap):
             else:
                 die("unknown modifier %s for %s" % (modifier, path))
 
-        diffcmd = "git diff-tree -p \"%s\"" % (id)
+        diffcmd = "git diff-tree --full-index -p \"%s\"" % (id)
         patchcmd = diffcmd + " | git apply "
         tryPatchCmd = patchcmd + "--check -"
         applyPatchCmd = patchcmd + "--check --apply -"
-- 
1.7.9.5

Re: [PATCH] git-p4: format-patch to diff-tree change breaks binary patches

[Pete Wyckoff]

tolga.ceylan@gmail.com wrote on Thu, 24 Apr 2014 21:46 -0700:
> When applying binary patches a full index is required. format-patch
> already handles this, but diff-tree needs '--full-index' argument
> to always output full index.
> 
> Signed-off-by: Tolga Ceylan <tolga.ceylan@gmail.com>
> ---
>  git-p4.py |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index cdfa2df..4ee6739 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1311,7 +1311,7 @@ class P4Submit(Command, P4UserMap):
>              else:
>                  die("unknown modifier %s for %s" % (modifier, path))
>  
> -        diffcmd = "git diff-tree -p \"%s\"" % (id)
> +        diffcmd = "git diff-tree --full-index -p \"%s\"" % (id)
>          patchcmd = diffcmd + " | git apply "
>          tryPatchCmd = patchcmd + "--check -"
>          applyPatchCmd = patchcmd + "--check --apply -"
> -- 

This looks like a straightforward change, but can you give a
bit more background on why a full index is required?  Do you
mean that "git apply" will reject a patch with abbreviated
blob object names?

		-- Pete

Re: [PATCH] git-p4: format-patch to diff-tree change breaks binary patches

[tolga ceylan]

Yes, when git-p4 runs git-apply to test the patch, this fails
due to abbreviated blob object names. I think git-apply requires
full object names for binary patches.

On 04/26/2014 05:43 AM, Pete Wyckoff wrote:
> tolga.ceylan@gmail.com wrote on Thu, 24 Apr 2014 21:46 -0700:
>> When applying binary patches a full index is required. format-patch
>> already handles this, but diff-tree needs '--full-index' argument
>> to always output full index.
>>
>> Signed-off-by: Tolga Ceylan <tolga.ceylan@gmail.com>
>> ---
>>   git-p4.py |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index cdfa2df..4ee6739 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1311,7 +1311,7 @@ class P4Submit(Command, P4UserMap):
>>               else:
>>                   die("unknown modifier %s for %s" % (modifier, path))
>>
>> -        diffcmd = "git diff-tree -p \"%s\"" % (id)
>> +        diffcmd = "git diff-tree --full-index -p \"%s\"" % (id)
>>           patchcmd = diffcmd + " | git apply "
>>           tryPatchCmd = patchcmd + "--check -"
>>           applyPatchCmd = patchcmd + "--check --apply -"
>> --
>
> This looks like a straightforward change, but can you give a
> bit more background on why a full index is required?  Do you
> mean that "git apply" will reject a patch with abbreviated
> blob object names?
>
> 		-- Pete
>

Re: [PATCH] git-p4: format-patch to diff-tree change breaks binary patches

[tolga ceylan]

On 04/26/2014 02:12 PM, tolga ceylan wrote:
> Yes, when git-p4 runs git-apply to test the patch, this fails
> due to abbreviated blob object names. I think git-apply requires
> full object names for binary patches.
>
>> This looks like a straightforward change, but can you give a
>> bit more background on why a full index is required?  Do you
>> mean that "git apply" will reject a patch with abbreviated
>> blob object names?
>>
>>         -- Pete
>>

This is the error message git-apply emits in this case:

error: cannot apply binary patch to '<filename>' without full index line
error: <filename>: patch does not apply

Cheers,
Tolga

Re: [PATCH] git-p4: format-patch to diff-tree change breaks binary patches

[tolga ceylan]

>
> This is the error message git-apply emits in this case:
>
> error: cannot apply binary patch to '<filename>' without full index line
> error: <filename>: patch does not apply
>
> Cheers,
> Tolga

Any feedback is appreciated.
Cheers,
Tolga

Re: [PATCH] git-p4: format-patch to diff-tree change breaks binary patches

[Pete Wyckoff]

tolga.ceylan@gmail.com wrote on Fri, 02 May 2014 22:40 -0700:
> 
> >
> >This is the error message git-apply emits in this case:
> >
> >error: cannot apply binary patch to '<filename>' without full index line
> >error: <filename>: patch does not apply
> >
> >Cheers,
> >Tolga
> 
> Any feedback is appreciated.

Sorry, travel delay.  This explanation is pretty
straight-forward, thanks.

Suggest you include it in the commit message along with the
other text you had, and resend to the list, cc me and junio.
Oh, and include an ack:

    Acked-by: Pete Wyckoff <pw@padd.com>

[PATCH] git-p4: format-patch to diff-tree change breaks binary patches

[Tolga Ceylan]

When applying binary patches a full index is required. format-patch
already handles this, but diff-tree needs '--full-index' argument
to always output full index. When git-p4 runs git-apply to test
the patch, git-apply rejects the patch due to abbreviated blob
object names. This is the error message git-apply emits in this
case:

error: cannot apply binary patch to '<filename>' without full index line
error: <filename>: patch does not apply

Signed-off-by: Tolga Ceylan <tolga.ceylan@gmail.com>
Acked-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index cdfa2df..4ee6739 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1311,7 +1311,7 @@ class P4Submit(Command, P4UserMap):
             else:
                 die("unknown modifier %s for %s" % (modifier, path))
 
-        diffcmd = "git diff-tree -p \"%s\"" % (id)
+        diffcmd = "git diff-tree --full-index -p \"%s\"" % (id)
         patchcmd = diffcmd + " | git apply "
         tryPatchCmd = patchcmd + "--check -"
         applyPatchCmd = patchcmd + "--check --apply -"
-- 
1.7.9.5

Re: [PATCH] git-p4: format-patch to diff-tree change breaks binary patches

[Junio C Hamano]

Tolga Ceylan <tolga.ceylan@gmail.com> writes:

> When applying binary patches a full index is required. format-patch
> already handles this, but diff-tree needs '--full-index' argument
> to always output full index. When git-p4 runs git-apply to test
> the patch, git-apply rejects the patch due to abbreviated blob
> object names. This is the error message git-apply emits in this
> case:
>
> error: cannot apply binary patch to '<filename>' without full index line
> error: <filename>: patch does not apply
>
> Signed-off-by: Tolga Ceylan <tolga.ceylan@gmail.com>
> Acked-by: Pete Wyckoff <pw@padd.com>
> ---

Because the original breakage was already in 1.9, not a regression
between 1.9 and master, as the matter of principle our default is to
defer until 2.0 final to avoid risking unintended additional
breakages elsewhere.  But this fix is an obviously correct and
trivial single liner that were eyeballed by more than one person,
and that affects only three calls to os.system(), and its
correctness can be seen even without knowing p4 at all by somebody
like me ;-)

So let's queue it for 2.0.

Thanks.

>  git-p4.py |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index cdfa2df..4ee6739 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1311,7 +1311,7 @@ class P4Submit(Command, P4UserMap):
>              else:
>                  die("unknown modifier %s for %s" % (modifier, path))
>  
> -        diffcmd = "git diff-tree -p \"%s\"" % (id)
> +        diffcmd = "git diff-tree --full-index -p \"%s\"" % (id)
>          patchcmd = diffcmd + " | git apply "
>          tryPatchCmd = patchcmd + "--check -"
>          applyPatchCmd = patchcmd + "--check --apply -"