threads / patch / 4996

patch, 2 partst7001: add test for git-mv dir1 dir2/

Subject: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

## tl;dr

22 messages between Jul 26, 2006 and Jul 28, 2006. Diffs are folded; open one to read it.

replies: 21people: 6as markdown or json

Johannes Schindelin· Jul 26, 2006, 17:41 UTC · lore

If dir2 already exists, git-mv should move dir1 _into_dir2/. Noticed by Jon Smirl.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
	Jon, this was your problem, right?
 t/t7001-mv.sh |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
Show changes to t/t7001-mv.sh +15 −0
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index d78c56a..322eaad 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -59,4 +59,19 @@ test_expect_success \
      git-diff-tree -r -M --name-status  HEAD^ HEAD | \
      grep -E "^R100.+path0/README.+path2/README"'
 
+test_expect_success \
+    'moving whole subdirectory into subdirectory' \
+    'git-mv path2 path1'
+
+test_expect_success \
+    'commiting the change' \
+    'git-commit -m dir-move -a'
+
+test_expect_success \
+    'checking the commit' \
+    'git-diff-tree -r -M --name-status  HEAD^ HEAD | \
+     grep -E "^R100.+path2/COPYING.+path1/path2/COPYING" &&
+     git-diff-tree -r -M --name-status  HEAD^ HEAD | \
+     grep -E "^R100.+path2/README.+path1/path2/README"'
+
 test_done
-- 
1.4.2.rc2.g96f2-dirty
Jon Smirl· Jul 26, 2006, 17:50 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

An exact test case:

git clone git foo git clone git foo1 cd foo mkdir zzz git mv gitweb zzz cg diff >patch cd ../foo1 cg patch <../foo/patch

This patch won't apply because zzz does not exist in foo1
-- 
Jon Smirl
jonsmirl@gmail.com
Junio C Hamano· Jul 26, 2006, 18:23 UTC · re: Jon Smirl · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

"Jon Smirl" <jonsmirl@gmail.com> writes:
Show 12 quoted lines
> An exact test case:
>
> git clone git foo
> git clone git foo1
> cd foo
> mkdir zzz
> git mv gitweb zzz
> cg diff >patch
> cd ../foo1
> cg patch <../foo/patch
>
> This patch won't apply because zzz does not exist in foo1

A short and sweet reproduction recipe is essential for a problem report like this, and I appreciate it very much. "git mv" in "master" should do the right thing and I see Johannes fixing (or fixed) the problem of "git mv" failing in his version.

We should correctly handle cases that fit your general description (the test t/t4112-apply-renames.sh has a file in "klibc/arch/x86_64" which is renamed and copied to two different locations under "include/arch"). The above does not reproduce for me if I used "git diff HEAD >patch" in place of "cg diff" (I cannot make cg behave on my machine).

$ mkdir zzz $ ~/git-master/bin/git-mv gitweb zzz/ $ LANG=C LC_ALL=C find gitweb zzz -type f -print find: gitweb: No such file or directory zzz/gitweb/README zzz/gitweb/gitweb.cgi zzz/gitweb/gitweb.css zzz/gitweb/test/M??rchen zzz/gitweb/test/file with spaces zzz/gitweb/test/file+plus+sign $ git diff HEAD >patch $ git checkout -f HEAD $ LANG=C LC_ALL=C find gitweb zzz -type f -print gitweb/README gitweb/gitweb.cgi gitweb/gitweb.css gitweb/test/M??rchen gitweb/test/file with spaces gitweb/test/file+plus+sign find: zzz: No such file or directory $ git apply --index <patch $ LANG=C LC_ALL=C find gitweb zzz -type f -print zzz/gitweb/README zzz/gitweb/gitweb.cgi zzz/gitweb/gitweb.css zzz/gitweb/test/M??rchen zzz/gitweb/test/file with spaces zzz/gitweb/test/file+plus+sign

A couple of weeks ago test t/t4114-apply-typechange.sh was added by Eric Wong, and it caught cases where git-apply was assuming that leading directories of a new file already exists or was complaining when you create file "foo" and remove a file "foo/bar" (that is, you used to have directory at "foo"). The problems that test found were all fixed as far as I recall.

Jon Smirl· Jul 26, 2006, 18:31 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

On 7/26/06, Junio C Hamano <junkio@cox.net> wrote:
Show 6 quoted lines
> We should correctly handle cases that fit your general
> description (the test t/t4112-apply-renames.sh has a file in
> "klibc/arch/x86_64" which is renamed and copied to two different
> locations under "include/arch").  The above does not reproduce
> for me if I used "git diff HEAD >patch" in place of "cg diff" (I
> cannot make cg behave on my machine).
cg diff makes a patch this looks like this
Show changes to 5 files +0 −0

