Hi,
Thanks for the suggestions and remarks.
I rewrote bulk-checkin.c:finish_bulk_checkin() using strbuf. But saw
that Sun He has already implemented the same way I have done.
Should I submit my implementation as a patch?
Secondly,
I tried implementing this WITHOUT changing the prototype of the
function pack-write.c:finish_tmp_packfile().
For this I detached the buffer from strbuf in finish_bulk_checkin()
using strbuf_detach() and passed it to finish_tmp_packfile().
Inside finish_tmp_packfile, I attached the same buffer to a local
struct strbuf using strbuf_attach().
Now the problem is, two of the arguments to strbuf_attach() are
'alloc' and 'len' which are private members of the struct strbuf.
But since I am just passing the detached buffer, the information of
alloc and len is lost which is required at the time of attaching.
I cannot think of any better way of using strbuf and NOT modify the
prototype of finish_tmp_packfile()
As a workaround, I can determine alloc = (strlen(buf) + 1) and len =
strlen(buf) but AFAIK this is not always true and may break.
Any suggestions?
Thanks.
On Fri, Feb 28, 2014 at 2:45 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Fri, Feb 28, 2014 at 2:58 AM, Faiz Kothari <faiz.off93@xxxxxxxxx> wrote:
>> Signed-off-by: Faiz Kothari <faiz.off93@xxxxxxxxx>
>>
>> Notes:
>> I finally got what's happening, and why the errors were caused.
>> packname is supposed to contain the complete path to the .pack file.
>> Packs are stored as /path/to/<SHA1>.pack which I overlooked earlier.
>> After inspecting what is happening in pack-write.c:finish_tmp_packfile()
>> which indirectly modifies packname by appending the SHA1 and ".pack" to packname
>> This is happening in these code snippets:
>> char *end_of_name_prefix = strrchr(name_buffer, 0);
>>
>> and later
>> sprintf(end_of_name_prefix, "%s.pack", sha1_to_hex(sha1));
>>
>> name_buffer is packname.buf
>> Using const for the first argument of pack-write.c:finish_tmp_packfile()
>> doesnot raise any compile time warning or error and not any runtime errors,
>> since the packname.buf is on heap and has extra space to which more char can be written.
>> If this was not the case,
>> for e.g. passing a constant string and modifying it.
>> This will result in a segmentation fault.
>> ---
>
> This notes section is important to the ongoing email discussion,
> however, it should be placed below the "---" line so that it does not
> become part of the recorded commit message when the patch is applied
> via "git am".
>
>> bulk-checkin.c | 8 +++++---
>> pack-write.c | 2 +-
>> pack.h | 2 +-
>> 3 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/bulk-checkin.c b/bulk-checkin.c
>> index 118c625..bbdf1ec 100644
>> --- a/bulk-checkin.c
>> +++ b/bulk-checkin.c
>> @@ -23,7 +23,7 @@ static struct bulk_checkin_state {
>> static void finish_bulk_checkin(struct bulk_checkin_state *state)
>> {
>> unsigned char sha1[20];
>> - char packname[PATH_MAX];
>> + struct strbuf packname = STRBUF_INIT;
>> int i;
>>
>> if (!state->f)
>> @@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
>> state->offset);
>> close(fd);
>> }
>> + strbuf_addf(&packname, "%s/pack/pack-", get_object_directory());
>> + strbuf_grow(&packname, 40 + 5);
>
> There are several problems with this. First, magic numbers 40 and 5
> convey no meaning to the reader. At the very least, they should be
> named constants or a comment should explain them. More seriously,
> though, this code is fragile since it has far too intimate knowledge
> of the inner workings of finish_tmp_packfile(). If the implementation
> of finish_tmp_packfile() changes in the future such that it writes
> more than 45 additional characters to the incoming buffer, this will
> break.
>
> Rather than coupling finish_bulk_checkin() and finish_tmp_packfile()
> so tightly, consider finish_tmp_packfile() a black box which just
> "does its job" and then propose ways to make things work without
> finish_bulk_checkin() having to know how that job is done.
>
>> - sprintf(packname, "%s/pack/pack-", get_object_directory());
>> - finish_tmp_packfile(packname, state->pack_tmp_name,
>> + finish_tmp_packfile(packname.buf, state->pack_tmp_name,
>> state->written, state->nr_written,
>> &state->pack_idx_opts, sha1);
>> for (i = 0; i < state->nr_written; i++)
>> diff --git a/pack-write.c b/pack-write.c
>> index 605d01b..ac38867 100644
>> --- a/pack-write.c
>> +++ b/pack-write.c
>> @@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)
>> return sha1fd(fd, *pack_tmp_name);
>> }
>>
>> -void finish_tmp_packfile(char *name_buffer,
>> +void finish_tmp_packfile(const char *name_buffer,
>
> This is misleading and fragile. By specifying 'const',
> finish_tmp_packfile() promises not to modify the content of the
> incoming name_buffer, yet it breaks this promise by modifying the
> buffer through the non-const end_of_name_prefix variable (after
> dropping the 'const' via strrchr()).
>
>> const char *pack_tmp_name,
>> struct pack_idx_entry **written_list,
>> uint32_t nr_written,
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html