threads / patch / 4659

patch, 2 partsresurrect format-patch's patch id checking

Subject: [PATCH 0/2] resurrect format-patch's patch id checking

## tl;dr

20 messages between Jun 25, 2006 and Jun 27, 2006. Diffs are folded; open one to read it.

replies: 19people: 4as markdown or json

Johannes Schindelin· Jun 25, 2006, 01:50 UTC · lore
With these patches, you can say
	git format-patch --ignore-if-in-upstream upstream
to get a series of only the patches which are in HEAD, but not upstream.

The first patch adds diff_flush_patch_id() to calculate the patch id, after a diff queue was set up. (Earlier, I sent out a version which also adds a command line option to the diff family, but I'll postpone that until Timo's patches went in).

The second patch adds the actual patch id checking. There are two tricky things involved:

	- I add a pseudo-object for each patch of the upstream, which has
	  the patch id as sha1. This is no real object, but since we rely
	  on the hashes being unique for all practical purposes, and since
	  it has parsed == 0, it should be no problem.
	- To add the patch ids of the upstream, the revision walker must
	  be called twice.
	  So, if format-patch was called with a range "a..b" (a single
	  revision "a" is handled as "a..HEAD" by format-patch), a revision
	  walker is set up for "b..a", and the patch ids are calculated and
	  stored. This is done by toggling the UNINTERESTING bits of both
	  pending objects.
	  After that, the flags of all objects are reset to 0, so that the
	  revisions can be walked again. The flags of the two pending objects
	  are then reset to their original state.
Note that "--numbered" still works.
WARNING: since it is quite late in this part of the world, _and_ I am known
to produce not-always-optimal patches which could eat your children, please
give this a good beating.

Ciao, Dscho

Johannes Schindelin· Jun 26, 2006, 15:33 UTC · re: Johannes Schindelin · lore

[PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

It is cleaner, and it describes better what is the idea behind the code.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
	This goes on top of the --ignore-if-in-upstream patch.
	On Sun, 25 Jun 2006, Johannes Schindelin wrote:
	> - To add the patch ids of the upstream, the revision walker must
	> be called twice.
	> 
	> So, if format-patch was called with a range "a..b" (a single
	> revision "a" is handled as "a..HEAD" by format-patch), a 
	> revision walker is set up for "b..a", and the patch ids are 
	> calculated and stored. This is done by toggling the 
	> UNINTERESTING bits of both pending objects.
	> 
	> After that, the flags of all objects are reset to 0, so that the
	> revisions can be walked again. The flags of the two pending 
	> objects are then reset to their original state.
	It is not clean to reset the flags of all objects to 0. Instead, 
	the commits are walked directly. Not that it matters in that
	particular case (the only read objects _are_ these commits).
 builtin-log.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)
Show changes to builtin-log.c +2 −10
diff --git a/builtin-log.c b/builtin-log.c
index e78a9a4..e2cd975 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -160,15 +160,6 @@ static void reopen_stdout(struct commit 
 	freopen(filename, "w", stdout);
 }
 
-static void reset_all_objects_flags()
-{
-	int i;
-
-	for (i = 0; i < obj_allocs; i++)
-		if (objs[i])
-			objs[i]->flags = 0;
-}
-
 static int get_patch_id(struct commit *commit, struct diff_options *options,
 		unsigned char *sha1)
 {
@@ -220,7 +211,8 @@ static void get_patch_ids(struct rev_inf
 	}
 
 	/* reset for next revision walk */
-	reset_all_objects_flags();
+	clear_commit_marks((struct commit *)o1, SEEN | UNINTERESTING);
+	clear_commit_marks((struct commit *)o2, SEEN | UNINTERESTING);
 	o1->flags = flags1;
 	o2->flags = flags2;
 }

-- 
1.4.1.rc1.gc792
Junio C Hamano· Jun 26, 2006, 16:09 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 	It is not clean to reset the flags of all objects to 0. Instead, 
> 	the commits are walked directly. Not that it matters in that
> 	particular case (the only read objects _are_ these commits).