gitweb/README, gitweb/gitweb.cgi, gitweb/gitweb.css, gitweb/test/file, gitweb/test/file+plus+sign

diff --git a/gitweb/README b/zzz/gitweb/README
similarity index 100%
rename from gitweb/README
rename to zzz/gitweb/README
diff --git a/gitweb/gitweb.cgi b/zzz/gitweb/gitweb.cgi
similarity index 100%
rename from gitweb/gitweb.cgi
rename to zzz/gitweb/gitweb.cgi
diff --git a/gitweb/gitweb.css b/zzz/gitweb/gitweb.css
similarity index 100%
rename from gitweb/gitweb.css
rename to zzz/gitweb/gitweb.css
diff --git "a/gitweb/test/M\303\244rchen" "b/zzz/gitweb/test/M\303\244rchen"
similarity index 100%
rename from "gitweb/test/M\303\244rchen"
rename to "zzz/gitweb/test/M\303\244rchen"
diff --git a/gitweb/test/file with spaces b/zzz/gitweb/test/file with spaces
similarity index 100%
rename from gitweb/test/file with spaces
rename to zzz/gitweb/test/file with spaces
diff --git a/gitweb/test/file+plus+sign b/zzz/gitweb/test/file+plus+sign
similarity index 100%
rename from gitweb/test/file+plus+sign
rename to zzz/gitweb/test/file+plus+sign

git diff HEAD makes a much longer patch that deltas out the existing
files and delta in the new file.

It's applying patches in the extended git format that fails.
-- 
Jon Smirl
jonsmirl@gmail.com
Junio C Hamano· Jul 26, 2006, 18:58 UTC · re: Jon Smirl · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

"Jon Smirl" <jonsmirl@gmail.com> writes:
> git diff HEAD makes a much longer patch that deltas out the existing
> files and delta in the new file.
>
> It's applying patches in the extended git format that fails.
Thanks.
Junio C Hamano· Jul 26, 2006, 19:31 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

Junio C Hamano <junkio@cox.net> writes:
Show 8 quoted lines
> "Jon Smirl" <jonsmirl@gmail.com> writes:
>
>> git diff HEAD makes a much longer patch that deltas out the existing
>> files and delta in the new file.
>>
>> It's applying patches in the extended git format that fails.
>
> Thanks.

... and it turns out that "git apply" (with or without --index) groks it just fine. I suspect the fix is quite recent (not in v1.4.2-rc1 but in v1.4.2-rc2).

Jon Smirl· Jul 26, 2006, 20:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

On 7/26/06, Junio C Hamano <junkio@cox.net> wrote:
Show 14 quoted lines
> Junio C Hamano <junkio@cox.net> writes:
>
> > "Jon Smirl" <jonsmirl@gmail.com> writes:
> >
> >> git diff HEAD makes a much longer patch that deltas out the existing
> >> files and delta in the new file.
> >>
> >> It's applying patches in the extended git format that fails.
> >
> > Thanks.
>
> ... and it turns out that "git apply" (with or without --index)
> groks it just fine.  I suspect the fix is quite recent (not in
> v1.4.2-rc1 but in v1.4.2-rc2).

I can confirm that git apply is handling this correctly with code from the current git tree. cogito is failing this case.

-- 
Jon Smirl
jonsmirl@gmail.com
Johannes Schindelin· Jul 26, 2006, 18:47 UTC · re: Jon Smirl · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

Hi,
On Wed, 26 Jul 2006, Jon Smirl wrote:
Show 12 quoted lines
> An exact test case:
> 
> git clone git foo
> git clone git foo1
> cd foo
> mkdir zzz
> git mv gitweb zzz
> cg diff >patch
> cd ../foo1
> cg patch <../foo/patch
> 
> This patch won't apply because zzz does not exist in foo1
Okay, I got it wrong, then. Thanks for the clarification!

Ciao, Dscho

Josef Weidendorfer· Jul 26, 2006, 18:39 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

On Wednesday 26 July 2006 19:41, Johannes Schindelin wrote:
> 
> If dir2 already exists, git-mv should move dir1 _into_dir2/.
> Noticed by Jon Smirl.

Thanks for adding this test. BTW, the original PERL script passes it quite fine.

I just looked at Jon's problem. Doesn't seem to be related to git-mv or git at all, but more a cogito problem. I have some cogito-0.18pre installed, and cg-patch is patching the stuff all itself, not using git for this. Pasky?

Doing the same with git, i.e. in a rep with existing dir/
 mkdir new
 git mv dir new
 git diff --cached -M -C >patch
 git reset --hard
 git apply <patch

