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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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(-)

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

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

[Junio C Hamano]

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?

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;
 	}

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

[Martin Langhoff]

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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

Hi,

On Mon, 26 Jun 2006, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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(-)

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

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

[Martin Langhoff]

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. 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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

Hi,

On Tue, 27 Jun 2006, Martin Langhoff wrote:

> 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?)

> 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

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

[Junio C Hamano]

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?

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

[Johannes Schindelin]

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(-)

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)

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

[Martin Langhoff (CatalystIT)]

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 ;-)

>>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
-----------------------------------------------------------------------

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

[Johannes Schindelin]

Hi,

On Tue, 27 Jun 2006, Martin Langhoff (CatalystIT) wrote:

> > > 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)?

>   $ ~/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.

> 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?"

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

[Johannes Schindelin]

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:

> 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.

>   $ ~/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

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

[Martin Langhoff]

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.

> 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...

> >   $ ~/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

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

[Johannes Schindelin]

Hi,

On Tue, 27 Jun 2006, Martin Langhoff wrote:

> 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.

> > (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.

> > 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

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

[Martin Langhoff]

On 6/27/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > 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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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(-)

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