From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 05 Jul 2018 19:13:07 GMT
Subject: Re: [PATCH v2 05/24] midx: write header information to lockfile
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:
> 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.

>
>> +	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