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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 12, 2014, 17:13 UTC
Message-ID: <541329B8.1050200@alum.mit.edu>
In-Reply-To: <CAL=YDWmgwwQsCSpVsQ5U+c=4RcLdzo-j_hmxDk-AJWdRUk_SsQ@mail.gmail.com>

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

Show 26 quoted lines

> 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