I think this makes sense, but the clear-commit-marks function itself looks fishy. I suspect a parent that has not been parsed could be already marked in which case the current code would leave it marked. Don't we need this perhaps?

Show changes to commit.c +1 −2
diff --git a/commit.c b/commit.c
index 946615d..69fbc41 100644
--- a/commit.c
+++ b/commit.c
@@ -397,8 +397,7 @@ void clear_commit_marks(struct commit *c
 	commit->object.flags &= ~mark;
 	while (parents) {
 		struct commit *parent = parents->item;
-		if (parent && parent->object.parsed &&
-		    (parent->object.flags & mark))
+		if (parent && (parent->object.flags & mark))
 			clear_commit_marks(parent, mark);
 		parents = parents->next;
 	}
Johannes Schindelin· Jun 26, 2006, 22:44 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Hi,
On Mon, 26 Jun 2006, Junio C Hamano wrote:
Show 11 quoted lines
> diff --git a/commit.c b/commit.c
> index 946615d..69fbc41 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -397,8 +397,7 @@ void clear_commit_marks(struct commit *c
>  	commit->object.flags &= ~mark;
>  	while (parents) {
>  		struct commit *parent = parents->item;
> -		if (parent && parent->object.parsed &&
> -		    (parent->object.flags & mark))
> +		if (parent && (parent->object.flags & mark))

This is probably not necessary for existing users, but it's a good change for the future: new users might be surprised to learn that there are unparsed objects, which still want to be handled.

Ciao, Dscho

Martin Langhoff· Jun 26, 2006, 22:20 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

git-format-patch is rather broken on pu right now (pre these patches). In my test git repo, git-format-patch.sh called with 2 parameters, like

  git-format-patch <remotehead> <myhead>

when HEAD != myhead does the right thing, but git format-patch doesn't, it seems to be messing up and not finding the merge base correctly:

  0001-Initial-revision-of-git-the-information-manager-from-hell.txt
And it errors out with ignore-if-in-upstream:
  $ ./git format-patch --ignore-if-in-upstream -o .patches origin master
  fatal: Not a range.
I'll retest later with these patches and post again.
martin
Johannes Schindelin· Jun 26, 2006, 22:39 UTC · re: Martin Langhoff · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Hi,
On Tue, 27 Jun 2006, Martin Langhoff wrote:
> And it errors out with ignore-if-in-upstream:
> 
>  $ ./git format-patch --ignore-if-in-upstream -o .patches origin master
>  fatal: Not a range.
Could you test with "origin..master" instead of "origin master"?

Thanks, Dscho

Martin Langhoff· Jun 26, 2006, 22:50 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

On 6/27/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 10 quoted lines
> Hi,
>
> On Tue, 27 Jun 2006, Martin Langhoff wrote:
>
> > And it errors out with ignore-if-in-upstream:
> >
> >  $ ./git format-patch --ignore-if-in-upstream -o .patches origin master
> >  fatal: Not a range.
>
> Could you test with "origin..master" instead of "origin master"?

Funny you mention that! Now it works ;-) and it even produces the patches I would expect. There is something strange though. I have a repo with ~150 pending patches to push, of which git-cherry spots ~100 as already merged upstream. So the old git-format-patch.sh would spit 50 patches, and the initial C version would do 150.

Now this version gives me 50 patches, regardless of --ignore-if-in-upstream. Is that expected?

Also, if the two heads are identical, it still says 'Fatal: Not a range", but that isn't so important.

cheers,
martin
Johannes Schindelin· Jun 26, 2006, 23:00 UTC · re: Martin Langhoff · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Hi,
On Tue, 27 Jun 2006, Martin Langhoff wrote:
Show 14 quoted lines
> On 6/27/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> > 
> > On Tue, 27 Jun 2006, Martin Langhoff wrote:
> > 
> > > And it errors out with ignore-if-in-upstream:
> > >
> > >  $ ./git format-patch --ignore-if-in-upstream -o .patches origin master
> > >  fatal: Not a range.
> > 
> > Could you test with "origin..master" instead of "origin master"?
> 
> Funny you mention that! Now it works ;-) and it even produces the
> patches I would expect.

The funny thing is: I did something to account for the old syntax, but only if you specified _one_ ref, not _two_. It would be easy, but is it needed? (I.e. are your fingers so trained on it?)

Show 7 quoted lines
> There is something strange though. I have a repo with ~150 pending 
> patches to push, of which git-cherry spots ~100 as already merged 
> upstream. So the old git-format-patch.sh would spit 50 patches, and the 
> initial C version would do 150.
> 
> Now this version gives me 50 patches, regardless of
> --ignore-if-in-upstream. Is that expected?
Hell, no! Something is really wrong there.
What does "git-rev-list their..my | wc" say?
> Also, if the two heads are identical, it still says 'Fatal: Not a
> range", but that isn't so important.

This is a consequence of my being too lazy to support the old "theirs mine" syntax (see above).

Ciao, Dscho

Junio C Hamano· Jun 26, 2006, 23:04 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
>> > Could you test with "origin..master" instead of "origin master"?
>> 
>> Funny you mention that! Now it works ;-) and it even produces the
>> patches I would expect.
>
> The funny thing is: I did something to account for the old syntax, but 
> only if you specified _one_ ref, not _two_. It would be easy, but is it 
> needed? (I.e. are your fingers so trained on it?)

If possible I'd rather correct the two syntaxes once and for all now. Maybe accept two with a warning for deprecation?

Johannes Schindelin· Jun 26, 2006, 23:14 UTC · re: Junio C Hamano · lore

[PATCH] format-patch: support really old non-range syntax, with a warning

Now you can say (again)
	git format-patch <theirs> <mine>
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
	Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some 
	adhocery
	On Mon, 26 Jun 2006, Junio C Hamano wrote:
	> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
	> 
	> >> > Could you test with "origin..master" instead of "origin master"?
	> >> 
	> >> Funny you mention that! Now it works ;-) and it even produces the
	> >> patches I would expect.
	> >
	> > The funny thing is: I did something to account for the old syntax, but 
	> > only if you specified _one_ ref, not _two_. It would be easy, but is it 
	> > needed? (I.e. are your fingers so trained on it?)
	> 
	> If possible I'd rather correct the two syntaxes once and for all now.
	> Maybe accept two with a warning for deprecation?
	Here you are. (Tested once -- works great!)
 builtin-log.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Show changes to builtin-log.c +5 −0
diff --git a/builtin-log.c b/builtin-log.c
index 44d2d13..64b2830 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -260,6 +260,11 @@ int cmd_format_patch(int argc, const cha
 	if (rev.pending.nr == 1) {
 		rev.pending.objects[0].item->flags |= UNINTERESTING;
 		add_head(&rev);
+	} else if (rev.pending.nr == 2
+			&& !(rev.pending.objects[0].item->flags & UNINTERESTING)
+			&& !(rev.pending.objects[1].item->flags & UNINTERESTING)) {
+		rev.pending.objects[0].item->flags |= UNINTERESTING;
+		fprintf(stderr, "WARNING: obsolete syntax (no range)!\n");
 	}
 
 	if (!use_stdout)
Martin Langhoff (CatalystIT)· Jun 26, 2006, 23:15 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Johannes Schindelin wrote:
> The funny thing is: I did something to account for the old syntax, but 
> only if you specified _one_ ref, not _two_. It would be easy, but is it 
> needed? (I.e. are your fingers so trained on it?)
My fingers are retrainable ;-)
Show 12 quoted lines
>>There is something strange though. I have a repo with ~150 pending 
>>patches to push, of which git-cherry spots ~100 as already merged 
>>upstream. So the old git-format-patch.sh would spit 50 patches, and the 
>>initial C version would do 150.
>>
>>Now this version gives me 50 patches, regardless of
>>--ignore-if-in-upstream. Is that expected?
> 
> 
> Hell, no! Something is really wrong there.
> 
> What does "git-rev-list their..my | wc" say?

