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

On Fri, Sep 12, 2014 at 10:13 AM, Michael Haggerty [off-list ref] wrote:

On 09/12/2014 12:42 AM, Ronnie Sahlberg wrote:

quoted

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.

It is OK with me.