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