Problem with git-apply?

Problem with git-apply?

[Kevin Shanahan]

Hi,

I seem to be having problems using git-apply to apply any patch which
creates a new file. Unfortunately it's been a few weeks since I last
used git here locally, but this seems like some behaviour change since
the last version I was using. I'm currently using version 1.4.3.3 from
Debian Sid. This little test script demonstrates the problem I'm
having:

#!/bin/sh

mkdir a b repo
echo foo > b/foo
diff -urN a b > test.diff

(
    cd repo
    echo bar > bar # need something to init the db
    git init-db
    git add .
    git commit -a -m "Test Commit"

    git apply ../test.diff
    git commit -a -m "Test Commit (file added)"
)

Here, this outputs for me:

defaulting to local storage area
Committing initial tree ee314a31b622b027c10981acaed7903a3607dbd4
error: foo: No such file or directory
nothing to commit

Has something broken or am I doing something wrong here?

Thanks,

Re: Problem with git-apply?

[Shawn Pearce]

Kevin Shanahan <kmshanah@disenchant.net> wrote:
>     git apply ../test.diff

Try:

      git apply --index ../test.diff

instead.  The --index is needed to record the new file in the index;
otherwise it won't actually be added during commit.

-- 

Re: Problem with git-apply?

[Kevin Shanahan]

On Sat, Nov 04, 2006 at 05:53:49PM +1030, Kevin Shanahan wrote:
> I seem to be having problems using git-apply to apply any patch which
> creates a new file. Unfortunately it's been a few weeks since I last
> used git here locally, but this seems like some behaviour change since
> the last version I was using. I'm currently using version 1.4.3.3 from
> Debian Sid. This little test script demonstrates the problem I'm
> having:

Sorry, I probably should have said to CC me, as I'm not subscribed.

Thanks for your reply Shawn, but --index doesn't seem to make any
difference. If it helps, I can confirm that the script works fine with
version 1.4.1.1.

> #!/bin/sh
> 
> mkdir a b repo
> echo foo > b/foo
> diff -urN a b > test.diff
> 
> (
>     cd repo
>     echo bar > bar # need something to init the db
>     git init-db
>     git add .
>     git commit -a -m "Test Commit"
> 
>     git apply ../test.diff
>     git commit -a -m "Test Commit (file added)"
> )
> 
> Here, this outputs for me:
> 
> defaulting to local storage area
> Committing initial tree ee314a31b622b027c10981acaed7903a3607dbd4
> error: foo: No such file or directory
> nothing to commit
> 
> Has something broken or am I doing something wrong here?
> 
> Thanks,

Re: Problem with git-apply?

[Jakub Narebski]

Kevin Shanahan wrote:

> I seem to be having problems using git-apply to apply any patch which
> creates a new file. Unfortunately it's been a few weeks since I last
> used git here locally, but this seems like some behaviour change since
> the last version I was using. I'm currently using version 1.4.3.3 from
> Debian Sid. This little test script demonstrates the problem I'm
> having:
> 
> #!/bin/sh
> 
> mkdir a b repo
> echo foo > b/foo
> diff -urN a b > test.diff

Which produces the following diff:

diff -urN a/foo b/foo
--- a/foo       1970-01-01 01:00:00.000000000 +0100
+++ b/foo       2006-11-04 10:05:04.000000000 +0100
@@ -0,0 +1 @@
+foo
 
> (
>     cd repo
>     echo bar > bar # need something to init the db
>     git init-db
>     git add .
>     git commit -a -m "Test Commit"
> 
>     git apply ../test.diff
>     git commit -a -m "Test Commit (file added)"
> )
> 
> Here, this outputs for me:
> 
> defaulting to local storage area
> Committing initial tree ee314a31b622b027c10981acaed7903a3607dbd4
> error: foo: No such file or directory
> nothing to commit

This I think is a bug, or rather misfeature in git-apply (or at least
inconsistency between GNU diff and git patch format). But if you change
from-file line from "--- a/foo" to "--- /dev/null" then git-apply happily
applies creation patch and creates file.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: Problem with git-apply?

[Kevin Shanahan]

On Sat, Nov 04, 2006 at 10:26:13AM +0100, Jakub Narebski wrote:
> This I think is a bug, or rather misfeature in git-apply (or at least
> inconsistency between GNU diff and git patch format). But if you change
> from-file line from "--- a/foo" to "--- /dev/null" then git-apply happily
> applies creation patch and creates file.

