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
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
