{"thread":{"id":"28105","subject":"[PATCH] commit: check return value of lookup_commit()","startedAt":"2011-08-15T15:38:36Z","lastAt":"2011-08-23T18:46:59Z","messageCount":21,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"173542","messageId":"1313422716-26432-1-git-send-email-pclouds@gmail.com","threadId":"28105","inReplyTo":null,"subject":"[PATCH] commit: check return value of lookup_commit()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-15T15:38:36Z","receivedAt":"2011-08-15T15:38:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If lookup_commit() returns NULL, there's usually serious error and the\ncommand aborts anyway. However it's still nicer to have a message\ntelling us where it aborts, rather than segmentation fault.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I suppose die() also cleans up $GIT_DIR/index.lock while sigsegv does not.\n\n builtin/commit.c | 17 +++++++++++++----\n 1 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2088b6b..bfb7a5a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1387,6 +1387,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tunsigned char commit_sha1[20];\n \tstruct ref_lock *ref_lock;\n \tstruct commit_list *parents = NULL, **pptr = &parents;\n+\tstruct commit *commit;\n \tstruct stat statbuf;\n \tint allow_fast_forward = 1;\n \tstruct wt_status s;\n@@ -1423,7 +1424,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treflog_msg = \"commit (initial)\";\n \t} else if (amend) {\n \t\tstruct commit_list *c;\n-\t\tstruct commit *commit;\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (amend)\";\n@@ -1439,7 +1439,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (merge)\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tcommit = lookup_commit(head_sha1);\n+\t\tif (!commit)\n+\t\t\tdie(_(\"could not parse HEAD commit\"));\n+\t\tpptr = &commit_list_insert(commit, pptr)->next;\n \t\tfp = fopen(git_path(\"MERGE_HEAD\"), \"r\");\n \t\tif (fp == NULL)\n \t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n@@ -1448,7 +1451,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\tunsigned char sha1[20];\n \t\t\tif (get_sha1_hex(m.buf, sha1) < 0)\n \t\t\t\tdie(_(\"Corrupt MERGE_HEAD file (%s)\"), m.buf);\n-\t\t\tpptr = &commit_list_insert(lookup_commit(sha1), pptr)->next;\n+\t\t\tcommit = lookup_commit(sha1);\n+\t\t\tif (!commit)\n+\t\t\t\tdie(_(\"could not parse commit %s\"), sha1_to_hex(sha1));\n+\t\t\tpptr = &commit_list_insert(commit, pptr)->next;\n \t\t}\n \t\tfclose(fp);\n \t\tstrbuf_release(&m);\n@@ -1465,7 +1471,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n \t\t\t\t\t? \"commit (cherry-pick)\"\n \t\t\t\t\t: \"commit\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tcommit = lookup_commit(head_sha1);\n+\t\tif (!commit)\n+\t\t\tdie(_(\"could not parse HEAD commit\"));\n+\t\tpptr = &commit_list_insert(commit, pptr)->next;\n \t}\n \n \t/* Finally, get the commit message */\n-- \n1.7.4.74.g639db\n"},{"id":"173549","messageId":"7vei0mlg8d.fsf@alter.siamese.dyndns.org","threadId":"28105","inReplyTo":"1313422716-26432-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] commit: check return value of lookup_commit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-15T17:46:58Z","receivedAt":"2011-08-15T17:46:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> If lookup_commit() returns NULL, there's usually serious error and the\n> command aborts anyway. However it's still nicer to have a message\n> telling us where it aborts, rather than segmentation fault.\n\nThe change itself looks good to me but a point and a half to think about:\n\n - In this if/elseif/.../else cascade, everybody except for the\n \"initial_commit\" case needs to make sure that head_sha1 points at a\n valid commit and get an commit object. Hoisting the scope of the\n variable \"commit\" one level in your patch is good, but it would make it\n easier to read and the future code modification much less error prone\n if (1) you called lookup_commit() and checked for errors before\n entering this if/elseif/... cascade, and (2) you renamed this variable\n to \"head_commit\".\n\n - Whether we like it or not, many people have a broken reimplementations\n of git that can put a non-commit in HEAD, and they won't be fixed\n overnight. Instead of erroring out, would it be nicer of us if we just\n warned, unwrapped the tag and used the tagged commit instead?\n"},{"id":"173606","messageId":"CACsJy8AusStKNWuw3j740r4Nc0FhzR+jJZJNaesxn68pr7dTqA@mail.gmail.com","threadId":"28105","inReplyTo":"7vei0mlg8d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit: check return value of lookup_commit()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-16T13:22:27Z","receivedAt":"2011-08-16T13:22:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/8/16 Junio C Hamano :\n> The change itself looks good to me but a point and a half to think about:\n>\n>  - In this if/elseif/.../else cascade, everybody except for the\n>   \"initial_commit\" case needs to make sure that head_sha1 points at a\n>   valid commit and get an commit object. Hoisting the scope of the\n>   variable \"commit\" one level in your patch is good, but it would make it\n>   easier to read and the future code modification much less error prone\n>   if (1) you called lookup_commit() and checked for errors before\n>   entering this if/elseif/... cascade, and (2) you renamed this variable\n>   to \"head_commit\".\n\nBut then I would need to avoid die()ing in \"initial_commit\" case. So\nit becomes two related condition blocks (head_commit check and the\nif/elseif...), more error prone to me.\n\n>  - Whether we like it or not, many people have a broken reimplementations\n>   of git that can put a non-commit in HEAD, and they won't be fixed\n>   overnight. Instead of erroring out, would it be nicer of us if we just\n>   warned, unwrapped the tag and used the tagged commit instead?\n\nHow about replacing those lookup_commit() with this? It would tolerate\ntag-in-branch case, but also warn users that something's gone wrong.\n\nstruct commit *lookup_expect_commit(const unsigned char *sha1,\n\t\t\t\t const char *ref_name)\n{\n\tstruct commit *c = lookup_commit_reference(sha1);\n\tif (!c)\n\t\tdie(_(\"could not parse %s\"), ref_name);\n\tif (hashcmp(sha1, c->object.sha1))\n\t\twarning(_(\"%s %s is not a commit!\"),\n\t\t\tref_name, sha1_to_hex(sha1));\n\treturn c;\n}\n-- \nDuy\n"},{"id":"173625","messageId":"7vy5ytgrps.fsf@alter.siamese.dyndns.org","threadId":"28105","inReplyTo":"CACsJy8AusStKNWuw3j740r4Nc0FhzR+jJZJNaesxn68pr7dTqA@mail.gmail.com","subject":"Re: [PATCH] commit: check return value of lookup_commit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-16T18:02:23Z","receivedAt":"2011-08-16T18:02:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> 2011/8/16 Junio C Hamano :\n>> The change itself looks good to me but a point and a half to think about:\n>>\n>>  - In this if/elseif/.../else cascade, everybody except for the\n>>   \"initial_commit\" case needs to make sure that head_sha1 points at a\n>>   valid commit and get an commit object. Hoisting the scope of the\n>>   variable \"commit\" one level in your patch is good, but it would make it\n>>   easier to read and the future code modification much less error prone\n>>   if (1) you called lookup_commit() and checked for errors before\n>>   entering this if/elseif/... cascade, and (2) you renamed this variable\n>>   to \"head_commit\".\n>\n> But then I would need to avoid die()ing in \"initial_commit\" case.\n\nThat's exactly what I said.\n\n\tif (!initial)\n\t /* we need to know the head_commit */\n head_commit = lookup_and_check(HEAD);\n\n\t/* depending on what kind of commit, we need different stuff */\n if (initial)\n \t... going to create a parentless commit\n\telse if (amending)\n\t\t... use the head_commit to learn parent, reuse the message\n ... from there\n\telse if ...\n\nThese two are independent if/else cascades in the sense that the first is\nabout learning the details of head_commit, and the latter is about\nlearning how the commit is done, and in a subset of the latter head_commit\nis used.\n\n>>  - Whether we like it or not, many people have a broken reimplementations\n>>   of git that can put a non-commit in HEAD, and they won't be fixed\n>>   overnight. Instead of erroring out, would it be nicer of us if we just\n>>   warned, unwrapped the tag and used the tagged commit instead?\n>\n> How about replacing those lookup_commit() with this? It would tolerate\n> tag-in-branch case, but also warn users that something's gone wrong.\n\nYes, that is exactly what I meant.\n\nThakns.\n"},{"id":"173653","messageId":"CACsJy8An1HwDkH86DoL2Kd9jfaD1cTkBN8sn=vfeHYVLbr0jAA@mail.gmail.com","threadId":"28105","inReplyTo":"7vy5ytgrps.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit: check return value of lookup_commit()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-17T01:32:13Z","receivedAt":"2011-08-17T01:32:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 17, 2011 at 1:02 AM, Junio C Hamano >> But then I would\nneed to avoid die()ing in \"initial_commit\" case.\n>\n> That's exactly what I said.\n>\n>        if (!initial)\n>                /* we need to know the head_commit */\n>                head_commit = lookup_and_check(HEAD);\n>\n>        /* depending on what kind of commit, we need different stuff */\n>        if (initial)\n>                ... going to create a parentless commit\n>        else if (amending)\n>                ... use the head_commit to learn parent, reuse the message\n>                ... from there\n>        else if ...\n>\n> These two are independent if/else cascades in the sense that the first is\n> about learning the details of head_commit, and the latter is about\n> learning how the commit is done, and in a subset of the latter head_commit\n> is used.\n\nAnd some months from now, someone may add more code in between the two\n\"if\" blocks. Some more months past, someone tweaks the \"if (initial)\"\nblock but forgets to also update the \"if (!initial)\" above. This is\nwhat I want to avoid.\n-- \nDuy\n"},{"id":"173654","messageId":"1313545369-7096-1-git-send-email-pclouds@gmail.com","threadId":"28105","inReplyTo":"1313422716-26432-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2] commit: accept tag objects in HEAD/MERGE_HEAD","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-17T01:42:49Z","receivedAt":"2011-08-17T01:42:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"HEAD, MERGE_HEAD (or any other branches) should only have SHA-1 of a\ncommit object. However broken tools can put a tag object there. While\nit's wrong, it'd be better to tolerate the situation and move on\n(\"commit\" is an often used operation, unable to commit could be bad).\n\nIn worse cases, where we cannot even resolve HEAD to a commit, die()\ninstead of segfault.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/commit.c | 15 +++++++++------\n commit.c | 12 ++++++++++++\n commit.h | 1 +\n 3 files changed, 22 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2088b6b..f327595 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1387,6 +1387,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tunsigned char commit_sha1[20];\n \tstruct ref_lock *ref_lock;\n \tstruct commit_list *parents = NULL, **pptr = &parents;\n+\tstruct commit *commit;\n \tstruct stat statbuf;\n \tint allow_fast_forward = 1;\n \tstruct wt_status s;\n@@ -1423,12 +1424,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treflog_msg = \"commit (initial)\";\n \t} else if (amend) {\n \t\tstruct commit_list *c;\n-\t\tstruct commit *commit;\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (amend)\";\n-\t\tcommit = lookup_commit(head_sha1);\n-\t\tif (!commit || parse_commit(commit))\n+\t\tcommit = lookup_expect_commit(head_sha1, \"HEAD\");\n+\t\tif (parse_commit(commit))\n \t\t\tdie(_(\"could not parse HEAD commit\"));\n \n \t\tfor (c = commit->parents; c; c = c->next)\n@@ -1439,7 +1439,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (merge)\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tcommit = lookup_expect_commit(head_sha1, \"HEAD\");\n+\t\tpptr = &commit_list_insert(commit, pptr)->next;\n \t\tfp = fopen(git_path(\"MERGE_HEAD\"), \"r\");\n \t\tif (fp == NULL)\n \t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n@@ -1448,7 +1449,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\tunsigned char sha1[20];\n \t\t\tif (get_sha1_hex(m.buf, sha1) < 0)\n \t\t\t\tdie(_(\"Corrupt MERGE_HEAD file (%s)\"), m.buf);\n-\t\t\tpptr = &commit_list_insert(lookup_commit(sha1), pptr)->next;\n+\t\t\tcommit = lookup_expect_commit(sha1, \"MERGE_HEAD\");\n+\t\t\tpptr = &commit_list_insert(commit, pptr)->next;\n \t\t}\n \t\tfclose(fp);\n \t\tstrbuf_release(&m);\n@@ -1465,7 +1467,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n \t\t\t\t\t? \"commit (cherry-pick)\"\n \t\t\t\t\t: \"commit\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tcommit = lookup_expect_commit(head_sha1, \"HEAD\");\n+\t\tpptr = &commit_list_insert(commit, pptr)->next;\n \t}\n \n \t/* Finally, get the commit message */\ndiff --git a/commit.c b/commit.c\nindex ac337c7..dc22695 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -39,6 +39,18 @@ struct commit *lookup_commit_reference(const unsigned char *sha1)\n \treturn lookup_commit_reference_gently(sha1, 0);\n }\n \n+struct commit *lookup_expect_commit(const unsigned char *sha1,\n+\t\t\t\t const char *ref_name)\n+{\n+\tstruct commit *c = lookup_commit_reference(sha1);\n+\tif (!c)\n+\t\tdie(_(\"could not parse %s\"), ref_name);\n+\tif (hashcmp(sha1, c->object.sha1))\n+\t\twarning(_(\"%s %s is not a commit!\"),\n+\t\t\tref_name, sha1_to_hex(sha1));\n+\treturn c;\n+}\n+\n struct commit *lookup_commit(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\ndiff --git a/commit.h b/commit.h\nindex feb809f..0e36fd0 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -37,6 +37,7 @@ struct commit *lookup_commit_reference(const unsigned char *sha1);\n struct commit *lookup_commit_reference_gently(const unsigned char *sha1,\n \t\t\t\t\t int quiet);\n struct commit *lookup_commit_reference_by_name(const char *name);\n+struct commit *lookup_expect_commit(const unsigned char *sha1, const char *ref_name);\n \n int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size);\n int parse_commit(struct commit *item);\n-- \n1.7.4.74.g639db\n"},{"id":"173681","messageId":"7vy5yrex70.fsf@alter.siamese.dyndns.org","threadId":"28105","inReplyTo":"1313545369-7096-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] commit: accept tag objects in HEAD/MERGE_HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-17T17:59:15Z","receivedAt":"2011-08-17T17:59:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> HEAD, MERGE_HEAD (or any other branches) should only have SHA-1 of a\n> commit object. However broken tools can put a tag object there. While\n> it's wrong, it'd be better to tolerate the situation and move on\n\nThe best part in your patch is that you made it _warn_ when it happens; I\nwould suggest rewording this with s/situation/&, warn/.\n\n> (\"commit\" is an often used operation, unable to commit could be bad).\n\nNeither \"often used\" nor \"unable to commit\" is a good reason for this\nadded leniency. The real reason is that such a condition left by broken\ntools is cumbersome to fix by an end user with:\n\n\t$ git update-ref HEAD $(git rev-parse HEAD^{commit})\n\nwhich may look like a magic to a new person.\n\nBy the way, what happens when you try to merge when HEAD points at a tag\nthat points at a commit? Would we end up creating a commit that points at\na bogus parent?\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 2088b6b..f327595 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1387,6 +1387,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \tunsigned char commit_sha1[20];\n> \tstruct ref_lock *ref_lock;\n> \tstruct commit_list *parents = NULL, **pptr = &parents;\n> +\tstruct commit *commit;\n> \tstruct stat statbuf;\n> \tint allow_fast_forward = 1;\n> \tstruct wt_status s;\n\nHere, you are being inconsistent with your own argument you made in your\nprevious message \"later somebody may forget to update the former while\nupdating the latter\" when I suggested to separate the two logically\nseparate operations (grab the head_commit object when necessary, and\ndecide how the commit is made). By hoisting of the scope of \"commit\", you\nmade the variable undefined when dealing with the initial_commit, exposing\nthe code to the same risk that somebody may try to use \"commit\" variable\nafter the if/elseif/... cascade, where it may or may not be defined.\n\nNot that I buy your previous argument in this case---it's not like we have\ndeeply nested callchain that sometimes sets a variable and sometimes\ndoesn't. It's all there for the updater to see in a single function.\n\n> @@ -1423,12 +1424,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \t\t\treflog_msg = \"commit (initial)\";\n> \t} else if (amend) {\n> \t\tstruct commit_list *c;\n> -\t\tstruct commit *commit;\n> \n> \t\tif (!reflog_msg)\n> \t\t\treflog_msg = \"commit (amend)\";\n> -\t\tcommit = lookup_commit(head_sha1);\n> -\t\tif (!commit || parse_commit(commit))\n> +\t\tcommit = lookup_expect_commit(head_sha1, \"HEAD\");\n> +\t\tif (parse_commit(commit))\n> \t\t\tdie(_(\"could not parse HEAD commit\"));\n\nIs this still necessary? I think your lookup_expect_commit() already\nchecks this condition and barfs.\n"},{"id":"173712","messageId":"CACsJy8BxAOtaJKv50_Ntr2qjEpJNaWNq_obYv4u+L3O=z27wLA@mail.gmail.com","threadId":"28105","inReplyTo":"7vy5yrex70.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] commit: accept tag objects in HEAD/MERGE_HEAD","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-18T02:10:43Z","receivedAt":"2011-08-18T02:10:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/8/18 Junio C Hamano :\n> By the way, what happens when you try to merge when HEAD points at a tag\n> that points at a commit? Would we end up creating a commit that points at\n> a bogus parent?\n\nI guess I'd segfault the same way commit does. It does\nlookup_commit(head) without checking the result.\n\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 2088b6b..f327595 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1387,6 +1387,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>       unsigned char commit_sha1[20];\n>>       struct ref_lock *ref_lock;\n>>       struct commit_list *parents = NULL, **pptr = &parents;\n>> +     struct commit *commit;\n>>       struct stat statbuf;\n>>       int allow_fast_forward = 1;\n>>       struct wt_status s;\n>\n> Here, you are being inconsistent with your own argument you made in your\n> previous message \"later somebody may forget to update the former while\n> updating the latter\" when I suggested to separate the two logically\n> separate operations (grab the head_commit object when necessary, and\n> decide how the commit is made). By hoisting of the scope of \"commit\", you\n> made the variable undefined when dealing with the initial_commit, exposing\n> the code to the same risk that somebody may try to use \"commit\" variable\n> after the if/elseif/... cascade, where it may or may not be defined.\n\nFair enough.\n\n>> @@ -1423,12 +1424,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>                       reflog_msg = \"commit (initial)\";\n>>       } else if (amend) {\n>>               struct commit_list *c;\n>> -             struct commit *commit;\n>>\n>>               if (!reflog_msg)\n>>                       reflog_msg = \"commit (amend)\";\n>> -             commit = lookup_commit(head_sha1);\n>> -             if (!commit || parse_commit(commit))\n>> +             commit = lookup_expect_commit(head_sha1, \"HEAD\");\n>> +             if (parse_commit(commit))\n>>                       die(_(\"could not parse HEAD commit\"));\n>\n> Is this still necessary? I think your lookup_expect_commit() already\n> checks this condition and barfs.\n\nIt is. lookup_expect_commit() does not parse commit content, only\nmakes sure it's a commit object.\n-- \nDuy\n"},{"id":"173756","messageId":"1313674994-22902-1-git-send-email-pclouds@gmail.com","threadId":"28105","inReplyTo":"1313545369-7096-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3] Accept tags in HEAD or MERGE_HEAD","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-18T13:43:14Z","receivedAt":"2011-08-18T13:43:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"HEAD and MERGE_HEAD (among other branch tips) should never hold a\ntag. That can only be caused by broken tools and is cumbersome to fix\nby an end user with:\n\n $ git update-ref HEAD $(git rev-parse HEAD^{commit})\n\nwhich may look like a magic to a new person.\n\nBe easy, warn users (so broken tools can be fixed) and move on.\n\nBe robust, if the given SHA-1 cannot be resolved to a commit object,\ndie (therefore return value is always valid).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/commit.c | 25 ++++++++++++-------------\n builtin/fmt-merge-msg.c | 2 +-\n builtin/merge.c | 19 +++++++++++--------\n commit.c | 12 ++++++++++++\n commit.h | 1 +\n http-push.c | 8 ++++----\n revision.c | 6 ++++--\n 7 files changed, 45 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex cb73857..f78b449 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -63,6 +63,7 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n \"Otherwise, please use 'git reset'\\n\");\n \n static unsigned char head_sha1[20];\n+static struct commit *head_commit;\n \n static const char *use_message_buffer;\n static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n@@ -518,11 +519,8 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int\n \treturn s->commitable;\n }\n \n-static int is_a_merge(const unsigned char *sha1)\n+static int is_a_merge(struct commit *commit)\n {\n-\tstruct commit *commit = lookup_commit(sha1);\n-\tif (!commit || parse_commit(commit))\n-\t\tdie(_(\"could not parse HEAD commit\"));\n \treturn !!(commit->parents && commit->parents->next);\n }\n \n@@ -848,7 +846,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t * empty due to conflict resolution, which the user should okay.\n \t */\n \tif (!commitable && whence != FROM_MERGE && !allow_empty &&\n-\t !(amend && is_a_merge(head_sha1))) {\n+\t !(amend && is_a_merge(head_commit))) {\n \t\trun_status(stdout, index_file, prefix, 0, s);\n \t\tif (amend)\n \t\t\tfputs(_(empty_amend_advice), stderr);\n@@ -1028,6 +1026,9 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \n \tif (get_sha1(\"HEAD\", head_sha1))\n \t\tinitial_commit = 1;\n+\telse\n+\t\thead_commit = lookup_expect_commit(head_sha1, \"HEAD\");\n+\n \n \t/* Sanity check options */\n \tif (amend && initial_commit)\n@@ -1421,23 +1422,20 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treflog_msg = \"commit (initial)\";\n \t} else if (amend) {\n \t\tstruct commit_list *c;\n-\t\tstruct commit *commit;\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (amend)\";\n-\t\tcommit = lookup_commit(head_sha1);\n-\t\tif (!commit || parse_commit(commit))\n-\t\t\tdie(_(\"could not parse HEAD commit\"));\n \n-\t\tfor (c = commit->parents; c; c = c->next)\n+\t\tfor (c = head_commit->parents; c; c = c->next)\n \t\t\tpptr = &commit_list_insert(c->item, pptr)->next;\n \t} else if (whence == FROM_MERGE) {\n \t\tstruct strbuf m = STRBUF_INIT;\n+\t\tstruct commit *commit;\n \t\tFILE *fp;\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (merge)\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tpptr = &commit_list_insert(head_commit, pptr)->next;\n \t\tfp = fopen(git_path(\"MERGE_HEAD\"), \"r\");\n \t\tif (fp == NULL)\n \t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n@@ -1446,7 +1444,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\tunsigned char sha1[20];\n \t\t\tif (get_sha1_hex(m.buf, sha1) < 0)\n \t\t\t\tdie(_(\"Corrupt MERGE_HEAD file (%s)\"), m.buf);\n-\t\t\tpptr = &commit_list_insert(lookup_commit(sha1), pptr)->next;\n+\t\t\tcommit = lookup_expect_commit(sha1, \"MERGE_HEAD\");\n+\t\t\tpptr = &commit_list_insert(commit, pptr)->next;\n \t\t}\n \t\tfclose(fp);\n \t\tstrbuf_release(&m);\n@@ -1463,7 +1462,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n \t\t\t\t\t? \"commit (cherry-pick)\"\n \t\t\t\t\t: \"commit\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tpptr = &commit_list_insert(head_commit, pptr)->next;\n \t}\n \n \t/* Finally, get the commit message */\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 7581632..1a31b64 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -293,7 +293,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,\n \t\tstruct commit *head;\n \t\tstruct rev_info rev;\n \n-\t\thead = lookup_commit(head_sha1);\n+\t\thead = lookup_expect_commit(head_sha1, \"HEAD\");\n \t\tinit_revisions(&rev, NULL);\n \t\trev.commit_format = CMIT_FMT_ONELINE;\n \t\trev.ignore_merges = 1;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 325891e..22e98e8 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -51,6 +51,7 @@ static int allow_trivial = 1, have_message;\n static struct strbuf merge_msg;\n static struct commit_list *remoteheads;\n static unsigned char head[20], stash[20];\n+static struct commit* head_commit;\n static struct strategy **use_strategies;\n static size_t use_strategies_nr, use_strategies_alloc;\n static const char **xopts;\n@@ -326,7 +327,7 @@ static void squash_message(void)\n \trev.ignore_merges = 1;\n \trev.commit_format = CMIT_FMT_MEDIUM;\n \n-\tcommit = lookup_commit(head);\n+\tcommit = head_commit;\n \tcommit->object.flags |= UNINTERESTING;\n \tadd_pending_object(&rev, &commit->object, NULL);\n \n@@ -709,7 +710,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\tcommit_list_insert(j->item, &reversed);\n \n \t\tindex_fd = hold_locked_index(lock, 1);\n-\t\tclean = merge_recursive(&o, lookup_commit(head),\n+\t\tclean = merge_recursive(&o, head_commit,\n \t\t\t\tremoteheads->item, reversed, &result);\n \t\tif (active_cache_changed &&\n \t\t\t\t(write_cache(index_fd, active_cache, active_nr) ||\n@@ -867,7 +868,7 @@ static int merge_trivial(void)\n \n \twrite_tree_trivial(result_tree);\n \tprintf(_(\"Wonderful.\\n\"));\n-\tparent->item = lookup_commit(head);\n+\tparent->item = head_commit;\n \tparent->next = xmalloc(sizeof(*parent->next));\n \tparent->next->item = remoteheads->item;\n \tparent->next->next = NULL;\n@@ -889,12 +890,12 @@ static int finish_automerge(struct commit_list *common,\n \tfree_commit_list(common);\n \tif (allow_fast_forward) {\n \t\tparents = remoteheads;\n-\t\tcommit_list_insert(lookup_commit(head), &parents);\n+\t\tcommit_list_insert(head_commit, &parents);\n \t\tparents = reduce_heads(parents);\n \t} else {\n \t\tstruct commit_list **pptr = &parents;\n \n-\t\tpptr = &commit_list_insert(lookup_commit(head),\n+\t\tpptr = &commit_list_insert(head_commit,\n \t\t\t\tpptr)->next;\n \t\tfor (j = remoteheads; j; j = j->next)\n \t\t\tpptr = &commit_list_insert(j->item, pptr)->next;\n@@ -1030,6 +1031,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tbranch += 11;\n \tif (is_null_sha1(head))\n \t\thead_invalid = 1;\n+\telse\n+\t\thead_commit = lookup_expect_commit(head, \"HEAD\");\n \n \tgit_config(git_merge_config, NULL);\n \n@@ -1203,11 +1206,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (!remoteheads->next)\n-\t\tcommon = get_merge_bases(lookup_commit(head),\n+\t\tcommon = get_merge_bases(head_commit,\n \t\t\t\tremoteheads->item, 1);\n \telse {\n \t\tstruct commit_list *list = remoteheads;\n-\t\tcommit_list_insert(lookup_commit(head), &list);\n+\t\tcommit_list_insert(head_commit, &list);\n \t\tcommon = get_octopus_merge_bases(list);\n \t\tfree(list);\n \t}\n@@ -1292,7 +1295,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t * merge_bases again, otherwise \"git merge HEAD^\n \t\t\t * HEAD^^\" would be missed.\n \t\t\t */\n-\t\t\tcommon_one = get_merge_bases(lookup_commit(head),\n+\t\t\tcommon_one = get_merge_bases(head_commit,\n \t\t\t\tj->item, 1);\n \t\t\tif (hashcmp(common_one->item->object.sha1,\n \t\t\t\tj->item->object.sha1)) {\ndiff --git a/commit.c b/commit.c\nindex ac337c7..dc22695 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -39,6 +39,18 @@ struct commit *lookup_commit_reference(const unsigned char *sha1)\n \treturn lookup_commit_reference_gently(sha1, 0);\n }\n \n+struct commit *lookup_expect_commit(const unsigned char *sha1,\n+\t\t\t\t const char *ref_name)\n+{\n+\tstruct commit *c = lookup_commit_reference(sha1);\n+\tif (!c)\n+\t\tdie(_(\"could not parse %s\"), ref_name);\n+\tif (hashcmp(sha1, c->object.sha1))\n+\t\twarning(_(\"%s %s is not a commit!\"),\n+\t\t\tref_name, sha1_to_hex(sha1));\n+\treturn c;\n+}\n+\n struct commit *lookup_commit(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\ndiff --git a/commit.h b/commit.h\nindex a2d571b..f36c913 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -37,6 +37,7 @@ struct commit *lookup_commit_reference(const unsigned char *sha1);\n struct commit *lookup_commit_reference_gently(const unsigned char *sha1,\n \t\t\t\t\t int quiet);\n struct commit *lookup_commit_reference_by_name(const char *name);\n+struct commit *lookup_expect_commit(const unsigned char *sha1, const char *ref_name);\n \n int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size);\n int parse_commit(struct commit *item);\ndiff --git a/http-push.c b/http-push.c\nindex 6e8f6d0..31297af 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1606,10 +1606,10 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)\n \tstrbuf_release(&buffer);\n }\n \n-static int verify_merge_base(unsigned char *head_sha1, unsigned char *branch_sha1)\n+static int verify_merge_base(unsigned char *head_sha1, struct ref *remote)\n {\n-\tstruct commit *head = lookup_commit(head_sha1);\n-\tstruct commit *branch = lookup_commit(branch_sha1);\n+\tstruct commit *head = lookup_expect_commit(head_sha1, \"HEAD\");\n+\tstruct commit *branch = lookup_expect_commit(remote->old_sha1, remote->name);\n \tstruct commit_list *merge_bases = get_merge_bases(head, branch, 1);\n \n \treturn (merge_bases && !merge_bases->next && merge_bases->item == branch);\n@@ -1680,7 +1680,7 @@ static int delete_remote_branch(const char *pattern, int force)\n \t\t\treturn error(\"Remote branch %s resolves to object %s\\nwhich does not exist locally, perhaps you need to fetch?\", remote_ref->name, sha1_to_hex(remote_ref->old_sha1));\n \n \t\t/* Remote branch must be an ancestor of remote HEAD */\n-\t\tif (!verify_merge_base(head_sha1, remote_ref->old_sha1)) {\n+\t\tif (!verify_merge_base(head_sha1, remote_ref)) {\n \t\t\treturn error(\"The branch '%s' is not an ancestor \"\n \t\t\t\t \"of your current HEAD.\\n\"\n \t\t\t\t \"If you are sure you want to delete it,\"\ndiff --git a/revision.c b/revision.c\nindex c46cfaa..f926412 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -986,10 +986,12 @@ static void prepare_show_merge(struct rev_info *revs)\n \tconst char **prune = NULL;\n \tint i, prune_num = 1; /* counting terminating NULL */\n \n-\tif (get_sha1(\"HEAD\", sha1) || !(head = lookup_commit(sha1)))\n+\tif (get_sha1(\"HEAD\", sha1))\n \t\tdie(\"--merge without HEAD?\");\n-\tif (get_sha1(\"MERGE_HEAD\", sha1) || !(other = lookup_commit(sha1)))\n+\thead = lookup_expect_commit(sha1, \"HEAD\");\n+\tif (get_sha1(\"MERGE_HEAD\", sha1))\n \t\tdie(\"--merge without MERGE_HEAD?\");\n+\tother = lookup_expect_commit(sha1, \"MERGE_HEAD\");\n \tadd_pending_object(revs, &head->object, \"HEAD\");\n \tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n \tbases = get_merge_bases(head, other, 1);\n-- \n1.7.4.74.g639db\n"},{"id":"173777","messageId":"7vd3g2a431.fsf@alter.siamese.dyndns.org","threadId":"28105","inReplyTo":"1313674994-22902-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3] Accept tags in HEAD or MERGE_HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-18T18:54:43Z","receivedAt":"2011-08-18T18:54:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> HEAD and MERGE_HEAD (among other branch tips) should never hold a\n> tag. That can only be caused by broken tools and is cumbersome to fix\n> by an end user with:\n>\n> $ git update-ref HEAD $(git rev-parse HEAD^{commit})\n>\n> which may look like a magic to a new person.\n>\n> Be easy, warn users (so broken tools can be fixed) and move on.\n\nWe do not really fix broken tools; we just fix breakages caused by them.\n\n> Be robust, if the given SHA-1 cannot be resolved to a commit object,\n> die (therefore return value is always valid).\n\nMakes sense.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index cb73857..f78b449 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -63,6 +63,7 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n> \"Otherwise, please use 'git reset'\\n\");\n> \n> static unsigned char head_sha1[20];\n> +static struct commit *head_commit;\n\nI was not happy with the file-scope global head_sha1[] already, and this\nmakes me even less happy. Was it too much trouble to keep them local to\ncmd_commit() and pass them around as arguments where necessary? If you\npass around head_commit, is_null_sha1(head_sha1) can be replaced with a\ncheck !head_commit so we may even be able to lose the head_sha1[] global.\n\nAnd actually removing head_sha1[] is a necessary step from the correctness\npoint of view. The repository may have given an object name for a tag in\nhead_sha1[] and lookup_expect_commit() may have peeled it to a commit.\nThe code may want to add the \"HEAD\" as one of the parents of a new commit,\nand head_sha1[] is no longer the place to pick that information up after\nyour fix. The code needs to look at head_commit->object.sha1[] instead.\nLosing the use of head_sha1[] variable and forcing the code to go to\nhead_commit->object.sha1[] reduces the risk of such confusion.\n\n> @@ -1028,6 +1026,9 @@ static int parse_and_validate_options(int argc, const char *argv[],\n> \n> \tif (get_sha1(\"HEAD\", head_sha1))\n> \t\tinitial_commit = 1;\n> +\telse\n> +\t\thead_commit = lookup_expect_commit(head_sha1, \"HEAD\");\n\nIt may be just me, but the name feels a bit funny. The original name was\n\"look up\" (verb) + \"commit\" (direct object of the verb), and what you are\ndoing is lookup_and_expect_commit(), but it is too long.\n\nPerhaps lookup_commit_or_die()?\n\nTo a naïve reader of the caller, the function looks as if it would do\n\n\tstruct commit *lookup_expect_commit(unsigned char *head_sha1, char *name)\n {\n \tstruct commit *c;\n\t\tif (get_sha1(\"HEAD\", head_sha1) ||\n\t !(c = lookup_commit(head_sha1))) die(...);\n\t\treturn c;\n\t}\n\nbut in fact it does not use \"HEAD\" for anything other than error reporting\nand it is the caller's responsibility to supply head_sha1[].\n\nIt is documented in the function declaration by making the memory\nhead_sha1 points at as a \"const\" (i.e. the function can't be calling\nget_sha() on the refname), but it may also need a comment or two there.\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 325891e..22e98e8 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -51,6 +51,7 @@ static int allow_trivial = 1, have_message;\n> static struct strbuf merge_msg;\n> static struct commit_list *remoteheads;\n> static unsigned char head[20], stash[20];\n> +static struct commit* head_commit;\n\nThis is C, not C++; asterisk sticks to the variable, not type.\n\n> @@ -1030,6 +1031,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \t\tbranch += 11;\n> \tif (is_null_sha1(head))\n> \t\thead_invalid = 1;\n> +\telse\n> +\t\thead_commit = lookup_expect_commit(head, \"HEAD\");\n> \n> \tgit_config(git_merge_config, NULL);\n> \n\nThe same comment as head[] vs head_commit->object.sha1[] redundancy\napplies from builtin/commit.c here.\n\nOverall, the changes to buitlin/merge.c look very nice, getting rid of the\nrepeated lookup_commit(head). In a separate patch after this fix gets\nready, we may want to further clean it up so that head_commit being NULL\nmeans head_invalid, losing a redundant variable.\n"},{"id":"173856","messageId":"CACsJy8CWFv3dvhd1rWdgc6i5f8xTz-skHxEvgw=hgVUPUW6Fmw@mail.gmail.com","threadId":"28105","inReplyTo":"7vd3g2a431.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] Accept tags in HEAD or MERGE_HEAD","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-19T12:53:54Z","receivedAt":"2011-08-19T12:53:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/8/19 Junio C Hamano :\n>>  static unsigned char head_sha1[20];\n>> +static struct commit *head_commit;\n>\n> I was not happy with the file-scope global head_sha1[] already, and this\n> makes me even less happy. Was it too much trouble to keep them local to\n> cmd_commit() and pass them around as arguments where necessary?  If you\n> pass around head_commit, is_null_sha1(head_sha1) can be replaced with a\n> check !head_commit so we may even be able to lose the head_sha1[] global.\n>\n> And actually removing head_sha1[] is a necessary step from the correctness\n> point of view. The repository may have given an object name for a tag in\n> head_sha1[] and lookup_expect_commit() may have peeled it to a commit.\n> The code may want to add the \"HEAD\" as one of the parents of a new commit,\n> and head_sha1[] is no longer the place to pick that information up after\n> your fix. The code needs to look at head_commit->object.sha1[] instead.\n> Losing the use of head_sha1[] variable and forcing the code to go to\n> head_commit->object.sha1[] reduces the risk of such confusion.\n\nI actually wanted to get rid of head_sha1 but it made changes bigger.\nBut the patch's become big already. Let me remove head_sha1[] (and\nhead[] in merge.c) globally, then put this fix on top.\n-- \nDuy\n"},{"id":"173861","messageId":"1313765407-29925-1-git-send-email-pclouds@gmail.com","threadId":"28105","inReplyTo":"1313674994-22902-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4 1/4] commit: remove global variable head_sha1[]","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-19T14:50:04Z","receivedAt":"2011-08-19T14:50:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/commit.c | 52 ++++++++++++++++++++++++++--------------------------\n 1 files changed, 26 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex cb73857..c9c4ea5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -62,8 +62,6 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n \"\\n\"\n \"Otherwise, please use 'git reset'\\n\");\n \n-static unsigned char head_sha1[20];\n-\n static const char *use_message_buffer;\n static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n static struct lock_file index_lock; /* real index */\n@@ -296,7 +294,7 @@ static void add_remove_files(struct string_list *list)\n \t}\n }\n \n-static void create_base_index(void)\n+static void create_base_index(const unsigned char *head_sha1)\n {\n \tstruct tree *tree;\n \tstruct unpack_trees_options opts;\n@@ -334,7 +332,8 @@ static void refresh_cache_or_die(int refresh_flags)\n \t\tdie_resolve_conflict(\"commit\");\n }\n \n-static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)\n+static char *prepare_index(int argc, const char **argv, const char *prefix,\n+\t\t\t const unsigned char *head_sha1, int is_status)\n {\n \tint fd;\n \tstruct string_list partial;\n@@ -469,7 +468,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int\n \t\t\t\t\t\t(uintmax_t) getpid()),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \n-\tcreate_base_index();\n+\tcreate_base_index(head_sha1);\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n \n@@ -518,11 +517,8 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int\n \treturn s->commitable;\n }\n \n-static int is_a_merge(const unsigned char *sha1)\n+static int is_a_merge(struct commit *commit)\n {\n-\tstruct commit *commit = lookup_commit(sha1);\n-\tif (!commit || parse_commit(commit))\n-\t\tdie(_(\"could not parse HEAD commit\"));\n \treturn !!(commit->parents && commit->parents->next);\n }\n \n@@ -627,7 +623,7 @@ static char *cut_ident_timestamp_part(char *string)\n }\n \n static int prepare_to_commit(const char *index_file, const char *prefix,\n-\t\t\t struct wt_status *s,\n+\t\t\t struct commit *head_commit, struct wt_status *s,\n \t\t\t struct strbuf *author_ident)\n {\n \tstruct stat statbuf;\n@@ -848,7 +844,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t * empty due to conflict resolution, which the user should okay.\n \t */\n \tif (!commitable && whence != FROM_MERGE && !allow_empty &&\n-\t !(amend && is_a_merge(head_sha1))) {\n+\t !(amend && is_a_merge(head_commit))) {\n \t\trun_status(stdout, index_file, prefix, 0, s);\n \t\tif (amend)\n \t\t\tfputs(_(empty_amend_advice), stderr);\n@@ -1026,9 +1022,6 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tif (!use_editor)\n \t\tsetenv(\"GIT_EDITOR\", \":\", 1);\n \n-\tif (get_sha1(\"HEAD\", head_sha1))\n-\t\tinitial_commit = 1;\n-\n \t/* Sanity check options */\n \tif (amend && initial_commit)\n \t\tdie(_(\"You have nothing to amend.\"));\n@@ -1102,12 +1095,12 @@ static int parse_and_validate_options(int argc, const char *argv[],\n }\n \n static int dry_run_commit(int argc, const char **argv, const char *prefix,\n-\t\t\t struct wt_status *s)\n+\t\t\t const unsigned char *head_sha1, struct wt_status *s)\n {\n \tint commitable;\n \tconst char *index_file;\n \n-\tindex_file = prepare_index(argc, argv, prefix, 1);\n+\tindex_file = prepare_index(argc, argv, prefix, head_sha1, 1);\n \tcommitable = run_status(stdout, index_file, prefix, 0, s);\n \trollback_index_files();\n \n@@ -1383,11 +1376,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tconst char *index_file, *reflog_msg;\n \tchar *nl, *p;\n \tunsigned char commit_sha1[20];\n+\tunsigned char head_sha1[20];\n \tstruct ref_lock *ref_lock;\n \tstruct commit_list *parents = NULL, **pptr = &parents;\n \tstruct stat statbuf;\n \tint allow_fast_forward = 1;\n \tstruct wt_status s;\n+\tstruct commit *head_commit;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n@@ -1396,6 +1391,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tgit_config(git_commit_config, &s);\n \tdetermine_whence(&s);\n \n+\tif (get_sha1(\"HEAD\", head_sha1))\n+\t\tinitial_commit = 1;\n+\telse {\n+\t\thead_commit = lookup_commit(head_sha1);\n+\t\tif (!head_commit || parse_commit(head_commit))\n+\t\t\tdie(_(\"could not parse HEAD commit\"));\n+\t}\n+\n \tif (s.use_color == -1)\n \t\ts.use_color = git_use_color_default;\n \targc = parse_and_validate_options(argc, argv, builtin_commit_usage,\n@@ -1403,13 +1406,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tif (dry_run) {\n \t\tif (diff_use_color_default == -1)\n \t\t\tdiff_use_color_default = git_use_color_default;\n-\t\treturn dry_run_commit(argc, argv, prefix, &s);\n+\t\treturn dry_run_commit(argc, argv, prefix, head_sha1, &s);\n \t}\n-\tindex_file = prepare_index(argc, argv, prefix, 0);\n+\tindex_file = prepare_index(argc, argv, prefix, head_sha1, 0);\n \n \t/* Set up everything for writing the commit object. This includes\n \t running hooks, writing the trees, and interacting with the user. */\n-\tif (!prepare_to_commit(index_file, prefix, &s, &author_ident)) {\n+\tif (!prepare_to_commit(index_file, prefix, head_commit,\n+\t\t\t &s, &author_ident)) {\n \t\trollback_index_files();\n \t\treturn 1;\n \t}\n@@ -1421,15 +1425,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treflog_msg = \"commit (initial)\";\n \t} else if (amend) {\n \t\tstruct commit_list *c;\n-\t\tstruct commit *commit;\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (amend)\";\n-\t\tcommit = lookup_commit(head_sha1);\n-\t\tif (!commit || parse_commit(commit))\n-\t\t\tdie(_(\"could not parse HEAD commit\"));\n \n-\t\tfor (c = commit->parents; c; c = c->next)\n+\t\tfor (c = head_commit->parents; c; c = c->next)\n \t\t\tpptr = &commit_list_insert(c->item, pptr)->next;\n \t} else if (whence == FROM_MERGE) {\n \t\tstruct strbuf m = STRBUF_INIT;\n@@ -1437,7 +1437,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (merge)\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tpptr = &commit_list_insert(head_commit, pptr)->next;\n \t\tfp = fopen(git_path(\"MERGE_HEAD\"), \"r\");\n \t\tif (fp == NULL)\n \t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n@@ -1463,7 +1463,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n \t\t\t\t\t? \"commit (cherry-pick)\"\n \t\t\t\t\t: \"commit\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tpptr = &commit_list_insert(head_commit, pptr)->next;\n \t}\n \n \t/* Finally, get the commit message */\n-- \n1.7.4.74.g639db\n"},{"id":"173862","messageId":"1313765407-29925-2-git-send-email-pclouds@gmail.com","threadId":"28105","inReplyTo":"1313765407-29925-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4 2/4] merge: keep stash[] a local variable","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-19T14:50:05Z","receivedAt":"2011-08-19T14:50:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A stash is created by save_state() and used by restore_state(). Pass\nSHA-1 explicitly for clarity and keep stash[] to cmd_merge().\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/merge.c | 33 ++++++++++++++++-----------------\n 1 files changed, 16 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 325891e..a068660 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -50,7 +50,7 @@ static int fast_forward_only;\n static int allow_trivial = 1, have_message;\n static struct strbuf merge_msg;\n static struct commit_list *remoteheads;\n-static unsigned char head[20], stash[20];\n+static unsigned char head[20];\n static struct strategy **use_strategies;\n static size_t use_strategies_nr, use_strategies_alloc;\n static const char **xopts;\n@@ -217,7 +217,7 @@ static void drop_save(void)\n \tunlink(git_path(\"MERGE_MODE\"));\n }\n \n-static void save_state(void)\n+static int save_state(unsigned char *stash)\n {\n \tint len;\n \tstruct child_process cp;\n@@ -236,11 +236,12 @@ static void save_state(void)\n \n \tif (finish_command(&cp) || len < 0)\n \t\tdie(_(\"stash failed\"));\n-\telse if (!len)\n-\t\treturn;\n+\telse if (!len)\t\t/* no changes */\n+\t\treturn -1;\n \tstrbuf_setlen(&buffer, buffer.len-1);\n \tif (get_sha1(buffer.buf, stash))\n \t\tdie(_(\"not a valid object: %s\"), buffer.buf);\n+\treturn 0;\n }\n \n static void read_empty(unsigned const char *sha1, int verbose)\n@@ -278,7 +279,7 @@ static void reset_hard(unsigned const char *sha1, int verbose)\n \t\tdie(_(\"read-tree failed\"));\n }\n \n-static void restore_state(void)\n+static void restore_state(const unsigned char *stash)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *args[] = { \"stash\", \"apply\", NULL, NULL };\n@@ -1010,6 +1011,7 @@ static int setup_with_upstream(const char ***argv)\n int cmd_merge(int argc, const char **argv, const char *prefix)\n {\n \tunsigned char result_tree[20];\n+\tunsigned char stash[20];\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *head_arg;\n \tint flag, head_invalid = 0, i;\n@@ -1320,21 +1322,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * sync with the head commit. The strategies are responsible\n \t * to ensure this.\n \t */\n-\tif (use_strategies_nr != 1) {\n-\t\t/*\n-\t\t * Stash away the local changes so that we can try more\n-\t\t * than one.\n-\t\t */\n-\t\tsave_state();\n-\t} else {\n-\t\tmemcpy(stash, null_sha1, 20);\n-\t}\n+\tif (use_strategies_nr == 1 ||\n+\t /*\n+\t * Stash away the local changes so that we can try more than one.\n+\t */\n+\t save_state(stash))\n+\t\thashcpy(stash, null_sha1);\n \n \tfor (i = 0; i < use_strategies_nr; i++) {\n \t\tint ret;\n \t\tif (i) {\n \t\t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n-\t\t\trestore_state();\n+\t\t\trestore_state(stash);\n \t\t}\n \t\tif (use_strategies_nr != 1)\n \t\t\tprintf(_(\"Trying merge strategy %s...\\n\"),\n@@ -1395,7 +1394,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * it up.\n \t */\n \tif (!best_strategy) {\n-\t\trestore_state();\n+\t\trestore_state(stash);\n \t\tif (use_strategies_nr > 1)\n \t\t\tfprintf(stderr,\n \t\t\t\t_(\"No merge strategy handled the merge.\\n\"));\n@@ -1407,7 +1406,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t; /* We already have its result in the working tree. */\n \telse {\n \t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n-\t\trestore_state();\n+\t\trestore_state(stash);\n \t\tprintf(_(\"Using the %s to prepare resolving by hand.\\n\"),\n \t\t\tbest_strategy);\n \t\ttry_merge_strategy(best_strategy, common, head_arg);\n-- \n1.7.4.74.g639db\n"},{"id":"173863","messageId":"1313765407-29925-3-git-send-email-pclouds@gmail.com","threadId":"28105","inReplyTo":"1313765407-29925-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4 3/4] merge: remove global variable head[]","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-19T14:50:06Z","receivedAt":"2011-08-19T14:50:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/merge.c | 80 +++++++++++++++++++++++++++++-------------------------\n 1 files changed, 43 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a068660..b7260f5 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -50,7 +50,6 @@ static int fast_forward_only;\n static int allow_trivial = 1, have_message;\n static struct strbuf merge_msg;\n static struct commit_list *remoteheads;\n-static unsigned char head[20];\n static struct strategy **use_strategies;\n static size_t use_strategies_nr, use_strategies_alloc;\n static const char **xopts;\n@@ -279,7 +278,8 @@ static void reset_hard(unsigned const char *sha1, int verbose)\n \t\tdie(_(\"read-tree failed\"));\n }\n \n-static void restore_state(const unsigned char *stash)\n+static void restore_state(const unsigned char *head,\n+\t\t\t const unsigned char *stash)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *args[] = { \"stash\", \"apply\", NULL, NULL };\n@@ -309,10 +309,9 @@ static void finish_up_to_date(const char *msg)\n \tdrop_save();\n }\n \n-static void squash_message(void)\n+static void squash_message(struct commit *commit)\n {\n \tstruct rev_info rev;\n-\tstruct commit *commit;\n \tstruct strbuf out = STRBUF_INIT;\n \tstruct commit_list *j;\n \tint fd;\n@@ -327,7 +326,6 @@ static void squash_message(void)\n \trev.ignore_merges = 1;\n \trev.commit_format = CMIT_FMT_MEDIUM;\n \n-\tcommit = lookup_commit(head);\n \tcommit->object.flags |= UNINTERESTING;\n \tadd_pending_object(&rev, &commit->object, NULL);\n \n@@ -356,9 +354,11 @@ static void squash_message(void)\n \tstrbuf_release(&out);\n }\n \n-static void finish(const unsigned char *new_head, const char *msg)\n+static void finish(struct commit *head_commit,\n+\t\t const unsigned char *new_head, const char *msg)\n {\n \tstruct strbuf reflog_message = STRBUF_INIT;\n+\tconst unsigned char *head = head_commit->object.sha1;\n \n \tif (!msg)\n \t\tstrbuf_addstr(&reflog_message, getenv(\"GIT_REFLOG_ACTION\"));\n@@ -369,7 +369,7 @@ static void finish(const unsigned char *new_head, const char *msg)\n \t\t\tgetenv(\"GIT_REFLOG_ACTION\"), msg);\n \t}\n \tif (squash) {\n-\t\tsquash_message();\n+\t\tsquash_message(head_commit);\n \t} else {\n \t\tif (verbosity >= 0 && !merge_msg.len)\n \t\t\tprintf(_(\"No merge message -- not updating HEAD\\n\"));\n@@ -664,7 +664,7 @@ int try_merge_command(const char *strategy, size_t xopts_nr,\n }\n \n static int try_merge_strategy(const char *strategy, struct commit_list *common,\n-\t\t\t const char *head_arg)\n+\t\t\t struct commit *head, const char *head_arg)\n {\n \tint index_fd;\n \tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n@@ -710,7 +710,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\tcommit_list_insert(j->item, &reversed);\n \n \t\tindex_fd = hold_locked_index(lock, 1);\n-\t\tclean = merge_recursive(&o, lookup_commit(head),\n+\t\tclean = merge_recursive(&o, head,\n \t\t\t\tremoteheads->item, reversed, &result);\n \t\tif (active_cache_changed &&\n \t\t\t\t(write_cache(index_fd, active_cache, active_nr) ||\n@@ -861,25 +861,26 @@ static void run_prepare_commit_msg(void)\n \tread_merge_msg();\n }\n \n-static int merge_trivial(void)\n+static int merge_trivial(struct commit *head)\n {\n \tunsigned char result_tree[20], result_commit[20];\n \tstruct commit_list *parent = xmalloc(sizeof(*parent));\n \n \twrite_tree_trivial(result_tree);\n \tprintf(_(\"Wonderful.\\n\"));\n-\tparent->item = lookup_commit(head);\n+\tparent->item = head;\n \tparent->next = xmalloc(sizeof(*parent->next));\n \tparent->next->item = remoteheads->item;\n \tparent->next->next = NULL;\n \trun_prepare_commit_msg();\n \tcommit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);\n-\tfinish(result_commit, \"In-index merge\");\n+\tfinish(head, result_commit, \"In-index merge\");\n \tdrop_save();\n \treturn 0;\n }\n \n-static int finish_automerge(struct commit_list *common,\n+static int finish_automerge(struct commit *head,\n+\t\t\t struct commit_list *common,\n \t\t\t unsigned char *result_tree,\n \t\t\t const char *wt_strategy)\n {\n@@ -890,12 +891,12 @@ static int finish_automerge(struct commit_list *common,\n \tfree_commit_list(common);\n \tif (allow_fast_forward) {\n \t\tparents = remoteheads;\n-\t\tcommit_list_insert(lookup_commit(head), &parents);\n+\t\tcommit_list_insert(head, &parents);\n \t\tparents = reduce_heads(parents);\n \t} else {\n \t\tstruct commit_list **pptr = &parents;\n \n-\t\tpptr = &commit_list_insert(lookup_commit(head),\n+\t\tpptr = &commit_list_insert(head,\n \t\t\t\tpptr)->next;\n \t\tfor (j = remoteheads; j; j = j->next)\n \t\t\tpptr = &commit_list_insert(j->item, pptr)->next;\n@@ -905,7 +906,7 @@ static int finish_automerge(struct commit_list *common,\n \trun_prepare_commit_msg();\n \tcommit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);\n \tstrbuf_addf(&buf, \"Merge made by %s.\", wt_strategy);\n-\tfinish(result_commit, buf.buf);\n+\tfinish(head, result_commit, buf.buf);\n \tstrbuf_release(&buf);\n \tdrop_save();\n \treturn 0;\n@@ -939,7 +940,8 @@ static int suggest_conflicts(int renormalizing)\n \treturn 1;\n }\n \n-static struct commit *is_old_style_invocation(int argc, const char **argv)\n+static struct commit *is_old_style_invocation(int argc, const char **argv,\n+\t\t\t\t\t const unsigned char *head)\n {\n \tstruct commit *second_token = NULL;\n \tif (argc > 2) {\n@@ -1012,9 +1014,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n {\n \tunsigned char result_tree[20];\n \tunsigned char stash[20];\n+\tunsigned char head[20];\n+\tstruct commit *head_commit = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *head_arg;\n-\tint flag, head_invalid = 0, i;\n+\tint flag, i;\n \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n@@ -1030,8 +1034,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tbranch = resolve_ref(\"HEAD\", head, 0, &flag);\n \tif (branch && !prefixcmp(branch, \"refs/heads/\"))\n \t\tbranch += 11;\n-\tif (is_null_sha1(head))\n-\t\thead_invalid = 1;\n+\tif (!is_null_sha1(head)) {\n+\t\thead_commit = lookup_commit(head);\n+\t\tif (!head_commit)\n+\t\t\tdie(_(\"could not parse HEAD\"));\n+\t}\n \n \tgit_config(git_merge_config, NULL);\n \n@@ -1112,12 +1119,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * additional safety measure to check for it.\n \t */\n \n-\tif (!have_message && is_old_style_invocation(argc, argv)) {\n+\tif (!have_message && is_old_style_invocation(argc, argv, head)) {\n \t\tstrbuf_addstr(&merge_msg, argv[0]);\n \t\thead_arg = argv[1];\n \t\targv += 2;\n \t\targc -= 2;\n-\t} else if (head_invalid) {\n+\t} else if (!head_commit) {\n \t\tstruct object *remote_head;\n \t\t/*\n \t\t * If the merged head is a valid one there is no reason\n@@ -1164,7 +1171,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (head_invalid || !argc)\n+\tif (!head_commit || !argc)\n \t\tusage_with_options(builtin_merge_usage,\n \t\t\tbuiltin_merge_options);\n \n@@ -1205,11 +1212,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (!remoteheads->next)\n-\t\tcommon = get_merge_bases(lookup_commit(head),\n-\t\t\t\tremoteheads->item, 1);\n+\t\tcommon = get_merge_bases(head_commit, remoteheads->item, 1);\n \telse {\n \t\tstruct commit_list *list = remoteheads;\n-\t\tcommit_list_insert(lookup_commit(head), &list);\n+\t\tcommit_list_insert(head_commit, &list);\n \t\tcommon = get_octopus_merge_bases(list);\n \t\tfree(list);\n \t}\n@@ -1254,7 +1260,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tif (checkout_fast_forward(head, remoteheads->item->object.sha1))\n \t\t\treturn 1;\n \n-\t\tfinish(o->sha1, msg.buf);\n+\t\tfinish(head_commit, o->sha1, msg.buf);\n \t\tdrop_save();\n \t\treturn 0;\n \t} else if (!remoteheads->next && common->next)\n@@ -1275,7 +1281,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n \t\t\tif (!read_tree_trivial(common->item->object.sha1,\n \t\t\t\t\thead, remoteheads->item->object.sha1))\n-\t\t\t\treturn merge_trivial();\n+\t\t\t\treturn merge_trivial(head_commit);\n \t\t\tprintf(_(\"Nope.\\n\"));\n \t\t}\n \t} else {\n@@ -1294,8 +1300,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t * merge_bases again, otherwise \"git merge HEAD^\n \t\t\t * HEAD^^\" would be missed.\n \t\t\t */\n-\t\t\tcommon_one = get_merge_bases(lookup_commit(head),\n-\t\t\t\tj->item, 1);\n+\t\t\tcommon_one = get_merge_bases(head_commit, j->item, 1);\n \t\t\tif (hashcmp(common_one->item->object.sha1,\n \t\t\t\tj->item->object.sha1)) {\n \t\t\t\tup_to_date = 0;\n@@ -1333,7 +1338,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tint ret;\n \t\tif (i) {\n \t\t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n-\t\t\trestore_state(stash);\n+\t\t\trestore_state(head, stash);\n \t\t}\n \t\tif (use_strategies_nr != 1)\n \t\t\tprintf(_(\"Trying merge strategy %s...\\n\"),\n@@ -1345,7 +1350,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\twt_strategy = use_strategies[i]->name;\n \n \t\tret = try_merge_strategy(use_strategies[i]->name,\n-\t\t\tcommon, head_arg);\n+\t\t\t\t\t common, head_commit, head_arg);\n \t\tif (!option_commit && !ret) {\n \t\t\tmerge_was_ok = 1;\n \t\t\t/*\n@@ -1387,14 +1392,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * auto resolved the merge cleanly.\n \t */\n \tif (automerge_was_ok)\n-\t\treturn finish_automerge(common, result_tree, wt_strategy);\n+\t\treturn finish_automerge(head_commit, common, result_tree,\n+\t\t\t\t\twt_strategy);\n \n \t/*\n \t * Pick the result from the best strategy and have the user fix\n \t * it up.\n \t */\n \tif (!best_strategy) {\n-\t\trestore_state(stash);\n+\t\trestore_state(head, stash);\n \t\tif (use_strategies_nr > 1)\n \t\t\tfprintf(stderr,\n \t\t\t\t_(\"No merge strategy handled the merge.\\n\"));\n@@ -1406,14 +1412,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t; /* We already have its result in the working tree. */\n \telse {\n \t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n-\t\trestore_state(stash);\n+\t\trestore_state(head, stash);\n \t\tprintf(_(\"Using the %s to prepare resolving by hand.\\n\"),\n \t\t\tbest_strategy);\n-\t\ttry_merge_strategy(best_strategy, common, head_arg);\n+\t\ttry_merge_strategy(best_strategy, common, head_commit, head_arg);\n \t}\n \n \tif (squash)\n-\t\tfinish(NULL, NULL);\n+\t\tfinish(head_commit, NULL, NULL);\n \telse {\n \t\tint fd;\n \t\tstruct commit_list *j;\n-- \n1.7.4.74.g639db\n"},{"id":"173864","messageId":"1313765407-29925-4-git-send-email-pclouds@gmail.com","threadId":"28105","inReplyTo":"1313765407-29925-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4 4/4] Accept tags in HEAD or MERGE_HEAD","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-19T14:50:07Z","receivedAt":"2011-08-19T14:50:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"HEAD and MERGE_HEAD (among other branch tips) should never hold a\ntag. That can only be caused by broken tools and is cumbersome to fix\nby an end user with:\n\n $ git update-ref HEAD $(git rev-parse HEAD^{commit})\n\nwhich may look like a magic to a new person.\n\nBe easy, warn users (so broken tools can be fixed if they bother to\nreport) and move on.\n\nBe robust, if the given SHA-1 cannot be resolved to a commit object,\ndie (therefore return value is always valid).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Junio's point, that if HEAD holds a tag, then head_sha1 and head->object.sha1\n in statement [1] are different, is entirely correct. However, favoring\n head->object.sha1 over head_sha1 is not enough. The variable head_sha1 is\n still there. Somewhere, some time, people may misuse it.\n\n [1] head = lookup_commit_or_die(head_sha1, \"HEAD\");\n\n Better update head_sha1 to new value in this case, which is the new change\n in lookup_commit_or_die().\n\n Or maybe a better approach is\n\n int get_commit_sha1(const char *ref, unsigned char *sha1);\n \n where it only returns zero if it can resolve to SHA-1 of a commit.\n\n builtin/commit.c | 11 +++++------\n builtin/fmt-merge-msg.c | 2 +-\n builtin/merge.c | 7 ++-----\n commit.c | 19 +++++++++++++++++++\n commit.h | 1 +\n http-push.c | 8 ++++----\n revision.c | 6 ++++--\n 7 files changed, 36 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex c9c4ea5..72e2cc5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1393,11 +1393,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tif (get_sha1(\"HEAD\", head_sha1))\n \t\tinitial_commit = 1;\n-\telse {\n-\t\thead_commit = lookup_commit(head_sha1);\n-\t\tif (!head_commit || parse_commit(head_commit))\n-\t\t\tdie(_(\"could not parse HEAD commit\"));\n-\t}\n+\telse\n+\t\thead_commit = lookup_commit_or_die(head_sha1, \"HEAD\");\n \n \tif (s.use_color == -1)\n \t\ts.use_color = git_use_color_default;\n@@ -1433,6 +1430,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\tpptr = &commit_list_insert(c->item, pptr)->next;\n \t} else if (whence == FROM_MERGE) {\n \t\tstruct strbuf m = STRBUF_INIT;\n+\t\tstruct commit *commit;\n \t\tFILE *fp;\n \n \t\tif (!reflog_msg)\n@@ -1446,7 +1444,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\tunsigned char sha1[20];\n \t\t\tif (get_sha1_hex(m.buf, sha1) < 0)\n \t\t\t\tdie(_(\"Corrupt MERGE_HEAD file (%s)\"), m.buf);\n-\t\t\tpptr = &commit_list_insert(lookup_commit(sha1), pptr)->next;\n+\t\t\tcommit = lookup_commit_or_die(sha1, \"MERGE_HEAD\");\n+\t\t\tpptr = &commit_list_insert(commit, pptr)->next;\n \t\t}\n \t\tfclose(fp);\n \t\tstrbuf_release(&m);\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 7581632..7e2f225 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -293,7 +293,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,\n \t\tstruct commit *head;\n \t\tstruct rev_info rev;\n \n-\t\thead = lookup_commit(head_sha1);\n+\t\thead = lookup_commit_or_die(head_sha1, \"HEAD\");\n \t\tinit_revisions(&rev, NULL);\n \t\trev.commit_format = CMIT_FMT_ONELINE;\n \t\trev.ignore_merges = 1;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b7260f5..39d9ac8 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1034,11 +1034,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tbranch = resolve_ref(\"HEAD\", head, 0, &flag);\n \tif (branch && !prefixcmp(branch, \"refs/heads/\"))\n \t\tbranch += 11;\n-\tif (!is_null_sha1(head)) {\n-\t\thead_commit = lookup_commit(head);\n-\t\tif (!head_commit)\n-\t\t\tdie(_(\"could not parse HEAD\"));\n-\t}\n+\tif (!is_null_sha1(head))\n+\t\thead_commit = lookup_commit_or_die(head, \"HEAD\");\n \n \tgit_config(git_merge_config, NULL);\n \ndiff --git a/commit.c b/commit.c\nindex ac337c7..9e7f7ef 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -39,6 +39,25 @@ struct commit *lookup_commit_reference(const unsigned char *sha1)\n \treturn lookup_commit_reference_gently(sha1, 0);\n }\n \n+/*\n+ * Look sha1 up for a commit, defer if needed. If deference occurs,\n+ * update \"sha1\" for consistency with retval->object.sha1. Also warn\n+ * users this case because it is expected that sha1 points directly to\n+ * a commit.\n+ */\n+struct commit *lookup_commit_or_die(unsigned char *sha1, const char *ref_name)\n+{\n+\tstruct commit *c = lookup_commit_reference(sha1);\n+\tif (!c)\n+\t\tdie(_(\"could not parse %s\"), ref_name);\n+\tif (hashcmp(sha1, c->object.sha1)) {\n+\t\twarning(_(\"%s %s is not a commit!\"),\n+\t\t\tref_name, sha1_to_hex(sha1));\n+\t\thashcpy(sha1, c->object.sha1);\n+\t}\n+\treturn c;\n+}\n+\n struct commit *lookup_commit(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\ndiff --git a/commit.h b/commit.h\nindex a2d571b..a098b4c 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -37,6 +37,7 @@ struct commit *lookup_commit_reference(const unsigned char *sha1);\n struct commit *lookup_commit_reference_gently(const unsigned char *sha1,\n \t\t\t\t\t int quiet);\n struct commit *lookup_commit_reference_by_name(const char *name);\n+struct commit *lookup_commit_or_die(unsigned char *sha1, const char *ref_name);\n \n int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size);\n int parse_commit(struct commit *item);\ndiff --git a/http-push.c b/http-push.c\nindex 6e8f6d0..7ccff8f 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1606,10 +1606,10 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)\n \tstrbuf_release(&buffer);\n }\n \n-static int verify_merge_base(unsigned char *head_sha1, unsigned char *branch_sha1)\n+static int verify_merge_base(unsigned char *head_sha1, struct ref *remote)\n {\n-\tstruct commit *head = lookup_commit(head_sha1);\n-\tstruct commit *branch = lookup_commit(branch_sha1);\n+\tstruct commit *head = lookup_commit_or_die(head_sha1, \"HEAD\");\n+\tstruct commit *branch = lookup_commit_or_die(remote->old_sha1, remote->name);\n \tstruct commit_list *merge_bases = get_merge_bases(head, branch, 1);\n \n \treturn (merge_bases && !merge_bases->next && merge_bases->item == branch);\n@@ -1680,7 +1680,7 @@ static int delete_remote_branch(const char *pattern, int force)\n \t\t\treturn error(\"Remote branch %s resolves to object %s\\nwhich does not exist locally, perhaps you need to fetch?\", remote_ref->name, sha1_to_hex(remote_ref->old_sha1));\n \n \t\t/* Remote branch must be an ancestor of remote HEAD */\n-\t\tif (!verify_merge_base(head_sha1, remote_ref->old_sha1)) {\n+\t\tif (!verify_merge_base(head_sha1, remote_ref)) {\n \t\t\treturn error(\"The branch '%s' is not an ancestor \"\n \t\t\t\t \"of your current HEAD.\\n\"\n \t\t\t\t \"If you are sure you want to delete it,\"\ndiff --git a/revision.c b/revision.c\nindex c46cfaa..5e057a0 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -986,10 +986,12 @@ static void prepare_show_merge(struct rev_info *revs)\n \tconst char **prune = NULL;\n \tint i, prune_num = 1; /* counting terminating NULL */\n \n-\tif (get_sha1(\"HEAD\", sha1) || !(head = lookup_commit(sha1)))\n+\tif (get_sha1(\"HEAD\", sha1))\n \t\tdie(\"--merge without HEAD?\");\n-\tif (get_sha1(\"MERGE_HEAD\", sha1) || !(other = lookup_commit(sha1)))\n+\thead = lookup_commit_or_die(sha1, \"HEAD\");\n+\tif (get_sha1(\"MERGE_HEAD\", sha1))\n \t\tdie(\"--merge without MERGE_HEAD?\");\n+\tother = lookup_commit_or_die(sha1, \"MERGE_HEAD\");\n \tadd_pending_object(revs, &head->object, \"HEAD\");\n \tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n \tbases = get_merge_bases(head, other, 1);\n-- \n1.7.4.74.g639db\n"},{"id":"173876","messageId":"7vfwkx2pqw.fsf@alter.siamese.dyndns.org","threadId":"28105","inReplyTo":"1313765407-29925-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v4 1/4] commit: remove global variable head_sha1[]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-19T18:57:43Z","receivedAt":"2011-08-19T18:57:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nThanks for a re-roll.\n\n> @@ -1383,11 +1376,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \tconst char *index_file, *reflog_msg;\n> \tchar *nl, *p;\n> \tunsigned char commit_sha1[20];\n> +\tunsigned char head_sha1[20];\n> \tstruct ref_lock *ref_lock;\n> \tstruct commit_list *parents = NULL, **pptr = &parents;\n> \tstruct stat statbuf;\n> \tint allow_fast_forward = 1;\n> \tstruct wt_status s;\n> +\tstruct commit *head_commit;\n\nhead_sha1[] is not initialized to NULs; neither is head_commit to NULL.\n\n\tcmd_commit()\n -> if it is an initial_commit, neither head_sha1[] nor head_commit\n is defined;\n -> prepare_to_commit()\n -> is_a_merge(head_commit) gets called to see if this is an\n\t empty non-merge commit if --amend was passed.\n\nAttempting to --amend an initial commit should be an error and\nparse_and_validate_options() checks that condition so the above is not\npossible, but it still feels wrong.\n\nAlso wouldn't these three be equivalents?\n\n\thead_commit == NULL\n is_null_sha1(head_sha1)\n initial_commit\n\nPerhaps like this instead?\n\n-- >8 --\nSubject: commit: reduce use of redundant global variables\n\nThe file-scope global variable head_sha1[] was used to hold the object\nname of the current HEAD commit (unless we are about to make an initial\ncommit). Also there is an independent \"static int initial_commit\".\n\nFix all the functions on the call-chain that use these two variables to\ntake a new \"(const) struct commit *current_head\" argument instead, and\nreplace their uses, e.g. \"if (initial_commit)\" becomes \"if (!current_head)\"\nand a reference to \"head_sha1\" becomes \"current_head->object.sha1\".\n\nSigned-off-by: Junio C Hamano \n---\n builtin/commit.c | 87 ++++++++++++++++++++++++++++--------------------------\n 1 files changed, 45 insertions(+), 42 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e1af9b1..1a65319 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -62,8 +62,6 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n \"\\n\"\n \"Otherwise, please use 'git reset'\\n\");\n \n-static unsigned char head_sha1[20];\n-\n static const char *use_message_buffer;\n static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n static struct lock_file index_lock; /* real index */\n@@ -102,7 +100,7 @@ static enum {\n static char *cleanup_arg;\n \n static enum commit_whence whence;\n-static int use_editor = 1, initial_commit, include_status = 1;\n+static int use_editor = 1, include_status = 1;\n static int show_ignored_in_status;\n static const char *only_include_assumed;\n static struct strbuf message;\n@@ -294,13 +292,13 @@ static void add_remove_files(struct string_list *list)\n \t}\n }\n \n-static void create_base_index(void)\n+static void create_base_index(const struct commit *current_head)\n {\n \tstruct tree *tree;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t;\n \n-\tif (initial_commit) {\n+\tif (!current_head) {\n \t\tdiscard_cache();\n \t\treturn;\n \t}\n@@ -313,7 +311,7 @@ static void create_base_index(void)\n \topts.dst_index = &the_index;\n \n \topts.fn = oneway_merge;\n-\ttree = parse_tree_indirect(head_sha1);\n+\ttree = parse_tree_indirect(current_head->object.sha1);\n \tif (!tree)\n \t\tdie(_(\"failed to unpack HEAD tree object\"));\n \tparse_tree(tree);\n@@ -332,7 +330,8 @@ static void refresh_cache_or_die(int refresh_flags)\n \t\tdie_resolve_conflict(\"commit\");\n }\n \n-static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)\n+static char *prepare_index(int argc, const char **argv, const char *prefix,\n+\t\t\t const struct commit *current_head, int is_status)\n {\n \tint fd;\n \tstruct string_list partial;\n@@ -448,7 +447,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int\n \n \tmemset(&partial, 0, sizeof(partial));\n \tpartial.strdup_strings = 1;\n-\tif (list_paths(&partial, initial_commit ? NULL : \"HEAD\", prefix, pathspec))\n+\tif (list_paths(&partial, !current_head ? NULL : \"HEAD\", prefix, pathspec))\n \t\texit(1);\n \n \tdiscard_cache();\n@@ -467,7 +466,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int\n \t\t\t\t\t\t(uintmax_t) getpid()),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \n-\tcreate_base_index();\n+\tcreate_base_index(current_head);\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n \n@@ -516,12 +515,9 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int\n \treturn s->commitable;\n }\n \n-static int is_a_merge(const unsigned char *sha1)\n+static int is_a_merge(const struct commit *current_head)\n {\n-\tstruct commit *commit = lookup_commit(sha1);\n-\tif (!commit || parse_commit(commit))\n-\t\tdie(_(\"could not parse HEAD commit\"));\n-\treturn !!(commit->parents && commit->parents->next);\n+\treturn !!(current_head->parents && current_head->parents->next);\n }\n \n static const char sign_off_header[] = \"Signed-off-by: \";\n@@ -625,6 +621,7 @@ static char *cut_ident_timestamp_part(char *string)\n }\n \n static int prepare_to_commit(const char *index_file, const char *prefix,\n+\t\t\t struct commit *current_head,\n \t\t\t struct wt_status *s,\n \t\t\t struct strbuf *author_ident)\n {\n@@ -846,7 +843,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t * empty due to conflict resolution, which the user should okay.\n \t */\n \tif (!commitable && whence != FROM_MERGE && !allow_empty &&\n-\t !(amend && is_a_merge(head_sha1))) {\n+\t !(amend && is_a_merge(current_head))) {\n \t\trun_status(stdout, index_file, prefix, 0, s);\n \t\tif (amend)\n \t\t\tfputs(_(empty_amend_advice), stderr);\n@@ -1004,6 +1001,7 @@ static const char *read_commit_message(const char *name)\n static int parse_and_validate_options(int argc, const char *argv[],\n \t\t\t\t const char * const usage[],\n \t\t\t\t const char *prefix,\n+\t\t\t\t struct commit *current_head,\n \t\t\t\t struct wt_status *s)\n {\n \tint f = 0;\n@@ -1024,11 +1022,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tif (!use_editor)\n \t\tsetenv(\"GIT_EDITOR\", \":\", 1);\n \n-\tif (get_sha1(\"HEAD\", head_sha1))\n-\t\tinitial_commit = 1;\n-\n \t/* Sanity check options */\n-\tif (amend && initial_commit)\n+\tif (amend && !current_head)\n \t\tdie(_(\"You have nothing to amend.\"));\n \tif (amend && whence != FROM_COMMIT)\n \t\tdie(_(\"You are in the middle of a %s -- cannot amend.\"), whence_s());\n@@ -1100,12 +1095,12 @@ static int parse_and_validate_options(int argc, const char *argv[],\n }\n \n static int dry_run_commit(int argc, const char **argv, const char *prefix,\n-\t\t\t struct wt_status *s)\n+\t\t\t const struct commit *current_head, struct wt_status *s)\n {\n \tint commitable;\n \tconst char *index_file;\n \n-\tindex_file = prepare_index(argc, argv, prefix, 1);\n+\tindex_file = prepare_index(argc, argv, prefix, current_head, 1);\n \tcommitable = run_status(stdout, index_file, prefix, 0, s);\n \trollback_index_files();\n \n@@ -1258,7 +1253,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static void print_summary(const char *prefix, const unsigned char *sha1)\n+static void print_summary(const char *prefix, const unsigned char *sha1,\n+\t\t\t int initial_commit)\n {\n \tstruct rev_info rev;\n \tstruct commit *commit;\n@@ -1380,12 +1376,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstruct strbuf author_ident = STRBUF_INIT;\n \tconst char *index_file, *reflog_msg;\n \tchar *nl, *p;\n-\tunsigned char commit_sha1[20];\n+\tunsigned char sha1[20];\n \tstruct ref_lock *ref_lock;\n \tstruct commit_list *parents = NULL, **pptr = &parents;\n \tstruct stat statbuf;\n \tint allow_fast_forward = 1;\n \tstruct wt_status s;\n+\tstruct commit *current_head = NULL;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n@@ -1396,38 +1393,41 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tif (s.use_color == -1)\n \t\ts.use_color = git_use_color_default;\n+\tif (get_sha1(\"HEAD\", sha1))\n+\t\tcurrent_head = NULL;\n+\telse {\n+\t\tcurrent_head = lookup_commit(sha1);\n+\t\tif (!current_head || parse_commit(current_head))\n+\t\t\tdie(_(\"could not parse HEAD commit\"));\n+\t}\n \targc = parse_and_validate_options(argc, argv, builtin_commit_usage,\n-\t\t\t\t\t prefix, &s);\n+\t\t\t\t\t prefix, current_head, &s);\n \tif (dry_run) {\n \t\tif (diff_use_color_default == -1)\n \t\t\tdiff_use_color_default = git_use_color_default;\n-\t\treturn dry_run_commit(argc, argv, prefix, &s);\n+\t\treturn dry_run_commit(argc, argv, prefix, current_head, &s);\n \t}\n-\tindex_file = prepare_index(argc, argv, prefix, 0);\n+\tindex_file = prepare_index(argc, argv, prefix, current_head, 0);\n \n \t/* Set up everything for writing the commit object. This includes\n \t running hooks, writing the trees, and interacting with the user. */\n-\tif (!prepare_to_commit(index_file, prefix, &s, &author_ident)) {\n+\tif (!prepare_to_commit(index_file, prefix,\n+\t\t\t current_head, &s, &author_ident)) {\n \t\trollback_index_files();\n \t\treturn 1;\n \t}\n \n \t/* Determine parents */\n \treflog_msg = getenv(\"GIT_REFLOG_ACTION\");\n-\tif (initial_commit) {\n+\tif (!current_head) {\n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (initial)\";\n \t} else if (amend) {\n \t\tstruct commit_list *c;\n-\t\tstruct commit *commit;\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (amend)\";\n-\t\tcommit = lookup_commit(head_sha1);\n-\t\tif (!commit || parse_commit(commit))\n-\t\t\tdie(_(\"could not parse HEAD commit\"));\n-\n-\t\tfor (c = commit->parents; c; c = c->next)\n+\t\tfor (c = current_head->parents; c; c = c->next)\n \t\t\tpptr = &commit_list_insert(c->item, pptr)->next;\n \t} else if (whence == FROM_MERGE) {\n \t\tstruct strbuf m = STRBUF_INIT;\n@@ -1435,7 +1435,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (merge)\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tpptr = &commit_list_insert(current_head, pptr)->next;\n \t\tfp = fopen(git_path(\"MERGE_HEAD\"), \"r\");\n \t\tif (fp == NULL)\n \t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n@@ -1461,7 +1461,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n \t\t\t\t\t? \"commit (cherry-pick)\"\n \t\t\t\t\t: \"commit\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tpptr = &commit_list_insert(current_head, pptr)->next;\n \t}\n \n \t/* Finally, get the commit message */\n@@ -1487,7 +1487,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\texit(1);\n \t}\n \n-\tif (commit_tree(sb.buf, active_cache_tree->sha1, parents, commit_sha1,\n+\tif (commit_tree(sb.buf, active_cache_tree->sha1, parents, sha1,\n \t\t\tauthor_ident.buf)) {\n \t\trollback_index_files();\n \t\tdie(_(\"failed to write commit object\"));\n@@ -1495,7 +1495,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&author_ident);\n \n \tref_lock = lock_any_ref_for_update(\"HEAD\",\n-\t\t\t\t\t initial_commit ? NULL : head_sha1,\n+\t\t\t\t\t !current_head\n+\t\t\t\t\t ? NULL\n+\t\t\t\t\t : current_head->object.sha1,\n \t\t\t\t\t 0);\n \n \tnl = strchr(sb.buf, '\\n');\n@@ -1510,7 +1512,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\trollback_index_files();\n \t\tdie(_(\"cannot lock HEAD ref\"));\n \t}\n-\tif (write_ref_sha1(ref_lock, commit_sha1, sb.buf) < 0) {\n+\tif (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {\n \t\trollback_index_files();\n \t\tdie(_(\"cannot update HEAD ref\"));\n \t}\n@@ -1532,13 +1534,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tstruct notes_rewrite_cfg *cfg;\n \t\tcfg = init_copy_notes_for_rewrite(\"amend\");\n \t\tif (cfg) {\n-\t\t\tcopy_note_for_rewrite(cfg, head_sha1, commit_sha1);\n+\t\t\t/* we are amending, so current_head is not NULL */\n+\t\t\tcopy_note_for_rewrite(cfg, current_head->object.sha1, sha1);\n \t\t\tfinish_copy_notes_for_rewrite(cfg);\n \t\t}\n-\t\trun_rewrite_hook(head_sha1, commit_sha1);\n+\t\trun_rewrite_hook(current_head->object.sha1, sha1);\n \t}\n \tif (!quiet)\n-\t\tprint_summary(prefix, commit_sha1);\n+\t\tprint_summary(prefix, sha1, !current_head);\n \n \treturn 0;\n }\n"},{"id":"173878","messageId":"7vaab52m2r.fsf@alter.siamese.dyndns.org","threadId":"28105","inReplyTo":"1313765407-29925-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v4 4/4] Accept tags in HEAD or MERGE_HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-19T20:17:00Z","receivedAt":"2011-08-19T20:17:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Junio's point, that if HEAD holds a tag, then head_sha1 and head->object.sha1\n> in statement [1] are different, is entirely correct. However, favoring\n> head->object.sha1 over head_sha1 is not enough. The variable head_sha1 is\n> still there. Somewhere, some time, people may misuse it.\n\nThat is why I suggested _removing_ head_sha1[] altogether, so that there\nis only one source of information. is_initial becomes !current_head and \nhead_sha1 becomes (current_head ? current_head->object.sha1 : null_sha1).\n\n> diff --git a/commit.c b/commit.c\n> index ac337c7..9e7f7ef 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -39,6 +39,25 @@ struct commit *lookup_commit_reference(const unsigned char *sha1)\n> \treturn lookup_commit_reference_gently(sha1, 0);\n> }\n> \n> +/*\n> + * Look sha1 up for a commit, defer if needed. If deference occurs,\n> + * update \"sha1\" for consistency with retval->object.sha1. Also warn\n> + * users this case because it is expected that sha1 points directly to\n> + * a commit.\n> + */\n\nThat's de-reference, not deference ;-). You may want to be more explicit\nabout what kind of de-reference you are talking about.\n\n/*\n * Get a commit object for the given sha1, unwrapping a tag object that\n * point at a commit while at it. ref_name is only used when the result \n * is not a commit in the error message to report where we got the sha1\n * from.\n */\n\nI actually was hoping that you would have this comment in commit.h to help\npeople who want to add callers of this function, not next to the\nimplementation.\n\nAs I said earlier, I do not think updating sha1[] here is necessary. The\ncaller should be updated to use c->object.sha1 instead.\n\n> +struct commit *lookup_commit_or_die(unsigned char *sha1, const char *ref_name)\n> +{\n> +\tstruct commit *c = lookup_commit_reference(sha1);\n> +\tif (!c)\n> +\t\tdie(_(\"could not parse %s\"), ref_name);\n> +\tif (hashcmp(sha1, c->object.sha1)) {\n> +\t\twarning(_(\"%s %s is not a commit!\"),\n> +\t\t\tref_name, sha1_to_hex(sha1));\n> +\t\thashcpy(sha1, c->object.sha1);\n> +\t}\n> +\treturn c;\n> +}\n"},{"id":"173899","messageId":"7v8vqp0zyv.fsf@alter.siamese.dyndns.org","threadId":"28105","inReplyTo":"1313765407-29925-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v4 2/4] merge: keep stash[] a local variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-19T22:59:52Z","receivedAt":"2011-08-19T22:59:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> A stash is created by save_state() and used by restore_state(). Pass\n> SHA-1 explicitly for clarity and keep stash[] to cmd_merge().\n\nMakes tons of sense; thanks.\n"},{"id":"173914","messageId":"CACsJy8DpZoQ1zbArePi_vpLqmQP=vq2ZvGYTX-56_jHwtsJwvg@mail.gmail.com","threadId":"28105","inReplyTo":"7vfwkx2pqw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 1/4] commit: remove global variable head_sha1[]","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-20T12:03:32Z","receivedAt":"2011-08-20T12:03:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/8/20 Junio C Hamano :\n> Also wouldn't these three be equivalents?\n>\n>        head_commit == NULL\n>        is_null_sha1(head_sha1)\n>        initial_commit\n\nRight.\n\n> Perhaps like this instead?\n\nYup. Looks good.\n-- \nDuy\n"},{"id":"173919","messageId":"CACsJy8CLb-PrMdt05HEV-sHWwWbsu9MaC7-khAoT43O8vfkSDg@mail.gmail.com","threadId":"28105","inReplyTo":"7vaab52m2r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 4/4] Accept tags in HEAD or MERGE_HEAD","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-20T16:37:11Z","receivedAt":"2011-08-20T16:37:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/8/20 Junio C Hamano :\n> That's de-reference, not deference ;-). You may want to be more explicit\n> about what kind of de-reference you are talking about.\n>\n> /*\n>  * Get a commit object for the given sha1, unwrapping a tag object that\n>  * point at a commit while at it. ref_name is only used when the result\n>  * is not a commit in the error message to report where we got the sha1\n>  * from.\n>  */\n>\n> I actually was hoping that you would have this comment in commit.h to help\n> people who want to add callers of this function, not next to the\n> implementation.\n\nOK. It's because I tend to go straight to implementation instead of\nthe declaration when I want to know how to use it.\n\n> As I said earlier, I do not think updating sha1[] here is necessary. The\n> caller should be updated to use c->object.sha1 instead.\n\nThe usual pattern is\n\nget_sha1(ref, sha1);\ncommit = lookup_commit_or_die(sha1, ref);\n\nFrom a quick look, it's very easy to assume \"sha1\" is safe to use\nafterwards while it may be different from commit->object.sha1. I'm\ntempted to make lookup_commit_or_die() resolve ref to sha1 internally,\nno temporary sha1 variable will be hanging around, the pattern becomes\n\ncommit = lookup_commit_or_die(ref);\n\nThe only problem is MERGE_HEAD is not usual ref and cannot be treated this way.\n-- \nDuy\n"},{"id":"174110","messageId":"7vty98q82k.fsf@alter.siamese.dyndns.org","threadId":"28105","inReplyTo":"1313765407-29925-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v4 3/4] merge: remove global variable head[]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-23T18:46:59Z","receivedAt":"2011-08-23T18:46:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> @@ -1012,9 +1014,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> {\n> \tunsigned char result_tree[20];\n> \tunsigned char stash[20];\n> +\tunsigned char head[20];\n> +\tstruct commit *head_commit = NULL;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tconst char *head_arg;\n> -\tint flag, head_invalid = 0, i;\n> +\tint flag, i;\n> \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n> \tstruct commit_list *common = NULL;\n> \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n> @@ -1030,8 +1034,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \tbranch = resolve_ref(\"HEAD\", head, 0, &flag);\n> \tif (branch && !prefixcmp(branch, \"refs/heads/\"))\n> \t\tbranch += 11;\n> -\tif (is_null_sha1(head))\n> -\t\thead_invalid = 1;\n> +\tif (!is_null_sha1(head)) {\n> +\t\thead_commit = lookup_commit(head);\n> +\t\tif (!head_commit)\n> +\t\t\tdie(_(\"could not parse HEAD\"));\n> +\t}\n\nIs this is_null_sha1() valid without first clearing head[]?\n\nAlso, would it be too much trouble and code churn to employ the same\nstrategy as my rewrite of your [1/4] and pass only head_commit around in\nthe call chain?\n\nBecause this is the way to set up head_commit in the first place, this\nparticular resolve_ref() -> is_null_sha1() chain is unavoidable and needs\nto be written carefully, but after the\n\n !head_commit === is_null_sha1(head) === is_initial_commit\n\ninvariant is established, I suspect that it would reduce the chance of\nsimilar mistakes in later parts of the code if it can check and use only\none argument.\n"}]}