Re: [PATCH/RFC v2 2/3] make USE_NSEC work as expected

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 21, 2009, 05:42 UTC
Message-ID: <7vhc2os5vi.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <86mychifqj.fsf@broadpark.no>

Kjetil Barvik <barvik@broadpark.no> writes:

Show 16 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> +	ce->ce_ctime.sec = (unsigned int)st->st_ctime;
>>> +	ce->ce_mtime.sec = (unsigned int)st->st_mtime;
>>> +#ifdef USE_NSEC
>>> +	ce->ce_ctime.nsec = (unsigned int)st->st_ctim.tv_nsec;
>>> +	ce->ce_mtime.nsec = (unsigned int)st->st_mtim.tv_nsec;
>>> +#else
>>> +	ce->ce_ctime.nsec = 0;
>>> +	ce->ce_mtime.nsec = 0;
>>> +#endif
>>
>> How does this affect a use case where the same index file used with two 
>> instances of git (one compiled with and another without USE_NSEC)?
>
>   OK, I admit that I was thinking safe here,...

It was not a veiled objection in the guise of a rhetoric question. I just wanted to know what happens, when you have "/usr/bin/git" compiled without NSEC and "/usr/local/bin/git" compiled with NSEC, and tried to use the two interchangeably. A NSEC enabled one will leave nsec in the index entry, and the normal one reads from the index file (truncating the nsec from both file timestamp and on-disk index entries), and it is unclear what it does to the racy-git algorithm. If there is no adverse effect, that is great. Otherwise, even though the on-disk format might be compatible, we need to somehow tell people not to use two gits on the same index file.

Show 21 quoted lines

>>> diff --git a/unpack-trees.c b/unpack-trees.c
>>> index e547282..44714cc 100644
>>> --- a/unpack-trees.c
>>> +++ b/unpack-trees.c
>>> @@ -380,8 +380,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>>>  
>>>  	memset(&o->result, 0, sizeof(o->result));
>>>  	o->result.initialized = 1;
>>> -	if (o->src_index)
>>> -		o->result.timestamp = o->src_index->timestamp;
>>> +	if (o->src_index) {
>>> +		o->result.timestamp.sec = o->src_index->timestamp.sec;
>>> +#ifdef USE_NSEC
>>> +		o->result.timestamp.nsec = o->src_index->timestamp.nsec;
>>> +#endif
>>> +	}
>>
>> Do we need this hunk?
>
>   Since timestamp is now a 'struct cache_time' member, I converted the
>   usage of this if-test to be in line with the USE_NSEC usage.

My question was if it is wrong to leave this as "a->timestamp = b->timestamp" which now becomes structure assignment. You would need to move extra 4 (or 8) bytes if you are not using NSEC, but this is not per index entry but one assignment for the whole index file, so...