{"thread":{"id":"35989","subject":"[PATCH] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf","startedAt":"2014-02-28T07:58:25Z","lastAt":"2014-03-01T01:34:23Z","messageCount":5,"participants":["Faiz Kothari","Eric Sunshine","He Sun"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235554","messageId":"1393574305-24015-1-git-send-email-faiz.off93@gmail.com","threadId":"35989","inReplyTo":null,"subject":"[PATCH] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-02-28T07:58:25Z","receivedAt":"2014-02-28T07:58:25Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"Signed-off-by: Faiz Kothari \n\nNotes:\n I finally got what's happening, and why the errors were caused.\n packname is supposed to contain the complete path to the .pack file.\n Packs are stored as /path/to/.pack which I overlooked earlier.\n After inspecting what is happening in pack-write.c:finish_tmp_packfile()\n which indirectly modifies packname by appending the SHA1 and \".pack\" to packname\n This is happening in these code snippets:\n \tchar *end_of_name_prefix = strrchr(name_buffer, 0);\n \n and later\n \tsprintf(end_of_name_prefix, \"%s.pack\", sha1_to_hex(sha1));\n \n name_buffer is packname.buf\n Using const for the first argument of pack-write.c:finish_tmp_packfile()\n doesnot raise any compile time warning or error and not any runtime errors,\n since the packname.buf is on heap and has extra space to which more char can be written.\n If this was not the case,\n \tfor e.g. passing a constant string and modifying it.\n \tThis will result in a segmentation fault.\n---\n bulk-checkin.c | 8 +++++---\n pack-write.c | 2 +-\n pack.h | 2 +-\n 3 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 118c625..bbdf1ec 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -23,7 +23,7 @@ static struct bulk_checkin_state {\n static void finish_bulk_checkin(struct bulk_checkin_state *state)\n {\n \tunsigned char sha1[20];\n-\tchar packname[PATH_MAX];\n+\tstruct strbuf packname = STRBUF_INIT;\n \tint i;\n \n \tif (!state->f)\n@@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n \t\t\t\t\t state->offset);\n \t\tclose(fd);\n \t}\n+\tstrbuf_addf(&packname, \"%s/pack/pack-\", get_object_directory());\n+\tstrbuf_grow(&packname, 40 + 5);\n \n-\tsprintf(packname, \"%s/pack/pack-\", get_object_directory());\n-\tfinish_tmp_packfile(packname, state->pack_tmp_name,\n+\tfinish_tmp_packfile(packname.buf, state->pack_tmp_name,\n \t\t\t state->written, state->nr_written,\n \t\t\t &state->pack_idx_opts, sha1);\n \tfor (i = 0; i < state->nr_written; i++)\n@@ -53,6 +54,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n clear_exit:\n \tfree(state->written);\n \tmemset(state, 0, sizeof(*state));\n+\tstrbuf_release(&packname);\n \n \t/* Make objects we just wrote available to ourselves */\n \treprepare_packed_git();\ndiff --git a/pack-write.c b/pack-write.c\nindex 605d01b..ac38867 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)\n \treturn sha1fd(fd, *pack_tmp_name);\n }\n \n-void finish_tmp_packfile(char *name_buffer,\n+void finish_tmp_packfile(const char *name_buffer,\n \t\t\t const char *pack_tmp_name,\n \t\t\t struct pack_idx_entry **written_list,\n \t\t\t uint32_t nr_written,\ndiff --git a/pack.h b/pack.h\nindex 12d9516..3b9e033 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -91,6 +91,6 @@ extern int encode_in_pack_object_header(enum object_type, uintmax_t, unsigned ch\n extern int read_pack_header(int fd, struct pack_header *);\n \n extern struct sha1file *create_tmp_packfile(char **pack_tmp_name);\n-extern void finish_tmp_packfile(char *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);\n+extern void finish_tmp_packfile(const char *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);\n \n #endif\n-- \n1.7.9.5\n"},{"id":"235568","messageId":"CAPig+cTXK6=LDPDii6RQyO1fiMCq0Rai5uO0JYOzunM=4c_nUw@mail.gmail.com","threadId":"35989","inReplyTo":"1393574305-24015-1-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-28T09:15:50Z","receivedAt":"2014-02-28T09:15:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 28, 2014 at 2:58 AM, Faiz Kothari wrote:\n> Signed-off-by: Faiz Kothari \n>\n> Notes:\n> I finally got what's happening, and why the errors were caused.\n> packname is supposed to contain the complete path to the .pack file.\n> Packs are stored as /path/to/.pack which I overlooked earlier.\n> After inspecting what is happening in pack-write.c:finish_tmp_packfile()\n> which indirectly modifies packname by appending the SHA1 and \".pack\" to packname\n> This is happening in these code snippets:\n> char *end_of_name_prefix = strrchr(name_buffer, 0);\n>\n> and later\n> sprintf(end_of_name_prefix, \"%s.pack\", sha1_to_hex(sha1));\n>\n> name_buffer is packname.buf\n> Using const for the first argument of pack-write.c:finish_tmp_packfile()\n> doesnot raise any compile time warning or error and not any runtime errors,\n> since the packname.buf is on heap and has extra space to which more char can be written.\n> If this was not the case,\n> for e.g. passing a constant string and modifying it.\n> This will result in a segmentation fault.\n> ---\n\nThis notes section is important to the ongoing email discussion,\nhowever, it should be placed below the \"---\" line so that it does not\nbecome part of the recorded commit message when the patch is applied\nvia \"git am\".\n\n> bulk-checkin.c | 8 +++++---\n> pack-write.c | 2 +-\n> pack.h | 2 +-\n> 3 files changed, 7 insertions(+), 5 deletions(-)\n>\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 118c625..bbdf1ec 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -23,7 +23,7 @@ static struct bulk_checkin_state {\n> static void finish_bulk_checkin(struct bulk_checkin_state *state)\n> {\n> unsigned char sha1[20];\n> - char packname[PATH_MAX];\n> + struct strbuf packname = STRBUF_INIT;\n> int i;\n>\n> if (!state->f)\n> @@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n> state->offset);\n> close(fd);\n> }\n> + strbuf_addf(&packname, \"%s/pack/pack-\", get_object_directory());\n> + strbuf_grow(&packname, 40 + 5);\n\nThere are several problems with this. First, magic numbers 40 and 5\nconvey no meaning to the reader. At the very least, they should be\nnamed constants or a comment should explain them. More seriously,\nthough, this code is fragile since it has far too intimate knowledge\nof the inner workings of finish_tmp_packfile(). If the implementation\nof finish_tmp_packfile() changes in the future such that it writes\nmore than 45 additional characters to the incoming buffer, this will\nbreak.\n\nRather than coupling finish_bulk_checkin() and finish_tmp_packfile()\nso tightly, consider finish_tmp_packfile() a black box which just\n\"does its job\" and then propose ways to make things work without\nfinish_bulk_checkin() having to know how that job is done.\n\n> - sprintf(packname, \"%s/pack/pack-\", get_object_directory());\n> - finish_tmp_packfile(packname, state->pack_tmp_name,\n> + finish_tmp_packfile(packname.buf, state->pack_tmp_name,\n> state->written, state->nr_written,\n> &state->pack_idx_opts, sha1);\n> for (i = 0; i < state->nr_written; i++)\n> diff --git a/pack-write.c b/pack-write.c\n> index 605d01b..ac38867 100644\n> --- a/pack-write.c\n> +++ b/pack-write.c\n> @@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)\n> return sha1fd(fd, *pack_tmp_name);\n> }\n>\n> -void finish_tmp_packfile(char *name_buffer,\n> +void finish_tmp_packfile(const char *name_buffer,\n\nThis is misleading and fragile. By specifying 'const',\nfinish_tmp_packfile() promises not to modify the content of the\nincoming name_buffer, yet it breaks this promise by modifying the\nbuffer through the non-const end_of_name_prefix variable (after\ndropping the 'const' via strrchr()).\n\n> const char *pack_tmp_name,\n> struct pack_idx_entry **written_list,\n> uint32_t nr_written,\n"},{"id":"235651","messageId":"CAFbjVcnx41CTOV-SCGC9a91b9weNTgYoRP1+uVDukp6Z+0F_yg@mail.gmail.com","threadId":"35989","inReplyTo":"CAPig+cTXK6=LDPDii6RQyO1fiMCq0Rai5uO0JYOzunM=4c_nUw@mail.gmail.com","subject":"Re: [PATCH] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-02-28T18:27:20Z","receivedAt":"2014-02-28T18:27:20Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"Hi,\nThanks for the suggestions and remarks.\nI rewrote bulk-checkin.c:finish_bulk_checkin() using strbuf. But saw\nthat Sun He has already implemented the same way I have done.\nShould I submit my implementation as a patch?\n\nSecondly,\nI tried implementing this WITHOUT changing the prototype of the\nfunction pack-write.c:finish_tmp_packfile().\n\nFor this I detached the buffer from strbuf in finish_bulk_checkin()\nusing strbuf_detach() and passed it to finish_tmp_packfile().\n\nInside finish_tmp_packfile, I attached the same buffer to a local\nstruct strbuf using strbuf_attach().\nNow the problem is, two of the arguments to strbuf_attach() are\n'alloc' and 'len' which are private members of the struct strbuf.\nBut since I am just passing the detached buffer, the information of\nalloc and len is lost which is required at the time of attaching.\nI cannot think of any better way of using strbuf and NOT modify the\nprototype of finish_tmp_packfile()\n\nAs a workaround, I can determine alloc = (strlen(buf) + 1) and len =\nstrlen(buf) but AFAIK this is not always true and may break.\nAny suggestions?\n\nThanks.\n\nOn Fri, Feb 28, 2014 at 2:45 PM, Eric Sunshine wrote:\n> On Fri, Feb 28, 2014 at 2:58 AM, Faiz Kothari wrote:\n>> Signed-off-by: Faiz Kothari \n>>\n>> Notes:\n>> I finally got what's happening, and why the errors were caused.\n>> packname is supposed to contain the complete path to the .pack file.\n>> Packs are stored as /path/to/.pack which I overlooked earlier.\n>> After inspecting what is happening in pack-write.c:finish_tmp_packfile()\n>> which indirectly modifies packname by appending the SHA1 and \".pack\" to packname\n>> This is happening in these code snippets:\n>> char *end_of_name_prefix = strrchr(name_buffer, 0);\n>>\n>> and later\n>> sprintf(end_of_name_prefix, \"%s.pack\", sha1_to_hex(sha1));\n>>\n>> name_buffer is packname.buf\n>> Using const for the first argument of pack-write.c:finish_tmp_packfile()\n>> doesnot raise any compile time warning or error and not any runtime errors,\n>> since the packname.buf is on heap and has extra space to which more char can be written.\n>> If this was not the case,\n>> for e.g. passing a constant string and modifying it.\n>> This will result in a segmentation fault.\n>> ---\n>\n> This notes section is important to the ongoing email discussion,\n> however, it should be placed below the \"---\" line so that it does not\n> become part of the recorded commit message when the patch is applied\n> via \"git am\".\n>\n>> bulk-checkin.c | 8 +++++---\n>> pack-write.c | 2 +-\n>> pack.h | 2 +-\n>> 3 files changed, 7 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/bulk-checkin.c b/bulk-checkin.c\n>> index 118c625..bbdf1ec 100644\n>> --- a/bulk-checkin.c\n>> +++ b/bulk-checkin.c\n>> @@ -23,7 +23,7 @@ static struct bulk_checkin_state {\n>> static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>> {\n>> unsigned char sha1[20];\n>> - char packname[PATH_MAX];\n>> + struct strbuf packname = STRBUF_INIT;\n>> int i;\n>>\n>> if (!state->f)\n>> @@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>> state->offset);\n>> close(fd);\n>> }\n>> + strbuf_addf(&packname, \"%s/pack/pack-\", get_object_directory());\n>> + strbuf_grow(&packname, 40 + 5);\n>\n> There are several problems with this. First, magic numbers 40 and 5\n> convey no meaning to the reader. At the very least, they should be\n> named constants or a comment should explain them. More seriously,\n> though, this code is fragile since it has far too intimate knowledge\n> of the inner workings of finish_tmp_packfile(). If the implementation\n> of finish_tmp_packfile() changes in the future such that it writes\n> more than 45 additional characters to the incoming buffer, this will\n> break.\n>\n> Rather than coupling finish_bulk_checkin() and finish_tmp_packfile()\n> so tightly, consider finish_tmp_packfile() a black box which just\n> \"does its job\" and then propose ways to make things work without\n> finish_bulk_checkin() having to know how that job is done.\n>\n>> - sprintf(packname, \"%s/pack/pack-\", get_object_directory());\n>> - finish_tmp_packfile(packname, state->pack_tmp_name,\n>> + finish_tmp_packfile(packname.buf, state->pack_tmp_name,\n>> state->written, state->nr_written,\n>> &state->pack_idx_opts, sha1);\n>> for (i = 0; i < state->nr_written; i++)\n>> diff --git a/pack-write.c b/pack-write.c\n>> index 605d01b..ac38867 100644\n>> --- a/pack-write.c\n>> +++ b/pack-write.c\n>> @@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)\n>> return sha1fd(fd, *pack_tmp_name);\n>> }\n>>\n>> -void finish_tmp_packfile(char *name_buffer,\n>> +void finish_tmp_packfile(const char *name_buffer,\n>\n> This is misleading and fragile. By specifying 'const',\n> finish_tmp_packfile() promises not to modify the content of the\n> incoming name_buffer, yet it breaks this promise by modifying the\n> buffer through the non-const end_of_name_prefix variable (after\n> dropping the 'const' via strrchr()).\n>\n>> const char *pack_tmp_name,\n>> struct pack_idx_entry **written_list,\n>> uint32_t nr_written,\n"},{"id":"235672","messageId":"CAPig+cTGL+s5nT9rgcHhsM+oPTjoJPaorFKHmYtGJ=ZCk4Rm=A@mail.gmail.com","threadId":"35989","inReplyTo":"CAFbjVcnx41CTOV-SCGC9a91b9weNTgYoRP1+uVDukp6Z+0F_yg@mail.gmail.com","subject":"Re: [PATCH] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-28T21:59:16Z","receivedAt":"2014-02-28T21:59:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 28, 2014 at 1:27 PM, Faiz Kothari wrote:\n> Thanks for the suggestions and remarks.\n\n[Administrivia: On this list, top-posting is frowned upon; inline\nresponses are preferred.]\n\n> I rewrote bulk-checkin.c:finish_bulk_checkin() using strbuf. But saw\n> that Sun He has already implemented the same way I have done.\n> Should I submit my implementation as a patch?\n\nYes. The purpose of these micro-projects is to expose you to the Git\nproject's development process so that you know what will be expected\nof you as a GSoC student, and to give the GSoC mentors an opportunity\nto evaluate your abilities and observe how you interact with the\nreviewers.\n\n> Secondly,\n> I tried implementing this WITHOUT changing the prototype of the\n> function pack-write.c:finish_tmp_packfile().\n>\n> For this I detached the buffer from strbuf in finish_bulk_checkin()\n> using strbuf_detach() and passed it to finish_tmp_packfile().\n>\n> Inside finish_tmp_packfile, I attached the same buffer to a local\n> struct strbuf using strbuf_attach().\n> Now the problem is, two of the arguments to strbuf_attach() are\n> 'alloc' and 'len' which are private members of the struct strbuf.\n> But since I am just passing the detached buffer, the information of\n> alloc and len is lost which is required at the time of attaching.\n> I cannot think of any better way of using strbuf and NOT modify the\n> prototype of finish_tmp_packfile()\n>\n> As a workaround, I can determine alloc = (strlen(buf) + 1) and len =\n> strlen(buf) but AFAIK this is not always true and may break.\n> Any suggestions?\n\nThat's getting rather convoluted. You may want to ask yourself if it\nis really necessary for finish_tmp_packfile() to modify the buffer\npassed in by the caller or if finish_pack_file() should instead take\nresponsibility for itself by allocating its own buffer (strbuf) in\nwhich to do path manipulation.\n"},{"id":"235686","messageId":"CAJr59C12U0A2bOjD62RLS-wws0SePT8kd3bUhxh0NMi5aapVQQ@mail.gmail.com","threadId":"35989","inReplyTo":"CAFbjVcnx41CTOV-SCGC9a91b9weNTgYoRP1+uVDukp6Z+0F_yg@mail.gmail.com","subject":"Re: [PATCH] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf","fromName":"He Sun","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-03-01T01:34:23Z","receivedAt":"2014-03-01T01:34:23Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"2014-03-01 2:27 GMT+08:00 Faiz Kothari :\n> Hi,\n> Thanks for the suggestions and remarks.\n> I rewrote bulk-checkin.c:finish_bulk_checkin() using strbuf. But saw\n> that Sun He has already implemented the same way I have done.\n> Should I submit my implementation as a patch?\n>\n> Secondly,\n> I tried implementing this WITHOUT changing the prototype of the\n> function pack-write.c:finish_tmp_packfile().\n>\n\nI used to think the same as you.\n\n> For this I detached the buffer from strbuf in finish_bulk_checkin()\n> using strbuf_detach() and passed it to finish_tmp_packfile().\n>\n> Inside finish_tmp_packfile, I attached the same buffer to a local\n> struct strbuf using strbuf_attach().\n> Now the problem is, two of the arguments to strbuf_attach() are\n> 'alloc' and 'len' which are private members of the struct strbuf.\n> But since I am just passing the detached buffer, the information of\n> alloc and len is lost which is required at the time of attaching.\n\nOne stupid solution may be that, alloc 8 byte that attached to strbuf's buf,\nand fill in the len and alloc. We can take them out in finish_tmp_packfile.\n\nBut this may cause potential problems, that functions should have as rare\nrelationship as they could.\nThe goal of limiting the changes in one function is for this purpose.\n\nSo may be change the first paramater of finish_tmp_packfile is the best way\nto deal with this situation.\n\n> I cannot think of any better way of using strbuf and NOT modify the\n> prototype of finish_tmp_packfile()\n>\n> As a workaround, I can determine alloc = (strlen(buf) + 1) and len =\n> strlen(buf) but AFAIK this is not always true and may break.\n> Any suggestions?\n>\n> Thanks.\n>\n> On Fri, Feb 28, 2014 at 2:45 PM, Eric Sunshine wrote:\n>> On Fri, Feb 28, 2014 at 2:58 AM, Faiz Kothari wrote:\n>>> Signed-off-by: Faiz Kothari \n>>>\n>>> Notes:\n>>> I finally got what's happening, and why the errors were caused.\n>>> packname is supposed to contain the complete path to the .pack file.\n>>> Packs are stored as /path/to/.pack which I overlooked earlier.\n>>> After inspecting what is happening in pack-write.c:finish_tmp_packfile()\n>>> which indirectly modifies packname by appending the SHA1 and \".pack\" to packname\n>>> This is happening in these code snippets:\n>>> char *end_of_name_prefix = strrchr(name_buffer, 0);\n>>>\n>>> and later\n>>> sprintf(end_of_name_prefix, \"%s.pack\", sha1_to_hex(sha1));\n>>>\n>>> name_buffer is packname.buf\n>>> Using const for the first argument of pack-write.c:finish_tmp_packfile()\n>>> doesnot raise any compile time warning or error and not any runtime errors,\n>>> since the packname.buf is on heap and has extra space to which more char can be written.\n>>> If this was not the case,\n>>> for e.g. passing a constant string and modifying it.\n>>> This will result in a segmentation fault.\n>>> ---\n>>\n>> This notes section is important to the ongoing email discussion,\n>> however, it should be placed below the \"---\" line so that it does not\n>> become part of the recorded commit message when the patch is applied\n>> via \"git am\".\n>>\n>>> bulk-checkin.c | 8 +++++---\n>>> pack-write.c | 2 +-\n>>> pack.h | 2 +-\n>>> 3 files changed, 7 insertions(+), 5 deletions(-)\n>>>\n>>> diff --git a/bulk-checkin.c b/bulk-checkin.c\n>>> index 118c625..bbdf1ec 100644\n>>> --- a/bulk-checkin.c\n>>> +++ b/bulk-checkin.c\n>>> @@ -23,7 +23,7 @@ static struct bulk_checkin_state {\n>>> static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>>> {\n>>> unsigned char sha1[20];\n>>> - char packname[PATH_MAX];\n>>> + struct strbuf packname = STRBUF_INIT;\n>>> int i;\n>>>\n>>> if (!state->f)\n>>> @@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>>> state->offset);\n>>> close(fd);\n>>> }\n>>> + strbuf_addf(&packname, \"%s/pack/pack-\", get_object_directory());\n>>> + strbuf_grow(&packname, 40 + 5);\n>>\n>> There are several problems with this. First, magic numbers 40 and 5\n>> convey no meaning to the reader. At the very least, they should be\n>> named constants or a comment should explain them. More seriously,\n>> though, this code is fragile since it has far too intimate knowledge\n>> of the inner workings of finish_tmp_packfile(). If the implementation\n>> of finish_tmp_packfile() changes in the future such that it writes\n>> more than 45 additional characters to the incoming buffer, this will\n>> break.\n>>\n>> Rather than coupling finish_bulk_checkin() and finish_tmp_packfile()\n>> so tightly, consider finish_tmp_packfile() a black box which just\n>> \"does its job\" and then propose ways to make things work without\n>> finish_bulk_checkin() having to know how that job is done.\n>>\n>>> - sprintf(packname, \"%s/pack/pack-\", get_object_directory());\n>>> - finish_tmp_packfile(packname, state->pack_tmp_name,\n>>> + finish_tmp_packfile(packname.buf, state->pack_tmp_name,\n>>> state->written, state->nr_written,\n>>> &state->pack_idx_opts, sha1);\n>>> for (i = 0; i < state->nr_written; i++)\n>>> diff --git a/pack-write.c b/pack-write.c\n>>> index 605d01b..ac38867 100644\n>>> --- a/pack-write.c\n>>> +++ b/pack-write.c\n>>> @@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)\n>>> return sha1fd(fd, *pack_tmp_name);\n>>> }\n>>>\n>>> -void finish_tmp_packfile(char *name_buffer,\n>>> +void finish_tmp_packfile(const char *name_buffer,\n>>\n>> This is misleading and fragile. By specifying 'const',\n>> finish_tmp_packfile() promises not to modify the content of the\n>> incoming name_buffer, yet it breaks this promise by modifying the\n>> buffer through the non-const end_of_name_prefix variable (after\n>> dropping the 'const' via strrchr()).\n>>\n>>> const char *pack_tmp_name,\n>>> struct pack_idx_entry **written_list,\n>>> uint32_t nr_written,\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\nCheers,\nHe Sun\n"}]}