Re: [PATCH] Fixes git-cherry algorithmic flaws

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] Fixes git-cherry algorithmic flaws

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:41

Petr Baudis [off-list ref] writes:
Dear diary, on Mon, Aug 07, 2006 at 12:30:13PM CEST, I got a letter
where Ilpo Järvinen [off-list ref] said that...
quoted
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 <redacted>
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

From: Petr Baudis <hidden>
Date: 2016-06-15 22:42:41

Dear diary, on Sun, Sep 24, 2006 at 03:49:22AM CEST, I got a letter
where Junio C Hamano [off-list ref] said that...
Petr Baudis [off-list ref] writes:
quoted
Dear diary, on Mon, Aug 07, 2006 at 12:30:13PM CEST, I got a letter
where Ilpo Järvinen [off-list ref] said that...
quoted
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 <redacted>
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*',/((..)*)$/)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help