{"thread":{"id":"24591","subject":"[PATCH RFC] parse_object: pass on the original sha1, not the replaced one","startedAt":"2010-07-31T12:36:42Z","lastAt":"2010-08-14T02:03:32Z","messageCount":14,"participants":["Nguyễn Thái Ngọc Duy","Christian Couder","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"146856","messageId":"1280579802-8606-1-git-send-email-pclouds@gmail.com","threadId":"24591","inReplyTo":null,"subject":"[PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-07-31T12:36:42Z","receivedAt":"2010-07-31T12:36:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit 0e87c36 (object: call \"check_sha1_signature\" with the\nreplacement sha1) did this. I'm not sure if it's should be done this\nway.\n\nWith \"repl\" as the first argument to parse_object_buffer, the returned\nobj pointer will have the replaced SHA1 in obj->sha1, not the original\none. I sort of expect that, no matter the object is replaced,\nobj->sha1 should stay the same.\n\nThis was observed by replacing commit tip. git log would show the\nreplaced sha1, not the original one.\n---\n object.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 277b3dd..7adfda7 100644\n--- a/object.c\n+++ b/object.c\n@@ -199,7 +199,7 @@ struct object *parse_object(const unsigned char *sha1)\n \t\t\treturn NULL;\n \t\t}\n \n-\t\tobj = parse_object_buffer(repl, type, size, buffer, &eaten);\n+\t\tobj = parse_object_buffer(sha1, type, size, buffer, &eaten);\n \t\tif (!eaten)\n \t\t\tfree(buffer);\n \t\treturn obj;\n-- \n1.7.1.rc1.69.g24c2f7\n"},{"id":"146932","messageId":"201008020942.58137.chriscool@tuxfamily.org","threadId":"24591","inReplyTo":"1280579802-8606-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-08-02T07:42:57Z","receivedAt":"2010-08-02T07:42:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Saturday 31 July 2010 14:36:42 Nguyễn Thái Ngọc Duy wrote:\n> Commit 0e87c36 (object: call \"check_sha1_signature\" with the\n> replacement sha1) did this. I'm not sure if it's should be done this\n> way.\n> \n> With \"repl\" as the first argument to parse_object_buffer, the returned\n> obj pointer will have the replaced SHA1 in obj->sha1, not the original\n> one. I sort of expect that, no matter the object is replaced,\n> obj->sha1 should stay the same.\n> \n> This was observed by replacing commit tip. git log would show the\n> replaced sha1, not the original one.\n\nI am not sure I understand what you are saying. Do you mean that git log \nshould show the original sha1 but the content of the replacement commit, \ninstead of both the sha1 and the content of the replacement commit?\n\nI just tested your patch and indeed with it it seems to me that the result \nshown by git log is not consistent, as for example the commit message is the \none of the replacement commit but the commit sha1 is the one of the original \ncommit.\n\nThe idea with replaced object was that as much as possible everything except \nsome very low levels commands and transport commands should use the \nreplacement objects (except if the --no-replace-objects option or \nNO_REPLACE_OBJECTS_ENVIRONMENT is used).\n\nCould you explain why you need the object returned by parse_object_buffer to \nnot have the replacement SHA1 in obj->sha1 when it is parsing the buffer from \nthe replacement object?\n\nBest regards,\nChristian.\n"},{"id":"146933","messageId":"AANLkTin0eymuYFv_6hamDuan0TELogtCyW3Xwjm=pAwH@mail.gmail.com","threadId":"24591","inReplyTo":"201008020942.58137.chriscool@tuxfamily.org","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-08-02T09:31:23Z","receivedAt":"2010-08-02T09:31:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/8/2 Christian Couder <chriscool@tuxfamily.org>:\n> On Saturday 31 July 2010 14:36:42 Nguyễn Thái Ngọc Duy wrote:\n>> Commit 0e87c36 (object: call \"check_sha1_signature\" with the\n>> replacement sha1) did this. I'm not sure if it's should be done this\n>> way.\n>>\n>> With \"repl\" as the first argument to parse_object_buffer, the returned\n>> obj pointer will have the replaced SHA1 in obj->sha1, not the original\n>> one. I sort of expect that, no matter the object is replaced,\n>> obj->sha1 should stay the same.\n>>\n>> This was observed by replacing commit tip. git log would show the\n>> replaced sha1, not the original one.\n>\n> I am not sure I understand what you are saying. Do you mean that git log\n> should show the original sha1 but the content of the replacement commit,\n> instead of both the sha1 and the content of the replacement commit?\n\n\"original sha1, but the content of replacement commit\", yes. Isn't\nthat the intention of git-replace?\n\n> I just tested your patch and indeed with it it seems to me that the result\n> shown by git log is not consistent, as for example the commit message is the\n> one of the replacement commit but the commit sha1 is the one of the original\n> commit.\n\nThe consistency is already there. Suppose I want to replace commit A\nwith B. Depends on what function I call to fetch \"A\", I get different\nobject->sha1. (the content is always from B though).\n\nWith parse_object(A), I get an object pointer whose sha1 is B.\n\nWith  lookup_commit(A), I get an object pointer whose sha1 is A.\n\n> Could you explain why you need the object returned by parse_object_buffer to\n> not have the replacement SHA1 in obj->sha1 when it is parsing the buffer from\n> the replacement object?\n\nAs I said above, it's inconsistent. I'm not saying my way is correct.\nJust wondering.\n-- \nDuy\n"},{"id":"147018","messageId":"201008030700.40533.chriscool@tuxfamily.org","threadId":"24591","inReplyTo":"AANLkTin0eymuYFv_6hamDuan0TELogtCyW3Xwjm=pAwH@mail.gmail.com","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-08-03T05:00:40Z","receivedAt":"2010-08-03T05:00:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Monday 02 August 2010 11:31:23 Nguyen Thai Ngoc Duy wrote:\n> 2010/8/2 Christian Couder <chriscool@tuxfamily.org>:\n> > \n> > I am not sure I understand what you are saying. Do you mean that git log\n> > should show the original sha1 but the content of the replacement commit,\n> > instead of both the sha1 and the content of the replacement commit?\n> \n> \"original sha1, but the content of replacement commit\", yes. Isn't\n> that the intention of git-replace?\n\nI think it's better if the sha1 is replaced too. This way we keep the link \nbetween the sha1 and the content.\n\nAnd it's easier for the user to realize that we gave him a replacement object \nif we use the replacement sha1 as the user can more easily see that the sha1 \nchanged.\n\n> > I just tested your patch and indeed with it it seems to me that the\n> > result shown by git log is not consistent, as for example the commit\n> > message is the one of the replacement commit but the commit sha1 is the\n> > one of the original commit.\n> \n> The consistency is already there. Suppose I want to replace commit A\n> with B. Depends on what function I call to fetch \"A\", I get different\n> object->sha1. (the content is always from B though).\n> \n> With parse_object(A), I get an object pointer whose sha1 is B.\n> \n> With  lookup_commit(A), I get an object pointer whose sha1 is A.\n\nMaybe there is a bug somewhere and you should get an object pointer whose sha1 \nis B or maybe the content of the object that was inserted in the lookup table \nshould have been the content from A and not from B. I will try to have a \ndeeper look at that, but it would help if you could give an example of a \ncommand that triggers this behavior.\n\nBest regards,\nChristian.\n"},{"id":"147019","messageId":"AANLkTikaPhV+SkzCJ+G3V39mS4FORELP34ePrPWbHFeV@mail.gmail.com","threadId":"24591","inReplyTo":"201008030700.40533.chriscool@tuxfamily.org","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-08-03T06:01:32Z","receivedAt":"2010-08-03T06:01:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 3, 2010 at 3:00 PM, Christian Couder\n<chriscool@tuxfamily.org> wrote:\n>> With parse_object(A), I get an object pointer whose sha1 is B.\n>>\n>> With  lookup_commit(A), I get an object pointer whose sha1 is A.\n>\n> Maybe there is a bug somewhere and you should get an object pointer whose sha1\n> is B or maybe the content of the object that was inserted in the lookup table\n> should have been the content from A and not from B. I will try to have a\n> deeper look at that, but it would help if you could give an example of a\n> command that triggers this behavior.\n\nThe following patch add \"sha1\" command. These commands give different sha1:\n\ngit sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A\ngit sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B\n\n(hopefully gmail won't break the patch, otherwise I'll resend to you privately)\n\ndiff --git a/Makefile b/Makefile\nindex f33648d..8556c65 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -649,6 +649,7 @@ LIB_OBJS += ws.o\n LIB_OBJS += wt-status.o\n LIB_OBJS += xdiff-interface.o\n\n+BUILTIN_OBJS += builtin/sha1.o\n BUILTIN_OBJS += builtin/add.o\n BUILTIN_OBJS += builtin/annotate.o\n BUILTIN_OBJS += builtin/apply.o\ndiff --git a/builtin/sha1.c b/builtin/sha1.c\nnew file mode 100644\nindex 0000000..4700bc0\n--- /dev/null\n+++ b/builtin/sha1.c\n@@ -0,0 +1,21 @@\n+#include \"cache.h\"\n+#include \"commit.h\"\n+\n+int cmd_sha1(int argc, char **argv)\n+{\n+\tunsigned char old[20];\n+\tunsigned char new[20];\n+\tstruct object *obj;\n+\n+\tget_sha1_hex(argv[1], old);\n+\tget_sha1_hex(argv[2], new);\n+\tprintf(\"old  = %s\\nnew  = %s\\n\", argv[1], argv[2]);\n+\treplace_pair(old, new);\n+\tif (argv[3][0] == 'A')\n+\t\tobj = parse_object(old);\n+\telse\n+\t\tobj = lookup_commit(old);\n+\n+\tprintf(\"sha1 = %s\\n\", sha1_to_hex(obj->sha1));\n+\treturn 0;\n+}\ndiff --git a/git.c b/git.c\nindex f37028b..9bc2391 100644\n--- a/git.c\n+++ b/git.c\n@@ -288,6 +288,7 @@ static int run_builtin(struct cmd_struct *p, int\nargc, const char **argv)\n \treturn 0;\n }\n\n+int cmd_sha1(int, const char **, const char*);\n static void handle_internal_command(int argc, const char **argv)\n {\n \tconst char *cmd = argv[0];\n@@ -378,6 +379,7 @@ static void handle_internal_command(int argc,\nconst char **argv)\n \t\t{ \"revert\", cmd_revert, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"rm\", cmd_rm, RUN_SETUP },\n \t\t{ \"send-pack\", cmd_send_pack, RUN_SETUP },\n+\t\t{ \"sha1\", cmd_sha1, RUN_SETUP },\n \t\t{ \"shortlog\", cmd_shortlog, USE_PAGER },\n \t\t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n \t\t{ \"show\", cmd_show, RUN_SETUP | USE_PAGER },\ndiff --git a/replace_object.c b/replace_object.c\nindex eb59604..1ec5df7 100644\n--- a/replace_object.c\n+++ b/replace_object.c\n@@ -50,6 +50,16 @@ static int register_replace_object(struct\nreplace_object *replace,\n \treturn 0;\n }\n\n+int replace_pair(const unsigned char *old,\n+\t\t const unsigned char *new)\n+{\n+\tstruct replace_object *ro = xmalloc(sizeof(*ro));\n+\thashcpy(ro->sha1[0],old);\n+\thashcpy(ro->sha1[1],new);\n+\tregister_replace_object(ro, 1);\n+\treturn 0;\n+}\n+\n static int register_replace_ref(const char *refname,\n \t\t\t\tconst unsigned char *sha1,\n \t\t\t\tint flag, void *cb_data)\n\n-- \nDuy\n"},{"id":"147106","messageId":"201008041358.13081.chriscool@tuxfamily.org","threadId":"24591","inReplyTo":"AANLkTikaPhV+SkzCJ+G3V39mS4FORELP34ePrPWbHFeV@mail.gmail.com","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-08-04T11:58:12Z","receivedAt":"2010-08-04T11:58:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tuesday 03 August 2010 08:01:32 Nguyen Thai Ngoc Duy wrote:\n> On Tue, Aug 3, 2010 at 3:00 PM, Christian Couder\n> \n> <chriscool@tuxfamily.org> wrote:\n> >> With parse_object(A), I get an object pointer whose sha1 is B.\n> >> \n> >> With  lookup_commit(A), I get an object pointer whose sha1 is A.\n> > \n> > Maybe there is a bug somewhere and you should get an object pointer whose\n> > sha1 is B or maybe the content of the object that was inserted in the\n> > lookup table should have been the content from A and not from B.\n\nAfter having another look at that, I think the content of the object in the \nlookup table should be the content of A. It should be a bug if the object \nreturned by lookup_commit(A) does not have the content of A.\n\n> > I will\n> > try to have a deeper look at that, but it would help if you could give\n> > an example of a command that triggers this behavior.\n> \n> The following patch add \"sha1\" command. These commands give different sha1:\n> \n> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A\n> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B\n\nYes, but that does not mean that the content of the object returned by \nlookup_commit(A) is not the content of A.\n\nOr do you have an example where the content of the object returned by \nlookup_commit(A) is not the content of A?\n\nI mean that we  should always be consistent by having objects with an\nobj->sha1 field corresponding to the content.\n\nSo yes we have:\n\n> With parse_object(A), I get an object pointer whose sha1 is B.\n>\n> With  lookup_commit(A), I get an object pointer whose sha1 is A.\n\nbut it's not a problem, it's just the result from the fact that parse_object() \n(completely) replace objects and lookup_commit() does not.\n\nBest regards,\nChristian.\n"},{"id":"147109","messageId":"AANLkTimPQcZN-ASP+N+h9GaOSkgbnyNxiSktL+z-nBRu@mail.gmail.com","threadId":"24591","inReplyTo":"201008041358.13081.chriscool@tuxfamily.org","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-08-04T12:42:50Z","receivedAt":"2010-08-04T12:42:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 4, 2010 at 9:58 PM, Christian Couder\n<chriscool@tuxfamily.org> wrote:\n>> > I will\n>> > try to have a deeper look at that, but it would help if you could give\n>> > an example of a command that triggers this behavior.\n>>\n>> The following patch add \"sha1\" command. These commands give different sha1:\n>>\n>> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A\n>> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B\n>\n> Yes, but that does not mean that the content of the object returned by\n> lookup_commit(A) is not the content of A.\n>\n> Or do you have an example where the content of the object returned by\n> lookup_commit(A) is not the content of A?\n\nBoth return the content of B. I modified my patch a bit to also show\nthe content, ((struct commit*)obj)->buffer.\n\n$ ./git cat-file -p HEAD\ntree 13109340ff2bd55bd16271bbad7a9232f9052923\nparent 9a3028b987d73e4c85e8db9980720feba6a1b5c6\nparent 2aedccd3d51ec922020f7c7e39df5d2c4d3af515\nauthor Junio C Hamano <gitster@pobox.com> 1280274846 -0700\ncommitter Junio C Hamano <gitster@pobox.com> 1280274905 -0700\n\nSync with 1.7.2.1\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n\n$ ./git cat-file -p HEAD^\ntree 4ffcd88bed7a675e2d130a41203a7ebe026b6462\nparent 9a9fb5d3c4c8601beb2d7b8e3b9283c6c3815a2d\nauthor Ævar Arnfjörð Bjarmason <avarab@gmail.com> 1279925924 +0000\ncommitter Junio C Hamano <gitster@pobox.com> 1280124888 -0700\n\ntests: Ignore the Test::Harness .prove file\n\nWe document how to run prove with the --state option in t/README. This\nproduces a .prove YAML file in the current directory. Change the t/\ngitignore to ignore it, and clean it up on `make clean'.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n\n$ ./git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A\nold  = ba9523ea809dc496a14f3644bdc1dd6f486983c0\nnew  = 9a3028b987d73e4c85e8db9980720feba6a1b5c6\nsha1 = 9a3028b987d73e4c85e8db9980720feba6a1b5c6\ntree 4ffcd88bed7a675e2d130a41203a7ebe026b6462\nparent 9a9fb5d3c4c8601beb2d7b8e3b9283c6c3815a2d\nauthor Ævar Arnfjörð Bjarmason <avarab@gmail.com> 1279925924 +0000\ncommitter Junio C Hamano <gitster@pobox.com> 1280124888 -0700\n\ntests: Ignore the Test::Harness .prove file\n\nWe document how to run prove with the --state option in t/README. This\nproduces a .prove YAML file in the current directory. Change the t/\ngitignore to ignore it, and clean it up on `make clean'.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n\n$ ./git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B\nold  = ba9523ea809dc496a14f3644bdc1dd6f486983c0\nnew  = 9a3028b987d73e4c85e8db9980720feba6a1b5c6\nsha1 = ba9523ea809dc496a14f3644bdc1dd6f486983c0\ntree 4ffcd88bed7a675e2d130a41203a7ebe026b6462\nparent 9a9fb5d3c4c8601beb2d7b8e3b9283c6c3815a2d\nauthor Ævar Arnfjörð Bjarmason <avarab@gmail.com> 1279925924 +0000\ncommitter Junio C Hamano <gitster@pobox.com> 1280124888 -0700\n\ntests: Ignore the Test::Harness .prove file\n\nWe document how to run prove with the --state option in t/README. This\nproduces a .prove YAML file in the current directory. Change the t/\ngitignore to ignore it, and clean it up on `make clean'.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n-- \nDuy\n"},{"id":"147110","messageId":"201008041457.31975.chriscool@tuxfamily.org","threadId":"24591","inReplyTo":"AANLkTimPQcZN-ASP+N+h9GaOSkgbnyNxiSktL+z-nBRu@mail.gmail.com","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-08-04T12:57:31Z","receivedAt":"2010-08-04T12:57:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wednesday 04 August 2010 14:42:50 Nguyen Thai Ngoc Duy wrote:\n> On Wed, Aug 4, 2010 at 9:58 PM, Christian Couder\n> \n> <chriscool@tuxfamily.org> wrote:\n> >> > I will\n> >> > try to have a deeper look at that, but it would help if you could give\n> >> > an example of a command that triggers this behavior.\n> >> \n> >> The following patch add \"sha1\" command. These commands give different\n> >> sha1:\n> >> \n> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A\n> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B\n> > \n> > Yes, but that does not mean that the content of the object returned by\n> > lookup_commit(A) is not the content of A.\n> > \n> > Or do you have an example where the content of the object returned by\n> > lookup_commit(A) is not the content of A?\n> \n> Both return the content of B. I modified my patch a bit to also show\n> the content, ((struct commit*)obj)->buffer.\n\nI also modified your patch but I don't get any content shown when using \nlookup_commit()\n\nI use:\n\ndiff --git a/builtin/sha1.c b/builtin/sha1.c\nnew file mode 100644\nindex 0000000..8e081b2\n--- /dev/null\n+++ b/builtin/sha1.c\n@@ -0,0 +1,27 @@\n+#include \"cache.h\"\n+#include \"commit.h\"\n+\n+int cmd_sha1(int argc, char **argv)\n+{\n+       unsigned char old[20];\n+       unsigned char new[20];\n+       struct object *obj;\n+\n+       get_sha1_hex(argv[1], old);\n+       get_sha1_hex(argv[2], new);\n+       printf(\"old  = %s\\nnew  = %s\\n\", argv[1], argv[2]);\n+       replace_pair(old, new);\n+       if (argv[3][0] == 'A')\n+              obj = parse_object(old);\n+       else {\n+              struct commit *com = lookup_commit(old);\n+              if (com->buffer)\n+                      printf(\"commit buffer:\\n%s\", com->buffer);\n+              else\n+                      printf(\"no commit buffer\\n\");\n+              obj = (struct object *)com;\n+       }\n+\n+       printf(\"sha1 = %s\\n\", sha1_to_hex(obj->sha1));\n+       return 0;\n+}\n\nand I get:\n\n$ git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B\nold  = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d\nnew  = c9b402bd93105f80f3c5d67ecfccc8ba36810613\nno commit buffer\nsha1 = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d\n\nCould you show what code you use?\n\nThanks,\nChristian.\n"},{"id":"147137","messageId":"AANLkTimVnAO6kCAo8N6DM9GX3z+DjKJgKizdTnktyb8Q@mail.gmail.com","threadId":"24591","inReplyTo":"201008041457.31975.chriscool@tuxfamily.org","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-08-04T22:07:45Z","receivedAt":"2010-08-04T22:07:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 4, 2010 at 10:57 PM, Christian Couder\n<chriscool@tuxfamily.org> wrote:\n> On Wednesday 04 August 2010 14:42:50 Nguyen Thai Ngoc Duy wrote:\n>> On Wed, Aug 4, 2010 at 9:58 PM, Christian Couder\n>>\n>> <chriscool@tuxfamily.org> wrote:\n>> >> > I will\n>> >> > try to have a deeper look at that, but it would help if you could give\n>> >> > an example of a command that triggers this behavior.\n>> >>\n>> >> The following patch add \"sha1\" command. These commands give different\n>> >> sha1:\n>> >>\n>> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A\n>> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B\n>> >\n>> > Yes, but that does not mean that the content of the object returned by\n>> > lookup_commit(A) is not the content of A.\n>> >\n>> > Or do you have an example where the content of the object returned by\n>> > lookup_commit(A) is not the content of A?\n>>\n>> Both return the content of B. I modified my patch a bit to also show\n>> the content, ((struct commit*)obj)->buffer.\n>\n> I also modified your patch but I don't get any content shown when using\n> lookup_commit()\n>\n> I use:\n>\n> diff --git a/builtin/sha1.c b/builtin/sha1.c\n> new file mode 100644\n> index 0000000..8e081b2\n> --- /dev/null\n> +++ b/builtin/sha1.c\n> @@ -0,0 +1,27 @@\n> +#include \"cache.h\"\n> +#include \"commit.h\"\n> +\n> +int cmd_sha1(int argc, char **argv)\n> +{\n> +       unsigned char old[20];\n> +       unsigned char new[20];\n> +       struct object *obj;\n> +\n> +       get_sha1_hex(argv[1], old);\n> +       get_sha1_hex(argv[2], new);\n> +       printf(\"old  = %s\\nnew  = %s\\n\", argv[1], argv[2]);\n> +       replace_pair(old, new);\n> +       if (argv[3][0] == 'A')\n> +              obj = parse_object(old);\n> +       else {\n> +              struct commit *com = lookup_commit(old);\n> +              if (com->buffer)\n> +                      printf(\"commit buffer:\\n%s\", com->buffer);\n> +              else\n> +                      printf(\"no commit buffer\\n\");\n> +              obj = (struct object *)com;\n> +       }\n> +\n> +       printf(\"sha1 = %s\\n\", sha1_to_hex(obj->sha1));\n> +       return 0;\n> +}\n>\n> and I get:\n>\n> $ git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B\n> old  = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d\n> new  = c9b402bd93105f80f3c5d67ecfccc8ba36810613\n> no commit buffer\n> sha1 = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d\n>\n> Could you show what code you use?\n\nYou need parse_commit() (unless somebody already did that before\nlookup_commit()).\n-- \nDuy\n"},{"id":"147179","messageId":"201008051341.08632.chriscool@tuxfamily.org","threadId":"24591","inReplyTo":"AANLkTimVnAO6kCAo8N6DM9GX3z+DjKJgKizdTnktyb8Q@mail.gmail.com","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-08-05T11:41:08Z","receivedAt":"2010-08-05T11:41:08Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thursday 05 August 2010 00:07:45 Nguyen Thai Ngoc Duy wrote:\n> On Wed, Aug 4, 2010 at 10:57 PM, Christian Couder\n> \n> <chriscool@tuxfamily.org> wrote:\n> > On Wednesday 04 August 2010 14:42:50 Nguyen Thai Ngoc Duy wrote:\n> >> On Wed, Aug 4, 2010 at 9:58 PM, Christian Couder\n> >> \n> >> <chriscool@tuxfamily.org> wrote:\n> >> >> > I will\n> >> >> > try to have a deeper look at that, but it would help if you could\n> >> >> > give an example of a command that triggers this behavior.\n> >> >> \n> >> >> The following patch add \"sha1\" command. These commands give different\n> >> >> sha1:\n> >> >> \n> >> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A\n> >> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B\n> >> > \n> >> > Yes, but that does not mean that the content of the object returned by\n> >> > lookup_commit(A) is not the content of A.\n> >> > \n> >> > Or do you have an example where the content of the object returned by\n> >> > lookup_commit(A) is not the content of A?\n> >> \n> >> Both return the content of B. I modified my patch a bit to also show\n> >> the content, ((struct commit*)obj)->buffer.\n> > \n> > I also modified your patch but I don't get any content shown when using\n> > lookup_commit()\n> > \n> > I use:\n> > \n> > diff --git a/builtin/sha1.c b/builtin/sha1.c\n> > new file mode 100644\n> > index 0000000..8e081b2\n> > --- /dev/null\n> > +++ b/builtin/sha1.c\n> > @@ -0,0 +1,27 @@\n> > +#include \"cache.h\"\n> > +#include \"commit.h\"\n> > +\n> > +int cmd_sha1(int argc, char **argv)\n> > +{\n> > +       unsigned char old[20];\n> > +       unsigned char new[20];\n> > +       struct object *obj;\n> > +\n> > +       get_sha1_hex(argv[1], old);\n> > +       get_sha1_hex(argv[2], new);\n> > +       printf(\"old  = %s\\nnew  = %s\\n\", argv[1], argv[2]);\n> > +       replace_pair(old, new);\n> > +       if (argv[3][0] == 'A')\n> > +              obj = parse_object(old);\n> > +       else {\n> > +              struct commit *com = lookup_commit(old);\n> > +              if (com->buffer)\n> > +                      printf(\"commit buffer:\\n%s\", com->buffer);\n> > +              else\n> > +                      printf(\"no commit buffer\\n\");\n> > +              obj = (struct object *)com;\n> > +       }\n> > +\n> > +       printf(\"sha1 = %s\\n\", sha1_to_hex(obj->sha1));\n> > +       return 0;\n> > +}\n> > \n> > and I get:\n> > \n> > $ git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B\n> > old  = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d\n> > new  = c9b402bd93105f80f3c5d67ecfccc8ba36810613\n> > no commit buffer\n> > sha1 = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d\n> > \n> > Could you show what code you use?\n> \n> You need parse_commit() (unless somebody already did that before\n> lookup_commit()).\n\nIt looks like parse_commit() is buggy regarding replaced objects. But I am not \nsure how it should be fixed.\n\nAnyway if you use parse_object(), then you don't need parse_commit(). So if \npossible you should use parse_object() instead of both lookup_commit() and \nparse_commit().\n\nBest regards,\nChristian.\n"},{"id":"147373","messageId":"AANLkTim9vMiEuBJJdCdGqjTOz8cs-nH+LyJF-rPGOTvS@mail.gmail.com","threadId":"24591","inReplyTo":"201008051341.08632.chriscool@tuxfamily.org","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-08-07T04:03:05Z","receivedAt":"2010-08-07T04:03:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 5, 2010 at 9:41 PM, Christian Couder\n<chriscool@tuxfamily.org> wrote:\n> It looks like parse_commit() is buggy regarding replaced objects. But I am not\n> sure how it should be fixed.\n\nIt could be fixed the same way you did with parse_object(): replace\nread_sha1_file() with read_sha1_file_repl(). You would also need to\nfix parse_tree() and parse_tag(). But..\n\n> Anyway if you use parse_object(), then you don't need parse_commit(). So if\n> possible you should use parse_object() instead of both lookup_commit() and\n> parse_commit().\n\nThat's how those functions are used. For example, in\ntraverse_commit_list(), lookup_*() may be called and uninteresting\nobjects marked UNINTERESTING. Later on in process_{tree,blob,tag},\nparse_* may be called if their content is interesting.\n\nTo me, the fix above will leave a gap when object->sha1 is the\noriginal sha1, until parse_*() is called. It just does not sound good.\n\nOr, you could lookup_replace_object() inside\nlookup_{object,tree,commit,tag,blob} and update object->sha1. Hm..\n-- \nDuy\n"},{"id":"147989","messageId":"201008130559.33640.chriscool@tuxfamily.org","threadId":"24591","inReplyTo":"AANLkTim9vMiEuBJJdCdGqjTOz8cs-nH+LyJF-rPGOTvS@mail.gmail.com","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-08-13T03:59:33Z","receivedAt":"2010-08-13T03:59:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Saturday 07 August 2010 06:03:05 Nguyen Thai Ngoc Duy wrote:\n> On Thu, Aug 5, 2010 at 9:41 PM, Christian Couder\n> \n> <chriscool@tuxfamily.org> wrote:\n> > It looks like parse_commit() is buggy regarding replaced objects. But I\n> > am not sure how it should be fixed.\n> \n> It could be fixed the same way you did with parse_object(): replace\n> read_sha1_file() with read_sha1_file_repl(). You would also need to\n> fix parse_tree() and parse_tag(). But..\n> \n> > Anyway if you use parse_object(), then you don't need parse_commit(). So\n> > if possible you should use parse_object() instead of both\n> > lookup_commit() and parse_commit().\n> \n> That's how those functions are used. For example, in\n> traverse_commit_list(), lookup_*() may be called and uninteresting\n> objects marked UNINTERESTING. Later on in process_{tree,blob,tag},\n> parse_* may be called if their content is interesting.\n> \n> To me, the fix above will leave a gap when object->sha1 is the\n> original sha1, until parse_*() is called. It just does not sound good.\n\nWhat do you think about adding a parse_commit_repl() function like the patch \nbelow and then using it instead of parse_commit()?\n\n------- >8 ---------------------------------------------------\n\ndiff --git a/commit.c b/commit.c\nindex 652c1ba..183a735 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -316,6 +316,50 @@ int parse_commit(struct commit *item)\n        return ret;\n }\n \n+int parse_commit_repl(struct commit **commit)\n+{\n+       enum object_type type;\n+       void *buffer;\n+       unsigned long size;\n+       int ret;\n+       const unsigned char *repl;\n+       struct commit *item = *commit;\n+\n+       if (!item)\n+               return -1;\n+       if (item->object.parsed)\n+               return 0;\n+       buffer = read_sha1_file_repl(item->object.sha1, &type, &size, &repl);\n+       if (!buffer)\n+               return error(\"Could not read %s\",\n+                            sha1_to_hex(item->object.sha1));\n+\n+       if (item->object.sha1 != repl) {\n+               struct commit *repl_item = lookup_commit(repl);\n+               if (!repl_item) {\n+                       free(buffer);\n+                       return error(\"Bad replacement %s for commit %s\",\n+                                    sha1_to_hex(repl),\n+                                    sha1_to_hex(item->object.sha1));\n+               }\n+               repl_item->object.flags = item->object.flags;\n+               *commit = item = repl_item;\n+       }\n+\n+       if (type != OBJ_COMMIT) {\n+               free(buffer);\n+               return error(\"Object %s not a commit\",\n+                            sha1_to_hex(item->object.sha1));\n+       }\n+       ret = parse_commit_buffer(item, buffer, size);\n+       if (save_commit_buffer && !ret) {\n+               item->buffer = buffer;\n+               return 0;\n+       }\n+       free(buffer);\n+       return ret;\n+}\n+\n int find_commit_subject(const char *commit_buffer, const char **subject)\n {\n        const char *eol;\n\n-------- 8< --------------------------------------------------\n\nThanks,\nChristian.\n"},{"id":"147994","messageId":"AANLkTinmJ0=VxFAaCXNjB2WAiPYHq3jXmRnzVoEiw_-f@mail.gmail.com","threadId":"24591","inReplyTo":"201008130559.33640.chriscool@tuxfamily.org","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-08-13T09:02:40Z","receivedAt":"2010-08-13T09:02:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 13, 2010 at 1:59 PM, Christian Couder\n<chriscool@tuxfamily.org> wrote:\n> On Saturday 07 August 2010 06:03:05 Nguyen Thai Ngoc Duy wrote:\n>> On Thu, Aug 5, 2010 at 9:41 PM, Christian Couder\n>>\n>> <chriscool@tuxfamily.org> wrote:\n>> > It looks like parse_commit() is buggy regarding replaced objects. But I\n>> > am not sure how it should be fixed.\n>>\n>> It could be fixed the same way you did with parse_object(): replace\n>> read_sha1_file() with read_sha1_file_repl(). You would also need to\n>> fix parse_tree() and parse_tag(). But..\n>>\n>> > Anyway if you use parse_object(), then you don't need parse_commit(). So\n>> > if possible you should use parse_object() instead of both\n>> > lookup_commit() and parse_commit().\n>>\n>> That's how those functions are used. For example, in\n>> traverse_commit_list(), lookup_*() may be called and uninteresting\n>> objects marked UNINTERESTING. Later on in process_{tree,blob,tag},\n>> parse_* may be called if their content is interesting.\n>>\n>> To me, the fix above will leave a gap when object->sha1 is the\n>> original sha1, until parse_*() is called. It just does not sound good.\n>\n> What do you think about adding a parse_commit_repl() function like the patch\n> below and then using it instead of parse_commit()?\n\nHow do you plan to use this new function? #define parse_commit(c)\nparse_commit_repl(c) or use the new function explictly when needed?\n\nYou are going to need parse_tree_repl() too unless you declare\ntree/blob replacement is not supported and make git-replace refuse\nblob/tree replacement.\n\nAnother thing to address is, there will be a duration between\nlookup_commit() and parse_commit_repl(), where object.sha1 is the\noriginal one. If it is saved elsewhere, troubles are ahead.\n\n> ------- >8 ---------------------------------------------------\n>\n> diff --git a/commit.c b/commit.c\n> index 652c1ba..183a735 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -316,6 +316,50 @@ int parse_commit(struct commit *item)\n>        return ret;\n>  }\n>\n> +int parse_commit_repl(struct commit **commit)\n> +{\n> +       enum object_type type;\n> +       void *buffer;\n> +       unsigned long size;\n> +       int ret;\n> +       const unsigned char *repl;\n> +       struct commit *item = *commit;\n> +\n> +       if (!item)\n> +               return -1;\n> +       if (item->object.parsed)\n> +               return 0;\n> +       buffer = read_sha1_file_repl(item->object.sha1, &type, &size, &repl);\n> +       if (!buffer)\n> +               return error(\"Could not read %s\",\n> +                            sha1_to_hex(item->object.sha1));\n> +\n> +       if (item->object.sha1 != repl) {\n> +               struct commit *repl_item = lookup_commit(repl);\n> +               if (!repl_item) {\n> +                       free(buffer);\n> +                       return error(\"Bad replacement %s for commit %s\",\n> +                                    sha1_to_hex(repl),\n> +                                    sha1_to_hex(item->object.sha1));\n> +               }\n\nYou need to use lookup_object() instead here. lookup_commit() wil\ncreate new object if \"repl\" is not found.\n-- \nDuy\n"},{"id":"148069","messageId":"201008140403.33164.chriscool@tuxfamily.org","threadId":"24591","inReplyTo":"AANLkTinmJ0=VxFAaCXNjB2WAiPYHq3jXmRnzVoEiw_-f@mail.gmail.com","subject":"Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-08-14T02:03:32Z","receivedAt":"2010-08-14T02:03:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Friday 13 August 2010 11:02:40 Nguyen Thai Ngoc Duy wrote:\n> On Fri, Aug 13, 2010 at 1:59 PM, Christian Couder\n> \n> <chriscool@tuxfamily.org> wrote:\n> > On Saturday 07 August 2010 06:03:05 Nguyen Thai Ngoc Duy wrote:\n> >> On Thu, Aug 5, 2010 at 9:41 PM, Christian Couder\n> >> \n> >> <chriscool@tuxfamily.org> wrote:\n> >> > It looks like parse_commit() is buggy regarding replaced objects. But\n> >> > I am not sure how it should be fixed.\n> >> \n> >> It could be fixed the same way you did with parse_object(): replace\n> >> read_sha1_file() with read_sha1_file_repl(). You would also need to\n> >> fix parse_tree() and parse_tag(). But..\n> >> \n> >> > Anyway if you use parse_object(), then you don't need parse_commit().\n> >> > So if possible you should use parse_object() instead of both\n> >> > lookup_commit() and parse_commit().\n> >> \n> >> That's how those functions are used. For example, in\n> >> traverse_commit_list(), lookup_*() may be called and uninteresting\n> >> objects marked UNINTERESTING. Later on in process_{tree,blob,tag},\n> >> parse_* may be called if their content is interesting.\n> >> \n> >> To me, the fix above will leave a gap when object->sha1 is the\n> >> original sha1, until parse_*() is called. It just does not sound good.\n> > \n> > What do you think about adding a parse_commit_repl() function like the\n> > patch below and then using it instead of parse_commit()?\n> \n> How do you plan to use this new function? #define parse_commit(c)\n> parse_commit_repl(c) or use the new function explictly when needed?\n\nWe have to use the new function explicitly instead of parse_commit() but I \nthink in most cases we just need to change parse_commit(stuff) into \nparse_commit_repl(&stuff) and it will work.\n \n> You are going to need parse_tree_repl() too \n\nYes, probably, I did not look at that yet.\n\n> unless you declare\n> tree/blob replacement is not supported and make git-replace refuse\n> blob/tree replacement.\n\nI think they should be supported as much as possible in the long run but it is \nnot as important as supporting commits.\n\n> Another thing to address is, there will be a duration between\n> lookup_commit() and parse_commit_repl(), where object.sha1 is the\n> original one. If it is saved elsewhere, troubles are ahead.\n\nYes, parse_commit_repl() may not be the only solution needed in some cases but \nI think it is good enough in most cases.\n\n> > ------- >8 ---------------------------------------------------\n> > \n> > diff --git a/commit.c b/commit.c\n> > index 652c1ba..183a735 100644\n> > --- a/commit.c\n> > +++ b/commit.c\n> > @@ -316,6 +316,50 @@ int parse_commit(struct commit *item)\n> >        return ret;\n> >  }\n> > \n> > +int parse_commit_repl(struct commit **commit)\n> > +{\n> > +       enum object_type type;\n> > +       void *buffer;\n> > +       unsigned long size;\n> > +       int ret;\n> > +       const unsigned char *repl;\n> > +       struct commit *item = *commit;\n> > +\n> > +       if (!item)\n> > +               return -1;\n> > +       if (item->object.parsed)\n> > +               return 0;\n> > +       buffer = read_sha1_file_repl(item->object.sha1, &type, &size,\n> > &repl); +       if (!buffer)\n> > +               return error(\"Could not read %s\",\n> > +                            sha1_to_hex(item->object.sha1));\n> > +\n> > +       if (item->object.sha1 != repl) {\n> > +               struct commit *repl_item = lookup_commit(repl);\n> > +               if (!repl_item) {\n> > +                       free(buffer);\n> > +                       return error(\"Bad replacement %s for commit %s\",\n> > +                                    sha1_to_hex(repl),\n> > +                                    sha1_to_hex(item->object.sha1));\n> > +               }\n> \n> You need to use lookup_object() instead here. lookup_commit() wil\n> create new object if \"repl\" is not found.\n\nWe are inside the \"if (item->object.sha1 != repl)\" block, so we know that we \nwill have to do \"*commit = repl_item\" with a repl_item that is different from \nitem. So we have to create the repl_item commit if it doesn't exist.\n\nThanks,\nChristian.\n"}]}