Ok, I cooked the numbers up a bit, it was 60 total, with 10 merged upstream. Here's what I have today:

   $ git-cherry svnhead..master | grep -c '+'
   52
   $ git-rev-list svnhead..master  | wc -l
   61
   $ ~/local/git/git-format-patch.sh  -o .patchesold svnhead master
   ...
   $ ls .patchesold | wc -l
   52
   $ ~/local/git/git format-patch  -o .patchesnewall svnhead..master
   ...
   $ ls .patchesnewall/ | wc -l
   53
   $ ~/local/git/git format-patch --ignore-if-in-upstream -o 
.patchesnewignore svnhead..master
   ...
   $ ls .patchesnewignore | wc -l
   52
This is a public repo --

master tracks origin which is http://git.catalyst.net.nz/git/elgg-r2.git#nzvleportfolio

svnhead is http://git.catalyst.net.nz/git/elgg-r2.git#svnhead

cheers,
m
-- 
-----------------------------------------------------------------------
Martin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington
WEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St
OFFICE: +64(4)916-7224                              MOB: +64(21)364-017
       Make things as simple as possible, but no simpler - Einstein
-----------------------------------------------------------------------
Johannes Schindelin· Jun 26, 2006, 23:21 UTC · re: Martin Langhoff (CatalystIT) · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Hi,
On Tue, 27 Jun 2006, Martin Langhoff (CatalystIT) wrote:
Show 30 quoted lines
> > > There is something strange though. I have a repo with ~150 pending patches
> > > to push, of which git-cherry spots ~100 as already merged upstream. So the
> > > old git-format-patch.sh would spit 50 patches, and the initial C version
> > > would do 150.
> > > 
> > > Now this version gives me 50 patches, regardless of
> > > --ignore-if-in-upstream. Is that expected?
> > 
> > 
> > Hell, no! Something is really wrong there.
> > 
> > What does "git-rev-list their..my | wc" say?
> 
> Ok, I cooked the numbers up a bit, it was 60 total, with 10 merged upstream.
> Here's what I have today:
> 
>   $ git-cherry svnhead..master | grep -c '+'
>   52
>   $ git-rev-list svnhead..master  | wc -l
>   61
> 
>   $ ~/local/git/git-format-patch.sh  -o .patchesold svnhead master
>   ...
>   $ ls .patchesold | wc -l
>   52
> 
>   $ ~/local/git/git format-patch  -o .patchesnewall svnhead..master
>   ...
>   $ ls .patchesnewall/ | wc -l
>   53

That is very, very strange. One more! Do you have any idea which patch it is (53 being one more than 52 I suspect there is one slipping through)?

Show 5 quoted lines
>   $ ~/local/git/git format-patch --ignore-if-in-upstream -o .patchesnewignore
> svnhead..master
>   ...
>   $ ls .patchesnewignore | wc -l
>   52

Could it be that the other patches are merges? format-patch _must_ ignore these.

Show 7 quoted lines
> This is a public repo --
> 
> master tracks origin which is
> http://git.catalyst.net.nz/git/elgg-r2.git#nzvleportfolio
> 
> svnhead is
> http://git.catalyst.net.nz/git/elgg-r2.git#svnhead

Even if I do not use cogito, I will fetch them. However, that must wait for tomorrow, as I can no longer keep awake.

Ciao, Dscho "where do you put the middle name if your nick is only one word?"

Johannes Schindelin· Jun 27, 2006, 08:31 UTC · re: Martin Langhoff (CatalystIT) · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Hi,

I just cloned your repo, and as far as I can tell, the latest commit is on June 23rd. So your numbers should be the same as mine. But not all are.

