Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed
- From
Ramsay Jones <ramsay@ramsay1.demon.co.uk>
- Date
- Jul 21, 2014, 14:23 UTC
- Message-ID
- <53CD227F.5070708@ramsay1.demon.co.uk>
- In-Reply-To
- <CACsJy8AXc4jvLPNpGyGdY9uzrnN-SbEeiksLDpS_=29gJ1KMnQ@mail.gmail.com>
On 21/07/14 14:47, Duy Nguyen wrote:
Show 27 quoted lines
> On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones
> <ramsay@ramsay1.demon.co.uk> wrote:
>>> +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! :)
>
> Conservative :) I'm still scared from 044bbbc (Make git_dir a path
> relative to work_tree in setup_work_tree() - 2008-06-19). But yeah
> looking through "grep hold_" I think none of the locks is in critical
> path. absolute_path() can die() if cwd is longer than PATH_MAX (and
> doing this reduces the chances of that happening). But René is adding
> strbuf_getcwd() that can remove that PATH_MAX. So I guess we should be
> fine with putting absolute_path() in hold_lock_file_...*Hmm, yes, thank you for reminding me about 044bbbc. So, yes it could cause a (small) performance hit and a change in behaviour (die) in deeply nested working directories. Hmm, OK.
ATB, Ramsay Jones