[PATCH] Fixes git-cherry algorithmic flaws

[PATCH] Fixes git-cherry algorithmic flaws

[Ilpo Järvinen]

Old algorithm:
        - printed IDs of identical patches with minus (-) sign; they
	  should not be printed at all
        - did not print anything from the changes in the upstream

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 git-cherry.sh |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/git-cherry.sh b/git-cherry.sh
index f0e8831..fdf3de7 100755
--- a/git-cherry.sh
+++ b/git-cherry.sh
@@ -74,7 +74,8 @@ do
 	then
 		if test -f "$patch/$2"
 		then
-			sign=-
+			rm -rf "$patch/$2"
+			continue
 		else
 			sign=+
 		fi
@@ -88,6 +89,29 @@ do
 		esac
 	fi
 done
+
+for c in $inup
+do
+	set x `git-diff-tree -p $c | git-patch-id`
+	if test "$2" != ""
+	then
+		if test -f "$patch/$2"
+		then
+			sign=-
+		else
+			continue
+		fi
+		case "$verbose" in
+		t)
+			c=$(git-rev-list --pretty=oneline --max-count=1 $c)
+		esac
+		case "$O" in
+		'')	O="$sign $c" ;;
+		*)	O="$sign $c$LF$O" ;;
+		esac
+	fi
+done
+
 case "$O" in
 '') ;;
 *)  echo "$O" ;;
-- 
1.4.1

Re: [PATCH] Fixes git-cherry algorithmic flaws

[Petr Baudis]

Dear diary, on Mon, Aug 07, 2006 at 12:30:13PM CEST, I got a letter
where Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> said that...
> Old algorithm:
>         - printed IDs of identical patches with minus (-) sign; they
> 	  should not be printed at all
>         - did not print anything from the changes in the upstream
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Ping? Is this patch bogus or was it just forgotten?

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Re: [PATCH] Fixes git-cherry algorithmic flaws

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> Dear diary, on Mon, Aug 07, 2006 at 12:30:13PM CEST, I got a letter
> where Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> said that...
>> Old algorithm:
>>         - printed IDs of identical patches with minus (-) sign; they
>> 	  should not be printed at all
>>         - did not print anything from the changes in the upstream
>> 
>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
>
> Ping? Is this patch bogus or was it just forgotten?

These are not fixes to "algorithmic flaws".  It's more like that
Ilpo is writing a different program to fill different needs, and
I did not see what workflow wanted to have the list of changes
that were in the upstream and our changes.  Maybe what Ilpo
wanted to see was something like "git log upstream...mine"
(three-dots not two to mean symmetric difference).  I dunno.
That operation certainly did not exist when we did git-cherry
originally.

The original purpose of git-cherry (which probably is different
from what Ilpo wanted to have, and that is why Ilpo modified it
into a different program) is for a developer in the contributor
role to see which ones of local patches have been accepted
upstream and which ones still remain unapplied -- the intent is
to help rebase only the latter and keep trying to convince
upstream that these remaining ones are also worth applying.

So minus (-) lines are very much needed to if you want to see
which ones have been accepted.  Plus lines are used to pick
which ones to rebase by older version of git-rebase, but I do
not think we do that anymore.  And in any case we are _not_
interested in whatever happened in the upstream that did not
come from the branch we are looking at.

I suspect we do not use it anywhere anymore.  Maybe we can
remove it?

	... goes and looks ...
	git grep -e git.cherry --and --not -e git.cherry-pick

Nah, no such luck.  One of the documentation suggests that you
drive cvsexportcommit using its output, like this:

	git cherry cvs mine | sed -n -e 's/^\+ //p' |
        xargs -L 1 git-cvsexportcommit -c -p -v

and I can see why cherry is (perhaps slightly) more desirable
than "git rev-list cvs..mine"

So unless we come up with an alternative way to do this, we
cannot change it or drop it.  Not yet.

Re: [PATCH] Fixes git-cherry algorithmic flaws

[Petr Baudis]

Dear diary, on Sun, Sep 24, 2006 at 03:49:22AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> said that...
> Petr Baudis <pasky@suse.cz> writes:
> 
> > Dear diary, on Mon, Aug 07, 2006 at 12:30:13PM CEST, I got a letter
> > where Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> said that...
> >> Old algorithm:
> >>         - printed IDs of identical patches with minus (-) sign; they
> >> 	  should not be printed at all
> >>         - did not print anything from the changes in the upstream
> >> 
> >> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> >
> > Ping? Is this patch bogus or was it just forgotten?
> 
> These are not fixes to "algorithmic flaws".  It's more like that
> Ilpo is writing a different program to fill different needs, and
> I did not see what workflow wanted to have the list of changes
> that were in the upstream and our changes.  Maybe what Ilpo
> wanted to see was something like "git log upstream...mine"
> (three-dots not two to mean symmetric difference).  I dunno.
> That operation certainly did not exist when we did git-cherry
> originally.
> 
> The original purpose of git-cherry (which probably is different
> from what Ilpo wanted to have, and that is why Ilpo modified it
> into a different program) is for a developer in the contributor
> role to see which ones of local patches have been accepted
> upstream and which ones still remain unapplied -- the intent is
> to help rebase only the latter and keep trying to convince
> upstream that these remaining ones are also worth applying.
> 
> So minus (-) lines are very much needed to if you want to see
> which ones have been accepted.  Plus lines are used to pick
> which ones to rebase by older version of git-rebase, but I do
> not think we do that anymore.  And in any case we are _not_
> interested in whatever happened in the upstream that did not
> come from the branch we are looking at.

Hmm, well, what's curious is that the documentation says

	Every commit with a changeset that doesn't exist in the other branch
	has its id (sha1) reported, prefixed by a symbol.  Those existing only
	in the <upstream> branch are prefixed with a minus (-) sign, and those
	that only exist in the <head> branch are prefixed with a plus (+)
	symbol.

which is in contradiction of Ilpo's description of the old algorithm
(and also your description of it). It would seem he just wants to fix it
according to the documented behaviour.

I guess the documentation is what's broken then?

-- 
				Petr "Pasky the Let's See How Long I Can
					Manage Arguing Without Actually
					Looking at the Code" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Re: [PATCH] Fixes git-cherry algorithmic flaws

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> Hmm, well, what's curious is that the documentation says
>
> 	Every commit with a changeset that doesn't exist in the other branch
> 	has its id (sha1) reported, prefixed by a symbol.  Those existing only
> 	in the <upstream> branch are prefixed with a minus (-) sign, and those
> 	that only exist in the <head> branch are prefixed with a plus (+)
> 	symbol.
>
> which is in contradiction of Ilpo's description of the old algorithm
> (and also your description of it). It would seem he just wants to fix it
> according to the documented behaviour.
>
> I guess the documentation is what's broken then?

Ah I did not realize that, but yes the documentation is
incorrect.

I wonder if we can kill it by introducing a new rev notation and
using regular rev-list family of commands instead.

What we want here is a way to say "give me commits that are in B
but not in A, but before returning a commit see if there is an
equivalent change in the set of commits that are in A but not in
B, and filter it out".

Time for "rev-list A....B"? ;-)

Re: [PATCH] Fixes git-cherry algorithmic flaws

[Ilpo Järvinen]

On Sun, 24 Sep 2006, Junio C Hamano wrote:

> Petr Baudis <pasky@suse.cz> writes:
> 
> > Hmm, well, what's curious is that the documentation says
> >
> > 	Every commit with a changeset that doesn't exist in the other branch
> > 	has its id (sha1) reported, prefixed by a symbol.  Those existing only
> > 	in the <upstream> branch are prefixed with a minus (-) sign, and those
> > 	that only exist in the <head> branch are prefixed with a plus (+)
> > 	symbol.
> >
> > which is in contradiction of Ilpo's description of the old algorithm
> > (and also your description of it). It would seem he just wants to fix it
> > according to the documented behaviour.
> >
> > I guess the documentation is what's broken then?
> 
> Ah I did not realize that, but yes the documentation is
> incorrect.

I was going to do a same conclusion but didn't send it just yet... :-) I 
found out what the documentation says when looking a tool to do a job. 
Then I wonder how such obvious bug could have passed unnoticed... Of 
course I have no clue what the "original purpose" is supposed to be... 
;-) Then I "fixed" it and as it is _so easy_ to send patches with git I 
thought I could contribute the "fix"... I was a bit turned down though 
from not receiving any reply or so, well, until now... :-) Though I 
remember now that I was wandering whether the tool was correct and that 
documentation is not... But since I thought that when I'm cherry-picking 
(and, e.g., cleaning up log messages) between topic-old and topic-cleaner, 
the patch id based _difference_ seems to be the most useful one... 

> I wonder if we can kill it by introducing a new rev notation and
> using regular rev-list family of commands instead.
> 
> What we want here is a way to say "give me commits that are in B
> but not in A, but before returning a commit see if there is an
> equivalent change in the set of commits that are in A but not in
> B, and filter it out".

I think that your formalization is very close to what I was expecting to 
get (sort of one-way definition)... However, my git-cherry way produces 
"difference" but on a higher level (than git-diff) since it includes both 
+ and - "changes". Of course, when I have then modified one of the 
changesets slightly, I have different patch id, and thus + and - with same 
log message (with verbose), which IMHO is a good thing to notice, 
especially if I return to the work after 2 weeks or so :-).  

A real life example: In a branch, I have changed tcp_packets_in_flight 
(~10 callers) to input sk instead of tp in a single changeset and >10 
minor changesets. I would love to see tcp_packets_in_fligth change 
information just once when doing diffing topic-old topic-new during cherry 
picking, instead of a lengthy diff full of search-and-replace "noise", 
which increases possiblity of an human error...

But anyway, I'm not claiming that your approach is less useful...


-- 
 i.

Re: [PATCH] Fixes git-cherry algorithmic flaws

[Jakub Narebski]

Ilpo Järvinen wrote:

> On Sun, 24 Sep 2006, Junio C Hamano wrote:

>> I wonder if we can kill it by introducing a new rev notation and
>> using regular rev-list family of commands instead.
>> 
>> What we want here is a way to say "give me commits that are in B
>> but not in A, but before returning a commit see if there is an
>> equivalent change in the set of commits that are in A but not in
>> B, and filter it out".
> 
> I think that your formalization is very close to what I was expecting to 
> get (sort of one-way definition)... However, my git-cherry way produces 
> "difference" but on a higher level (than git-diff) since it includes both 
> + and - "changes". Of course, when I have then modified one of the 
> changesets slightly, I have different patch id, and thus + and - with same 
> log message (with verbose), which IMHO is a good thing to notice, 
> especially if I return to the work after 2 weeks or so :-).  
> 
> A real life example: In a branch, I have changed tcp_packets_in_flight 
> (~10 callers) to input sk instead of tp in a single changeset and >10 
> minor changesets. I would love to see tcp_packets_in_fligth change 
> information just once when doing diffing topic-old topic-new during cherry 
> picking, instead of a lengthy diff full of search-and-replace "noise", 
> which increases possiblity of an human error...
> 
> But anyway, I'm not claiming that your approach is less useful...

+1 on an idea that git-cherry is "diff of logs". Perhaps to add some header.
Of course use patch_ids _and_ the commit message to compare (we can have
both match, patch id match only, commit message match only, and commit
title (first line) match only), but that can be selected using options. 
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git