From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 22 Sep 2014 15:25:18 GMT
Subject: Re: [PATCH v5 10/35] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
Message-ID: <54203F5E.5090805@alum.mit.edu>
In-Reply-To: <20140916210538.GK29050@google.com>

On 09/16/2014 11:05 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> There are a few places that use these values, so define constants for
>> them.
> 
> Seems like a symptom of the API leaving out a useful helper (e.g.,
> something that strips off the lock suffix and returns a memdupz'd
> filename).

I will add a function get_locked_file_path().

> [...]
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -570,6 +570,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
>>  #define REFRESH_IN_PORCELAIN	0x0020	/* user friendly output, not "needs update" */
>>  extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
>>  
>> +/* String appended to a filename to derive the lockfile name: */
>> +#define LOCK_SUFFIX ".lock"
>> +#define LOCK_SUFFIX_LEN 5
> 
> My suspicion is that error handling would be better if fewer callers
> outside of lockfile.c did the '- LOCK_SUFFIX_LEN' dance, so this seems
> like a step in the wrong direction.
> 
> Adding constants in lockfile.c would make sense, though.

I agree that other call sites shouldn't be grubbing around in the
lock_file data structure. But with the addition of
get_locked_file_path(), the only remaining user of these constants is in
refs.c, in check_refname_component():

	if (cp - refname >= LOCK_SUFFIX_LEN &&
	    !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))
		return -1; /* Refname ends with ".lock". */

I think it is forgivable there.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu