Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed
On Thu, Jul 31, 2014 at 10:01 AM, Yue Lin Ho wrote: > Hi: > How do I trace these patches applied? They are not applied yet. I'll needto redo them on top of rs/strbuf-getcwd. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed
Hi: > 2014-07-23 19:55 GMT+08:00 Duy Nguyen : > On Tue, Jul 22, 2014 at 12:04 AM, Junio C Hamano > wrote: > > Duy Nguyen writes: [snip] > > OK, we should center these efforts around the strbuf_getcwd() topic, > > basing the other topic on realpath() and this one on it then? > > OK. > -- > Duy Excuse me. How do I trace these patches applied? I just fetch from https://github.com/gitster/git.git Then tried to find these patches if it is applied. (Seems not.) Then, I took a look at http://git.661346.n2.nabble.com/What-s-cooking-in-git-git-Jul-2014-04-Tue-22-td7615627.html seems no related information there. So, could you please tell me how to trace it? Thank you. ^_^ Yue Lin Ho -- View this message in context: http://git.661346.n2.nabble.com/PATCH-Make-locked-paths-absolute-when-current-directory-is-changed-tp7615398p7616119.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed
On Tue, Jul 22, 2014 at 12:04 AM, Junio C Hamano wrote:
> Duy Nguyen writes:
>
>> On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones
>> 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_...*
>
> OK, we should center these efforts around the strbuf_getcwd() topic,
> basing the other topic on realpath() and this one on it then?
OK.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed
Duy Nguyen writes:
> On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones
> 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_...*
OK, we should center these efforts around the strbuf_getcwd() topic,
basing the other topic on realpath() and this one on it then?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed
On 21/07/14 14:47, Duy Nguyen wrote:
> On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed
On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones
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_...*
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed
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
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] Make locked paths absolute when current directory is changed
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
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Compared to v1, make_locked_paths_absolute() now always succeeds (ok
absolute_path could die inside, but that's a separate problem) and
supports Windows.
abspath.c | 2 +-
cache.h | 6 ++
git.c | 6 +++---
lockfile.c| 12
path.c| 4 ++--
run-command.c | 2 +-
setup.c | 3 ++-
unix-socket.c | 2 +-
8 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/abspath.c b/abspath.c
index ca33558..78c963f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -87,7 +87,7 @@ static const char *real_path_internal(const char *path, int
die_on_error)
goto error_out;
}
- if (chdir(buf)) {
+ if (chdir_safe(buf)) {
if (die_on_error)
die_errno("Could not switch to '%s'",
buf);
else
diff --git a/cache.h b/cache.h
index 9ecb636..48ffa21 100644
--- a/cache.h
+++ b/cache.h
@@ -564,6 +564,12 @@ extern int hold_lock_file_for_update(struct lock_file *,
const char *path, int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
extern int commit_lock_file(struct lock_file *);
extern void update_index_if_able(struct index_state *, struct lock_file *);
+extern void make_locked_paths_absolute(void);
+static inline int chdir_safe(const char *path)
+{
+ make_locked_paths_absolute();
+ return chdir(path);
+}
extern int hold_locked_index(struct lock_file *, int);
extern int commit_locked_index(struct lock_file *);
diff --git a/git.c b/git.c
index 5b6c761..27766c3 100644
--- a/git.c
+++ b/git.c
@@ -48,7 +48,7 @@ static void save_env(void)
static void restore_env(void)
{
int i;
- if (*orig_cwd && chdir(orig_cwd))
+ if (*orig_cwd && chdir_safe(orig_cwd))
die_errno("could not move to %s", orig_cwd);
for (i = 0; i < ARRAY_SIZE(env_names); i++) {
if (orig_env[i])
@@ -206,7 +206,7 @@ static int handle_options(const char ***argv, int *argc,
int *envchanged)
fprintf(stderr, "No directory given for -C.\n"
);
usage(git_usage_string);
}
- if (chdir((*argv)[1]))
+ if (chdir_safe((*argv)[1]))
die_errno("Cannot change to '%s'", (*argv)[1]);
if (envchanged)
*envchanged = 1;
@@ -292,7 +292,7 @@ static int handle_alias(int *argcp, const char ***argv)
ret = 1;
}
- if (subdir && chdir(subdir))
+ if (subdir && chdir_safe(subdir))
die_errno("Cannot change to '%s'", subdir);
errno = saved_errno;
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);
+ }
+ }
+}
diff --git a/path.c b/path.c
index bc804a3..9e8e101 100644
--- a/path.c
+++ b/path.c
@@ -372,11 +372,11 @@ const char *enter_repo(const char *path, int strict)
gitfile = read_gitfile(used_path) ;
if (gitfile)
strcpy(used_path, gitfile);
- if (chdir(used_path))
+ if (chdir_safe(used_path))
return NULL;
path = validated_path;
}
- else if (chdir(path))
+ else if (chdir_safe(path))
return NULL;
if (access("objects", X_OK) == 0 && access("refs", X_O