On Tue, 27 Jun 2006, Martin Langhoff (CatalystIT) wrote:
Show 5 quoted lines
> Ok, I cooked the numbers up a bit, it was 60 total, with 10 merged upstream.
> Here's what I have today:
> 
>   $ git-cherry svnhead..master | grep -c '+'
>   52
Here, it is 51!

The problem I see: from the 53 non-merges in nzvleportfolio (who makes up your branch names anyway?), there are two already in upstream: e3f56c and 7e448c5c. So it really should be 51.

>   $ git-rev-list svnhead..master  | wc -l
>   61
Same on my repo.
>   $ ~/local/git/git-format-patch.sh  -o .patchesold svnhead master
>   ...
>   $ ls .patchesold | wc -l
>   52

I guess this propagates from git-cherry. (Did not test here, since I do not have an old git-format-patch.sh handy, and am too lazy to get the last version from my git repo.)

>   $ ~/local/git/git format-patch  -o .patchesnewall svnhead..master
>   ...
>   $ ls .patchesnewall/ | wc -l
>   53

Same on my repo. Correct, since "git rev-list --parents svnhead..master | grep -c \ .*\ " yields 8, which is exactly 61 - 53.

Show 5 quoted lines
>   $ ~/local/git/git format-patch --ignore-if-in-upstream -o .patchesnewignore
> svnhead..master
>   ...
>   $ ls .patchesnewignore | wc -l
>   52
Again, I get 51.

I do not know what is happening, but it could be that there was a bug fixed lately. I run a slightly modified version of 'next', updated about 15 minutes ago.

But anyway, looking at your numbers I take it that the new format-patch with --ignore-if-in-upstream has the same output as the old format-patch, right?

Ciao, Dscho

Martin Langhoff· Jun 27, 2006, 09:52 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

On 6/27/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> I just cloned your repo, and as far as I can tell, the latest commit is on
> June 23rd. So your numbers should be the same as mine. But not all are.

Taht repo is on a machine at work, but it's possible that I've cherry picked a new commit onto master that isn't on the publc repo yet.

Show 8 quoted lines
> The problem I see: from the 53 non-merges in nzvleportfolio (who makes up
> your branch names anyway?), there are two already in upstream: e3f56c and
> 7e448c5c. So it really should be 51.
>
> >   $ git-rev-list svnhead..master  | wc -l
> >   61
>
> Same on my repo.
And if you add --no-merges it'll give you 51 or 52, depending...
Show 8 quoted lines
> >   $ ~/local/git/git-format-patch.sh  -o .patchesold svnhead master
> >   ...
> >   $ ls .patchesold | wc -l
> >   52
>
> I guess this propagates from git-cherry. (Did not test here, since I do
> not have an old git-format-patch.sh handy, and am too lazy to get the last
> version from my git repo.)
I think I did something like
  git-cat-file blob v1.3.3:git-format-patch.sh > git-format-patch.sh
  chmod ugo+x git-format-patch.sh
> But anyway, looking at your numbers I take it that the new format-patch
> with --ignore-if-in-upstream has the same output as the old format-patch,
> right?

It does, but it may be "right" even though it's not realising that some of the patches were cherry picked. git-cherry doesn't either. So that algorythm isn't so hot in this case :-/

I jumped to conclusions earlier because I called git format-patch with the old syntax (theirs ours) and it gave me 186 patches, which may very well be the whole repo history, like it did earlier with git itself. I thought that 186 were the patches pending, and that git-cherry was cutting that to 52. Not so: it was a syntax issue. Sorry about the noise!

