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

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

It is cleaner, and it describes better what is the idea behind the code.

Signed-off-by: Johannes Schindelin <redacted>

---

	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

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

On 6/27/06, Johannes Schindelin [off-list ref] wrote:

Hi,

On Tue, 27 Jun 2006, Martin Langhoff wrote:

quoted

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

Hi,

On Tue, 27 Jun 2006, Martin Langhoff wrote:

On 6/27/06, Johannes Schindelin [off-list ref] wrote:

quoted

Hi,

On Tue, 27 Jun 2006, Martin Langhoff wrote:

quoted

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

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

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

quoted

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

Hi,

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

quoted

quoted

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

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

On 6/27/06, Johannes Schindelin [off-list ref] 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.

quoted

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

quoted

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

Hi,

On Tue, 27 Jun 2006, Martin Langhoff wrote:

On 6/27/06, Johannes Schindelin [off-list ref] wrote:

quoted

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.

quoted

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

quoted

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

On 6/27/06, Johannes Schindelin [off-list ref] wrote:

quoted

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

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