* [PATCH RFC] parse_object: pass on the original sha1, not the replaced one @ 2010-07-31 12:36 Nguyễn Thái Ngọc Duy 2010-08-02 7:42 ` Christian Couder 0 siblings, 1 reply; 14+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2010-07-31 12:36 UTC (permalink / raw) To: git, chriscool; +Cc: Nguyễn Thái Ngọc Duy Commit 0e87c36 (object: call "check_sha1_signature" with the replacement sha1) did this. I'm not sure if it's should be done this way. With "repl" as the first argument to parse_object_buffer, the returned obj pointer will have the replaced SHA1 in obj->sha1, not the original one. I sort of expect that, no matter the object is replaced, obj->sha1 should stay the same. This was observed by replacing commit tip. git log would show the replaced sha1, not the original one. --- object.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/object.c b/object.c index 277b3dd..7adfda7 100644 --- a/object.c +++ b/object.c @@ -199,7 +199,7 @@ struct object *parse_object(const unsigned char *sha1) return NULL; } - obj = parse_object_buffer(repl, type, size, buffer, &eaten); + obj = parse_object_buffer(sha1, type, size, buffer, &eaten); if (!eaten) free(buffer); return obj; -- 1.7.1.rc1.69.g24c2f7 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-07-31 12:36 [PATCH RFC] parse_object: pass on the original sha1, not the replaced one Nguyễn Thái Ngọc Duy @ 2010-08-02 7:42 ` Christian Couder 2010-08-02 9:31 ` Nguyen Thai Ngoc Duy 0 siblings, 1 reply; 14+ messages in thread From: Christian Couder @ 2010-08-02 7:42 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git On Saturday 31 July 2010 14:36:42 Nguyễn Thái Ngọc Duy wrote: > Commit 0e87c36 (object: call "check_sha1_signature" with the > replacement sha1) did this. I'm not sure if it's should be done this > way. > > With "repl" as the first argument to parse_object_buffer, the returned > obj pointer will have the replaced SHA1 in obj->sha1, not the original > one. I sort of expect that, no matter the object is replaced, > obj->sha1 should stay the same. > > This was observed by replacing commit tip. git log would show the > replaced sha1, not the original one. I am not sure I understand what you are saying. Do you mean that git log should show the original sha1 but the content of the replacement commit, instead of both the sha1 and the content of the replacement commit? I just tested your patch and indeed with it it seems to me that the result shown by git log is not consistent, as for example the commit message is the one of the replacement commit but the commit sha1 is the one of the original commit. The idea with replaced object was that as much as possible everything except some very low levels commands and transport commands should use the replacement objects (except if the --no-replace-objects option or NO_REPLACE_OBJECTS_ENVIRONMENT is used). Could you explain why you need the object returned by parse_object_buffer to not have the replacement SHA1 in obj->sha1 when it is parsing the buffer from the replacement object? Best regards, Christian. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-02 7:42 ` Christian Couder @ 2010-08-02 9:31 ` Nguyen Thai Ngoc Duy 2010-08-03 5:00 ` Christian Couder 0 siblings, 1 reply; 14+ messages in thread From: Nguyen Thai Ngoc Duy @ 2010-08-02 9:31 UTC (permalink / raw) To: Christian Couder; +Cc: git 2010/8/2 Christian Couder <chriscool@tuxfamily.org>: > On Saturday 31 July 2010 14:36:42 Nguyễn Thái Ngọc Duy wrote: >> Commit 0e87c36 (object: call "check_sha1_signature" with the >> replacement sha1) did this. I'm not sure if it's should be done this >> way. >> >> With "repl" as the first argument to parse_object_buffer, the returned >> obj pointer will have the replaced SHA1 in obj->sha1, not the original >> one. I sort of expect that, no matter the object is replaced, >> obj->sha1 should stay the same. >> >> This was observed by replacing commit tip. git log would show the >> replaced sha1, not the original one. > > I am not sure I understand what you are saying. Do you mean that git log > should show the original sha1 but the content of the replacement commit, > instead of both the sha1 and the content of the replacement commit? "original sha1, but the content of replacement commit", yes. Isn't that the intention of git-replace? > I just tested your patch and indeed with it it seems to me that the result > shown by git log is not consistent, as for example the commit message is the > one of the replacement commit but the commit sha1 is the one of the original > commit. The consistency is already there. Suppose I want to replace commit A with B. Depends on what function I call to fetch "A", I get different object->sha1. (the content is always from B though). With parse_object(A), I get an object pointer whose sha1 is B. With lookup_commit(A), I get an object pointer whose sha1 is A. > Could you explain why you need the object returned by parse_object_buffer to > not have the replacement SHA1 in obj->sha1 when it is parsing the buffer from > the replacement object? As I said above, it's inconsistent. I'm not saying my way is correct. Just wondering. -- Duy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-02 9:31 ` Nguyen Thai Ngoc Duy @ 2010-08-03 5:00 ` Christian Couder 2010-08-03 6:01 ` Nguyen Thai Ngoc Duy 0 siblings, 1 reply; 14+ messages in thread From: Christian Couder @ 2010-08-03 5:00 UTC (permalink / raw) To: Nguyen Thai Ngoc Duy; +Cc: git On Monday 02 August 2010 11:31:23 Nguyen Thai Ngoc Duy wrote: > 2010/8/2 Christian Couder <chriscool@tuxfamily.org>: > > > > I am not sure I understand what you are saying. Do you mean that git log > > should show the original sha1 but the content of the replacement commit, > > instead of both the sha1 and the content of the replacement commit? > > "original sha1, but the content of replacement commit", yes. Isn't > that the intention of git-replace? I think it's better if the sha1 is replaced too. This way we keep the link between the sha1 and the content. And it's easier for the user to realize that we gave him a replacement object if we use the replacement sha1 as the user can more easily see that the sha1 changed. > > I just tested your patch and indeed with it it seems to me that the > > result shown by git log is not consistent, as for example the commit > > message is the one of the replacement commit but the commit sha1 is the > > one of the original commit. > > The consistency is already there. Suppose I want to replace commit A > with B. Depends on what function I call to fetch "A", I get different > object->sha1. (the content is always from B though). > > With parse_object(A), I get an object pointer whose sha1 is B. > > With lookup_commit(A), I get an object pointer whose sha1 is A. Maybe there is a bug somewhere and you should get an object pointer whose sha1 is B or maybe the content of the object that was inserted in the lookup table should have been the content from A and not from B. I will try to have a deeper look at that, but it would help if you could give an example of a command that triggers this behavior. Best regards, Christian. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-03 5:00 ` Christian Couder @ 2010-08-03 6:01 ` Nguyen Thai Ngoc Duy 2010-08-04 11:58 ` Christian Couder 0 siblings, 1 reply; 14+ messages in thread From: Nguyen Thai Ngoc Duy @ 2010-08-03 6:01 UTC (permalink / raw) To: Christian Couder; +Cc: git On Tue, Aug 3, 2010 at 3:00 PM, Christian Couder <chriscool@tuxfamily.org> wrote: >> With parse_object(A), I get an object pointer whose sha1 is B. >> >> With lookup_commit(A), I get an object pointer whose sha1 is A. > > Maybe there is a bug somewhere and you should get an object pointer whose sha1 > is B or maybe the content of the object that was inserted in the lookup table > should have been the content from A and not from B. I will try to have a > deeper look at that, but it would help if you could give an example of a > command that triggers this behavior. The following patch add "sha1" command. These commands give different sha1: git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B (hopefully gmail won't break the patch, otherwise I'll resend to you privately) diff --git a/Makefile b/Makefile index f33648d..8556c65 100644 --- a/Makefile +++ b/Makefile @@ -649,6 +649,7 @@ LIB_OBJS += ws.o LIB_OBJS += wt-status.o LIB_OBJS += xdiff-interface.o +BUILTIN_OBJS += builtin/sha1.o BUILTIN_OBJS += builtin/add.o BUILTIN_OBJS += builtin/annotate.o BUILTIN_OBJS += builtin/apply.o diff --git a/builtin/sha1.c b/builtin/sha1.c new file mode 100644 index 0000000..4700bc0 --- /dev/null +++ b/builtin/sha1.c @@ -0,0 +1,21 @@ +#include "cache.h" +#include "commit.h" + +int cmd_sha1(int argc, char **argv) +{ + unsigned char old[20]; + unsigned char new[20]; + struct object *obj; + + get_sha1_hex(argv[1], old); + get_sha1_hex(argv[2], new); + printf("old = %s\nnew = %s\n", argv[1], argv[2]); + replace_pair(old, new); + if (argv[3][0] == 'A') + obj = parse_object(old); + else + obj = lookup_commit(old); + + printf("sha1 = %s\n", sha1_to_hex(obj->sha1)); + return 0; +} diff --git a/git.c b/git.c index f37028b..9bc2391 100644 --- a/git.c +++ b/git.c @@ -288,6 +288,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) return 0; } +int cmd_sha1(int, const char **, const char*); static void handle_internal_command(int argc, const char **argv) { const char *cmd = argv[0]; @@ -378,6 +379,7 @@ static void handle_internal_command(int argc, const char **argv) { "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE }, { "rm", cmd_rm, RUN_SETUP }, { "send-pack", cmd_send_pack, RUN_SETUP }, + { "sha1", cmd_sha1, RUN_SETUP }, { "shortlog", cmd_shortlog, USE_PAGER }, { "show-branch", cmd_show_branch, RUN_SETUP }, { "show", cmd_show, RUN_SETUP | USE_PAGER }, diff --git a/replace_object.c b/replace_object.c index eb59604..1ec5df7 100644 --- a/replace_object.c +++ b/replace_object.c @@ -50,6 +50,16 @@ static int register_replace_object(struct replace_object *replace, return 0; } +int replace_pair(const unsigned char *old, + const unsigned char *new) +{ + struct replace_object *ro = xmalloc(sizeof(*ro)); + hashcpy(ro->sha1[0],old); + hashcpy(ro->sha1[1],new); + register_replace_object(ro, 1); + return 0; +} + static int register_replace_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data) -- Duy ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-03 6:01 ` Nguyen Thai Ngoc Duy @ 2010-08-04 11:58 ` Christian Couder 2010-08-04 12:42 ` Nguyen Thai Ngoc Duy 0 siblings, 1 reply; 14+ messages in thread From: Christian Couder @ 2010-08-04 11:58 UTC (permalink / raw) To: Nguyen Thai Ngoc Duy; +Cc: git On Tuesday 03 August 2010 08:01:32 Nguyen Thai Ngoc Duy wrote: > On Tue, Aug 3, 2010 at 3:00 PM, Christian Couder > > <chriscool@tuxfamily.org> wrote: > >> With parse_object(A), I get an object pointer whose sha1 is B. > >> > >> With lookup_commit(A), I get an object pointer whose sha1 is A. > > > > Maybe there is a bug somewhere and you should get an object pointer whose > > sha1 is B or maybe the content of the object that was inserted in the > > lookup table should have been the content from A and not from B. After having another look at that, I think the content of the object in the lookup table should be the content of A. It should be a bug if the object returned by lookup_commit(A) does not have the content of A. > > I will > > try to have a deeper look at that, but it would help if you could give > > an example of a command that triggers this behavior. > > The following patch add "sha1" command. These commands give different sha1: > > git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A > git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B Yes, but that does not mean that the content of the object returned by lookup_commit(A) is not the content of A. Or do you have an example where the content of the object returned by lookup_commit(A) is not the content of A? I mean that we should always be consistent by having objects with an obj->sha1 field corresponding to the content. So yes we have: > With parse_object(A), I get an object pointer whose sha1 is B. > > With lookup_commit(A), I get an object pointer whose sha1 is A. but it's not a problem, it's just the result from the fact that parse_object() (completely) replace objects and lookup_commit() does not. Best regards, Christian. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-04 11:58 ` Christian Couder @ 2010-08-04 12:42 ` Nguyen Thai Ngoc Duy 2010-08-04 12:57 ` Christian Couder 0 siblings, 1 reply; 14+ messages in thread From: Nguyen Thai Ngoc Duy @ 2010-08-04 12:42 UTC (permalink / raw) To: Christian Couder; +Cc: git On Wed, Aug 4, 2010 at 9:58 PM, Christian Couder <chriscool@tuxfamily.org> wrote: >> > I will >> > try to have a deeper look at that, but it would help if you could give >> > an example of a command that triggers this behavior. >> >> The following patch add "sha1" command. These commands give different sha1: >> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B > > Yes, but that does not mean that the content of the object returned by > lookup_commit(A) is not the content of A. > > Or do you have an example where the content of the object returned by > lookup_commit(A) is not the content of A? Both return the content of B. I modified my patch a bit to also show the content, ((struct commit*)obj)->buffer. $ ./git cat-file -p HEAD tree 13109340ff2bd55bd16271bbad7a9232f9052923 parent 9a3028b987d73e4c85e8db9980720feba6a1b5c6 parent 2aedccd3d51ec922020f7c7e39df5d2c4d3af515 author Junio C Hamano <gitster@pobox.com> 1280274846 -0700 committer Junio C Hamano <gitster@pobox.com> 1280274905 -0700 Sync with 1.7.2.1 Signed-off-by: Junio C Hamano <gitster@pobox.com> $ ./git cat-file -p HEAD^ tree 4ffcd88bed7a675e2d130a41203a7ebe026b6462 parent 9a9fb5d3c4c8601beb2d7b8e3b9283c6c3815a2d author Ævar Arnfjörð Bjarmason <avarab@gmail.com> 1279925924 +0000 committer Junio C Hamano <gitster@pobox.com> 1280124888 -0700 tests: Ignore the Test::Harness .prove file We document how to run prove with the --state option in t/README. This produces a .prove YAML file in the current directory. Change the t/ gitignore to ignore it, and clean it up on `make clean'. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> $ ./git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A old = ba9523ea809dc496a14f3644bdc1dd6f486983c0 new = 9a3028b987d73e4c85e8db9980720feba6a1b5c6 sha1 = 9a3028b987d73e4c85e8db9980720feba6a1b5c6 tree 4ffcd88bed7a675e2d130a41203a7ebe026b6462 parent 9a9fb5d3c4c8601beb2d7b8e3b9283c6c3815a2d author Ævar Arnfjörð Bjarmason <avarab@gmail.com> 1279925924 +0000 committer Junio C Hamano <gitster@pobox.com> 1280124888 -0700 tests: Ignore the Test::Harness .prove file We document how to run prove with the --state option in t/README. This produces a .prove YAML file in the current directory. Change the t/ gitignore to ignore it, and clean it up on `make clean'. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> $ ./git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B old = ba9523ea809dc496a14f3644bdc1dd6f486983c0 new = 9a3028b987d73e4c85e8db9980720feba6a1b5c6 sha1 = ba9523ea809dc496a14f3644bdc1dd6f486983c0 tree 4ffcd88bed7a675e2d130a41203a7ebe026b6462 parent 9a9fb5d3c4c8601beb2d7b8e3b9283c6c3815a2d author Ævar Arnfjörð Bjarmason <avarab@gmail.com> 1279925924 +0000 committer Junio C Hamano <gitster@pobox.com> 1280124888 -0700 tests: Ignore the Test::Harness .prove file We document how to run prove with the --state option in t/README. This produces a .prove YAML file in the current directory. Change the t/ gitignore to ignore it, and clean it up on `make clean'. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> -- Duy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-04 12:42 ` Nguyen Thai Ngoc Duy @ 2010-08-04 12:57 ` Christian Couder 2010-08-04 22:07 ` Nguyen Thai Ngoc Duy 0 siblings, 1 reply; 14+ messages in thread From: Christian Couder @ 2010-08-04 12:57 UTC (permalink / raw) To: Nguyen Thai Ngoc Duy; +Cc: git On Wednesday 04 August 2010 14:42:50 Nguyen Thai Ngoc Duy wrote: > On Wed, Aug 4, 2010 at 9:58 PM, Christian Couder > > <chriscool@tuxfamily.org> wrote: > >> > I will > >> > try to have a deeper look at that, but it would help if you could give > >> > an example of a command that triggers this behavior. > >> > >> The following patch add "sha1" command. These commands give different > >> sha1: > >> > >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A > >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B > > > > Yes, but that does not mean that the content of the object returned by > > lookup_commit(A) is not the content of A. > > > > Or do you have an example where the content of the object returned by > > lookup_commit(A) is not the content of A? > > Both return the content of B. I modified my patch a bit to also show > the content, ((struct commit*)obj)->buffer. I also modified your patch but I don't get any content shown when using lookup_commit() I use: diff --git a/builtin/sha1.c b/builtin/sha1.c new file mode 100644 index 0000000..8e081b2 --- /dev/null +++ b/builtin/sha1.c @@ -0,0 +1,27 @@ +#include "cache.h" +#include "commit.h" + +int cmd_sha1(int argc, char **argv) +{ + unsigned char old[20]; + unsigned char new[20]; + struct object *obj; + + get_sha1_hex(argv[1], old); + get_sha1_hex(argv[2], new); + printf("old = %s\nnew = %s\n", argv[1], argv[2]); + replace_pair(old, new); + if (argv[3][0] == 'A') + obj = parse_object(old); + else { + struct commit *com = lookup_commit(old); + if (com->buffer) + printf("commit buffer:\n%s", com->buffer); + else + printf("no commit buffer\n"); + obj = (struct object *)com; + } + + printf("sha1 = %s\n", sha1_to_hex(obj->sha1)); + return 0; +} and I get: $ git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B old = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d new = c9b402bd93105f80f3c5d67ecfccc8ba36810613 no commit buffer sha1 = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d Could you show what code you use? Thanks, Christian. ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-04 12:57 ` Christian Couder @ 2010-08-04 22:07 ` Nguyen Thai Ngoc Duy 2010-08-05 11:41 ` Christian Couder 0 siblings, 1 reply; 14+ messages in thread From: Nguyen Thai Ngoc Duy @ 2010-08-04 22:07 UTC (permalink / raw) To: Christian Couder; +Cc: git [-- Attachment #1: Type: text/plain, Size: 2639 bytes --] On Wed, Aug 4, 2010 at 10:57 PM, Christian Couder <chriscool@tuxfamily.org> wrote: > On Wednesday 04 August 2010 14:42:50 Nguyen Thai Ngoc Duy wrote: >> On Wed, Aug 4, 2010 at 9:58 PM, Christian Couder >> >> <chriscool@tuxfamily.org> wrote: >> >> > I will >> >> > try to have a deeper look at that, but it would help if you could give >> >> > an example of a command that triggers this behavior. >> >> >> >> The following patch add "sha1" command. These commands give different >> >> sha1: >> >> >> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A >> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B >> > >> > Yes, but that does not mean that the content of the object returned by >> > lookup_commit(A) is not the content of A. >> > >> > Or do you have an example where the content of the object returned by >> > lookup_commit(A) is not the content of A? >> >> Both return the content of B. I modified my patch a bit to also show >> the content, ((struct commit*)obj)->buffer. > > I also modified your patch but I don't get any content shown when using > lookup_commit() > > I use: > > diff --git a/builtin/sha1.c b/builtin/sha1.c > new file mode 100644 > index 0000000..8e081b2 > --- /dev/null > +++ b/builtin/sha1.c > @@ -0,0 +1,27 @@ > +#include "cache.h" > +#include "commit.h" > + > +int cmd_sha1(int argc, char **argv) > +{ > + unsigned char old[20]; > + unsigned char new[20]; > + struct object *obj; > + > + get_sha1_hex(argv[1], old); > + get_sha1_hex(argv[2], new); > + printf("old = %s\nnew = %s\n", argv[1], argv[2]); > + replace_pair(old, new); > + if (argv[3][0] == 'A') > + obj = parse_object(old); > + else { > + struct commit *com = lookup_commit(old); > + if (com->buffer) > + printf("commit buffer:\n%s", com->buffer); > + else > + printf("no commit buffer\n"); > + obj = (struct object *)com; > + } > + > + printf("sha1 = %s\n", sha1_to_hex(obj->sha1)); > + return 0; > +} > > and I get: > > $ git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B > old = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d > new = c9b402bd93105f80f3c5d67ecfccc8ba36810613 > no commit buffer > sha1 = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d > > Could you show what code you use? You need parse_commit() (unless somebody already did that before lookup_commit()). -- Duy [-- Attachment #2: sha1.c --] [-- Type: application/octet-stream, Size: 516 bytes --] #include "cache.h" #include "commit.h" int cmd_sha1(int argc, char **argv) { unsigned char old[20]; unsigned char new[20]; struct object *obj; get_sha1_hex(argv[1], old); get_sha1_hex(argv[2], new); printf("old = %s\nnew = %s\n", argv[1], argv[2]); replace_pair(old, new); if (argv[3][0] == 'A') obj = parse_object(old); else obj = lookup_commit(old); parse_commit((struct commit *)obj); printf("sha1 = %s\n", sha1_to_hex(obj->sha1)); printf("%s\n", ((struct commit*)obj)->buffer); return 0; } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-04 22:07 ` Nguyen Thai Ngoc Duy @ 2010-08-05 11:41 ` Christian Couder 2010-08-07 4:03 ` Nguyen Thai Ngoc Duy 0 siblings, 1 reply; 14+ messages in thread From: Christian Couder @ 2010-08-05 11:41 UTC (permalink / raw) To: Nguyen Thai Ngoc Duy; +Cc: git On Thursday 05 August 2010 00:07:45 Nguyen Thai Ngoc Duy wrote: > On Wed, Aug 4, 2010 at 10:57 PM, Christian Couder > > <chriscool@tuxfamily.org> wrote: > > On Wednesday 04 August 2010 14:42:50 Nguyen Thai Ngoc Duy wrote: > >> On Wed, Aug 4, 2010 at 9:58 PM, Christian Couder > >> > >> <chriscool@tuxfamily.org> wrote: > >> >> > I will > >> >> > try to have a deeper look at that, but it would help if you could > >> >> > give an example of a command that triggers this behavior. > >> >> > >> >> The following patch add "sha1" command. These commands give different > >> >> sha1: > >> >> > >> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` A > >> >> git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B > >> > > >> > Yes, but that does not mean that the content of the object returned by > >> > lookup_commit(A) is not the content of A. > >> > > >> > Or do you have an example where the content of the object returned by > >> > lookup_commit(A) is not the content of A? > >> > >> Both return the content of B. I modified my patch a bit to also show > >> the content, ((struct commit*)obj)->buffer. > > > > I also modified your patch but I don't get any content shown when using > > lookup_commit() > > > > I use: > > > > diff --git a/builtin/sha1.c b/builtin/sha1.c > > new file mode 100644 > > index 0000000..8e081b2 > > --- /dev/null > > +++ b/builtin/sha1.c > > @@ -0,0 +1,27 @@ > > +#include "cache.h" > > +#include "commit.h" > > + > > +int cmd_sha1(int argc, char **argv) > > +{ > > + unsigned char old[20]; > > + unsigned char new[20]; > > + struct object *obj; > > + > > + get_sha1_hex(argv[1], old); > > + get_sha1_hex(argv[2], new); > > + printf("old = %s\nnew = %s\n", argv[1], argv[2]); > > + replace_pair(old, new); > > + if (argv[3][0] == 'A') > > + obj = parse_object(old); > > + else { > > + struct commit *com = lookup_commit(old); > > + if (com->buffer) > > + printf("commit buffer:\n%s", com->buffer); > > + else > > + printf("no commit buffer\n"); > > + obj = (struct object *)com; > > + } > > + > > + printf("sha1 = %s\n", sha1_to_hex(obj->sha1)); > > + return 0; > > +} > > > > and I get: > > > > $ git sha1 `git rev-parse HEAD` `git rev-parse HEAD^` B > > old = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d > > new = c9b402bd93105f80f3c5d67ecfccc8ba36810613 > > no commit buffer > > sha1 = 5b4585a035e2ba61573273dacc6d17d7e8fcbc7d > > > > Could you show what code you use? > > You need parse_commit() (unless somebody already did that before > lookup_commit()). It looks like parse_commit() is buggy regarding replaced objects. But I am not sure how it should be fixed. Anyway if you use parse_object(), then you don't need parse_commit(). So if possible you should use parse_object() instead of both lookup_commit() and parse_commit(). Best regards, Christian. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-05 11:41 ` Christian Couder @ 2010-08-07 4:03 ` Nguyen Thai Ngoc Duy 2010-08-13 3:59 ` Christian Couder 0 siblings, 1 reply; 14+ messages in thread From: Nguyen Thai Ngoc Duy @ 2010-08-07 4:03 UTC (permalink / raw) To: Christian Couder; +Cc: git On Thu, Aug 5, 2010 at 9:41 PM, Christian Couder <chriscool@tuxfamily.org> wrote: > It looks like parse_commit() is buggy regarding replaced objects. But I am not > sure how it should be fixed. It could be fixed the same way you did with parse_object(): replace read_sha1_file() with read_sha1_file_repl(). You would also need to fix parse_tree() and parse_tag(). But.. > Anyway if you use parse_object(), then you don't need parse_commit(). So if > possible you should use parse_object() instead of both lookup_commit() and > parse_commit(). That's how those functions are used. For example, in traverse_commit_list(), lookup_*() may be called and uninteresting objects marked UNINTERESTING. Later on in process_{tree,blob,tag}, parse_* may be called if their content is interesting. To me, the fix above will leave a gap when object->sha1 is the original sha1, until parse_*() is called. It just does not sound good. Or, you could lookup_replace_object() inside lookup_{object,tree,commit,tag,blob} and update object->sha1. Hm.. -- Duy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-07 4:03 ` Nguyen Thai Ngoc Duy @ 2010-08-13 3:59 ` Christian Couder 2010-08-13 9:02 ` Nguyen Thai Ngoc Duy 0 siblings, 1 reply; 14+ messages in thread From: Christian Couder @ 2010-08-13 3:59 UTC (permalink / raw) To: Nguyen Thai Ngoc Duy; +Cc: git On Saturday 07 August 2010 06:03:05 Nguyen Thai Ngoc Duy wrote: > On Thu, Aug 5, 2010 at 9:41 PM, Christian Couder > > <chriscool@tuxfamily.org> wrote: > > It looks like parse_commit() is buggy regarding replaced objects. But I > > am not sure how it should be fixed. > > It could be fixed the same way you did with parse_object(): replace > read_sha1_file() with read_sha1_file_repl(). You would also need to > fix parse_tree() and parse_tag(). But.. > > > Anyway if you use parse_object(), then you don't need parse_commit(). So > > if possible you should use parse_object() instead of both > > lookup_commit() and parse_commit(). > > That's how those functions are used. For example, in > traverse_commit_list(), lookup_*() may be called and uninteresting > objects marked UNINTERESTING. Later on in process_{tree,blob,tag}, > parse_* may be called if their content is interesting. > > To me, the fix above will leave a gap when object->sha1 is the > original sha1, until parse_*() is called. It just does not sound good. What do you think about adding a parse_commit_repl() function like the patch below and then using it instead of parse_commit()? ------- >8 --------------------------------------------------- diff --git a/commit.c b/commit.c index 652c1ba..183a735 100644 --- a/commit.c +++ b/commit.c @@ -316,6 +316,50 @@ int parse_commit(struct commit *item) return ret; } +int parse_commit_repl(struct commit **commit) +{ + enum object_type type; + void *buffer; + unsigned long size; + int ret; + const unsigned char *repl; + struct commit *item = *commit; + + if (!item) + return -1; + if (item->object.parsed) + return 0; + buffer = read_sha1_file_repl(item->object.sha1, &type, &size, &repl); + if (!buffer) + return error("Could not read %s", + sha1_to_hex(item->object.sha1)); + + if (item->object.sha1 != repl) { + struct commit *repl_item = lookup_commit(repl); + if (!repl_item) { + free(buffer); + return error("Bad replacement %s for commit %s", + sha1_to_hex(repl), + sha1_to_hex(item->object.sha1)); + } + repl_item->object.flags = item->object.flags; + *commit = item = repl_item; + } + + if (type != OBJ_COMMIT) { + free(buffer); + return error("Object %s not a commit", + sha1_to_hex(item->object.sha1)); + } + ret = parse_commit_buffer(item, buffer, size); + if (save_commit_buffer && !ret) { + item->buffer = buffer; + return 0; + } + free(buffer); + return ret; +} + int find_commit_subject(const char *commit_buffer, const char **subject) { const char *eol; -------- 8< -------------------------------------------------- Thanks, Christian. ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-13 3:59 ` Christian Couder @ 2010-08-13 9:02 ` Nguyen Thai Ngoc Duy 2010-08-14 2:03 ` Christian Couder 0 siblings, 1 reply; 14+ messages in thread From: Nguyen Thai Ngoc Duy @ 2010-08-13 9:02 UTC (permalink / raw) To: Christian Couder; +Cc: git On Fri, Aug 13, 2010 at 1:59 PM, Christian Couder <chriscool@tuxfamily.org> wrote: > On Saturday 07 August 2010 06:03:05 Nguyen Thai Ngoc Duy wrote: >> On Thu, Aug 5, 2010 at 9:41 PM, Christian Couder >> >> <chriscool@tuxfamily.org> wrote: >> > It looks like parse_commit() is buggy regarding replaced objects. But I >> > am not sure how it should be fixed. >> >> It could be fixed the same way you did with parse_object(): replace >> read_sha1_file() with read_sha1_file_repl(). You would also need to >> fix parse_tree() and parse_tag(). But.. >> >> > Anyway if you use parse_object(), then you don't need parse_commit(). So >> > if possible you should use parse_object() instead of both >> > lookup_commit() and parse_commit(). >> >> That's how those functions are used. For example, in >> traverse_commit_list(), lookup_*() may be called and uninteresting >> objects marked UNINTERESTING. Later on in process_{tree,blob,tag}, >> parse_* may be called if their content is interesting. >> >> To me, the fix above will leave a gap when object->sha1 is the >> original sha1, until parse_*() is called. It just does not sound good. > > What do you think about adding a parse_commit_repl() function like the patch > below and then using it instead of parse_commit()? How do you plan to use this new function? #define parse_commit(c) parse_commit_repl(c) or use the new function explictly when needed? You are going to need parse_tree_repl() too unless you declare tree/blob replacement is not supported and make git-replace refuse blob/tree replacement. Another thing to address is, there will be a duration between lookup_commit() and parse_commit_repl(), where object.sha1 is the original one. If it is saved elsewhere, troubles are ahead. > ------- >8 --------------------------------------------------- > > diff --git a/commit.c b/commit.c > index 652c1ba..183a735 100644 > --- a/commit.c > +++ b/commit.c > @@ -316,6 +316,50 @@ int parse_commit(struct commit *item) > return ret; > } > > +int parse_commit_repl(struct commit **commit) > +{ > + enum object_type type; > + void *buffer; > + unsigned long size; > + int ret; > + const unsigned char *repl; > + struct commit *item = *commit; > + > + if (!item) > + return -1; > + if (item->object.parsed) > + return 0; > + buffer = read_sha1_file_repl(item->object.sha1, &type, &size, &repl); > + if (!buffer) > + return error("Could not read %s", > + sha1_to_hex(item->object.sha1)); > + > + if (item->object.sha1 != repl) { > + struct commit *repl_item = lookup_commit(repl); > + if (!repl_item) { > + free(buffer); > + return error("Bad replacement %s for commit %s", > + sha1_to_hex(repl), > + sha1_to_hex(item->object.sha1)); > + } You need to use lookup_object() instead here. lookup_commit() wil create new object if "repl" is not found. -- Duy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] parse_object: pass on the original sha1, not the replaced one 2010-08-13 9:02 ` Nguyen Thai Ngoc Duy @ 2010-08-14 2:03 ` Christian Couder 0 siblings, 0 replies; 14+ messages in thread From: Christian Couder @ 2010-08-14 2:03 UTC (permalink / raw) To: Nguyen Thai Ngoc Duy; +Cc: git On Friday 13 August 2010 11:02:40 Nguyen Thai Ngoc Duy wrote: > On Fri, Aug 13, 2010 at 1:59 PM, Christian Couder > > <chriscool@tuxfamily.org> wrote: > > On Saturday 07 August 2010 06:03:05 Nguyen Thai Ngoc Duy wrote: > >> On Thu, Aug 5, 2010 at 9:41 PM, Christian Couder > >> > >> <chriscool@tuxfamily.org> wrote: > >> > It looks like parse_commit() is buggy regarding replaced objects. But > >> > I am not sure how it should be fixed. > >> > >> It could be fixed the same way you did with parse_object(): replace > >> read_sha1_file() with read_sha1_file_repl(). You would also need to > >> fix parse_tree() and parse_tag(). But.. > >> > >> > Anyway if you use parse_object(), then you don't need parse_commit(). > >> > So if possible you should use parse_object() instead of both > >> > lookup_commit() and parse_commit(). > >> > >> That's how those functions are used. For example, in > >> traverse_commit_list(), lookup_*() may be called and uninteresting > >> objects marked UNINTERESTING. Later on in process_{tree,blob,tag}, > >> parse_* may be called if their content is interesting. > >> > >> To me, the fix above will leave a gap when object->sha1 is the > >> original sha1, until parse_*() is called. It just does not sound good. > > > > What do you think about adding a parse_commit_repl() function like the > > patch below and then using it instead of parse_commit()? > > How do you plan to use this new function? #define parse_commit(c) > parse_commit_repl(c) or use the new function explictly when needed? We have to use the new function explicitly instead of parse_commit() but I think in most cases we just need to change parse_commit(stuff) into parse_commit_repl(&stuff) and it will work. > You are going to need parse_tree_repl() too Yes, probably, I did not look at that yet. > unless you declare > tree/blob replacement is not supported and make git-replace refuse > blob/tree replacement. I think they should be supported as much as possible in the long run but it is not as important as supporting commits. > Another thing to address is, there will be a duration between > lookup_commit() and parse_commit_repl(), where object.sha1 is the > original one. If it is saved elsewhere, troubles are ahead. Yes, parse_commit_repl() may not be the only solution needed in some cases but I think it is good enough in most cases. > > ------- >8 --------------------------------------------------- > > > > diff --git a/commit.c b/commit.c > > index 652c1ba..183a735 100644 > > --- a/commit.c > > +++ b/commit.c > > @@ -316,6 +316,50 @@ int parse_commit(struct commit *item) > > return ret; > > } > > > > +int parse_commit_repl(struct commit **commit) > > +{ > > + enum object_type type; > > + void *buffer; > > + unsigned long size; > > + int ret; > > + const unsigned char *repl; > > + struct commit *item = *commit; > > + > > + if (!item) > > + return -1; > > + if (item->object.parsed) > > + return 0; > > + buffer = read_sha1_file_repl(item->object.sha1, &type, &size, > > &repl); + if (!buffer) > > + return error("Could not read %s", > > + sha1_to_hex(item->object.sha1)); > > + > > + if (item->object.sha1 != repl) { > > + struct commit *repl_item = lookup_commit(repl); > > + if (!repl_item) { > > + free(buffer); > > + return error("Bad replacement %s for commit %s", > > + sha1_to_hex(repl), > > + sha1_to_hex(item->object.sha1)); > > + } > > You need to use lookup_object() instead here. lookup_commit() wil > create new object if "repl" is not found. We are inside the "if (item->object.sha1 != repl)" block, so we know that we will have to do "*commit = repl_item" with a repl_item that is different from item. So we have to create the repl_item commit if it doesn't exist. Thanks, Christian. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2010-08-14 2:03 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-07-31 12:36 [PATCH RFC] parse_object: pass on the original sha1, not the replaced one Nguyễn Thái Ngọc Duy 2010-08-02 7:42 ` Christian Couder 2010-08-02 9:31 ` Nguyen Thai Ngoc Duy 2010-08-03 5:00 ` Christian Couder 2010-08-03 6:01 ` Nguyen Thai Ngoc Duy 2010-08-04 11:58 ` Christian Couder 2010-08-04 12:42 ` Nguyen Thai Ngoc Duy 2010-08-04 12:57 ` Christian Couder 2010-08-04 22:07 ` Nguyen Thai Ngoc Duy 2010-08-05 11:41 ` Christian Couder 2010-08-07 4:03 ` Nguyen Thai Ngoc Duy 2010-08-13 3:59 ` Christian Couder 2010-08-13 9:02 ` Nguyen Thai Ngoc Duy 2010-08-14 2:03 ` Christian Couder
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).