Re: Problem with git-apply?

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,

Kevin Shanahan <kmshanah@disenchant.net> wrote:

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.

On Sat, Nov 04, 2006 at 05:53:49PM +1030, Kevin Shanahan wrote:

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.

Kevin Shanahan wrote:

Which produces the following diff:

diff -urN a/foo b/foo

On Sat, Nov 04, 2006 at 10:26:13AM +0100, Jakub Narebski wrote:

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

Cheers,

Kevin Shanahan <kmshanah@disenchant.net> writes:

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.

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.

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

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

---

On Sat, Nov 04, 2006 at 02:15:55AM -0800, Junio C Hamano wrote:

Yes, this patch works for me.

Cheers, Kev.

On Sat, 4 Nov 2006, Junio C Hamano wrote:

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. 

Linus Torvalds <torvalds@osdl.org> writes:

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