However, "git status" shows the "new/" directory totally untracked afterwards. Is this expected?

Josef
Junio C Hamano· Jul 26, 2006, 19:05 UTC · re: Josef Weidendorfer · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

Josef Weidendorfer <Josef.Weidendorfer@gmx.de> writes:
Show 12 quoted lines
> On Wednesday 26 July 2006 19:41, Johannes Schindelin wrote:
>> 
>> If dir2 already exists, git-mv should move dir1 _into_dir2/.
>> Noticed by Jon Smirl.
>
> Thanks for adding this test.
> BTW, the original PERL script passes it quite fine.
>
> I just looked at Jon's problem. Doesn't seem to be related to
> git-mv or git at all, but more a cogito problem.
> I have some cogito-0.18pre installed, and cg-patch is patching
> the stuff all itself, not using git for this. Pasky?
"git apply" seems to grok this just fine.
Show 10 quoted lines
> Doing the same with git, i.e. in a rep with existing dir/
>
>  mkdir new
>  git mv dir new
>  git diff --cached -M -C >patch
>  git reset --hard
>  git apply <patch
>
> However, "git status" shows the "new/" directory totally
> untracked afterwards. Is this expected?

Running "git apply --index <patch" I see the "renamed: " in there and zzz (or your "new") is tracked.

Petr Baudis· Jul 28, 2006, 01:30 UTC · re: Josef Weidendorfer · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

Dear diary, on Wed, Jul 26, 2006 at 08:39:24PM CEST, I got a letter where Josef Weidendorfer <Josef.Weidendorfer@gmx.de> said that...

Show 12 quoted lines
> On Wednesday 26 July 2006 19:41, Johannes Schindelin wrote:
> > 
> > If dir2 already exists, git-mv should move dir1 _into_dir2/.
> > Noticed by Jon Smirl.
> 
> Thanks for adding this test.
> BTW, the original PERL script passes it quite fine.
> 
> I just looked at Jon's problem. Doesn't seem to be related to
> git-mv or git at all, but more a cogito problem.
> I have some cogito-0.18pre installed, and cg-patch is patching
> the stuff all itself, not using git for this. Pasky?

Unfortunately, git-apply is still quite unusable for Cogito. It can do fuzzy merging now, but here's some random list of more issues I still have with it (I'm leaving for some two weeks or so of holiday soon so I won't have to fix them soon personally; it'd be nice if someone did, though ;) :

  (i) No git-apply -R - well, it seems to me that I revert patches all
the time, don't you?
  (ii) I'd like git-apply to be as verbose as patch is, that is list
the files it touches as it goes
  (iii) There's no reject handling besides "panic" right now - it should
be able to create .rej files so that the user can fix things up
  (iv) I need git-apply to add/remove to/from index new/gone files,
while at the same time...
  (v) I want to allow applying of patches to working copy that is not
completely clean, even on top of modified files

But yes, I'd like cg-patch to move to use git-apply. It's currently _way_ too scary.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Snow falling on Perl. White noise covering line noise.
Hides all the bugs too. -- J. Putnam
Junio C Hamano· Jul 28, 2006, 02:41 UTC · re: Petr Baudis · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

Petr Baudis <pasky@suse.cz> writes:
Show 14 quoted lines
>   (i) No git-apply -R - well, it seems to me that I revert patches all
> the time, don't you?
>
>   (ii) I'd like git-apply to be as verbose as patch is, that is list
> the files it touches as it goes
>
>   (iii) There's no reject handling besides "panic" right now - it should
> be able to create .rej files so that the user can fix things up
>
>   (iv) I need git-apply to add/remove to/from index new/gone files,
> while at the same time...
>
>   (v) I want to allow applying of patches to working copy that is not
> completely clean, even on top of modified files

You probably should be able to talk me into doing these, but doesn't it already do (iv) and (v)?

Petr Baudis· Jul 28, 2006, 02:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

Dear diary, on Fri, Jul 28, 2006 at 04:41:04AM CEST, I got a letter where Junio C Hamano <junkio@cox.net> said that...

Show 9 quoted lines
> Petr Baudis <pasky@suse.cz> writes:
> >   (iv) I need git-apply to add/remove to/from index new/gone files,
> > while at the same time...
> >
> >   (v) I want to allow applying of patches to working copy that is not
> > completely clean, even on top of modified files
> 
> You probably should be able to talk me into doing these, but
> doesn't it already do (iv) and (v)?

Well, at once? I can do (iv) by adding --index but that contradicts (v). But maybe I'm missing something.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Snow falling on Perl. White noise covering line noise.
Hides all the bugs too. -- J. Putnam
Junio C Hamano· Jul 28, 2006, 04:48 UTC · re: Petr Baudis · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