cheers,
martin
Johannes Schindelin· Jun 27, 2006, 10:54 UTC · re: Martin Langhoff · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Hi,
On Tue, 27 Jun 2006, Martin Langhoff wrote:
Show 7 quoted lines
> On 6/27/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > 
> > I just cloned your repo, and as far as I can tell, the latest commit is on
> > June 23rd. So your numbers should be the same as mine. But not all are.
> 
> Taht repo is on a machine at work, but it's possible that I've cherry
> picked a new commit onto master that isn't on the publc repo yet.
I hoped that much.
Show 7 quoted lines
> > (Did not test here, since I do not have an old git-format-patch.sh 
> > handy, and am too lazy to get the last version from my git repo.)
> 
> I think I did something like
> 
>  git-cat-file blob v1.3.3:git-format-patch.sh > git-format-patch.sh
>  chmod ugo+x git-format-patch.sh
Yes, I am lazy.
Show 7 quoted lines
> > But anyway, looking at your numbers I take it that the new format-patch
> > with --ignore-if-in-upstream has the same output as the old format-patch,
> > right?
> 
> It does, but it may be "right" even though it's not realising that
> some of the patches were cherry picked. git-cherry doesn't either. So
> that algorythm isn't so hot in this case :-/

What do you mean? Is the patch-id of the cherry-picked different? (If there was a conflict which was manually resolved, I think there is no way we can detect that that patch was cherry-picked, but if it applied cleanly, the patch-id should be equal both in upstream and downstream.)

Ciao, Dscho

Martin Langhoff· Jun 27, 2006, 11:09 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

On 6/27/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 8 quoted lines
> > It does, but it may be "right" even though it's not realising that
> > some of the patches were cherry picked. git-cherry doesn't either. So
> > that algorythm isn't so hot in this case :-/
>
> What do you mean? Is the patch-id of the cherry-picked different? (If
> there was a conflict which was manually resolved, I think there is no way
> we can detect that that patch was cherry-picked, but if it applied
> cleanly, the patch-id should be equal both in upstream and downstream.)

I'll look more into it tomorrow at work. Knowing the codebase, some of the patches should be rather obvious, GNU patch recognises them as 'already applied', but they may be slightly different in ways that break the hashing. Maybe. I'll play with git-cherry to see, I assume that you've copied the logic from there.

cheers,
martin
Johannes Schindelin· Jun 27, 2006, 12:39 UTC · re: Martin Langhoff · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Hi,
On Tue, 27 Jun 2006, Martin Langhoff wrote:
> the patches should be rather obvious, GNU patch recognises them as
> 'already applied', but they may be slightly different in ways that
> break the hashing.

Did you try with git-apply? This is much more anal than GNU patch, and should give you an idea what is happening.

> I'll play with git-cherry to see, I assume that you've copied the logic 
> from there.
Yup. Well, from git-patch-id, but that was used by git-cherry.

Note that I made sure that the patch-id is the same as for git-patch-id. It only differs with renaming patches (the "---" and "+++" lines are hashed, to make sure it is the same patch, but the "rename from" and "rename to" lines are not generated by diff_flush_patch_id()).

Ciao, Dscho

Junio C Hamano· Jun 26, 2006, 22:48 UTC · re: Martin Langhoff · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

I'll be pushing out a new test for format-patch shortly in "next".

