Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Somewhat underexplained, given that it seems to add some new
semantics.

> +static void clear_filename(struct lock_file *lk)
> +{
> +	free(lk->filename);
> +	lk->filename = NULL;
> +}

It is good to abstract out lk->filename[0] = '\0', which used to be
the way we say that we are done with the lock.  But I am somewhat
surprised to see that there aren't so many locations that used to
check !!lk->filename[0] to see if we are done with the lock to require
a corresponding wrapper.

>  static void remove_lock_file(void)
>  {
>  	pid_t me = getpid();
>  
>  	while (lock_file_list) {
>  		if (lock_file_list->owner == me &&
> -		    lock_file_list->filename[0]) {
> +		    lock_file_list->filename) {

... and this seems to be the only location?

> @@ -124,17 +136,12 @@ static char *resolve_symlink(char *p, size_t s)
>  
>  static int lock_file(struct lock_file *lk, const char *path, int flags)
>  {
> -	/*
> -	 * subtract 5 from size to make sure there's room for adding
> -	 * ".lock" for the lock file name
> -	 */
> -	static const size_t max_path_len = sizeof(lk->filename) - 5;
> -
> -	if (strlen(path) >= max_path_len)
> +	int len;
> +	if (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))
>  		return -1;

Somehow I found it unnecessarily denser; had to read it twice before
caffeine kicked in ;-)

> @@ -231,16 +238,17 @@ int close_lock_file(struct lock_file *lk)
>  
>  int commit_lock_file(struct lock_file *lk)
>  {
> -	char result_file[PATH_MAX];
> -	size_t i;
> -	if (lk->fd >= 0 && close_lock_file(lk))
> +	char *result_file;
> +	if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)
>  		return -1;

We did not protect against somebody calling this with an already
closed lock, but we now return early without attempting renameing
etc., which is a good change but is not explained.  Was there a
specific code path that you needed this change for?

Also the order of the check is not consistent with how the same
check is done in rollback_lock_file().  The order you use in this
new code (and also in commit_locked_index()) may be better than the
existing order in the rollback code path; we want to see the fd
closed, if it is open, even if lk->filename has already been
cleared.  On the other hand, one could argue that anything such a
broken caller tells this function is suspicious, and we shouldn't
close random file descriptor that is likely not owned by the caller
in the first place.  I dunno.

> @@ -273,10 +281,10 @@ int commit_locked_index(struct lock_file *lk)
>  
>  void rollback_lock_file(struct lock_file *lk)
>  {
> -	if (lk->filename[0]) {
> +	if (lk->filename) {
>  		if (lk->fd >= 0)
>  			close(lk->fd);
>  		unlink_or_warn(lk->filename);
>  	}
> -	lk->filename[0] = 0;
> +	clear_filename(lk);
>  }