Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

On 20/07/14 13:13, Nguyễn Thái Ngọc Duy wrote:
> Locked paths are saved in a linked list so that if something wrong
> happens, *.lock are removed. This works fine if we keep cwd the same,
> which is true 99% of time except:
> 
>  - update-index and read-tree hold the lock on $GIT_DIR/index really
>    early, then later on may call setup_work_tree() to move cwd.
> 
>  - Suppose a lock is being held (e.g. by "git add") then somewhere
>    down the line, somebody calls real_path (e.g. "link_alt_odb_entry"),
>    which temporarily moves cwd away and back.
> 
> During that time when cwd is moved (either permanently or temporarily)
> and we decide to die(), attempts to remove relative *.lock will fail,
> and the next operation will complain that some files are still locked.
> 
> Avoid this case by turning relative paths to absolute when chdir() is
> called (or soon to be called, in setup_git_directory_gently case).
> 
> Reported-by: Yue Lin Ho <yuelinho777@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

[snip]

> diff --git a/lockfile.c b/lockfile.c
> index 968b28f..cf1e795 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -288,3 +288,15 @@ void rollback_lock_file(struct lock_file *lk)
>  	}
>  	clear_filename(lk);
>  }
> +
> +void make_locked_paths_absolute(void)
> +{
> +	struct lock_file *lk;
> +	for (lk = lock_file_list; lk != NULL; lk = lk->next) {
> +		if (lk->filename && !is_absolute_path(lk->filename)) {
> +			char *to_free = lk->filename;
> +			lk->filename = xstrdup(absolute_path(lk->filename));
> +			free(to_free);
> +		}
> +	}
> +}

I just have to ask, why are we putting relative pathnames in this
list to begin with? Why not use an absolute path when taking the
lock in all cases? (calling absolute_path() and using the result
to take the lock, storing it in the lock_file list, should not be
in the critical path, right? Not that I have measured it, of course! :)

ATB,
Ramsay Jones