Petr Baudis <pasky@suse.cz> writes:
Show 14 quoted lines
> Dear diary, on Fri, Jul 28, 2006 at 04:41:04AM CEST, I got a letter
> where Junio C Hamano <junkio@cox.net> said that...
>> Petr Baudis <pasky@suse.cz> writes:
>> >   (iv) I need git-apply to add/remove to/from index new/gone files,
>> > while at the same time...
>> >
>> >   (v) I want to allow applying of patches to working copy that is not
>> > completely clean, even on top of modified files
>> 
>> You probably should be able to talk me into doing these, but
>> doesn't it already do (iv) and (v)?
>
> Well, at once? I can do (iv) by adding --index but that contradicts (v).
> But maybe I'm missing something.

What should the semantics of such operation be? Apply to index on paths that are clean while leave the index entries untouched for paths that are dirty? What should happen on renamed paths that are dirty?

Petr Baudis· Jul 28, 2006, 15:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] t7001: add test for git-mv dir1 dir2/

Dear diary, on Fri, Jul 28, 2006 at 06:48:49AM CEST, I got a letter where Junio C Hamano <junkio@cox.net> said that...

Show 8 quoted lines
> Petr Baudis <pasky@suse.cz> writes:
> > Well, at once? I can do (iv) by adding --index but that contradicts (v).
> > But maybe I'm missing something.
> 
> What should the semantics of such operation be?  Apply to index
> on paths that are clean while leave the index entries untouched
> for paths that are dirty?  What should happen on renamed paths
> that are dirty?

Keep the original sha1 but change the name. If an entry with the new name already exists, we might just leave the index alone and create a .rej file. (Alternatively we might create two stages in the index file but we can't do that in case of regular rejects so I'd rather stay consistent.)

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Snow falling on Perl. White noise covering line noise.
Hides all the bugs too. -- J. Putnam
Johannes Schindelin· Jul 28, 2006, 10:14 UTC · re: Petr Baudis · lore

[PATCH] Teach git-apply about '-R'

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
On Fri, 28 Jul 2006, Petr Baudis wrote:
>   (i) No git-apply -R - well, it seems to me that I revert patches all
> the time, don't you?
 builtin-apply.c         |   68 ++++++++++++++++++++++++++++++++++++++++-------
 t/t4102-apply-rename.sh |   24 +++++++++++++++--
 2 files changed, 80 insertions(+), 12 deletions(-)
Show changes to 2 files +79 −12

builtin-apply.c, t/t4102-apply-rename.sh

diff --git a/builtin-apply.c b/builtin-apply.c
index d924ac3..eb3eda1 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -120,7 +120,7 @@ struct fragment {
 struct patch {
 	char *new_name, *old_name, *def_name;
 	unsigned int old_mode, new_mode;
-	int is_rename, is_copy, is_new, is_delete, is_binary;
+	int is_rename, is_copy, is_new, is_delete, is_binary, is_reverse;
 #define BINARY_DELTA_DEFLATED 1
 #define BINARY_LITERAL_DEFLATED 2
 	unsigned long deflate_origlen;
@@ -1119,6 +1119,37 @@ static int parse_chunk(char *buffer, uns
 	return offset + hdrsize + patchsize;
 }
 
+/* a and b may not overlap! */
+static void memswap(void *a, void *b, unsigned int len)
+{
+	char *ap = a, *bp = b;
+	int i;
+	char c;
+
+	for (i = 0; i < len; i++) {
+		c = ap[i]; ap[i] = bp[i]; bp[i] = c;
+	}
+}
+
+static void reverse_patches(struct patch *p)
+{
+	for (; p; p = p->next) {
+		struct fragment *frag = p->fragments;
+
+		memswap(p->new_name, p->old_name, sizeof(char *));
+		memswap(&p->new_mode, &p->old_mode, sizeof(unsigned int));
+		memswap(&p->is_new, &p->is_delete, sizeof(int));
+		memswap(&p->lines_added, &p->lines_deleted, sizeof(int));
+		memswap(p->old_sha1_prefix, p->new_sha1_prefix, 41);
+
+		for (; frag; frag = frag->next) {
+			memswap(&frag->newpos, &frag->oldpos, sizeof(int));
+			memswap(&frag->newlines, &frag->oldlines, sizeof(int));
+		}
+		p->is_reverse = !p->is_reverse;
+	}
+}
+
 static const char pluses[] = "++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++";
 static const char minuses[]= "----------------------------------------------------------------------";
 
@@ -1336,7 +1367,7 @@ static int apply_line(char *output, cons
 }
 
 static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag,
-	int inaccurate_eof)
+	int reverse, int inaccurate_eof)
 {
 	int match_beginning, match_end;
 	char *buf = desc->buffer;
@@ -1350,6 +1381,7 @@ static int apply_one_fragment(struct buf
 	int pos, lines;
 
 	while (size > 0) {
+		char first;
 		int len = linelen(patch, size);
 		int plen;
 
@@ -1366,16 +1398,23 @@ static int apply_one_fragment(struct buf
 		plen = len-1;
 		if (len < size && patch[len] == '\\')
 			plen--;
-		switch (*patch) {
+		first = *patch;
+		if (reverse) {
+			if (first == '-')
+				first = '+';
+			else if (first == '+')
+				first = '-';
+		}
+		switch (first) {
 		case ' ':
 		case '-':
 			memcpy(old + oldsize, patch + 1, plen);
 			oldsize += plen;
-			if (*patch == '-')
+			if (first == '-')
 				break;
 		/* Fall-through for ' ' */
 		case '+':
-			if (*patch != '+' || !no_add)
+			if (first != '+' || !no_add)
 				newsize += apply_line(new + newsize, patch,
 						      plen);
 			break;
@@ -1615,7 +1654,8 @@ static int apply_fragments(struct buffer
 		return apply_binary(desc, patch);
 
 	while (frag) {
-		if (apply_one_fragment(desc, frag, patch->inaccurate_eof) < 0)
+		if (apply_one_fragment(desc, frag, patch->is_reverse,
+					patch->inaccurate_eof) < 0)
 			return error("patch failed: %s:%ld",
 				     name, frag->oldpos);
 		frag = frag->next;
@@ -2142,7 +2182,8 @@ static int use_patch(struct patch *p)
 	return 1;
 }
 
-static int apply_patch(int fd, const char *filename, int inaccurate_eof)
+static int apply_patch(int fd, const char *filename,
+		int reverse, int inaccurate_eof)
 {
 	unsigned long offset, size;
 	char *buffer = read_patch_file(fd, &size);
@@ -2162,6 +2203,8 @@ static int apply_patch(int fd, const cha
 		nr = parse_chunk(buffer + offset, size, patch);
 		if (nr < 0)
 			break;
+		if (reverse)
+			reverse_patches(patch);
 		if (use_patch(patch)) {
 			patch_stats(patch);
 			*listp = patch;
@@ -2226,6 +2269,7 @@ int cmd_apply(int argc, const char **arg
 {
 	int i;
 	int read_stdin = 1;
+	int reverse = 0;
 	int inaccurate_eof = 0;
 
 	const char *whitespace_option = NULL;
@@ -2236,7 +2280,7 @@ int cmd_apply(int argc, const char **arg
 		int fd;
 
 		if (!strcmp(arg, "-")) {
-			apply_patch(0, "<stdin>", inaccurate_eof);
+			apply_patch(0, "<stdin>", reverse, inaccurate_eof);
 			read_stdin = 0;
 			continue;
 		}
@@ -2313,6 +2357,10 @@ int cmd_apply(int argc, const char **arg
 			parse_whitespace_option(arg + 13);
 			continue;
 		}
+		if (!strcmp(arg, "-R") || !strcmp(arg, "--reverse")) {
+			reverse = 1;
+			continue;
+		}
 		if (!strcmp(arg, "--inaccurate-eof")) {
 			inaccurate_eof = 1;
 			continue;
@@ -2333,12 +2381,12 @@ int cmd_apply(int argc, const char **arg
 			usage(apply_usage);
 		read_stdin = 0;
 		set_default_whitespace_mode(whitespace_option);
-		apply_patch(fd, arg, inaccurate_eof);
+		apply_patch(fd, arg, reverse, inaccurate_eof);
 		close(fd);
 	}
 	set_default_whitespace_mode(whitespace_option);
 	if (read_stdin)
-		apply_patch(0, "<stdin>", inaccurate_eof);
+		apply_patch(0, "<stdin>", reverse, inaccurate_eof);
 	if (whitespace_error) {
 		if (squelch_whitespace_errors &&
 		    squelch_whitespace_errors < whitespace_error) {
diff --git a/t/t4102-apply-rename.sh b/t/t4102-apply-rename.sh
index fbb508d..22da6a0 100755
--- a/t/t4102-apply-rename.sh
+++ b/t/t4102-apply-rename.sh
@@ -13,8 +13,8 @@ # setup
 cat >test-patch <<\EOF
 diff --git a/foo b/bar
 similarity index 47%
-copy from foo
-copy to bar
+rename from foo
+rename to bar
 --- a/foo
 +++ b/bar
 @@ -1 +1 @@
@@ -39,4 +39,24 @@ else
 	    'test -f bar && ls -l bar | grep "^-..x......"'
 fi
 
+test_expect_success 'apply reverse' \
+    'git-apply -R --index --stat --summary --apply test-patch &&
+     test "$(cat foo)" = "This is foo"'
+
+cat >test-patch <<\EOF
+diff --git a/foo b/bar
+similarity index 47%
+copy from foo
+copy to bar
+--- a/foo
++++ b/bar
+@@ -1 +1 @@
+-This is foo
++This is bar
+EOF
+
+test_expect_success 'apply copy' \
+    'git-apply --index --stat --summary --apply test-patch &&
+     test "$(cat bar)" = "This is bar" -a "$(cat foo)" = "This is foo"'
+
 test_done
-- 
1.4.2.rc2.g813d5-dirty
Linus Torvalds· Jul 28, 2006, 15:14 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Teach git-apply about '-R'

On Fri, 28 Jul 2006, Johannes Schindelin wrote:
>
> +/* a and b may not overlap! */
> +static void memswap(void *a, void *b, unsigned int len)
This is disgusting.
Especially since it's also slow as hell.
Show 9 quoted lines
> +		memswap(p->new_name, p->old_name, sizeof(char *));
> +		memswap(&p->new_mode, &p->old_mode, sizeof(unsigned int));
> +		memswap(&p->is_new, &p->is_delete, sizeof(int));
> +		memswap(&p->lines_added, &p->lines_deleted, sizeof(int));
> +		memswap(p->old_sha1_prefix, p->new_sha1_prefix, 41);
> +
> +		for (; frag; frag = frag->next) {
> +			memswap(&frag->newpos, &frag->oldpos, sizeof(int));
> +			memswap(&frag->newlines, &frag->oldlines, sizeof(int));

All but one of those are register sizes, so doing a horribly ugly "memswap()"to do them is truly nasty, when you could have done

	#define swap(a,b) myswap((a),(b),sizeof(a))
	#define myswap(a,b,size) do {		\
		unsigned char mytmp[size];	\
		memcpy(tmp, &a, size);		\
		memcpy(&a, &b, size);		\
		memcpy(&b, mytmp, size);	\
	} while (0)

and it would have worked MUCH more efficiently, since any sane compiler would immediately have noticed that you're doing word-sized copies, and optimized the hell out of it.

(Untested, of course).
		Linus
Johannes Schindelin· Jul 28, 2006, 15:46 UTC · re: Linus Torvalds · lore

[PATCH v2] Teach git-apply about '-R'

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
On Fri, 28 Jul 2006, Linus Torvalds wrote:
Show 6 quoted lines
> On Fri, 28 Jul 2006, Johannes Schindelin wrote:
> >
> > +/* a and b may not overlap! */
> > +static void memswap(void *a, void *b, unsigned int len)
> 
> This is disgusting.

Yes, it is. Sorry, was just meant for prototyping. I had in mind to replace it with temporary variables, but your solution seems way nicer.

 builtin-apply.c         |   65 ++++++++++++++++++++++++++++++++++++++++-------
 t/t4102-apply-rename.sh |   24 ++++++++++++++++-
 2 files changed, 77 insertions(+), 12 deletions(-)
Show changes to 2 files +76 −12

builtin-apply.c, t/t4102-apply-rename.sh

diff --git a/builtin-apply.c b/builtin-apply.c
index d924ac3..6b38a8a 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -120,7 +120,7 @@ struct fragment {
 struct patch {
 	char *new_name, *old_name, *def_name;
 	unsigned int old_mode, new_mode;
-	int is_rename, is_copy, is_new, is_delete, is_binary;
+	int is_rename, is_copy, is_new, is_delete, is_binary, is_reverse;
 #define BINARY_DELTA_DEFLATED 1
 #define BINARY_LITERAL_DEFLATED 2
 	unsigned long deflate_origlen;
@@ -1119,6 +1119,34 @@ static int parse_chunk(char *buffer, uns
 	return offset + hdrsize + patchsize;
 }
 
+#define swap(a,b) myswap((a),(b),sizeof(a))
+
+#define myswap(a, b, size) do {		\
+	unsigned char mytmp[size];	\
+	memcpy(mytmp, &a, size);		\
+	memcpy(&a, &b, size);		\
+	memcpy(&b, mytmp, size);		\
+} while (0)
+
+static void reverse_patches(struct patch *p)
+{
+	for (; p; p = p->next) {
+		struct fragment *frag = p->fragments;
+
+		swap(p->new_name, p->old_name);
+		swap(p->new_mode, p->old_mode);
+		swap(p->is_new, p->is_delete);
+		swap(p->lines_added, p->lines_deleted);
+		swap(p->old_sha1_prefix, p->new_sha1_prefix);
+
+		for (; frag; frag = frag->next) {
+			swap(frag->newpos, frag->oldpos);
+			swap(frag->newlines, frag->oldlines);
+		}
+		p->is_reverse = !p->is_reverse;
+	}
+}
+
 static const char pluses[] = "++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++";
 static const char minuses[]= "----------------------------------------------------------------------";
 
@@ -1336,7 +1364,7 @@ static int apply_line(char *output, cons
 }
 
 static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag,
-	int inaccurate_eof)
+	int reverse, int inaccurate_eof)
 {
 	int match_beginning, match_end;
 	char *buf = desc->buffer;
@@ -1350,6 +1378,7 @@ static int apply_one_fragment(struct buf
 	int pos, lines;
 
 	while (size > 0) {
+		char first;
 		int len = linelen(patch, size);
 		int plen;
 
@@ -1366,16 +1395,23 @@ static int apply_one_fragment(struct buf
 		plen = len-1;
 		if (len < size && patch[len] == '\\')
 			plen--;
-		switch (*patch) {
+		first = *patch;
+		if (reverse) {
+			if (first == '-')
+				first = '+';
+			else if (first == '+')
+				first = '-';
+		}
+		switch (first) {
 		case ' ':
 		case '-':
 			memcpy(old + oldsize, patch + 1, plen);
 			oldsize += plen;
-			if (*patch == '-')
+			if (first == '-')
 				break;
 		/* Fall-through for ' ' */
 		case '+':
-			if (*patch != '+' || !no_add)
+			if (first != '+' || !no_add)
 				newsize += apply_line(new + newsize, patch,
 						      plen);
 			break;
@@ -1615,7 +1651,8 @@ static int apply_fragments(struct buffer
 		return apply_binary(desc, patch);
 
 	while (frag) {
-		if (apply_one_fragment(desc, frag, patch->inaccurate_eof) < 0)
+		if (apply_one_fragment(desc, frag, patch->is_reverse,
+					patch->inaccurate_eof) < 0)
 			return error("patch failed: %s:%ld",
 				     name, frag->oldpos);
 		frag = frag->next;
@@ -2142,7 +2179,8 @@ static int use_patch(struct patch *p)
 	return 1;
 }
 
-static int apply_patch(int fd, const char *filename, int inaccurate_eof)
+static int apply_patch(int fd, const char *filename,
+		int reverse, int inaccurate_eof)
 {
 	unsigned long offset, size;
 	char *buffer = read_patch_file(fd, &size);
@@ -2162,6 +2200,8 @@ static int apply_patch(int fd, const cha
 		nr = parse_chunk(buffer + offset, size, patch);
 		if (nr < 0)
 			break;
+		if (reverse)
+			reverse_patches(patch);
 		if (use_patch(patch)) {
 			patch_stats(patch);
 			*listp = patch;
@@ -2226,6 +2266,7 @@ int cmd_apply(int argc, const char **arg
 {
 	int i;
 	int read_stdin = 1;
+	int reverse = 0;
 	int inaccurate_eof = 0;
 
 	const char *whitespace_option = NULL;
@@ -2236,7 +2277,7 @@ int cmd_apply(int argc, const char **arg
 		int fd;
 
 		if (!strcmp(arg, "-")) {
-			apply_patch(0, "<stdin>", inaccurate_eof);
+			apply_patch(0, "<stdin>", reverse, inaccurate_eof);
 			read_stdin = 0;
 			continue;
 		}
@@ -2313,6 +2354,10 @@ int cmd_apply(int argc, const char **arg
 			parse_whitespace_option(arg + 13);
 			continue;
 		}
+		if (!strcmp(arg, "-R") || !strcmp(arg, "--reverse")) {
+			reverse = 1;
+			continue;
+		}
 		if (!strcmp(arg, "--inaccurate-eof")) {
 			inaccurate_eof = 1;
 			continue;
@@ -2333,12 +2378,12 @@ int cmd_apply(int argc, const char **arg
 			usage(apply_usage);
 		read_stdin = 0;
 		set_default_whitespace_mode(whitespace_option);
-		apply_patch(fd, arg, inaccurate_eof);
+		apply_patch(fd, arg, reverse, inaccurate_eof);
 		close(fd);
 	}
 	set_default_whitespace_mode(whitespace_option);
 	if (read_stdin)
-		apply_patch(0, "<stdin>", inaccurate_eof);
+		apply_patch(0, "<stdin>", reverse, inaccurate_eof);
 	if (whitespace_error) {
 		if (squelch_whitespace_errors &&
 		    squelch_whitespace_errors < whitespace_error) {
diff --git a/t/t4102-apply-rename.sh b/t/t4102-apply-rename.sh
index fbb508d..22da6a0 100755
--- a/t/t4102-apply-rename.sh
+++ b/t/t4102-apply-rename.sh
@@ -13,8 +13,8 @@ # setup
 cat >test-patch <<\EOF
 diff --git a/foo b/bar
 similarity index 47%
-copy from foo
-copy to bar
+rename from foo
+rename to bar
 --- a/foo
 +++ b/bar
 @@ -1 +1 @@
@@ -39,4 +39,24 @@ else
 	    'test -f bar && ls -l bar | grep "^-..x......"'
 fi
 
+test_expect_success 'apply reverse' \
+    'git-apply -R --index --stat --summary --apply test-patch &&
+     test "$(cat foo)" = "This is foo"'
+
+cat >test-patch <<\EOF
+diff --git a/foo b/bar
+similarity index 47%
+copy from foo
+copy to bar
+--- a/foo
++++ b/bar
+@@ -1 +1 @@
+-This is foo
++This is bar
+EOF
+
+test_expect_success 'apply copy' \
+    'git-apply --index --stat --summary --apply test-patch &&
+     test "$(cat bar)" = "This is bar" -a "$(cat foo)" = "This is foo"'
+
 test_done
-- 
1.4.2.rc2.g8b063-dirty
Junio C Hamano· Jul 28, 2006, 15:36 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Teach git-apply about '-R'

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

A quick comment without looking much at the code. Do you sanely bail out when asked to reverse-apply a binary patch?

Also what was the reason to change an existing test vector in 4102? That does not look like related to -R flag.

Johannes Schindelin· Jul 28, 2006, 15:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] Teach git-apply about '-R'

Hi,
On Fri, 28 Jul 2006, Junio C Hamano wrote:
Show 6 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> A quick comment without looking much at the code.  Do you sanely
> bail out when asked to reverse-apply a binary patch?

Nope. I swap old_sha1_prefix and new_sha1_prefix in that case, I hoped that is enough?

> Also what was the reason to change an existing test vector in
> 4102?  That does not look like related to -R flag.

I changed "copy" to "rename", so that I could reuse that patch to test -R, too. Note that in the end, the original test vector is written, and tested.

Ciao, Dscho

Junio C Hamano· Jul 28, 2006, 19:20 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Teach git-apply about '-R'

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 7 quoted lines
> On Fri, 28 Jul 2006, Junio C Hamano wrote:
>> 
>> A quick comment without looking much at the code.  Do you sanely
>> bail out when asked to reverse-apply a binary patch?
>
> Nope. I swap old_sha1_prefix and new_sha1_prefix in that case, I hoped 
> that is enough?

You would need something like this, at least for now, since both deflated literal and deflated delta methods are irreversible. I'll cook up another binary diff output that can go bidirectional.

Note that --allow-binary-replacement uses the blob object name recorded on index lines of binary patch, and uses it to cheat (iow, when it knows your version is the old version recorded on index line and your repository happens to have the resulting blob, it just uses the blob without looking at the binary contents recorded in the patch), so you have to work a bit hard to cause it to fail in t/trash repository after you run t4103 test. Resetting to "master", apply BF.diff, and immediately reverse apply BF.diff would _work_, only because "master" and "binary" branch keep both preimage and postimage.

-- >8 --
Show changes to builtin-apply.c +6 −0
diff --git a/builtin-apply.c b/builtin-apply.c
index 6b38a8a..d4381d9 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1535,6 +1535,12 @@ static int apply_binary_fragment(struct 
 	void *data;
 	void *result;
 
+	/* Binary patch is irreversible */
+	if (patch->is_reverse)
+		return error("cannot reverse-apply a binary patch to '%s'",
+			     patch->new_name
+			     ? patch->new_name : patch->old_name);
+
 	data = inflate_it(fragment->patch, fragment->size,
 			  patch->deflate_origlen);
 	if (!data)
Johannes Schindelin· Jul 28, 2006, 19:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] Teach git-apply about '-R'

Hi,
On Fri, 28 Jul 2006, Junio C Hamano wrote:
Show 12 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Fri, 28 Jul 2006, Junio C Hamano wrote:
> >> 
> >> A quick comment without looking much at the code.  Do you sanely
> >> bail out when asked to reverse-apply a binary patch?
> >
> > Nope. I swap old_sha1_prefix and new_sha1_prefix in that case, I hoped 
> > that is enough?
> 
> You would need something like this, at least for now, since both
> deflated literal and deflated delta methods are irreversible.

Somehow I had the impression that binary diff meant that you needed both objects in the object database. I was wrong.

Ciao, Dscho

← back to recent threads