>From ece3c67f9c8f0074cae76204a648cbfc6075bb44 Mon Sep 17 00:00:00 2001
From: Junio C Hamano <junkio@cox.net>
Date: Mon, 26 Jun 2006 15:40:09 -0700
Subject: [PATCH] t4014: add format-patch --ignore-if-in-upstream test
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 t/t4014-format-patch.sh |   69 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)
Show changes to t/t4014-format-patch.sh +69 −0
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
new file mode 100755
index 0000000..c044044
--- /dev/null
+++ b/t/t4014-format-patch.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 Junio C Hamano
+#
+
+test_description='Format-patch skipping already incorporated patches'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	for i in 1 2 3 4 5 6 7 8 9 10; do echo "$i"; done >file &&
+	git add file &&
+	git commit -m Initial &&
+	git checkout -b side &&
+
+	for i in 1 2 5 6 A B C 7 8 9 10; do echo "$i"; done >file &&
+	git update-index file &&
+	git commit -m "Side change #1" &&
+
+	for i in D E F; do echo "$i"; done >>file &&
+	git update-index file &&
+	git commit -m "Side change #2" &&
+	git tag C1 &&
+
+	for i in 5 6 1 2 3 A 4 B C 7 8 9 10 D E F; do echo "$i"; done >file &&
+	git update-index file &&
+	git commit -m "Side change #3" &&
+
+	git checkout master &&
+	git diff-tree -p C1 | git apply --index &&
+	git commit -m "Master accepts moral equivalent of #1"
+
+'
+
+test_expect_success "format-patch --ignore-if-in-upstream" '
+
+	git format-patch --stdout master..side >patch0 &&
+	cnt=`grep "^From " patch0 | wc -l` &&
+	test "$cnt" = 3
+
+'
+
+test_expect_success "format-patch --ignore-if-in-upstream" '
+
+	git format-patch --stdout \
+		--ignore-if-in-upstream master..side >patch1 &&
+	cnt=`grep "^From " patch1 | wc -l` &&
+	test "$cnt" = 2
+
+'
+
+test_expect_success "format-patch result applies" '
+
+	git checkout -b rebuild-0 master &&
+	git am -3 patch0 &&
+	cnt=`git rev-list master.. | wc -l` &&
+	test "$cnt" = 2
+'
+
+test_expect_success "format-patch --ignore-if-in-upstream result applies" '
+
+	git checkout -b rebuild-1 master &&
+	git am -3 patch1 &&
+	cnt=`git rev-list master.. | wc -l` &&
+	test "$cnt" = 2
+'
+
+test_done

-- 
1.4.1.rc1.g96b82
Johannes Schindelin· Jun 26, 2006, 22:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/2] format-patch: use clear_commit_marks() instead of some adhocery

Hi,
On Mon, 26 Jun 2006, Junio C Hamano wrote:
> I'll be pushing out a new test for format-patch shortly in
> "next".

Thanks. I had a little test, but it was not nearly as complete as I liked it...

Ciao, Dscho

Johannes Schindelin· Jun 27, 2006, 20:38 UTC · re: Junio C Hamano · lore

[PATCH] format-patch: use clear_commit_marks() instead of some adhocery

It is cleaner, and it describes better what is the idea behind the code.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
	On Mon, 26 Jun 2006, Junio C Hamano wrote:
	> I'll be pushing out a new test for format-patch shortly in
	> "next".
	... and this test fails with my original patch: We also need to 
	reset ADDED, and I threw in SHOWN for good measure.
	Maybe there is room for improvement of the revision walker here;
	It smells a little like ADDED is not only used to avoid duplicate
	parsing (which I guess is now happening, even with
	reset_all_objects_flags() instead of clear_commit_marks()), but
	also to decide if the revision walker should walk on.
	We are getting more and more users of the revision walker, and this
	is just the first to call the walker more than once.
 builtin-log.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)
Show changes to builtin-log.c +4 −10
diff --git a/builtin-log.c b/builtin-log.c
index 4ee5891..f9515a8 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -160,15 +160,6 @@ static void reopen_stdout(struct commit 
 	freopen(filename, "w", stdout);
 }
 
-static void reset_all_objects_flags()
-{
-	int i;
-
-	for (i = 0; i < obj_allocs; i++)
-		if (objs[i])
-			objs[i]->flags = 0;
-}
-
 static int get_patch_id(struct commit *commit, struct diff_options *options,
 		unsigned char *sha1)
 {
@@ -220,7 +211,10 @@ static void get_patch_ids(struct rev_inf
 	}
 
 	/* reset for next revision walk */
-	reset_all_objects_flags();
+	clear_commit_marks((struct commit *)o1,
+			SEEN | UNINTERESTING | SHOWN | ADDED);
+	clear_commit_marks((struct commit *)o2,
+			SEEN | UNINTERESTING | SHOWN | ADDED);
 	o1->flags = flags1;
 	o2->flags = flags2;
 }

-- 
1.4.1.rc1.g9de8f-dirty

← back to recent threads