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@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