Thanks. At least that gives me a workaround for now.

Cheers,

Re: Problem with git-apply?

[Junio C Hamano]

Kevin Shanahan <kmshanah@disenchant.net> writes:

> #!/bin/sh
>
> mkdir a b repo
> echo foo > b/foo
> diff -urN a b > test.diff

It is *very* surprising that this issue did not come up earlier,
given that we used to use GNU diff internally to generate our
own diff.

If you cat the test.diff file, you will see "a/foo" and "b/foo",
not "/dev/null".

The problem appears that GNU diff _never_ uses "--- /dev/null"
or "+++ /dev/null" to indicate creation or deletion of the file,
but the "traditional patch parser" builtin-apply has assumed
that is what the traditional diff output from day one.  Where we
got that idea is mystery to me (this is Linus's code), but I
suspect it is what other SCMs did.

With the GNU diff output, the only way to guess it is a file
creation patch is to notice that the patch has only a single
hunk that inserts into -0,0 (this tests that the file is
originally empty), and it is followed by a HT and the UNIX epoch
(i.e. 1970-01-01 00:00:00 GMT) timestamp in localtime + offset
format (the latter is a guess to tell creation from modification
to a file that was originally empty).  This means that we do not
have a way to tell if it was a file that had UNIX epoch
timestamp that was modified or if this is a creation.

Having to parse the localtime + offset and compare it with UNIX
epoch is already crazy, but the saddest part is that this is
quite GNU diff specific right now, and I'd rather not to require
the trailing HT + timestamp (which is now being written down in
new POSIX) to the "traditional patch" input format.

Probably a reasonable compromise is to treat a non-git patch
(i.e. the ones that does not begin with "diff --git") that
touches an empty file as a file creation patch, but I need to
look at the code to see how much damage such a change needs to
cause.  If I remember collectly it wants to decide if it is a
creation patch or modification patch upfront, so it may not be 
as trivial as it sounds.  We'll see.

Re: Problem with git-apply?

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Kevin Shanahan <kmshanah@disenchant.net> writes:
>
>> #!/bin/sh
>>
>> mkdir a b repo
>> echo foo > b/foo
>> diff -urN a b > test.diff
>
> It is *very* surprising that this issue did not come up earlier,
> given that we used to use GNU diff internally to generate our
> own diff.
>
> If you cat the test.diff file, you will see "a/foo" and "b/foo",
> not "/dev/null".
>
> The problem appears that GNU diff _never_ uses "--- /dev/null"
> or "+++ /dev/null" to indicate creation or deletion of the file,
> but the "traditional patch parser" builtin-apply has assumed
> that is what the traditional diff output from day one.  Where we
> got that idea is mystery to me (this is Linus's code), but I
> suspect it is what other SCMs did.

*BLUSH*  A prime example of "you should not speak before
thinking".

Please forget everything I said.  The patch parsing is just fine
with or without "/dev/null".  This must be a recent breakage
around write_out_one_result().  Will take a look.

Re: Problem with git-apply?

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> *BLUSH*  A prime example of "you should not speak before
> thinking".
>
> Please forget everything I said.  The patch parsing is just fine
> with or without "/dev/null".  This must be a recent breakage
> around write_out_one_result().  Will take a look.


Actually it was a small problem in the patch parsing code.  Can
you give this a try?

---
diff --git a/builtin-apply.c b/builtin-apply.c
index 11397f5..db7cdce 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1043,10 +1043,14 @@ static int parse_single_patch(char *line
 		 * then not having oldlines means the patch is creation,
 		 * and not having newlines means the patch is deletion.
 		 */
-		if (patch->is_new < 0 && !oldlines)
+		if (patch->is_new < 0 && !oldlines) {
 			patch->is_new = 1;
-		if (patch->is_delete < 0 && !newlines)
+			patch->old_name = NULL;
+		}
+		if (patch->is_delete < 0 && !newlines) {
 			patch->is_delete = 1;
+			patch->new_name = NULL;
+		}
 	}
 
 	if (0 < patch->is_new && oldlines)

Re: Problem with git-apply?

[Kevin Shanahan]

On Sat, Nov 04, 2006 at 02:15:55AM -0800, Junio C Hamano wrote:
> Actually it was a small problem in the patch parsing code.  Can
> you give this a try?

Yes, this patch works for me.

Cheers,
Kev.

> ---
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 11397f5..db7cdce 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -1043,10 +1043,14 @@ static int parse_single_patch(char *line
>  		 * then not having oldlines means the patch is creation,
>  		 * and not having newlines means the patch is deletion.
>  		 */
> -		if (patch->is_new < 0 && !oldlines)
> +		if (patch->is_new < 0 && !oldlines) {
>  			patch->is_new = 1;
> -		if (patch->is_delete < 0 && !newlines)
> +			patch->old_name = NULL;
> +		}
> +		if (patch->is_delete < 0 && !newlines) {
>  			patch->is_delete = 1;
> +			patch->new_name = NULL;
> +		}
>  	}
>  
>  	if (0 < patch->is_new && oldlines)

Re: Problem with git-apply?

[Linus Torvalds]

On Sat, 4 Nov 2006, Junio C Hamano wrote:
> 
> The problem appears that GNU diff _never_ uses "--- /dev/null" or "+++ 
> /dev/null" to indicate creation or deletion of the file, but the 
> "traditional patch parser" builtin-apply has assumed that is what the 
> traditional diff output from day one.  Where we got that idea is mystery 
> to me (this is Linus's code), but I suspect it is what other SCMs did.

No, the original code used to trigger a "create" diff on
 - source was /dev/null
OR
 - source had zero patches.

It used to have code like

        if (patch->is_new < 0) {
                patch->is_new =  !oldlines;
                if (!oldlines)
                        patch->old_name = NULL;
        }
        if (patch->is_delete < 0) {
                patch->is_delete = !newlines;
                if (!newlines)
                        patch->new_name = NULL;
        }

and I think the person who broke it was you ;)

According to git-pickaxe, the buggy commit is 4be60962.

You should know by now that I never have bugs. 

Re: Problem with git-apply?

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Sat, 4 Nov 2006, Junio C Hamano wrote:
>> 
>> The problem appears that GNU diff _never_ uses "--- /dev/null" or "+++ 
>> /dev/null" to indicate creation or deletion of the file, but the 
>> "traditional patch parser" builtin-apply has assumed that is what the 
>> traditional diff output from day one.  Where we got that idea is mystery 
>> to me (this is Linus's code), but I suspect it is what other SCMs did.
>
> No, the original code used to trigger a "create" diff on
>  - source was /dev/null
> OR
>  - source had zero patches.
>
> It used to have code like
>...
> and I think the person who broke it was you ;)

Sorry, if you followed the threads by now you know I know that.
A few messages down there is a fix for the breakage caused by
4be60962, which I'll have in 'maint'.

I think it is a time for a 1.4.3.4; these are queued since
we did 1.4.3.3.

Andy Parkins (2):
  Minor grammar fixes for git-diff-index.txt
  git-clone documentation didn't mention --origin as equivalent of -o

Christian Couder (3):
  Remove --syslog in git-daemon inetd documentation examples.
  Documentation: add upload-archive service to git-daemon.
  Documentation: add git in /etc/services.

Edgar Toernig (1):
  Use memmove instead of memcpy for overlapping areas

J. Bruce Fields (1):
  Documentation: updates to "Everyday GIT"

Jakub Narebski (3):
  diff-format.txt: Combined diff format documentation supplement
  diff-format.txt: Correct information about pathnames quoting in patch format
  gitweb: Check git base URLs before generating URL from it

Jan Harkes (1):
  Continue traversal when rev-list --unpacked finds a packed commit.

Johannes Schindelin (1):
  link_temp_to_file: call adjust_shared_perm() only when we created the directory

Junio C Hamano (9):
  Documentation: clarify refname disambiguation rules.
  combine-diff: a few more finishing touches.
  combine-diff: fix hunk_comment_line logic.
  combine-diff: honour --no-commit-id
  Surround "#define DEBUG 0" with "#ifndef DEBUG..#endif"
  quote.c: ensure the same quoting across platforms.
  revision traversal: --unpacked does not limit commit list anymore.
  link_temp_to_file: don't leave the path truncated on adjust_shared_perm failure
  apply: handle "traditional" creation/deletion diff correctly.

Nicolas Pitre (1):
  pack-objects doesn't create random pack names

Rene Scharfe (1):
  git-cherry: document limit and add diagram