{"thread":{"id":"26572","subject":"[RFC/PATCH] rev-list: new --cherry-pick=loose option","startedAt":"2011-02-21T19:49:29Z","lastAt":"2011-02-22T02:54:55Z","messageCount":3,"participants":["Jay Soffian","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"161834","messageId":"1298317769-65830-1-git-send-email-jaysoffian@gmail.com","threadId":"26572","inReplyTo":null,"subject":"[RFC/PATCH] rev-list: new --cherry-pick=loose option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-02-21T19:49:29Z","receivedAt":"2011-02-21T19:49:29Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"rev-list --cherry-pick may be used in combination with the\nsymmetric difference operator (A...B) to squelch showing commits\nwhich are identical in content.\n\nSometimes this is too strict of a test, for example if the\ncherry-picked commits required conflict resolution, altering\ntheir patch-ids. However, it is still useful to be able to squelch\nsuch commits as they need not be cherry-picked again.\n\nUsing --cherry-pick=loose tells git to ignore the patch content\nand instead examine its metadata. Specifically, commits with\nidentical authorship (name and timestamp) and subject are not\nshown in the output.\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\n\nThis is a bit of a hack just to stimulate conversation. My use\ncase for this is that I'm cherry-picking commits from a release branch\nonto a development branch. (I cannot use a different branching strategy\nthat would allow me to merge instead.) Sometimes those cherry-picks require\nconflict resolution. The current --cherry-pick behavior requires that I\nmaintain (externally to git) a list of commits which have already been\ncherry-picked.\n\nSo I started experimenting with a wrapper around rev-list A...B that\nlooks at commit metadata to determine what's been cherry-picked and\nwhat not, and it works quite well for me.\n\nThe functionality in this patch would let me get rid of that wrapper.\n\nThoughts?\n\n patch-ids.c |   23 +++++++++++++++++++++--\n patch-ids.h |    2 +-\n revision.c  |    9 +++++++++\n revision.h  |    1 +\n 4 files changed, 32 insertions(+), 3 deletions(-)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 5717257..6a1f3e3 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -16,6 +16,20 @@ static int commit_patch_id(struct commit *commit, struct diff_options *options,\n \treturn diff_flush_patch_id(options, sha1);\n }\n \n+static int commit_patch_id_loose(struct commit *commit, unsigned char *sha1)\n+{\n+\tgit_SHA_CTX ctx;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct pretty_print_context pctx = {0};\n+\n+\tgit_SHA1_Init(&ctx);\n+\tformat_commit_message(commit, \"%an %ae %at %s\", &buf, &pctx);\n+\tgit_SHA1_Update(&ctx, buf.buf, buf.len);\n+\tstrbuf_release(&buf);\n+\tgit_SHA1_Final(sha1, &ctx);\n+\treturn 0;\n+}\n+\n static const unsigned char *patch_id_access(size_t index, void *table)\n {\n \tstruct patch_id **id_table = table;\n@@ -65,8 +79,13 @@ static struct patch_id *add_commit(struct commit *commit,\n \tunsigned char sha1[20];\n \tint pos;\n \n-\tif (commit_patch_id(commit, &ids->diffopts, sha1))\n-\t\treturn NULL;\n+\tif (ids->loose) {\n+\t\tif (commit_patch_id_loose(commit, sha1))\n+\t\t\treturn NULL;\n+\t} else {\n+\t\tif (commit_patch_id(commit, &ids->diffopts, sha1))\n+\t\t\treturn NULL;\n+\t}\n \tpos = patch_pos(ids->table, ids->nr, sha1);\n \tif (0 <= pos)\n \t\treturn ids->table[pos];\ndiff --git a/patch-ids.h b/patch-ids.h\nindex c8c7ca1..f54b0ee 100644\n--- a/patch-ids.h\n+++ b/patch-ids.h\n@@ -8,7 +8,7 @@ struct patch_id {\n \n struct patch_ids {\n \tstruct diff_options diffopts;\n-\tint nr, alloc;\n+\tint nr, alloc, loose;\n \tstruct patch_id **table;\n \tstruct patch_id_bucket *patches;\n };\ndiff --git a/revision.c b/revision.c\nindex 7b9eaef..6dbbf5d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -558,6 +558,8 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)\n \t\tids.diffopts.paths = revs->diffopt.paths;\n \t\tids.diffopts.pathlens = revs->diffopt.pathlens;\n \t}\n+\tif (revs->cherry_pick_loose)\n+\t\tids.loose = 1;\n \n \t/* Compute patch-ids for one side */\n \tfor (p = list; p; p = p->next) {\n@@ -1268,6 +1270,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--cherry-pick\")) {\n \t\trevs->cherry_pick = 1;\n \t\trevs->limited = 1;\n+\t} else if (!prefixcmp(arg, \"--cherry-pick=\")) {\n+\t\trevs->cherry_pick = 1;\n+\t\trevs->limited = 1;\n+\t\tif (!strcmp(arg+14, \"loose\"))\n+\t\t\trevs->cherry_pick_loose = 1;\n+\t\telse if (strcmp(arg+14, \"strict\"))\n+\t\t\treturn error(\"bad --cherry-pick argument\");\n \t} else if (!strcmp(arg, \"--objects\")) {\n \t\trevs->tag_objects = 1;\n \t\trevs->tree_objects = 1;\ndiff --git a/revision.h b/revision.h\nindex 05659c6..3f94b27 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -66,6 +66,7 @@ struct rev_info {\n \t\t\treverse:1,\n \t\t\treverse_output_stage:1,\n \t\t\tcherry_pick:1,\n+\t\t\tcherry_pick_loose:1,\n \t\t\tbisect:1,\n \t\t\tancestry_path:1,\n \t\t\tfirst_parent_only:1;\n-- \n1.7.4.1.51.g2cc0c.dirty\n"},{"id":"161848","messageId":"7vfwrghqjp.fsf@alter.siamese.dyndns.org","threadId":"26572","inReplyTo":"1298317769-65830-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [RFC/PATCH] rev-list: new --cherry-pick=loose option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-22T00:16:26Z","receivedAt":"2011-02-22T00:16:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian <jaysoffian@gmail.com> writes:\n\n> Thoughts?\n\nI am personally not very interested in this particular \"author, timestamp,\ntitle and nothing else\" implementation, as that is probably too loose (in\nmany projects, title by itself is not descriptive enough) to be safe.\nAlso people would probably want other loose modes with varying degree\n(e.g. throwing in the list of touched paths to your mix might make it a\nbit safer), so \"loose\" feels a bit too broad a word to give to this\nparticular implementation (iow, it does not say in what way it is loose).\n\n> @@ -65,8 +79,13 @@ static struct patch_id *add_commit(struct commit *commit,\n>  \tunsigned char sha1[20];\n>  \tint pos;\n>  \n> -\tif (commit_patch_id(commit, &ids->diffopts, sha1))\n> -\t\treturn NULL;\n> +\tif (ids->loose) {\n> +\t\tif (commit_patch_id_loose(commit, sha1))\n> +\t\t\treturn NULL;\n> +\t} else {\n> +\t\tif (commit_patch_id(commit, &ids->diffopts, sha1))\n> +\t\t\treturn NULL;\n> +\t}\n\nIf the purpose of the patch is to stir the discussion, it is fine to have\nany crappy \"here is a strawman\" algorithm as an example of an alternative\npatch ID computation, but one thing it _should_ do well is to show where\nthe necessary change should be hooked into, and I think the above \"if\"\nstatement is placed in a wrong function.  If you change commit_patch_id()\nto take a pointer to the whole ids instead of just &ids->diffopts, it can\ndecide how the \"commit patch ID\" is computed without affecting the\ncallers, no?\n\nAnd then we could instead introduce patch-id-algo=<foo>, and instead of\n\"loose\" call this particular algorithm \"authorship-subject\" or something.\nComing up with a pluggable interface to let the end user compute patch\nequivalence might be a plus.\n\nCertain patch equivalence might not be easy to define by \"do they hash to\nthe same small value\" but by \"here are two patches---compare them and tell\nme if they are equivalent\".  If we can update the code to support that\nkind of patch equivalence it would be great, but it is not within the\nreach/scope of this patch (not a complaint, but something you may want to\ntackle next).\n"},{"id":"161856","messageId":"AANLkTi=dvkf3_Bkn0tCruDmYSnqYSGLQ55-NhCEuysnF@mail.gmail.com","threadId":"26572","inReplyTo":"7vfwrghqjp.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] rev-list: new --cherry-pick=loose option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-02-22T02:54:55Z","receivedAt":"2011-02-22T02:54:55Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Feb 21, 2011 at 7:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jay Soffian <jaysoffian@gmail.com> writes:\n>\n>> Thoughts?\n>\n> I am personally not very interested in this particular \"author, timestamp,\n> title and nothing else\" implementation, as that is probably too loose (in\n> many projects, title by itself is not descriptive enough) to be safe.\n\nWell, right, it's not title by itself, it's title + authorship. I am\nscratching my head at a project that would have commits identical in\nauthor, timestamp, and title.\n\n> Also people would probably want other loose modes with varying degree\n\nInitially I thought this too and was going to let it take pretty-print\nspecifiers (--cherry-pick=format:...), but I can't see which other\nmetadata would be useful. Maybe the commit body, but if there were\nconflicts, that's something that would have very likely been changed\nto add a Conflicts: section.\n\n> (e.g. throwing in the list of touched paths to your mix might make it a\n> bit safer), so \"loose\" feels a bit too broad a word to give to this\n> particular implementation (iow, it does not say in what way it is loose).\n\nThrowing in touched paths does make it safer, but for my use case is\ntoo strict for some commits (I sometimes have to account for file\nrenames when I cherry-pick).\n\n>> @@ -65,8 +79,13 @@ static struct patch_id *add_commit(struct commit *commit,\n>>       unsigned char sha1[20];\n>>       int pos;\n>>\n>> -     if (commit_patch_id(commit, &ids->diffopts, sha1))\n>> -             return NULL;\n>> +     if (ids->loose) {\n>> +             if (commit_patch_id_loose(commit, sha1))\n>> +                     return NULL;\n>> +     } else {\n>> +             if (commit_patch_id(commit, &ids->diffopts, sha1))\n>> +                     return NULL;\n>> +     }\n>\n> If the purpose of the patch is to stir the discussion, it is fine to have\n> any crappy \"here is a strawman\" algorithm as an example of an alternative\n> patch ID computation, but one thing it _should_ do well is to show where\n> the necessary change should be hooked into, and I think the above \"if\"\n> statement is placed in a wrong function.  If you change commit_patch_id()\n> to take a pointer to the whole ids instead of just &ids->diffopts, it can\n> decide how the \"commit patch ID\" is computed without affecting the\n> callers, no?\n\nGood idea.\n\n> And then we could instead introduce patch-id-algo=<foo>, and instead of\n> \"loose\" call this particular algorithm \"authorship-subject\" or something.\n> Coming up with a pluggable interface to let the end user compute patch\n> equivalence might be a plus.\n\nI think it's premature to make it pluggable. I was hoping that with\nthis mode (loose) and the existing mode (strict), the extremes would\nbe covered. And then we'd wait and see what latent demand might exist\nfor other modes. :-)\n\nI just don't feel like I know enough about what \"the end user\" wants\nto come up with a reasonably general scheme.\n\n(The best I can come up with that's pluggable and not horribly\nexpensive is something like open bi-directional pipes to an external\nprocess that gets fed diffs and returns patch-ids.)\n\n> Certain patch equivalence might not be easy to define by \"do they hash to\n> the same small value\" but by \"here are two patches---compare them and tell\n> me if they are equivalent\".  If we can update the code to support that\n> kind of patch equivalence it would be great, but it is not within the\n> reach/scope of this patch (not a complaint, but something you may want to\n> tackle next).\n\nMore than I can bite off at the moment.\n\nj.\n"}]}