Re: [PATCH v2 05/24] midx: write header information to lockfile
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jul 5, 2018, 19:13 UTC
- Message-ID
- <41812292-3f1e-adaa-3faa-33f5e8f8e47a@gmail.com>
- In-Reply-To
- <xmqqmuvioeqq.fsf@gitster-ct.c.googlers.com>
On 6/25/2018 3:19 PM, Junio C Hamano wrote:
Show 53 quoted lines
> Derrick Stolee <stolee@gmail.com> writes:
>
>> +#define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
>> +#define MIDX_VERSION 1
>> +#define MIDX_HASH_VERSION 1
>> +#define MIDX_HEADER_SIZE 12
>> +
>> +static char *get_midx_filename(const char *object_dir)
>> +{
>> + return xstrfmt("%s/pack/multi-pack-index", object_dir);
>> +}
>> +
>> +static size_t write_midx_header(struct hashfile *f,
>> + unsigned char num_chunks,
>> + uint32_t num_packs)
>> +{
>> + unsigned char byte_values[4];
>> + hashwrite_be32(f, MIDX_SIGNATURE);
> WARNING: Missing a blank line after declarations
> #48: FILE: midx.c:21:
> + unsigned char byte_values[4];
> + hashwrite_be32(f, MIDX_SIGNATURE);
>
>> + byte_values[0] = MIDX_VERSION;
>> + byte_values[1] = MIDX_HASH_VERSION;
>> + byte_values[2] = num_chunks;
>> + byte_values[3] = 0; /* unused */
>> + hashwrite(f, byte_values, sizeof(byte_values));
>> + hashwrite_be32(f, num_packs);
>> +
>> + return MIDX_HEADER_SIZE;
>> +}
>> +
>> int write_midx_file(const char *object_dir)
>> {
>> + unsigned char num_chunks = 0;
>> + char *midx_name;
>> + struct hashfile *f;
>> + struct lock_file lk;
>> +
>> + midx_name = get_midx_filename(object_dir);
>> + if (safe_create_leading_directories(midx_name)) {
>> + UNLEAK(midx_name);
>> + die_errno(_("unable to create leading directories of %s"),
>> + midx_name);
>> + }
>> +
>> + hold_lock_file_for_update(&lk, midx_name, LOCK_DIE_ON_ERROR);
>> + f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);
>> + FREE_AND_NULL(midx_name);
> I am not sure why people prefer FREE_AND_NULL over free() for things
> like this. It is on stack; it's not like this is a static variable
> visible after this function returns or anything like that.I default to FREE_AND_NULL(X) because a later change may introduce logic to use X later in the same code block. In this case, we add a 'cleanup:' at the end which would fail in a success case if we don't set midx_name to NULL here.
I think there are some other FREE_AND_NULLs that are currently at the end of a code block, so I'll work to clean those up.
Show 22 quoted lines
> >> + write_midx_header(f, num_chunks, 0); >> + >> + finalize_hashfile(f, NULL, CSUM_FSYNC | CSUM_HASH_IN_STREAM); >> + commit_lock_file(&lk); >> + >> return 0; >> } >> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh >> index ec3ddbe79c..8622a7cdce 100755 >> --- a/t/t5319-multi-pack-index.sh >> +++ b/t/t5319-multi-pack-index.sh >> @@ -4,7 +4,8 @@ test_description='multi-pack-indexes' >> . ./test-lib.sh >> >> test_expect_success 'write midx with no packs' ' >> - git multi-pack-index --object-dir=. write >> + git multi-pack-index --object-dir=. write && >> + test_path_is_file pack/multi-pack-index >> ' >> >> test_done