Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Michael Haggerty <mhagger@alum.mit.edu>]

On 09/12/2014 12:42 AM, Ronnie Sahlberg wrote:
> Maybe we should not have a public constant defined for the length :
> +#define LOCK_SUFFIX_LEN 5
> 
> since it encourages unsafe code like :  (this was unsafe long before
> your patch so not a regression)
> +       i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
>         result_file[i] = 0;
> 
> 
> 
> What about removing LOCK_SUFFIX_LEN from the public API and introduce
> a helper function something like :
> 
> 
> /* pointer to the character where the lock suffix starts */
> char *lock_suffix_ptr_safe(const char *filename)
> {
>     size_t len = strlen(filename);
>     if (len < 5)
>        die("BUG:...
>     if (strcmp(filename + len - 5, LOCK_SUFFIX)
>        die("BUG:...
>     return filename + len - 5;
> }
> 
> and use it instead?

At the end of this patch series, LOCK_SUFFIX_LEN is only used in two
places outside of lockfile.c:

* In check_refname_component(), to ensure that no component of a
reference name ends with ".lock". This only indirectly has anything to
do with lockfiles.

* In delete_ref_loose(), to derive the name of the loose reference file
from the name of the lockfile. It immediately xmemdupz()s the part of
the filename that it needs, so it is kosher.

I will add a function get_locked_file_path() for the use of the second
caller.

I like being able to use the symbolic constant at the first caller, and
it is not dangerous. I don't think it is so important to make the
constant private, because I think somebody programming sloppily wouldn't
be deterred for long by not seeing a symbolic constant for the suffix
length. So if it's OK with you I'll leave the constant.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu