{"thread":{"id":"5162","subject":"[PATCH] Fixes git-cherry algorithmic flaws","startedAt":"2006-08-07T10:30:13Z","lastAt":"2006-09-24T19:25:17Z","messageCount":7,"participants":["Ilpo Järvinen","Petr Baudis","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"24917","messageId":"Pine.LNX.4.58.0608071328200.22971@kivilampi-30.cs.helsinki.fi","threadId":"5162","inReplyTo":null,"subject":"[PATCH] Fixes git-cherry algorithmic flaws","fromName":"Ilpo Järvinen","fromEmail":"ilpo.jarvinen@helsinki.fi","sentAt":"2006-08-07T10:30:13Z","receivedAt":"2006-08-07T10:30:13Z","isPatch":true,"sender":{"key":"ilpo.jarvinen@helsinki.fi","avatar":null},"body":"Old algorithm:\n - printed IDs of identical patches with minus (-) sign; they\n\t should not be printed at all\n - did not print anything from the changes in the upstream\n\nSigned-off-by: Ilpo Järvinen \n---\n git-cherry.sh | 26 +++++++++++++++++++++++++-\n 1 files changed, 25 insertions(+), 1 deletions(-)\n\ndiff --git a/git-cherry.sh b/git-cherry.sh\nindex f0e8831..fdf3de7 100755\n--- a/git-cherry.sh\n+++ b/git-cherry.sh\n@@ -74,7 +74,8 @@ do\n \tthen\n \t\tif test -f \"$patch/$2\"\n \t\tthen\n-\t\t\tsign=-\n+\t\t\trm -rf \"$patch/$2\"\n+\t\t\tcontinue\n \t\telse\n \t\t\tsign=+\n \t\tfi\n@@ -88,6 +89,29 @@ do\n \t\tesac\n \tfi\n done\n+\n+for c in $inup\n+do\n+\tset x `git-diff-tree -p $c | git-patch-id`\n+\tif test \"$2\" != \"\"\n+\tthen\n+\t\tif test -f \"$patch/$2\"\n+\t\tthen\n+\t\t\tsign=-\n+\t\telse\n+\t\t\tcontinue\n+\t\tfi\n+\t\tcase \"$verbose\" in\n+\t\tt)\n+\t\t\tc=$(git-rev-list --pretty=oneline --max-count=1 $c)\n+\t\tesac\n+\t\tcase \"$O\" in\n+\t\t'')\tO=\"$sign $c\" ;;\n+\t\t*)\tO=\"$sign $c$LF$O\" ;;\n+\t\tesac\n+\tfi\n+done\n+\n case \"$O\" in\n '') ;;\n *) echo \"$O\" ;;\n-- \n1.4.1\n"},{"id":"27525","messageId":"20060924000051.GI20017@pasky.or.cz","threadId":"5162","inReplyTo":"Pine.LNX.4.58.0608071328200.22971@kivilampi-30.cs.helsinki.fi","subject":"Re: [PATCH] Fixes git-cherry algorithmic flaws","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2006-09-24T00:00:51Z","receivedAt":"2006-09-24T00:00:51Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Mon, Aug 07, 2006 at 12:30:13PM CEST, I got a letter\nwhere Ilpo Järvinen said that...\n> Old algorithm:\n> - printed IDs of identical patches with minus (-) sign; they\n> \t should not be printed at all\n> - did not print anything from the changes in the upstream\n> \n> Signed-off-by: Ilpo Järvinen \n\nPing? Is this patch bogus or was it just forgotten?\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\n#!/bin/perl -sp0777i writes:\n\n> Dear diary, on Mon, Aug 07, 2006 at 12:30:13PM CEST, I got a letter\n> where Ilpo Järvinen said that...\n>> Old algorithm:\n>> - printed IDs of identical patches with minus (-) sign; they\n>> \t should not be printed at all\n>> - did not print anything from the changes in the upstream\n>> \n>> Signed-off-by: Ilpo Järvinen \n>\n> Ping? Is this patch bogus or was it just forgotten?\n\nThese are not fixes to \"algorithmic flaws\". It's more like that\nIlpo is writing a different program to fill different needs, and\nI did not see what workflow wanted to have the list of changes\nthat were in the upstream and our changes. Maybe what Ilpo\nwanted to see was something like \"git log upstream...mine\"\n(three-dots not two to mean symmetric difference). I dunno.\nThat operation certainly did not exist when we did git-cherry\noriginally.\n\nThe original purpose of git-cherry (which probably is different\nfrom what Ilpo wanted to have, and that is why Ilpo modified it\ninto a different program) is for a developer in the contributor\nrole to see which ones of local patches have been accepted\nupstream and which ones still remain unapplied -- the intent is\nto help rebase only the latter and keep trying to convince\nupstream that these remaining ones are also worth applying.\n\nSo minus (-) lines are very much needed to if you want to see\nwhich ones have been accepted. Plus lines are used to pick\nwhich ones to rebase by older version of git-rebase, but I do\nnot think we do that anymore. And in any case we are _not_\ninterested in whatever happened in the upstream that did not\ncome from the branch we are looking at.\n\nI suspect we do not use it anywhere anymore. Maybe we can\nremove it?\n\n\t... goes and looks ...\n\tgit grep -e git.cherry --and --not -e git.cherry-pick\n\nNah, no such luck. One of the documentation suggests that you\ndrive cvsexportcommit using its output, like this:\n\n\tgit cherry cvs mine | sed -n -e 's/^\\+ //p' |\n xargs -L 1 git-cvsexportcommit -c -p -v\n\nand I can see why cherry is (perhaps slightly) more desirable\nthan \"git rev-list cvs..mine\"\n\nSo unless we come up with an alternative way to do this, we\ncannot change it or drop it. Not yet.\n"},{"id":"27541","messageId":"20060924111737.GL20017@pasky.or.cz","threadId":"5162","inReplyTo":"7virjem3tp.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fixes git-cherry algorithmic flaws","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2006-09-24T11:17:38Z","receivedAt":"2006-09-24T11:17:38Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Sun, Sep 24, 2006 at 03:49:22AM CEST, I got a letter\nwhere Junio C Hamano said that...\n> Petr Baudis writes:\n> \n> > Dear diary, on Mon, Aug 07, 2006 at 12:30:13PM CEST, I got a letter\n> > where Ilpo Järvinen said that...\n> >> Old algorithm:\n> >> - printed IDs of identical patches with minus (-) sign; they\n> >> \t should not be printed at all\n> >> - did not print anything from the changes in the upstream\n> >> \n> >> Signed-off-by: Ilpo Järvinen \n> >\n> > Ping? Is this patch bogus or was it just forgotten?\n> \n> These are not fixes to \"algorithmic flaws\". It's more like that\n> Ilpo is writing a different program to fill different needs, and\n> I did not see what workflow wanted to have the list of changes\n> that were in the upstream and our changes. Maybe what Ilpo\n> wanted to see was something like \"git log upstream...mine\"\n> (three-dots not two to mean symmetric difference). I dunno.\n> That operation certainly did not exist when we did git-cherry\n> originally.\n> \n> The original purpose of git-cherry (which probably is different\n> from what Ilpo wanted to have, and that is why Ilpo modified it\n> into a different program) is for a developer in the contributor\n> role to see which ones of local patches have been accepted\n> upstream and which ones still remain unapplied -- the intent is\n> to help rebase only the latter and keep trying to convince\n> upstream that these remaining ones are also worth applying.\n> \n> So minus (-) lines are very much needed to if you want to see\n> which ones have been accepted. Plus lines are used to pick\n> which ones to rebase by older version of git-rebase, but I do\n> not think we do that anymore. And in any case we are _not_\n> interested in whatever happened in the upstream that did not\n> come from the branch we are looking at.\n\nHmm, well, what's curious is that the documentation says\n\n\tEvery commit with a changeset that doesn't exist in the other branch\n\thas its id (sha1) reported, prefixed by a symbol. Those existing only\n\tin the branch are prefixed with a minus (-) sign, and those\n\tthat only exist in the branch are prefixed with a plus (+)\n\tsymbol.\n\nwhich is in contradiction of Ilpo's description of the old algorithm\n(and also your description of it). It would seem he just wants to fix it\naccording to the documented behaviour.\n\nI guess the documentation is what's broken then?\n\n-- \n\t\t\t\tPetr \"Pasky the Let's See How Long I Can\n\t\t\t\t\tManage Arguing Without Actually\n\t\t\t\t\tLooking at the Code\" Baudis\nStuff: http://pasky.or.cz/\n#!/bin/perl -sp0777i writes:\n\n> Hmm, well, what's curious is that the documentation says\n>\n> \tEvery commit with a changeset that doesn't exist in the other branch\n> \thas its id (sha1) reported, prefixed by a symbol. Those existing only\n> \tin the branch are prefixed with a minus (-) sign, and those\n> \tthat only exist in the branch are prefixed with a plus (+)\n> \tsymbol.\n>\n> which is in contradiction of Ilpo's description of the old algorithm\n> (and also your description of it). It would seem he just wants to fix it\n> according to the documented behaviour.\n>\n> I guess the documentation is what's broken then?\n\nAh I did not realize that, but yes the documentation is\nincorrect.\n\nI wonder if we can kill it by introducing a new rev notation and\nusing regular rev-list family of commands instead.\n\nWhat we want here is a way to say \"give me commits that are in B\nbut not in A, but before returning a commit see if there is an\nequivalent change in the set of commits that are in A but not in\nB, and filter it out\".\n\nTime for \"rev-list A....B\"? ;-)\n"},{"id":"27573","messageId":"Pine.LNX.4.58.0609242104080.32175@kivilampi-30.cs.helsinki.fi","threadId":"5162","inReplyTo":"7vodt59mxa.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fixes git-cherry algorithmic flaws","fromName":"Ilpo Järvinen","fromEmail":"ilpo.jarvinen@helsinki.fi","sentAt":"2006-09-24T18:43:50Z","receivedAt":"2006-09-24T18:43:50Z","isPatch":true,"sender":{"key":"ilpo.jarvinen@helsinki.fi","avatar":null},"body":"On Sun, 24 Sep 2006, Junio C Hamano wrote:\n\n> Petr Baudis writes:\n> \n> > Hmm, well, what's curious is that the documentation says\n> >\n> > \tEvery commit with a changeset that doesn't exist in the other branch\n> > \thas its id (sha1) reported, prefixed by a symbol. Those existing only\n> > \tin the branch are prefixed with a minus (-) sign, and those\n> > \tthat only exist in the branch are prefixed with a plus (+)\n> > \tsymbol.\n> >\n> > which is in contradiction of Ilpo's description of the old algorithm\n> > (and also your description of it). It would seem he just wants to fix it\n> > according to the documented behaviour.\n> >\n> > I guess the documentation is what's broken then?\n> \n> Ah I did not realize that, but yes the documentation is\n> incorrect.\n\nI was going to do a same conclusion but didn't send it just yet... :-) I \nfound out what the documentation says when looking a tool to do a job. \nThen I wonder how such obvious bug could have passed unnoticed... Of \ncourse I have no clue what the \"original purpose\" is supposed to be... \n;-) Then I \"fixed\" it and as it is _so easy_ to send patches with git I \nthought I could contribute the \"fix\"... I was a bit turned down though \nfrom not receiving any reply or so, well, until now... :-) Though I \nremember now that I was wandering whether the tool was correct and that \ndocumentation is not... But since I thought that when I'm cherry-picking \n(and, e.g., cleaning up log messages) between topic-old and topic-cleaner, \nthe patch id based _difference_ seems to be the most useful one... \n\n> I wonder if we can kill it by introducing a new rev notation and\n> using regular rev-list family of commands instead.\n> \n> What we want here is a way to say \"give me commits that are in B\n> but not in A, but before returning a commit see if there is an\n> equivalent change in the set of commits that are in A but not in\n> B, and filter it out\".\n\nI think that your formalization is very close to what I was expecting to \nget (sort of one-way definition)... However, my git-cherry way produces \n\"difference\" but on a higher level (than git-diff) since it includes both \n+ and - \"changes\". Of course, when I have then modified one of the \nchangesets slightly, I have different patch id, and thus + and - with same \nlog message (with verbose), which IMHO is a good thing to notice, \nespecially if I return to the work after 2 weeks or so :-). \n\nA real life example: In a branch, I have changed tcp_packets_in_flight \n(~10 callers) to input sk instead of tp in a single changeset and >10 \nminor changesets. I would love to see tcp_packets_in_fligth change \ninformation just once when doing diffing topic-old topic-new during cherry \npicking, instead of a lengthy diff full of search-and-replace \"noise\", \nwhich increases possiblity of an human error...\n\nBut anyway, I'm not claiming that your approach is less useful...\n\n\n-- \n i.\n"},{"id":"27575","messageId":"ef6m2g$cnk$1@sea.gmane.org","threadId":"5162","inReplyTo":"Pine.LNX.4.58.0609242104080.32175@kivilampi-30.cs.helsinki.fi","subject":"Re: [PATCH] Fixes git-cherry algorithmic flaws","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-24T19:25:17Z","receivedAt":"2006-09-24T19:25:17Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Ilpo Järvinen wrote:\n\n> On Sun, 24 Sep 2006, Junio C Hamano wrote:\n\n>> I wonder if we can kill it by introducing a new rev notation and\n>> using regular rev-list family of commands instead.\n>> \n>> What we want here is a way to say \"give me commits that are in B\n>> but not in A, but before returning a commit see if there is an\n>> equivalent change in the set of commits that are in A but not in\n>> B, and filter it out\".\n> \n> I think that your formalization is very close to what I was expecting to \n> get (sort of one-way definition)... However, my git-cherry way produces \n> \"difference\" but on a higher level (than git-diff) since it includes both \n> + and - \"changes\". Of course, when I have then modified one of the \n> changesets slightly, I have different patch id, and thus + and - with same \n> log message (with verbose), which IMHO is a good thing to notice, \n> especially if I return to the work after 2 weeks or so :-). \n> \n> A real life example: In a branch, I have changed tcp_packets_in_flight \n> (~10 callers) to input sk instead of tp in a single changeset and >10 \n> minor changesets. I would love to see tcp_packets_in_fligth change \n> information just once when doing diffing topic-old topic-new during cherry \n> picking, instead of a lengthy diff full of search-and-replace \"noise\", \n> which increases possiblity of an human error...\n> \n> But anyway, I'm not claiming that your approach is less useful...\n\n+1 on an idea that git-cherry is \"diff of logs\". Perhaps to add some header.\nOf course use patch_ids _and_ the commit message to compare (we can have\nboth match, patch id match only, commit message match only, and commit\ntitle (first line) match only), but that can be selected using options. \n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"}]}