{"thread":{"id":"30722","subject":"[PATCH 1/2] merge: separte the logic to check for a signed tag","startedAt":"2012-06-05T19:58:30Z","lastAt":"2012-06-07T16:17:03Z","messageCount":7,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"192946","messageId":"1338926312-4239-1-git-send-email-gitster@pobox.com","threadId":"30722","inReplyTo":null,"subject":"[PATCH 0/2] Feeding an annotated but unsigned tag to \"git merge\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-05T19:58:30Z","receivedAt":"2012-06-05T19:58:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When you give an annotated but unsigned tag to \"git merge\", if the\ntagged commit does not fast-forward, we create a merge commit and\nthe tagged commit (not the tag itself) becomes one of the parents of\nthe resulting merge commit. The merge commit log contains the\nmessage from the annotated tag.\n\nWhen the tagged commit is a descendant of the current HEAD, however,\nwe used to simply fast-forward such a merge (losing the content of\nthe annotated tag).\n\nPost 1.7.9, we no longer do so.  These three create a merge commit\nfor an annotated tag \"anno\" that points at a commit that is a\ndescendant of the HEAD:\n\n        $ git merge anno\n        $ git merge --ff anno\n        $ git merge --no-ff anno\n\nYou can force fast-forwarding with:\n\n        $ git merge anno^0\n\nbut you obvously cannot record the contents of the annotated tag, as\nthere is no new commit to record it.\n\nThe \"--ff\" option has always meant \"allow fast-forward\", i.e. \"if\nthe merge can be fast-forwarded, do so without creating a new merge\ncommit\", and without any of the \"ff\"-related options, the command\ndefaults to allow fast-forwarding.  \"--no-ff\" is \"I always want a\nnew merge commit made\", and \"--ff-only\" is \"fail the command if it\ncannot be fast-forwarded\".  In effect, in the post 1.7.9 world, we\nconsider that an annotated tag is what you cannot fast-forward to.\n\nThe above definition was loosened slightly with b5c9f1c (merge: do\nnot create a signed tag merge under --ff-only option, 2012-02-05).\n\"--ff-only\" is taught to consider an annotated or signed tag that\npoints at a commit that can be fast-forwarded as what you can\nfast-forward to, so that a user following along without adding\nanything can do this:\n\n\t$ git checkout v3.2.0\n\t$ git pull --ff-only v3.3.0\n\nwithout creating an extra merge commit.\n\nThis two-patch series further loosens the definition by considering\nthat an annotated but unsigned tag can be fast-forwarded as long as\nit points at a commit that can be fast-forwarded to.  So\n\n        $ git merge anno\n        $ git merge --ff anno\n\nwill now fast-forward (note that this will *not* happen for signed\ntags).\n\nI find this change somewhat iffy myself, as we are encouraging\npeople to lose information (i.e. the contents of the annotated tag\nis no longer recorded in the history) and some may see it as a\nregression in the post 1.7.10 world because of that.\n\nBut since I've written it already, I thought it might be worth\nshowing it to the list for discussion, if only to publicly reject\nthe idea ;-).\n\nJunio C Hamano (2):\n  merge: separte the logic to check for a signed tag\n  merge: allow fast-forwarding to an annotated but unsigned tag\n\n builtin/merge.c  | 26 ++++++++++++++++++++++----\n t/t7600-merge.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 60 insertions(+), 4 deletions(-)\n\n-- \n1.7.11.rc1.37.g09843ac\n"},{"id":"192945","messageId":"1338926312-4239-2-git-send-email-gitster@pobox.com","threadId":"30722","inReplyTo":"1338926312-4239-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/2] merge: separte the logic to check for a signed tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-05T19:58:31Z","receivedAt":"2012-06-05T19:58:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We drop allow_fast_forward when merging a signed tag, because we\nalways need to create a new commit to have a place to record the\nsigned tag payload.\n\nMove the logic to determine if the object given to merge is a signed\ntag into a separate helper function.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/merge.c | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f385b8a..23389f2 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1099,6 +1099,15 @@ static void write_merge_state(void)\n \tclose(fd);\n }\n \n+static int merging_signed_tag(struct commit *parent)\n+{\n+\tstruct merge_remote_desc *desc = merge_remote_util(parent);\n+\n+\tif (!desc || !desc->obj || desc->obj->type != OBJ_TAG)\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n int cmd_merge(int argc, const char **argv, const char *prefix)\n {\n \tunsigned char result_tree[20];\n@@ -1283,10 +1292,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t    sha1_to_hex(commit->object.sha1));\n \t\tsetenv(buf.buf, argv[i], 1);\n \t\tstrbuf_reset(&buf);\n-\t\tif (!fast_forward_only &&\n-\t\t    merge_remote_util(commit) &&\n-\t\t    merge_remote_util(commit)->obj &&\n-\t\t    merge_remote_util(commit)->obj->type == OBJ_TAG) {\n+\t\tif (!fast_forward_only && merging_signed_tag(commit)) {\n \t\t\tif (option_edit < 0)\n \t\t\t\toption_edit = 1;\n \t\t\tallow_fast_forward = 0;\n-- \n1.7.11.rc1.37.g09843ac\n"},{"id":"192947","messageId":"1338926312-4239-3-git-send-email-gitster@pobox.com","threadId":"30722","inReplyTo":"1338926312-4239-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/2] merge: allow fast-forwarding to an annotated but unsigned tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-05T19:58:32Z","receivedAt":"2012-06-05T19:58:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Update the merging_signed_tag() helper to check if the tag object\nactually has a signature-looking string, so that we do not forbid\nfast-forwarding to an annotated but unsigned tag.  By definition,\nthere will be no signed payload in such a tag to be moved to the\nmergetag header, so we are not losing anything by fast-forwarding.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/merge.c  | 14 +++++++++++++-\n t/t7600-merge.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 51 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 23389f2..82d343c 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -28,6 +28,7 @@\n #include \"remote.h\"\n #include \"fmt-merge-msg.h\"\n #include \"gpg-interface.h\"\n+#include \"tag.h\"\n \n #define DEFAULT_TWOHEAD (1<<0)\n #define DEFAULT_OCTOPUS (1<<1)\n@@ -1102,10 +1103,21 @@ static void write_merge_state(void)\n static int merging_signed_tag(struct commit *parent)\n {\n \tstruct merge_remote_desc *desc = merge_remote_util(parent);\n+\tunsigned long size;\n+\tenum object_type type;\n+\tchar *buf;\n+\tsize_t sig_offset;\n \n \tif (!desc || !desc->obj || desc->obj->type != OBJ_TAG)\n \t\treturn 0;\n-\treturn 1;\n+\n+\tbuf = read_sha1_file(desc->obj->sha1, &type, &size);\n+\tif (!buf || type != OBJ_TAG) {\n+\t\tfree(buf);\n+\t\treturn 0; /* error will be caught downstream */\n+\t}\n+\tsig_offset = parse_signature(buf, size);\n+\treturn (sig_offset < size);\n }\n \n int cmd_merge(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 9e27bbf..3c48327 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -695,4 +695,42 @@ test_expect_success GPG 'merge --no-edit tag should skip editor' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'merge ff annotated tag should just ff' '\n+\tgit reset --hard c0 &&\n+\tgit commit --allow-empty -m \"A newer commit\" &&\n+\tgit tag -a -m \"An annotated tag\" anno &&\n+\tgit reset --hard c0 &&\n+\n+\t# This should not even bother with an editor session; \"false\"\n+\t# will ensure that an attempt to run the editor is caught.\n+\tEDITOR=false git merge anno &&\n+\n+\tgit rev-parse anno^0 >expect &&\n+\tgit rev-parse HEAD >actual &&\n+\ttest_cmp actual expect &&\n+\n+\tgit rev-parse c0^0 >expect &&\n+\tgit rev-parse HEAD^ >actual &&\n+\ttest_cmp actual expect\n+'\n+\n+test_expect_success 'merge --no-ff annotated tag' '\n+\tgit reset --hard c0 &&\n+\tgit commit --allow-empty -m \"A newer commit\" &&\n+\tgit tag -f -a -m \"An annotated tag\" anno &&\n+\tgit reset --hard c0 &&\n+\n+\tEDITOR=./editor git merge --no-ff --edit anno &&\n+\tgit rev-parse anno^0 >expect &&\n+\tgit rev-parse HEAD^2 >actual &&\n+\ttest_cmp actual expect &&\n+\n+\tgit rev-parse c0^0 >expect &&\n+\tgit rev-parse HEAD^ >actual &&\n+\ttest_cmp actual expect &&\n+\n+\tgit cat-file commit HEAD >raw &&\n+\tgrep \"An annotated tag\" raw\n+'\n+\n test_done\n-- \n1.7.11.rc1.37.g09843ac\n"},{"id":"192990","messageId":"20120606134207.GB2597@sigill.intra.peff.net","threadId":"30722","inReplyTo":"1338926312-4239-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/2] Feeding an annotated but unsigned tag to \"git merge\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-06T13:42:07Z","receivedAt":"2012-06-06T13:42:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 05, 2012 at 12:58:30PM -0700, Junio C Hamano wrote:\n\n> This two-patch series further loosens the definition by considering\n> that an annotated but unsigned tag can be fast-forwarded as long as\n> it points at a commit that can be fast-forwarded to.  So\n> \n>         $ git merge anno\n>         $ git merge --ff anno\n> \n> will now fast-forward (note that this will *not* happen for signed\n> tags).\n> \n> I find this change somewhat iffy myself, as we are encouraging\n> people to lose information (i.e. the contents of the annotated tag\n> is no longer recorded in the history) and some may see it as a\n> regression in the post 1.7.10 world because of that.\n> \n> But since I've written it already, I thought it might be worth\n> showing it to the list for discussion, if only to publicly reject\n> the idea ;-).\n\nIt has been nearly a day, and nobody has publicly rejected it. So I will\ndo so. :)\n\nThis just doesn't make sense to me. Why would we treat annotated but\nunsigned tags differently from signed tags? In both cases, the new\nbehavior is keeping more information about what happened, which is\ngenerally a good thing.\n\nI haven't seen any good argument against creating these merges[1]. But\neven if there was one, I don't think \"signed versus unsigned\" is\nnecessarily the right distinguishing feature. It is probably more about\nper-project or per-user preferences (e.g., \"my project does not want too\nmany merges, because it makes our history less pretty\"). And in that\ncase, something like a config flag would be a better option (not that I\nam not saying that such a flag is a good idea, only that it might be\nless bad than this).\n\n-Peff\n\n[1] From the tone of your message, I think you are not the right person\n    to be arguing that side, anyway. It sounds as though you are not all\n    that invested in this series. :)\n"},{"id":"192996","messageId":"7vzk8gmm5b.fsf@alter.siamese.dyndns.org","threadId":"30722","inReplyTo":"20120606134207.GB2597@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Feeding an annotated but unsigned tag to \"git merge\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-06T16:37:52Z","receivedAt":"2012-06-06T16:37:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jun 05, 2012 at 12:58:30PM -0700, Junio C Hamano wrote:\n> ...\n>> But since I've written it already, I thought it might be worth\n>> showing it to the list for discussion, if only to publicly reject\n>> the idea ;-).\n>\n> It has been nearly a day, and nobody has publicly rejected it. So I will\n> do so. :)\n>\n> This just doesn't make sense to me. Why would we treat annotated but\n> unsigned tags differently from signed tags? In both cases, the new\n> behavior is keeping more information about what happened, which is\n> generally a good thing.\n>\n> I haven't seen any good argument against creating these merges[1].\n\nIt is in line with --ff-only special casing, though.  The argument\nagainst it is that \"we used to fast forward\", I would think, even\nthough in general my reaction to that would be \"so what?\" because I\nagree with your \"keeping more information instead of discarding as\nwe used to is a good feature enhancement, why should we retreat?\"\n\n> [1] From the tone of your message, I think you are not the right person\n>     to be arguing that side, anyway. It sounds as though you are not all\n>     that invested in this series. :)\n\nI am actually ambivalent; instead of being 0% supportive like I\nusually am for many topics, perhaps I am 30% sympathetic to this\none.\n\nThis was triggered by\nhttp://thread.gmane.org/gmane.comp.version-control.git/198828\n"},{"id":"193044","messageId":"20120607090933.GB6087@sigill.intra.peff.net","threadId":"30722","inReplyTo":"7vzk8gmm5b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/2] Feeding an annotated but unsigned tag to \"git merge\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-07T09:09:33Z","receivedAt":"2012-06-07T09:09:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 06, 2012 at 09:37:52AM -0700, Junio C Hamano wrote:\n\n> > This just doesn't make sense to me. Why would we treat annotated but\n> > unsigned tags differently from signed tags? In both cases, the new\n> > behavior is keeping more information about what happened, which is\n> > generally a good thing.\n> >\n> > I haven't seen any good argument against creating these merges[1].\n> \n> It is in line with --ff-only special casing, though.\n\nIs it?  My impression from reading b5c9f1c is that --ff-only trumps both\nannotated _and_ signed tags. Which makes sense to me. What I was\nobjecting to is that \"some tag objects are more equal than others\". It's\nOK to treat unannotated tags differently from tag objects, but treating\nannotated but unsigned objects differently from signed objects seems\nunnecessary and complex.\n\n> This was triggered by\n> http://thread.gmane.org/gmane.comp.version-control.git/198828\n\nThe complaint in that thread mentions git's v1.7.10.2, which is signed,\nand therefore would not be impacted by change. But perhaps that was\npurely for illustrative purposes.\n\n-Peff\n"},{"id":"193074","messageId":"7vhaunjdvk.fsf@alter.siamese.dyndns.org","threadId":"30722","inReplyTo":"20120607090933.GB6087@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Feeding an annotated but unsigned tag to \"git merge\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-07T16:17:03Z","receivedAt":"2012-06-07T16:17:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jun 06, 2012 at 09:37:52AM -0700, Junio C Hamano wrote:\n>\n>> > This just doesn't make sense to me. Why would we treat annotated but\n>> > unsigned tags differently from signed tags? In both cases, the new\n>> > behavior is keeping more information about what happened, which is\n>> > generally a good thing.\n>> >\n>> > I haven't seen any good argument against creating these merges[1].\n>> \n>> It is in line with --ff-only special casing, though.\n>\n> Is it?  My impression from reading b5c9f1c is that --ff-only trumps both\n> annotated _and_ signed tags. Which makes sense to me. What I was\n> objecting to is that \"some tag objects are more equal than others\". It's\n> OK to treat unannotated tags differently from tag objects, but treating\n> annotated but unsigned objects differently from signed objects seems\n> unnecessary and complex.\n\nOK, let's drop it.\n"}]}