[PATCH v2 0/4] getcwd without PATH_MAX
Paths longer than PATH_MAX can be created and used on at least on some file systems. Currently we use getcwd() generally with a PATH_MAX- sized buffer. This series adds two functions, strbuf_getcwd() and xgetcwd(), then uses them to reduce the number of fixed-sized buffers and to allow us to handle longer working directory paths. Not all getcwd() calls are replaced, however. I hope that at least some of the remaining ones can be avoided altogether. If that turns out to be too optimistic then we can use the added function to convert the rest. Changes in v2: * strbuf_getcwd() instead of strbuf_add_cwd(), because the former is simpler and sufficient for now; based on a suggestion by Duy * added patch 2 as an example for strbuf_getcwd() usage, suggested by Duy * made sure strbuf_getcwd() leaves the strbuf intact, no matter what getcwd() does * converted an easy getcwd() call in setup.c René Scharfe (4): strbuf: add strbuf_getcwd() use strbuf_getcwd() to get the current working directory without fixed-sized buffers wrapper: add xgetcwd() use xgetcwd() get the current directory or die Documentation/technical/api-strbuf.txt | 4 builtin/init-db.c | 25 - builtin/rev-parse.c| 6 +++--- dir.c | 12 git-compat-util.h | 1 + git.c | 6 -- setup.c| 6 +++--- strbuf.c | 21 + strbuf.h | 1 + trace.c| 7 --- wrapper.c | 8 11 files changed, 69 insertions(+), 28 deletions(-) -- 2.0.2 -- 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 1/4] strbuf: add strbuf_getcwd()
Add strbuf_getcwd(), which puts the current working directory intto
a strbuf. Because it doesn't use a fixed-size buffer it supports
arbitrarily long paths, as long as the platform's getcwd() does as
well. At least on Linux and FreeBSD it handles paths longer than
PATH_MAX just fine.
Suggested-by: Karsten Blees
Helped-by: Helped-by: Duy Nguyen
Signed-off-by: Rene Scharfe
---
Documentation/technical/api-strbuf.txt | 4
strbuf.c | 21 +
strbuf.h | 1 +
3 files changed, 26 insertions(+)
diff --git a/Documentation/technical/api-strbuf.txt
b/Documentation/technical/api-strbuf.txt
index f9c06a7..49e477d 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -307,6 +307,10 @@ same behaviour as well.
use it unless you need the correct position in the file
descriptor.
+`strbuf_getcwd`::
+
+ Set the buffer to the path of the current working directory.
+
`stripspace`::
Strip whitespace from a buffer. The second parameter controls if
diff --git a/strbuf.c b/strbuf.c
index 33018d8..2bf4dfa 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -406,6 +406,27 @@ int strbuf_readlink(struct strbuf *sb, const char *path,
size_t hint)
return -1;
}
+int strbuf_getcwd(struct strbuf *sb)
+{
+ size_t oldalloc = sb->alloc;
+ size_t guessed_len = 128;
+
+ for (;; guessed_len *= 2) {
+ strbuf_grow(sb, guessed_len);
+ if (getcwd(sb->buf, sb->alloc)) {
+ strbuf_setlen(sb, strlen(sb->buf));
+ return 0;
+ }
+ if (errno != ERANGE)
+ break;
+ }
+ if (oldalloc == 0)
+ strbuf_release(sb);
+ else
+ strbuf_reset(sb);
+ return -1;
+}
+
int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
{
int ch;
diff --git a/strbuf.h b/strbuf.h
index a7c0192..bc38bb9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -174,6 +174,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
+extern int strbuf_getcwd(struct strbuf *sb);
extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
extern int strbuf_getline(struct strbuf *, FILE *, int);
--
2.0.2
--
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/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers
Signed-off-by: Rene Scharfe
---
builtin/init-db.c | 8
git.c | 6 --
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..c4958b6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -535,10 +535,10 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
usage(init_db_usage[0]);
}
if (is_bare_repository_cfg == 1) {
- static char git_dir[PATH_MAX+1];
-
- setenv(GIT_DIR_ENVIRONMENT,
- getcwd(git_dir, sizeof(git_dir)), argc > 0);
+ struct strbuf cwd = STRBUF_INIT;
+ strbuf_getcwd(&cwd);
+ setenv(GIT_DIR_ENVIRONMENT, cwd.buf, argc > 0);
+ strbuf_release(&cwd);
}
if (init_shared_repository != -1)
diff --git a/git.c b/git.c
index 5b6c761..3f52c43 100644
--- a/git.c
+++ b/git.c
@@ -161,9 +161,11 @@ static int handle_options(const char ***argv, int *argc,
int *envchanged)
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--bare")) {
- static char git_dir[PATH_MAX+1];
+ struct strbuf cwd = STRBUF_INIT;
is_bare_repository_cfg = 1;
- setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
sizeof(git_dir)), 0);
+ strbuf_getcwd(&cwd);
+ setenv(GIT_DIR_ENVIRONMENT, cwd.buf, 0);
+ strbuf_release(&cwd);
setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
if (envchanged)
*envchanged = 1;
--
2.0.2
--
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 3/4] wrapper: add xgetcwd()
Add the helper function xgetcwd(), which returns the current directory
or dies. The returned string has to be free()d after use.
Helped-by: Duy Nguyen
Signed-off-by: Rene Scharfe
---
git-compat-util.h | 1 +
wrapper.c | 8
2 files changed, 9 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 0b53c9a..d64d012 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -605,6 +605,7 @@ extern int xmkstemp(char *template);
extern int xmkstemp_mode(char *template, int mode);
extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
+extern char *xgetcwd(void);
static inline size_t xsize_t(off_t len)
{
diff --git a/wrapper.c b/wrapper.c
index bc1bfb8..bd24cda 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -493,3 +493,11 @@ struct passwd *xgetpwuid_self(void)
errno ? strerror(errno) : _("no such user"));
return pw;
}
+
+char *xgetcwd(void)
+{
+ struct strbuf sb = STRBUF_INIT;
+ if (strbuf_getcwd(&sb))
+ die_errno(_("unable to get current working directory"));
+ return strbuf_detach(&sb, NULL);
+}
--
2.0.2
--
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 4/4] use xgetcwd() get the current directory or die
Convert several calls of getcwd() and die() to use xgetcwd() instead.
This way we get rid of fixed-size buffers (which can be too small
depending on the used file system) and gain consistent error messages.
Signed-off-by: Rene Scharfe
---
builtin/init-db.c | 17 -
builtin/rev-parse.c | 6 +++---
dir.c | 12
setup.c | 6 +++---
trace.c | 7 ---
5 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index c4958b6..dc226d1 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -426,8 +426,9 @@ int init_db(const char *template_dir, unsigned int flags)
static int guess_repository_type(const char *git_dir)
{
- char cwd[PATH_MAX];
const char *slash;
+ char *cwd;
+ int cwd_is_git_dir;
/*
* "GIT_DIR=. git init" is always bare.
@@ -435,9 +436,10 @@ static int guess_repository_type(const char *git_dir)
*/
if (!strcmp(".", git_dir))
return 1;
- if (!getcwd(cwd, sizeof(cwd)))
- die_errno(_("cannot tell cwd"));
- if (!strcmp(git_dir, cwd))
+ cwd = xgetcwd();
+ cwd_is_git_dir = !strcmp(git_dir, cwd);
+ free(cwd);
+ if (cwd_is_git_dir)
return 1;
/*
* "GIT_DIR=.git or GIT_DIR=something/.git is usually not.
@@ -572,11 +574,8 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
git_work_tree_cfg = xstrdup(real_path(rel));
free(rel);
}
- if (!git_work_tree_cfg) {
- git_work_tree_cfg = xcalloc(PATH_MAX, 1);
- if (!getcwd(git_work_tree_cfg, PATH_MAX))
- die_errno (_("Cannot access current working
directory"));
- }
+ if (!git_work_tree_cfg)
+ git_work_tree_cfg = xgetcwd();
if (work_tree)
set_git_work_tree(real_path(work_tree));
else
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 8102aaa..f6bbac7 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -735,7 +735,7 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
}
if (!strcmp(arg, "--git-dir")) {
const char *gitdir =
getenv(GIT_DIR_ENVIRONMENT);
- static char cwd[PATH_MAX];
+ char *cwd;
int len;
if (gitdir) {
puts(gitdir);
@@ -745,10 +745,10 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
puts(".git");
continue;
}
- if (!getcwd(cwd, PATH_MAX))
- die_errno("unable to get current
working directory");
+ cwd = xgetcwd();
len = strlen(cwd);
printf("%s%s.git\n", cwd, len && cwd[len-1] !=
'/' ? "/" : "");
+ free(cwd);
continue;
}
if (!strcmp(arg, "--resolve-git-dir")) {
diff --git a/dir.c b/dir.c
index e65888d..7b994d4 100644
--- a/dir.c
+++ b/dir.c
@@ -1499,12 +1499,16 @@ int dir_inside_of(const char *subdir, const char *dir)
int is_inside_dir(const char *dir)
{
- char cwd[PATH_MAX];
+ char *cwd;
+ int rc;
+
if (!dir)
return 0;
- if (!getcwd(cwd, sizeof(cwd)))
- die_errno("can't find the current directory");
- return dir_inside_of(cwd, dir) >= 0;
+
+ cwd = xgetcwd();
+ rc = (dir_inside_of(cwd, dir) >= 0);
+ free(cwd);
+ return rc;
}
int is_empty_dir(const char *path)
diff --git a/setup.c b/setup.c
index 0a22f8b..dc92d63 100644
--- a/setup.c
+++ b/setup.c
@@ -434,16 +434,16 @@ static const char *setup_explicit_git_dir(const char
*gitdirenv,
if (is_absolute_path(git_work_tree_cfg))
set_git_work_tree(git_work_tree_cfg);
else {
- char core_worktree[PATH_MAX];
+ char *core_worktree;
if (chdir(gitdirenv))
die_errno("Could not chdir to '%s'", gitdirenv);
if (chdir(git_work_tree_cfg))
die_errno("Could not chdir to '%s'",
git_work_tree_cfg);
- if (!getcwd(core_worktree, PATH_MAX))
- die_errno("Could not get directory '%s'",
git_work_tree_cfg);
+ core_worktree = xgetcwd()
Re: [PATCH v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers
Am 21.07.2014 04:33, schrieb Jeff King:
On Sun, Jul 20, 2014 at 06:49:54PM +0200, René Scharfe wrote:
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..c4958b6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -535,10 +535,10 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
usage(init_db_usage[0]);
}
if (is_bare_repository_cfg == 1) {
- static char git_dir[PATH_MAX+1];
-
- setenv(GIT_DIR_ENVIRONMENT,
- getcwd(git_dir, sizeof(git_dir)), argc > 0);
+ struct strbuf cwd = STRBUF_INIT;
+ strbuf_getcwd(&cwd);
+ setenv(GIT_DIR_ENVIRONMENT, cwd.buf, argc > 0);
+ strbuf_release(&cwd);
Hmm. You are not making anything worse here, as we already do not check
the return value of getcwd. But what happens if it fails? Looks like we
currently get a segfault, and the new code will silently set the
variable to the empty string. Neither is particularly helpful.
Should we be using the xgetcwd helper that you add in the next patch?
Probably. And I was so glad to have found an example case for getcwd
without dying and without touching the get-there-and-back cases. :)
Guess I'll have to look closer at setup.c and perhaps unix-socket.c for
a replacement.
By the way: Simply setting $GIT_DIR to "." probably won't work in the
two cases, I guess?
- setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
sizeof(git_dir)), 0);
+ strbuf_getcwd(&cwd);
+ setenv(GIT_DIR_ENVIRONMENT, cwd.buf, 0);
+ strbuf_release(&cwd);
Ditto here.
-Peff
--
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: Bug in get_pwd_cwd() in Windows?
Am 21.07.2014 16:13, schrieb Duy Nguyen:
This function tests if $PWD is the same as getcwd() using st_dev and
st_ino. But on Windows these fields are always zero
(mingw.c:do_lstat). If cwd is moved away, I think falling back to $PWD
is wrong. I don't understand the use of $PWD in the first place.
1b9a946 (Use nonrelative paths instead of absolute paths for cloned
repositories - 2008-06-05) does not explain much.
The commit message reads:
Particularly for the "alternates" file, if one will be created, we
want a path that doesn't depend on the current directory, but we want
to retain any symlinks in the path as given and any in the user's view
of the current directory when the path was given.
The intent of the patch seems to be to prefer $PWD if it points to the
same directory as the one returned by getcwd() in order to preserve "the
user's view". That's why it introduces make_nonrelative_path() (now
called absolute_path()), in contrast to make_absolute_path() (now called
real_path()).
I imagine it's useful e.g. if your home is accessed through a symlink:
/home/foo -> /some/boring/mountpoint
Then real_path("bar") would give you "/some/boring/mountpoint/bar",
while absolute_path("bar") returned "/home/foo/bar". Not resolving
symlinks keeps the path friendly in this case. And it keeps working
even after the user's home is migrated to /a/bigger/partition and
/home/foo is updated accordingly.
René
--
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 v3 0/10] getcwd without PATH_MAX
Paths longer than PATH_MAX can be created and used on at least on some file systems. Currently we use getcwd() generally with a PATH_MAX- sized buffer. This series adds two functions, strbuf_getcwd() and xgetcwd(), then uses them to reduce the number of fixed-sized buffers and to allow us to handle longer working directory paths. Changes in v3: * all getcwd() calls are converted * the two strbuf_getcwd() examples from last round use xgetcwd() now, as suggested by Jeff * strbuf_add_absolute_path() is introduced René Scharfe (10): strbuf: add strbuf_getcwd() unix-sockets: use strbuf_getcwd() setup: convert setup_git_directory_gently_1 et al. to strbuf abspath: use strbuf_getcwd() to remember original working directory abspath: convert real_path_internal() to strbuf wrapper: add xgetcwd() use xgetcwd() to get the current directory or die use xgetcwd() to set $GIT_DIR abspath: convert absolute_path() to strbuf use strbuf_add_absolute_path() to add absolute paths Documentation/technical/api-strbuf.txt | 10 +++ abspath.c | 124 + builtin/init-db.c | 24 +++ builtin/rev-parse.c| 6 +- dir.c | 12 ++-- exec_cmd.c | 6 +- git-compat-util.h | 1 + git.c | 13 ++-- setup.c| 91 sha1_file.c| 2 +- strbuf.c | 46 strbuf.h | 3 + trace.c| 7 +- unix-socket.c | 14 ++-- wrapper.c | 8 +++ 15 files changed, 190 insertions(+), 177 deletions(-) -- 2.0.2 -- 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 v3 01/10] strbuf: add strbuf_getcwd()
Add strbuf_getcwd(), which puts the current working directory into a
strbuf. Because it doesn't use a fixed-size buffer it supports
arbitrarily long paths, provided the platform's getcwd() does as well.
At least on Linux and FreeBSD it handles paths longer than PATH_MAX
just fine.
Suggested-by: Karsten Blees
Helped-by: Duy Nguyen
Signed-off-by: Rene Scharfe
---
Documentation/technical/api-strbuf.txt | 4
strbuf.c | 21 +
strbuf.h | 1 +
3 files changed, 26 insertions(+)
diff --git a/Documentation/technical/api-strbuf.txt
b/Documentation/technical/api-strbuf.txt
index f9c06a7..49e477d 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -307,6 +307,10 @@ same behaviour as well.
use it unless you need the correct position in the file
descriptor.
+`strbuf_getcwd`::
+
+ Set the buffer to the path of the current working directory.
+
`stripspace`::
Strip whitespace from a buffer. The second parameter controls if
diff --git a/strbuf.c b/strbuf.c
index 33018d8..2bf4dfa 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -406,6 +406,27 @@ int strbuf_readlink(struct strbuf *sb, const char *path,
size_t hint)
return -1;
}
+int strbuf_getcwd(struct strbuf *sb)
+{
+ size_t oldalloc = sb->alloc;
+ size_t guessed_len = 128;
+
+ for (;; guessed_len *= 2) {
+ strbuf_grow(sb, guessed_len);
+ if (getcwd(sb->buf, sb->alloc)) {
+ strbuf_setlen(sb, strlen(sb->buf));
+ return 0;
+ }
+ if (errno != ERANGE)
+ break;
+ }
+ if (oldalloc == 0)
+ strbuf_release(sb);
+ else
+ strbuf_reset(sb);
+ return -1;
+}
+
int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
{
int ch;
diff --git a/strbuf.h b/strbuf.h
index a7c0192..bc38bb9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -174,6 +174,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
+extern int strbuf_getcwd(struct strbuf *sb);
extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
extern int strbuf_getline(struct strbuf *, FILE *, int);
--
2.0.2
--
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 v3 02/10] unix-sockets: use strbuf_getcwd()
Instead of using a PATH_MAX-sized buffer, which can be too small on some
file systems, use strbuf_getcwd(), which handles any path getcwd()
returns. Also preserve the errno set by strbuf_getcwd() instead of
setting it to ENAMETOOLONG; that way a more appropriate error message
can be shown based on the actual reason for failing.
Signed-off-by: Rene Scharfe
---
unix-socket.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/unix-socket.c b/unix-socket.c
index 91bd6b8..19ed48b 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -18,12 +18,12 @@ static int chdir_len(const char *orig, int len)
}
struct unix_sockaddr_context {
- char orig_dir[PATH_MAX];
+ char *orig_dir;
};
static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)
{
- if (!ctx->orig_dir[0])
+ if (!ctx->orig_dir)
return;
/*
* If we fail, we can't just return an error, since we have
@@ -32,6 +32,7 @@ static void unix_sockaddr_cleanup(struct
unix_sockaddr_context *ctx)
*/
if (chdir(ctx->orig_dir) < 0)
die("unable to restore original working directory");
+ free(ctx->orig_dir);
}
static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
@@ -39,10 +40,11 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const
char *path,
{
int size = strlen(path) + 1;
- ctx->orig_dir[0] = '\0';
+ ctx->orig_dir = NULL;
if (size > sizeof(sa->sun_path)) {
const char *slash = find_last_dir_sep(path);
const char *dir;
+ struct strbuf cwd = STRBUF_INIT;
if (!slash) {
errno = ENAMETOOLONG;
@@ -56,11 +58,9 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const
char *path,
errno = ENAMETOOLONG;
return -1;
}
-
- if (!getcwd(ctx->orig_dir, sizeof(ctx->orig_dir))) {
- errno = ENAMETOOLONG;
+ if (strbuf_getcwd(&cwd))
return -1;
- }
+ ctx->orig_dir = strbuf_detach(&cwd, NULL);
if (chdir_len(dir, slash - dir) < 0)
return -1;
}
--
2.0.2
--
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 v3 03/10] setup: convert setup_git_directory_gently_1 et al. to strbuf
Convert setup_git_directory_gently_1() and its helper functions
setup_explicit_git_dir(), setup_discovered_git_dir() and
setup_bare_git_dir() to use a struct strbuf to hold the current working
directory. Replacing the PATH_MAX-sized buffer used before removes a
path length limition on some file systems. The functions are converted
all in one go because they all read and write the variable cwd.
Signed-off-by: Rene Scharfe
---
setup.c | 85 +
1 file changed, 43 insertions(+), 42 deletions(-)
diff --git a/setup.c b/setup.c
index 0a22f8b..c8b8a97 100644
--- a/setup.c
+++ b/setup.c
@@ -387,7 +387,7 @@ const char *read_gitfile(const char *path)
}
static const char *setup_explicit_git_dir(const char *gitdirenv,
- char *cwd, int len,
+ struct strbuf *cwd,
int *nongit_ok)
{
const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
@@ -441,7 +441,7 @@ static const char *setup_explicit_git_dir(const char
*gitdirenv,
die_errno("Could not chdir to '%s'",
git_work_tree_cfg);
if (!getcwd(core_worktree, PATH_MAX))
die_errno("Could not get directory '%s'",
git_work_tree_cfg);
- if (chdir(cwd))
+ if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
set_git_work_tree(core_worktree);
}
@@ -459,21 +459,20 @@ static const char *setup_explicit_git_dir(const char
*gitdirenv,
worktree = get_git_work_tree();
/* both get_git_work_tree() and cwd are already normalized */
- if (!strcmp(cwd, worktree)) { /* cwd == worktree */
+ if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */
set_git_dir(gitdirenv);
free(gitfile);
return NULL;
}
- offset = dir_inside_of(cwd, worktree);
+ offset = dir_inside_of(cwd->buf, worktree);
if (offset >= 0) { /* cwd inside worktree? */
set_git_dir(real_path(gitdirenv));
if (chdir(worktree))
die_errno("Could not chdir to '%s'", worktree);
- cwd[len++] = '/';
- cwd[len] = '\0';
+ strbuf_addch(cwd, '/');
free(gitfile);
- return cwd + offset;
+ return cwd->buf + offset;
}
/* cwd outside worktree */
@@ -483,7 +482,7 @@ static const char *setup_explicit_git_dir(const char
*gitdirenv,
}
static const char *setup_discovered_git_dir(const char *gitdir,
- char *cwd, int offset, int len,
+ struct strbuf *cwd, int offset,
int *nongit_ok)
{
if (check_repository_format_gently(gitdir, nongit_ok))
@@ -491,17 +490,17 @@ static const char *setup_discovered_git_dir(const char
*gitdir,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
- if (offset != len && !is_absolute_path(gitdir))
+ if (offset != cwd->len && !is_absolute_path(gitdir))
gitdir = xstrdup(real_path(gitdir));
- if (chdir(cwd))
+ if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
- return setup_explicit_git_dir(gitdir, cwd, len, nongit_ok);
+ return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
}
/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
if (is_bare_repository_cfg > 0) {
- set_git_dir(offset == len ? gitdir : real_path(gitdir));
- if (chdir(cwd))
+ set_git_dir(offset == cwd->len ? gitdir : real_path(gitdir));
+ if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
return NULL;
}
@@ -512,18 +511,18 @@ static const char *setup_discovered_git_dir(const char
*gitdir,
set_git_dir(gitdir);
inside_git_dir = 0;
inside_work_tree = 1;
- if (offset == len)
+ if (offset == cwd->len)
return NULL;
/* Make "offset" point to past the '/', and add a '/' at the end */
offset++;
- cwd[len++] = '/';
- cwd[len] = 0;
- return cwd + offset;
+ strbuf_addch(cwd, '/');
+ return cwd->buf + offset;
}
/* #16.1, #17.1, #20.1, #21.1, #22.1 (see t1510) */
-static const char *setup_bare_git_dir(char *cwd, int offset, int len, int
*nongit_ok)
+static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
+ int *nongit_ok)
[PATCH 04/10] abspath: use strbuf_getcwd() to remember original working directory
Store the original working directory in a strbuf instead of in a
fixed-sized buffer, in order to be able to handle longer paths.
Signed-off-by: Rene Scharfe
---
abspath.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/abspath.c b/abspath.c
index ca33558..911e931 100644
--- a/abspath.c
+++ b/abspath.c
@@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int
die_on_error)
* here so that we can chdir() back to it at the end of the
* function:
*/
- char cwd[1024] = "";
+ struct strbuf cwd = STRBUF_INIT;
int buf_index = 1;
@@ -80,7 +80,7 @@ static const char *real_path_internal(const char *path, int
die_on_error)
}
if (*buf) {
- if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
+ if (!cwd.len && strbuf_getcwd(&cwd)) {
if (die_on_error)
die_errno("Could not get current
working directory");
else
@@ -142,8 +142,9 @@ static const char *real_path_internal(const char *path, int
die_on_error)
retval = buf;
error_out:
free(last_elem);
- if (*cwd && chdir(cwd))
- die_errno("Could not change back to '%s'", cwd);
+ if (cwd.len && chdir(cwd.buf))
+ die_errno("Could not change back to '%s'", cwd.buf);
+ strbuf_release(&cwd);
return retval;
}
--
2.0.2
--
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 v3 06/10] wrapper: add xgetcwd()
Add the helper function xgetcwd(), which returns the current directory
or dies. The returned string has to be free()d after use.
Helped-by: Duy Nguyen
Signed-off-by: Rene Scharfe
---
git-compat-util.h | 1 +
wrapper.c | 8
2 files changed, 9 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 26e92f1..4d6edea 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -607,6 +607,7 @@ extern int xmkstemp(char *template);
extern int xmkstemp_mode(char *template, int mode);
extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
+extern char *xgetcwd(void);
static inline size_t xsize_t(off_t len)
{
diff --git a/wrapper.c b/wrapper.c
index bc1bfb8..bd24cda 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -493,3 +493,11 @@ struct passwd *xgetpwuid_self(void)
errno ? strerror(errno) : _("no such user"));
return pw;
}
+
+char *xgetcwd(void)
+{
+ struct strbuf sb = STRBUF_INIT;
+ if (strbuf_getcwd(&sb))
+ die_errno(_("unable to get current working directory"));
+ return strbuf_detach(&sb, NULL);
+}
--
2.0.2
--
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 v3 05/10] abspath: convert real_path_internal() to strbuf
Use strbuf instead of fixed-sized buffers in real_path() in order to
avoid the size limitations of the latter.
Signed-off-by: Rene Scharfe
---
abspath.c | 69 +++
1 file changed, 25 insertions(+), 44 deletions(-)
diff --git a/abspath.c b/abspath.c
index 911e931..16e7fa2 100644
--- a/abspath.c
+++ b/abspath.c
@@ -33,7 +33,7 @@ int is_directory(const char *path)
*/
static const char *real_path_internal(const char *path, int die_on_error)
{
- static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
+ static struct strbuf sb = STRBUF_INIT;
char *retval = NULL;
/*
@@ -43,14 +43,12 @@ static const char *real_path_internal(const char *path, int
die_on_error)
*/
struct strbuf cwd = STRBUF_INIT;
- int buf_index = 1;
-
int depth = MAXDEPTH;
char *last_elem = NULL;
struct stat st;
/* We've already done it */
- if (path == buf || path == next_buf)
+ if (path == sb.buf)
return path;
if (!*path) {
@@ -60,26 +58,22 @@ static const char *real_path_internal(const char *path, int
die_on_error)
goto error_out;
}
- if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {
- if (die_on_error)
- die("Too long path: %.*s", 60, path);
- else
- goto error_out;
- }
+ strbuf_init(&sb, 0);
+ strbuf_addstr(&sb, path);
while (depth--) {
- if (!is_directory(buf)) {
- char *last_slash = find_last_dir_sep(buf);
+ if (!is_directory(sb.buf)) {
+ char *last_slash = find_last_dir_sep(sb.buf);
if (last_slash) {
last_elem = xstrdup(last_slash + 1);
- last_slash[1] = '\0';
+ strbuf_setlen(&sb, last_slash - sb.buf + 1);
} else {
- last_elem = xstrdup(buf);
- *buf = '\0';
+ last_elem = xmemdupz(sb.buf, sb.len);
+ strbuf_reset(&sb);
}
}
- if (*buf) {
+ if (sb.len) {
if (!cwd.len && strbuf_getcwd(&cwd)) {
if (die_on_error)
die_errno("Could not get current
working directory");
@@ -87,14 +81,15 @@ static const char *real_path_internal(const char *path, int
die_on_error)
goto error_out;
}
- if (chdir(buf)) {
+ if (chdir(sb.buf)) {
if (die_on_error)
- die_errno("Could not switch to '%s'",
buf);
+ die_errno("Could not switch to '%s'",
+ sb.buf);
else
goto error_out;
}
}
- if (!getcwd(buf, PATH_MAX)) {
+ if (strbuf_getcwd(&sb)) {
if (die_on_error)
die_errno("Could not get current working
directory");
else
@@ -102,44 +97,30 @@ static const char *real_path_internal(const char *path,
int die_on_error)
}
if (last_elem) {
- size_t len = strlen(buf);
- if (len + strlen(last_elem) + 2 > PATH_MAX) {
- if (die_on_error)
- die("Too long path name: '%s/%s'",
- buf, last_elem);
- else
- goto error_out;
- }
- if (len && !is_dir_sep(buf[len - 1]))
- buf[len++] = '/';
- strcpy(buf + len, last_elem);
+ if (sb.len && !is_dir_sep(sb.buf[sb.len - 1]))
+ strbuf_addch(&sb, '/');
+ strbuf_addstr(&sb, last_elem);
free(last_elem);
last_elem = NULL;
}
- if (!lstat(buf, &st) && S_ISLNK(st.st_mode)) {
- ssize_t len = readlink(buf, next_buf, PATH_MAX);
+ if (!lstat(sb.buf, &st) && S_ISLNK(st.st_mode)) {
+ struct strbuf next_sb = STRBUF_INIT;
+ ssize_t len = strbuf_readlink(&next_sb, sb.buf, 0);
if (len < 0) {
if (die_on_error)
-
[PATCH v3 07/10] use xgetcwd() to get the current directory or die
Convert several calls of getcwd() and die() to use xgetcwd() instead.
This way we get rid of fixed-size buffers (which can be too small
depending on the used file system) and gain consistent error messages.
Signed-off-by: Rene Scharfe
---
builtin/init-db.c | 17 -
builtin/rev-parse.c | 6 +++---
dir.c | 12
git.c | 8
setup.c | 6 +++---
trace.c | 7 ---
6 files changed, 30 insertions(+), 26 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..f6dd172 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -426,8 +426,9 @@ int init_db(const char *template_dir, unsigned int flags)
static int guess_repository_type(const char *git_dir)
{
- char cwd[PATH_MAX];
const char *slash;
+ char *cwd;
+ int cwd_is_git_dir;
/*
* "GIT_DIR=. git init" is always bare.
@@ -435,9 +436,10 @@ static int guess_repository_type(const char *git_dir)
*/
if (!strcmp(".", git_dir))
return 1;
- if (!getcwd(cwd, sizeof(cwd)))
- die_errno(_("cannot tell cwd"));
- if (!strcmp(git_dir, cwd))
+ cwd = xgetcwd();
+ cwd_is_git_dir = !strcmp(git_dir, cwd);
+ free(cwd);
+ if (cwd_is_git_dir)
return 1;
/*
* "GIT_DIR=.git or GIT_DIR=something/.git is usually not.
@@ -572,11 +574,8 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
git_work_tree_cfg = xstrdup(real_path(rel));
free(rel);
}
- if (!git_work_tree_cfg) {
- git_work_tree_cfg = xcalloc(PATH_MAX, 1);
- if (!getcwd(git_work_tree_cfg, PATH_MAX))
- die_errno (_("Cannot access current working
directory"));
- }
+ if (!git_work_tree_cfg)
+ git_work_tree_cfg = xgetcwd();
if (work_tree)
set_git_work_tree(real_path(work_tree));
else
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 8102aaa..f6bbac7 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -735,7 +735,7 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
}
if (!strcmp(arg, "--git-dir")) {
const char *gitdir =
getenv(GIT_DIR_ENVIRONMENT);
- static char cwd[PATH_MAX];
+ char *cwd;
int len;
if (gitdir) {
puts(gitdir);
@@ -745,10 +745,10 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
puts(".git");
continue;
}
- if (!getcwd(cwd, PATH_MAX))
- die_errno("unable to get current
working directory");
+ cwd = xgetcwd();
len = strlen(cwd);
printf("%s%s.git\n", cwd, len && cwd[len-1] !=
'/' ? "/" : "");
+ free(cwd);
continue;
}
if (!strcmp(arg, "--resolve-git-dir")) {
diff --git a/dir.c b/dir.c
index fcb6872..bd274a7 100644
--- a/dir.c
+++ b/dir.c
@@ -1507,12 +1507,16 @@ int dir_inside_of(const char *subdir, const char *dir)
int is_inside_dir(const char *dir)
{
- char cwd[PATH_MAX];
+ char *cwd;
+ int rc;
+
if (!dir)
return 0;
- if (!getcwd(cwd, sizeof(cwd)))
- die_errno("can't find the current directory");
- return dir_inside_of(cwd, dir) >= 0;
+
+ cwd = xgetcwd();
+ rc = (dir_inside_of(cwd, dir) >= 0);
+ free(cwd);
+ return rc;
}
int is_empty_dir(const char *path)
diff --git a/git.c b/git.c
index 9c49519..47137db 100644
--- a/git.c
+++ b/git.c
@@ -20,7 +20,7 @@ const char git_more_info_string[] =
static struct startup_info git_startup_info;
static int use_pager = -1;
-static char orig_cwd[PATH_MAX];
+static char *orig_cwd;
static const char *env_names[] = {
GIT_DIR_ENVIRONMENT,
GIT_WORK_TREE_ENVIRONMENT,
@@ -36,8 +36,7 @@ static void save_env(void)
if (saved_environment)
return;
saved_environment = 1;
- if (!getcwd(orig_cwd, sizeof(orig_cwd)))
- die_errno("cannot getcwd");
+ orig_cwd = xgetcwd();
for (i = 0; i < ARRAY_SIZE(env_names); i++) {
orig_env[i] = getenv(env_names[i]);
if (orig_env[i])
@@ -48,8 +47,9 @@ static void save_env(void)
static void restore_env(void)
{
int i;
-
[PATCH v3 08/10] use xgetcwd() to set $GIT_DIR
Instead of dying of a segmentation fault if getcwd() returns NULL, use
xgetcwd() to make sure to write a useful error message and then exit
in an orderly fashion.
Suggested-by: Jeff King
Signed-off-by: Rene Scharfe
---
builtin/init-db.c | 7 +++
git.c | 5 +++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index f6dd172..ab0ea02 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -537,10 +537,9 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
usage(init_db_usage[0]);
}
if (is_bare_repository_cfg == 1) {
- static char git_dir[PATH_MAX+1];
-
- setenv(GIT_DIR_ENVIRONMENT,
- getcwd(git_dir, sizeof(git_dir)), argc > 0);
+ char *cwd = xgetcwd();
+ setenv(GIT_DIR_ENVIRONMENT, cwd, argc > 0);
+ free(cwd);
}
if (init_shared_repository != -1)
diff --git a/git.c b/git.c
index 47137db..210f1ae 100644
--- a/git.c
+++ b/git.c
@@ -161,9 +161,10 @@ static int handle_options(const char ***argv, int *argc,
int *envchanged)
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--bare")) {
- static char git_dir[PATH_MAX+1];
+ char *cwd = xgetcwd();
is_bare_repository_cfg = 1;
- setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
sizeof(git_dir)), 0);
+ setenv(GIT_DIR_ENVIRONMENT, cwd, 0);
+ free(cwd);
setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
if (envchanged)
*envchanged = 1;
--
2.0.2
--
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 v3 09/10] abspath: convert absolute_path() to strbuf
Move most of the code of absolute_path() into the new function
strbuf_add_absolute_path() and in the process transform it to use
struct strbuf and xgetcwd() instead of a PATH_MAX-sized buffer,
which can be too small on some file systems.
Signed-off-by: Rene Scharfe
---
Documentation/technical/api-strbuf.txt | 6 +
abspath.c | 46 +++---
strbuf.c | 25 ++
strbuf.h | 2 ++
4 files changed, 37 insertions(+), 42 deletions(-)
diff --git a/Documentation/technical/api-strbuf.txt
b/Documentation/technical/api-strbuf.txt
index 49e477d..430302c 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -311,6 +311,12 @@ same behaviour as well.
Set the buffer to the path of the current working directory.
+`strbuf_add_absolute_path`
+
+ Add a path to a buffer, converting a relative path to an
+ absolute one in the process. Symbolic links are not
+ resolved.
+
`stripspace`::
Strip whitespace from a buffer. The second parameter controls if
diff --git a/abspath.c b/abspath.c
index 16e7fa2..197af68 100644
--- a/abspath.c
+++ b/abspath.c
@@ -140,54 +140,16 @@ const char *real_path_if_valid(const char *path)
return real_path_internal(path, 0);
}
-static const char *get_pwd_cwd(void)
-{
- static char cwd[PATH_MAX + 1];
- char *pwd;
- struct stat cwd_stat, pwd_stat;
- if (getcwd(cwd, PATH_MAX) == NULL)
- return NULL;
- pwd = getenv("PWD");
- if (pwd && strcmp(pwd, cwd)) {
- stat(cwd, &cwd_stat);
- if ((cwd_stat.st_dev || cwd_stat.st_ino) &&
- !stat(pwd, &pwd_stat) &&
- pwd_stat.st_dev == cwd_stat.st_dev &&
- pwd_stat.st_ino == cwd_stat.st_ino) {
- strlcpy(cwd, pwd, PATH_MAX);
- }
- }
- return cwd;
-}
-
/*
* Use this to get an absolute path from a relative one. If you want
* to resolve links, you should use real_path.
- *
- * If the path is already absolute, then return path. As the user is
- * never meant to free the return value, we're safe.
*/
const char *absolute_path(const char *path)
{
- static char buf[PATH_MAX + 1];
-
- if (!*path) {
- die("The empty string is not a valid path");
- } else if (is_absolute_path(path)) {
- if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
- die("Too long path: %.*s", 60, path);
- } else {
- size_t len;
- const char *fmt;
- const char *cwd = get_pwd_cwd();
- if (!cwd)
- die_errno("Cannot determine the current working
directory");
- len = strlen(cwd);
- fmt = (len > 0 && is_dir_sep(cwd[len - 1])) ? "%s%s" : "%s/%s";
- if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
- die("Too long path: %.*s", 60, path);
- }
- return buf;
+ static struct strbuf sb;
+ strbuf_init(&sb, 0);
+ strbuf_add_absolute_path(&sb, path);
+ return sb.buf;
}
/*
diff --git a/strbuf.c b/strbuf.c
index 2bf4dfa..4d31443 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -576,6 +576,31 @@ void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes)
}
}
+void strbuf_add_absolute_path(struct strbuf *sb, const char *path)
+{
+ if (!*path)
+ die("The empty string is not a valid path");
+ if (!is_absolute_path(path)) {
+ struct stat cwd_stat, pwd_stat;
+ size_t orig_len = sb->len;
+ char *cwd = xgetcwd();
+ char *pwd = getenv("PWD");
+ if (pwd && strcmp(pwd, cwd) &&
+ !stat(cwd, &cwd_stat) &&
+ (cwd_stat.st_dev || cwd_stat.st_ino) &&
+ !stat(pwd, &pwd_stat) &&
+ pwd_stat.st_dev == cwd_stat.st_dev &&
+ pwd_stat.st_ino == cwd_stat.st_ino)
+ strbuf_addstr(sb, pwd);
+ else
+ strbuf_addstr(sb, cwd);
+ if (sb->len > orig_len && !is_dir_sep(sb->buf[sb->len - 1]))
+ strbuf_addch(sb, '/');
+ free(cwd);
+ }
+ strbuf_addstr(sb, path);
+}
+
int printf_ln(const char *fmt, ...)
{
int ret;
diff --git a/strbuf.h b/strbuf.h
index bc38bb9..7bdc1da 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -190,6 +190,8 @@ extern void strbuf_addstr_urlencode(struct strbuf *, const
char *,
int reserved);
extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);
+extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
+
__attribute__((format (printf,1,2)))
extern int printf_ln(const char *fmt, ...);
__attribute__((
[PATCH v3 10/10] use strbuf_add_absolute_path() to add absolute paths
Signed-off-by: Rene Scharfe
---
exec_cmd.c | 6 +-
sha1_file.c | 2 +-
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 125fa6f..698e752 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -86,11 +86,7 @@ const char *git_exec_path(void)
static void add_path(struct strbuf *out, const char *path)
{
if (path && *path) {
- if (is_absolute_path(path))
- strbuf_addstr(out, path);
- else
- strbuf_addstr(out, absolute_path(path));
-
+ strbuf_add_absolute_path(out, path);
strbuf_addch(out, PATH_SEP);
}
}
diff --git a/sha1_file.c b/sha1_file.c
index 3f70b1d..95afd20 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -350,7 +350,7 @@ static void link_alt_odb_entries(const char *alt, int len,
int sep,
return;
}
- strbuf_addstr(&objdirbuf, absolute_path(get_object_directory()));
+ strbuf_add_absolute_path(&objdirbuf, get_object_directory());
normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
alt_copy = xmemdupz(alt, len);
--
2.0.2
--
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 v3 04/10] abspath: use strbuf_getcwd() to remember original working directory
Store the original working directory in a strbuf instead of in a
fixed-sized buffer, in order to be able to handle longer paths.
Signed-off-by: Rene Scharfe
---
Resent with corrected subject.
abspath.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/abspath.c b/abspath.c
index ca33558..911e931 100644
--- a/abspath.c
+++ b/abspath.c
@@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int
die_on_error)
* here so that we can chdir() back to it at the end of the
* function:
*/
- char cwd[1024] = "";
+ struct strbuf cwd = STRBUF_INIT;
int buf_index = 1;
@@ -80,7 +80,7 @@ static const char *real_path_internal(const char *path, int
die_on_error)
}
if (*buf) {
- if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
+ if (!cwd.len && strbuf_getcwd(&cwd)) {
if (die_on_error)
die_errno("Could not get current
working directory");
else
@@ -142,8 +142,9 @@ static const char *real_path_internal(const char *path, int
die_on_error)
retval = buf;
error_out:
free(last_elem);
- if (*cwd && chdir(cwd))
- die_errno("Could not change back to '%s'", cwd);
+ if (cwd.len && chdir(cwd.buf))
+ die_errno("Could not change back to '%s'", cwd.buf);
+ strbuf_release(&cwd);
return retval;
}
--
2.0.2
--
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] init: avoid superfluous real_path() calls
Feeding the result of a real_path() call to real_path() again doesn't
change that result -- the returned path won't get any more real. Avoid
such a double call in set_git_dir_init() and for the same reason stop
calling real_path() before feeding paths to set_git_work_tree(), as the
latter already takes care of that.
Signed-off-by: Rene Scharfe
---
builtin/init-db.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..6d8ac2c 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -330,12 +330,12 @@ int set_git_dir_init(const char *git_dir, const char
*real_git_dir,
* moving the target repo later on in separate_git_dir()
*/
git_link = xstrdup(real_path(git_dir));
+ set_git_dir(real_path(real_git_dir));
}
else {
- real_git_dir = real_path(git_dir);
+ set_git_dir(real_path(git_dir));
git_link = NULL;
}
- set_git_dir(real_path(real_git_dir));
return 0;
}
@@ -578,7 +578,7 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
die_errno (_("Cannot access current working
directory"));
}
if (work_tree)
- set_git_work_tree(real_path(work_tree));
+ set_git_work_tree(work_tree);
else
set_git_work_tree(git_work_tree_cfg);
if (access(get_git_work_tree(), X_OK))
@@ -587,7 +587,7 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
}
else {
if (work_tree)
- set_git_work_tree(real_path(work_tree));
+ set_git_work_tree(work_tree);
}
set_git_dir_init(git_dir, real_git_dir, 1);
--
2.0.2
--
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 v3 05/10] abspath: convert real_path_internal() to strbuf
Am 28.07.2014 um 21:09 schrieb Jeff King:
On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote:
static const char *real_path_internal(const char *path, int die_on_error)
{
- static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
+ static struct strbuf sb = STRBUF_INIT;
Hrm. I thought at first that this was our usual trick of keeping two
"simultaneous" static buffers, so that we can do:
printf("paths '%s' and '%s'\n", real_path(foo), real_path(bar));
But it looks like that is not the case, and we only have two for
swapping back and forth as we figure out the answer (but they both need
to be static, because we do not know which one we will return in the
end). Is that right?
AFAICS it's only swapped to avoid copying the results of a readlink()
call against one buffer into the other. So, yes, that's how I
understand it as well.
René
--
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 v3 09/10] abspath: convert absolute_path() to strbuf
Am 28.07.2014 um 21:15 schrieb Jeff King:
On Mon, Jul 28, 2014 at 08:33:55PM +0200, René Scharfe wrote:
const char *absolute_path(const char *path)
{
- static char buf[PATH_MAX + 1];
-
- if (!*path) {
- die("The empty string is not a valid path");
- } else if (is_absolute_path(path)) {
- if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
- die("Too long path: %.*s", 60, path);
- } else {
- size_t len;
- const char *fmt;
- const char *cwd = get_pwd_cwd();
- if (!cwd)
- die_errno("Cannot determine the current working
directory");
- len = strlen(cwd);
- fmt = (len > 0 && is_dir_sep(cwd[len - 1])) ? "%s%s" : "%s/%s";
- if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
- die("Too long path: %.*s", 60, path);
- }
- return buf;
+ static struct strbuf sb;
+ strbuf_init(&sb, 0);
+ strbuf_add_absolute_path(&sb, path);
+ return sb.buf;
}
Is it right to strbuf_init here? That means that we are throwing away
the old buffer for each call. I would think you want instead:
static struct strbuf sb = STRBUF_INIT;
strbuf_reset(&sb);
...
I changed it from _reset to _init, but I can't remember why. :( Perhaps
it's the summer heat. Your version makes more sense to me now.
René
--
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 v3 05/10] abspath: convert real_path_internal() to strbuf
Am 28.07.2014 um 23:42 schrieb Junio C Hamano:
Jeff King writes:
On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote:
@@ -60,26 +58,22 @@ static const char *real_path_internal(const char *path, int
die_on_error)
goto error_out;
}
- if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {
- if (die_on_error)
- die("Too long path: %.*s", 60, path);
- else
- goto error_out;
- }
+ strbuf_init(&sb, 0);
+ strbuf_addstr(&sb, path);
As with the other patch I just mentioned, should this be strbuf_reset,
not strbuf_init? We want to reset the static buffer back to zero-size,
not throw it away and leak whatever was there.
-Peff
Yes, this one seems to be leaking.
"Next call to the function invalidates the return value the last
caller received" feels like playing with fire. Most existing
callers are safe in that the first thing they do to the returned
string is xstrdup() it, but we would need to check all the other
callers.
That's the price we pay for using static variables, no? Callers need to
consume them as long as they're fresh and multi-threading is not
allowed. Before, callers could use wrong buffer contents, after the
patch they could still have a pointer to freed memory, which should be
more noticeable in tests.
Getting a strbuf_add_real_path() in order to avoid static variables
would be nice. And it would also be nice if it worked without calling
chdir(). Nice topics for follow-up patches. :)
I briefly thought it is not OK for set_git_work_tree(), which gets
new_work_tree, calls real_path() to receive the value from the
function, and then calls real_path() again on it. The "We've
already done it" optimization is the only thing that makes it safe,
which feels overly fragile.
It wasn't introduced as an optimization, but to silence valgrind
(1d679de5: make_absolute_path: return the input path if it points to our
buffer). set_git_work_tree() calls real_path() only once in each of its
two branches. However, one caller (init) hands it a path returned by
real_path(); we can change that (sent a patch).
René
--
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
fixup for 09/10: plug leak
Noticed-by: Jeff King
---
abspath.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/abspath.c b/abspath.c
index cf9b404..5edb4e7 100644
--- a/abspath.c
+++ b/abspath.c
@@ -146,8 +146,8 @@ const char *real_path_if_valid(const char *path)
*/
const char *absolute_path(const char *path)
{
- static struct strbuf sb;
- strbuf_init(&sb, 0);
+ static struct strbuf sb = STRBUF_INIT;
+ strbuf_reset(&sb);
strbuf_add_absolute_path(&sb, path);
return sb.buf;
}
--
2.0.2
--
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
fixup for 05/10: plug leak
Noticed-by: Jeff King
---
abspath.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/abspath.c b/abspath.c
index 16e7fa2..6aa328f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -58,7 +58,7 @@ static const char *real_path_internal(const char *path, int
die_on_error)
goto error_out;
}
- strbuf_init(&sb, 0);
+ strbuf_reset(&sb);
strbuf_addstr(&sb, path);
while (depth--) {
--
2.0.2
--
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: Bug report about symlinks
Am 31.07.2014 um 21:50 schrieb Nikolay Avdeev: I've noticed something strange with git status when replacing a folder with symlink to another folder. There is a git repo with script with demo in the attachment. Let's try and make this a bit easier for folks to follow along. # Create test repo with two directories with two files each. $ git init Initialized empty Git repository in /tmp/.git/ $ mkdir a b $ echo x >a/equal $ echo x >b/equal $ echo y >a/different $ echo z >b/different $ git add a b $ git commit -minitial [master (root-commit) 6d36895] initial 4 files changed, 4 insertions(+) create mode 100644 a/different create mode 100644 a/equal create mode 100644 b/different create mode 100644 b/equal # Replace one directory with a symlink to the other. $ rm -rf b $ ln -s a b # First git status call. $ git status On branch master Changes not staged for commit: (use "git add/rm ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) deleted:b/different Untracked files: (use "git add ..." to include in what will be committed) b no changes added to commit (use "git add" and/or "git commit -a") # Stage the changes. $ git add b # Second git status call. $ git status On branch master Changes to be committed: (use "git reset HEAD ..." to unstage) new file: b deleted:b/different deleted:b/equal # Commit the staged changes. $ git commit -msymlinked [master 4498f28] symlinked 3 files changed, 1 insertion(+), 2 deletions(-) create mode 12 b delete mode 100644 b/different delete mode 100644 b/equal That the first and second status call report different results is a feature; staging changes using git add alters the status. A commit after the first status call would be empty. It could be debated if the first git status call should also report b/equal as deleted. René -- 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: Bug report about symlinks
Am 01.08.2014 um 18:23 schrieb Junio C Hamano:
> René Scharfe writes:
>
>> # Create test repo with two directories with two files each.
>> $ git init
>> Initialized empty Git repository in /tmp/.git/
>> $ mkdir a b
>> $ echo x >a/equal
>> $ echo x >b/equal
>> $ echo y >a/different
>> $ echo z >b/different
>> $ git add a b
>> $ git commit -minitial
>> [master (root-commit) 6d36895] initial
>> 4 files changed, 4 insertions(+)
>> create mode 100644 a/different
>> create mode 100644 a/equal
>> create mode 100644 b/different
>> create mode 100644 b/equal
>>
>> # Replace one directory with a symlink to the other.
>> $ rm -rf b
>> $ ln -s a b
>>
>> # First git status call.
>> $ git status
>> On branch master
>> Changes not staged for commit:
>>(use "git add/rm ..." to update what will be committed)
>>(use "git checkout -- ..." to discard changes in working directory)
>>
>> deleted:b/different
>>
>> Untracked files:
>>(use "git add ..." to include in what will be committed)
>>
>> b
>>
>> no changes added to commit (use "git add" and/or "git commit -a")
>> ...
>>
>> It could be debated if the first git status call should also report
>> b/equal as deleted.
>
> Hmph.
>
> I wonder if "could be" is an understatement. The difference of
> reporting between b/equal and b/different may indicate that somebody
> is mistakenly comparing the index with these paths, without first
> checking each path with has_symlink_leading_path(), or something,
> no?
How about the patch below? Before it checks if an index entry exists
in the work tree, it checks if its path includes a symlink. After
the patch, git status reports b/equal as deleted, too. The test
suite still passes.
And do we need to use the threaded_ variant of the function here?
diff --git a/read-cache.c b/read-cache.c
index 5d3c8bd..6f0057f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1064,6 +1064,14 @@ static struct cache_entry *refresh_cache_ent(struct
index_state *istate,
return ce;
}
+ if (has_symlink_leading_path(ce->name, ce_namelen(ce))) {
+ if (ignore_missing)
+ return ce;
+ if (err)
+ *err = ENOENT;
+ return NULL;
+ }
+
if (lstat(ce->name, &st) < 0) {
if (ignore_missing && errno == ENOENT)
return ce;
--
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: Bug report about symlinks
Am 03.08.2014 um 19:19 schrieb Junio C Hamano: René Scharfe writes: How about the patch below? Before it checks if an index entry exists in the work tree, it checks if its path includes a symlink. Honestly, I didn't expect the fix to be in the refresh-index code path, but doing this there sort of makes sense. I found it through observation, not understanding. Just looked for stat/lstat calls executed by git status for b/different and b/equal using strace; debugging printfs told me where they came from. And do we need to use the threaded_ variant of the function here? Hmmm, this is a tangent, but you comment made me wonder if we also need to adjust preload_thread() in preload-index.c somehow, but we do not touch CE_UPTODATE there, so it probably is not necessary. The function calls ce_mark_uptodate(), which does set CE_UPTODATE. It calls threaded_has_symlink_leading_path() before lstat() already, however. (Since f62ce3de: Make index preloading check the whole path to the file.) The caller of refresh_cache_ent() is walking an array of sorted pathnames aka istate->cache[] in a single-threaded fashion, possibly with a pathspec to limit the scan. There are two direct callers (refresh_index(), refresh_cache_entry()) and several indirect ones. Do we have a way to detect unsynchronized parallel access to the has_symlink_leading_path()-cache? Checking the full callers-of-callers tree manually looks a bit scary to me. Do you mean that we may want to make istate->cache[] scanned by multiple threads? I am not sure. No, I didn't want to suggest any performance improvements. I'm only interested in correctness for now. René -- 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] read-cache: check for leading symlinks when refreshing index
Don't add paths with leading symlinks to the index while refreshing; we
only track those symlinks themselves. We already ignore them while
preloading (see read_index_preload.c).
Reported-by: Nikolay Avdeev
Signed-off-by: Rene Scharfe
---
read-cache.c | 8
t/t7515-status-symlinks.sh | 43 +++
2 files changed, 51 insertions(+)
create mode 100755 t/t7515-status-symlinks.sh
diff --git a/read-cache.c b/read-cache.c
index 5d3c8bd..6f0057f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1064,6 +1064,14 @@ static struct cache_entry *refresh_cache_ent(struct
index_state *istate,
return ce;
}
+ if (has_symlink_leading_path(ce->name, ce_namelen(ce))) {
+ if (ignore_missing)
+ return ce;
+ if (err)
+ *err = ENOENT;
+ return NULL;
+ }
+
if (lstat(ce->name, &st) < 0) {
if (ignore_missing && errno == ENOENT)
return ce;
diff --git a/t/t7515-status-symlinks.sh b/t/t7515-status-symlinks.sh
new file mode 100755
index 000..9f989be
--- /dev/null
+++ b/t/t7515-status-symlinks.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='git status and symlinks'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ echo .gitignore >.gitignore &&
+ echo actual >>.gitignore &&
+ echo expect >>.gitignore &&
+ mkdir dir &&
+ echo x >dir/file1 &&
+ echo y >dir/file2 &&
+ git add dir &&
+ git commit -m initial &&
+ git tag initial
+'
+
+test_expect_success SYMLINKS 'symlink to a directory' '
+ test_when_finished "rm symlink" &&
+ ln -s dir symlink &&
+ echo "?? symlink" >expect &&
+ git status --porcelain >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success SYMLINKS 'symlink replacing a directory' '
+ test_when_finished "rm -rf copy && git reset --hard initial" &&
+ mkdir copy &&
+ cp dir/file1 copy/file1 &&
+ echo "changed in copy" >copy/file2 &&
+ git add copy &&
+ git commit -m second &&
+ rm -rf copy &&
+ ln -s dir copy &&
+ echo " D copy/file1" >expect &&
+ echo " D copy/file2" >>expect &&
+ echo "?? copy" >>expect &&
+ git status --porcelain >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.0.2
--
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] mailsplit.c: remove dead code
Am 11.08.2014 um 23:11 schrieb Stefan Beller: This was found by coverity. (Id: 290001) the variable 'output' is only assigned to a value inequal to NUL, after all gotos to the corrupt label. Therefore we can conclude the two removed lines are actually dead code. After reading the above for the first time I thought it meant the opposite of what's actually going on. Perhaps it's the placement of "only", the comma or a flawed understanding of grammar on my part? In any case, there is only one way to reach the label named corrupt, and the variable named output is always NULL if that branch is taken. That means the removed code was a no-op. With those two lines gone you also don't need to initialize output anymore, by the way. And since there is only a single goto, you could move the three remaining error handling lines up to the if statement. Keeping condition and dependent code together would be an improvement, I think. Signed-off-by: Stefan Beller --- builtin/mailsplit.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c index 06296d4..b499014 100644 --- a/builtin/mailsplit.c +++ b/builtin/mailsplit.c @@ -93,8 +93,6 @@ static int split_one(FILE *mbox, const char *name, int allow_bare) return status; corrupt: - if (output) - fclose(output); unlink(name); fprintf(stderr, "corrupt mailbox\n"); exit(1); -- 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] clean: use f(void) instead of f() to declare a pointer to a function without arguments
Explicitly state that menu_item functions like clean_cmd don't take
any arguments by using void instead of an empty parameter list.
Found using gcc -Wstrict-prototypes.
Signed-off-by: Rene Scharfe
---
builtin/clean.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 1032563..3beeea6 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -67,7 +67,7 @@ struct menu_item {
char hotkey;
const char *title;
int selected;
- int (*fn)();
+ int (*fn)(void);
};
enum menu_stuff_type {
--
2.1.0
--
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 v3 03/10] setup: convert setup_git_directory_gently_1 et al. to strbuf
> Is there a chance to squueze this in:
>
>
> $ git diff
> diff --git a/setup.c b/setup.c
> index 526cdf6..fb61860 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -734,7 +734,7 @@ static const char *setup_git_directory_gently_1(int
> *nongit_ok)
> string_list_clear(&ceiling_dirs, 0);
> }
>
> - if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
> + if (ceil_offset < 0 && has_dos_drive_prefix(cwd.buf))
> ceil_offset = 1;
>
>
Ouch, thanks for catching this.
Perhaps the following patch should go in as well.
-- >8 --
Subject: [PATCH] turn path macros into inline function
Use static inline functions instead of macros for has_dos_drive_prefix,
offset_1st_component, is_dir_sep and find_last_dir_sep in order to let
the compiler do type checking.
The definitions of offset_1st_component and is_dir_sep are switched
around because the former uses the latter.
Signed-off-by: Rene Scharfe
---
git-compat-util.h | 28 ++--
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index f587749..0b6c13a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -264,19 +264,35 @@ extern char *gitbasename(char *);
#endif
#ifndef has_dos_drive_prefix
-#define has_dos_drive_prefix(path) 0
+static inline int git_has_dos_drive_prefix(const char *path)
+{
+ return 0;
+}
+#define has_dos_drive_prefix git_has_dos_drive_prefix
#endif
-#ifndef offset_1st_component
-#define offset_1st_component(path) (is_dir_sep((path)[0]))
+#ifndef is_dir_sep
+static inline int git_is_dir_sep(int c)
+{
+ return c == '/';
+}
+#define is_dir_sep git_is_dir_sep
#endif
-#ifndef is_dir_sep
-#define is_dir_sep(c) ((c) == '/')
+#ifndef offset_1st_component
+static inline int git_offset_1st_component(const char *path)
+{
+ return is_dir_sep(path[0]);
+}
+#define offset_1st_component git_offset_1st_component
#endif
#ifndef find_last_dir_sep
-#define find_last_dir_sep(path) strrchr(path, '/')
+static inline char *git_find_last_dir_sep(const char *path)
+{
+ return strrchr(path, '/');
+}
+#define find_last_dir_sep git_find_last_dir_sep
#endif
#if defined(__HP_cc) && (__HP_cc >= 61000)
--
2.1.0
--
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] run-command: introduce CHILD_PROCESS_INIT
Most struct child_process variables are cleared using memset right after
declaration. Provide a macro, CHILD_PROCESS_INIT, that can be used to
initialize them statically instead. That's shorter, doesn't require a
function call and is slightly more readable (especially given that we
already have similar macros like STRBUF_INIT, ARGV_ARRAY_INIT etc.).
Signed-off-by: Rene Scharfe
---
Documentation/technical/api-run-command.txt | 4 ++--
archive-tar.c | 3 +--
builtin/add.c | 3 +--
builtin/commit.c| 3 +--
builtin/help.c | 3 +--
builtin/merge.c | 3 +--
builtin/notes.c | 3 +--
builtin/remote-ext.c| 3 +--
builtin/repack.c| 3 +--
builtin/replace.c | 4 ++--
builtin/verify-pack.c | 3 +--
bundle.c| 6 ++
connected.c | 3 +--
convert.c | 3 +--
credential-cache.c | 3 +--
credential.c| 3 +--
daemon.c| 8 +++-
diff.c | 3 +--
editor.c| 3 +--
fetch-pack.c| 3 +--
gpg-interface.c | 6 ++
http-backend.c | 3 +--
http.c | 3 +--
imap-send.c | 2 +-
prompt.c| 3 +--
remote-curl.c | 3 +--
remote-testsvn.c| 3 +--
run-command.h | 2 ++
send-pack.c | 3 +--
submodule.c | 21 +++--
test-run-command.c | 4 +---
test-subprocess.c | 3 +--
transport.c | 12
upload-pack.c | 3 +--
wt-status.c | 3 +--
35 files changed, 51 insertions(+), 93 deletions(-)
diff --git a/Documentation/technical/api-run-command.txt
b/Documentation/technical/api-run-command.txt
index 69510ae..ca066bf 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -96,8 +96,8 @@ command to run in a sub-process.
The caller:
-1. allocates and clears (memset(&chld, 0, sizeof(chld));) a
- struct child_process variable;
+1. allocates and clears (memset(&chld, 0, sizeof(chld)); or
+ using CHILD_PROCESS_INIT) a struct child_process variable;
2. initializes the members;
3. calls start_command();
4. processes the data;
diff --git a/archive-tar.c b/archive-tar.c
index 719b629..0d1e6bd 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -395,7 +395,7 @@ static int write_tar_filter_archive(const struct archiver
*ar,
struct archiver_args *args)
{
struct strbuf cmd = STRBUF_INIT;
- struct child_process filter;
+ struct child_process filter = CHILD_PROCESS_INIT;
const char *argv[2];
int r;
@@ -406,7 +406,6 @@ static int write_tar_filter_archive(const struct archiver
*ar,
if (args->compression_level >= 0)
strbuf_addf(&cmd, " -%d", args->compression_level);
- memset(&filter, 0, sizeof(filter));
argv[0] = cmd.buf;
argv[1] = NULL;
filter.argv = argv;
diff --git a/builtin/add.c b/builtin/add.c
index 4baf3a5..352b85e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -180,7 +180,7 @@ static int edit_patch(int argc, const char **argv, const
char *prefix)
char *file = git_pathdup("ADD_EDIT.patch");
const char *apply_argv[] = { "apply", "--recount", "--cached",
NULL, NULL };
- struct child_process child;
+ struct child_process child = CHILD_PROCESS_INIT;
struct rev_info rev;
int out;
struct stat st;
@@ -214,7 +214,6 @@ static int edit_patch(int argc, const char **argv, const
char *prefix)
if (!st.st_size)
die(_("Empty patch. Aborted."));
- memset(&child, 0, sizeof(child));
child.git_cmd = 1;
child.argv = apply_argv;
if (run_command(&child))
diff --git a/builtin/commit.c b/builtin/commit.c
index 5ed6036..b8b8663 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1508,7 +1508,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
{
/* oldsha1 SP newsha1 LF NUL */
static char buf[2*40 + 3];
- struct child_process proc;
+ struct child_process proc = CHILD_PROCESS_INIT;
const char *argv[3];
int code;
size_t n
Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT
Am 17.08.2014 um 09:12 schrieb Jeff King: On Sun, Aug 17, 2014 at 12:55:23AM +0200, René Scharfe wrote: Most struct child_process variables are cleared using memset right after declaration. Provide a macro, CHILD_PROCESS_INIT, that can be used to initialize them statically instead. That's shorter, doesn't require a function call and is slightly more readable (especially given that we already have similar macros like STRBUF_INIT, ARGV_ARRAY_INIT etc.). I think one reason we never had an INIT macro here is that you cannot simply use the struct after zero-ing it anyway. That's just the first step, and then you have to tweak a bunch of fields to get what you want. So the memset is just one setup line out of many. Some (or most?) of these steps could be converted to named initializers -- once all supported platforms provide them.. René -- 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 1/4] run-command: introduce CHILD_PROCESS_INIT
Most struct child_process variables are cleared using memset first after
declaration. Provide a macro, CHILD_PROCESS_INIT, that can be used to
initialize them statically instead. That's shorter, doesn't require a
function call and is slightly more readable (especially given that we
already have STRBUF_INIT, ARGV_ARRAY_INIT etc.).
Helped-by: Johannes Sixt
Signed-off-by: Rene Scharfe
---
Now with ARGV_ARRAY_INIT and more conversions.
Documentation/technical/api-run-command.txt | 4 ++--
archive-tar.c | 3 +--
builtin/add.c | 3 +--
builtin/commit.c| 3 +--
builtin/help.c | 3 +--
builtin/merge.c | 3 +--
builtin/notes.c | 3 +--
builtin/receive-pack.c | 12
builtin/remote-ext.c| 3 +--
builtin/repack.c| 3 +--
builtin/replace.c | 4 ++--
builtin/verify-pack.c | 3 +--
bundle.c| 6 ++
column.c| 2 +-
connect.c | 2 +-
connected.c | 3 +--
convert.c | 3 +--
credential-cache.c | 3 +--
credential.c| 3 +--
daemon.c| 8 +++-
diff.c | 3 +--
editor.c| 3 +--
fetch-pack.c| 3 +--
gpg-interface.c | 6 ++
http-backend.c | 3 +--
http.c | 3 +--
imap-send.c | 2 +-
pager.c | 2 +-
prompt.c| 3 +--
remote-curl.c | 3 +--
remote-testsvn.c| 3 +--
run-command.c | 3 +--
run-command.h | 2 ++
send-pack.c | 3 +--
submodule.c | 21 +++--
test-run-command.c | 4 +---
test-subprocess.c | 3 +--
transport.c | 12
upload-pack.c | 5 ++---
wt-status.c | 3 +--
40 files changed, 60 insertions(+), 107 deletions(-)
diff --git a/Documentation/technical/api-run-command.txt
b/Documentation/technical/api-run-command.txt
index 69510ae..ca066bf 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -96,8 +96,8 @@ command to run in a sub-process.
The caller:
-1. allocates and clears (memset(&chld, 0, sizeof(chld));) a
- struct child_process variable;
+1. allocates and clears (memset(&chld, 0, sizeof(chld)); or
+ using CHILD_PROCESS_INIT) a struct child_process variable;
2. initializes the members;
3. calls start_command();
4. processes the data;
diff --git a/archive-tar.c b/archive-tar.c
index 719b629..0d1e6bd 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -395,7 +395,7 @@ static int write_tar_filter_archive(const struct archiver
*ar,
struct archiver_args *args)
{
struct strbuf cmd = STRBUF_INIT;
- struct child_process filter;
+ struct child_process filter = CHILD_PROCESS_INIT;
const char *argv[2];
int r;
@@ -406,7 +406,6 @@ static int write_tar_filter_archive(const struct archiver
*ar,
if (args->compression_level >= 0)
strbuf_addf(&cmd, " -%d", args->compression_level);
- memset(&filter, 0, sizeof(filter));
argv[0] = cmd.buf;
argv[1] = NULL;
filter.argv = argv;
diff --git a/builtin/add.c b/builtin/add.c
index 4baf3a5..352b85e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -180,7 +180,7 @@ static int edit_patch(int argc, const char **argv, const
char *prefix)
char *file = git_pathdup("ADD_EDIT.patch");
const char *apply_argv[] = { "apply", "--recount", "--cached",
NULL, NULL };
- struct child_process child;
+ struct child_process child = CHILD_PROCESS_INIT;
struct rev_info rev;
int out;
struct stat st;
@@ -214,7 +214,6 @@ static int edit_patch(int argc, const char **argv, const
char *prefix)
if (!st.st_size)
die(_("Empty patch. Aborted."));
- memset(&child, 0, sizeof(child));
child.git_cmd = 1;
child.argv = apply_argv;
if (run_command(&child))
diff --git a/builtin/commit.c b/builtin/commit.c
index 5ed6036..b8b8663 100644
--- a/builtin/commit.c
++
[PATCH v2 3/4] run-command: call run_command_v_opt_cd_env() instead of duplicating it
Signed-off-by: Rene Scharfe
---
run-command.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/run-command.c b/run-command.c
index 47ab21b..9196ee0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -577,9 +577,7 @@ static void prepare_run_command_v_opt(struct child_process
*cmd,
int run_command_v_opt(const char **argv, int opt)
{
- struct child_process cmd;
- prepare_run_command_v_opt(&cmd, argv, opt);
- return run_command(&cmd);
+ return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
}
int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir,
const char *const *env)
--
2.1.0
--
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/4] run-command: introduce child_process_init()
Add a helper function for initializing those struct child_process
variables for which the macro CHILD_PROCESS_INIT can't be used.
Suggested-by: Jeff King
Signed-off-by: Rene Scharfe
---
Documentation/technical/api-run-command.txt | 8 ++--
connect.c | 6 --
run-command.c | 6 ++
run-command.h | 1 +
transport-helper.c | 5 +++--
5 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/Documentation/technical/api-run-command.txt
b/Documentation/technical/api-run-command.txt
index ca066bf..842b838 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -13,6 +13,10 @@ produces in the caller in order to process it.
Functions
-
+`child_process_init`
+
+ Initialize a struct child_process variable.
+
`start_command`::
Start a sub-process. Takes a pointer to a `struct child_process`
@@ -96,8 +100,8 @@ command to run in a sub-process.
The caller:
-1. allocates and clears (memset(&chld, 0, sizeof(chld)); or
- using CHILD_PROCESS_INIT) a struct child_process variable;
+1. allocates and clears (using child_process_init() or
+ CHILD_PROCESS_INIT) a struct child_process variable;
2. initializes the members;
3. calls start_command();
4. processes the data;
diff --git a/connect.c b/connect.c
index f5b930a..87b5202 100644
--- a/connect.c
+++ b/connect.c
@@ -537,7 +537,8 @@ static struct child_process *git_proxy_connect(int fd[2],
char *host)
get_host_and_port(&host, &port);
- proxy = xcalloc(1, sizeof(*proxy));
+ proxy = xmalloc(sizeof(*proxy));
+ child_process_init(proxy);
argv_array_push(&proxy->args, git_proxy_command);
argv_array_push(&proxy->args, host);
argv_array_push(&proxy->args, port);
@@ -694,7 +695,8 @@ struct child_process *git_connect(int fd[2], const char
*url,
target_host, 0);
free(target_host);
} else {
- conn = xcalloc(1, sizeof(*conn));
+ conn = xmalloc(sizeof(*conn));
+ child_process_init(conn);
strbuf_addstr(&cmd, prog);
strbuf_addch(&cmd, ' ');
diff --git a/run-command.c b/run-command.c
index a29a34f..47ab21b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -8,6 +8,12 @@
# define SHELL_PATH "/bin/sh"
#endif
+void child_process_init(struct child_process *child)
+{
+ memset(child, 0, sizeof(*child));
+ argv_array_init(&child->args);
+}
+
struct child_to_clean {
pid_t pid;
struct child_to_clean *next;
diff --git a/run-command.h b/run-command.h
index 5484400..1b135d1 100644
--- a/run-command.h
+++ b/run-command.h
@@ -45,6 +45,7 @@ struct child_process {
};
#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT }
+void child_process_init(struct child_process *);
int start_command(struct child_process *);
int finish_command(struct child_process *);
diff --git a/transport-helper.c b/transport-helper.c
index 3d8fe7d..080a7a6 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -118,7 +118,8 @@ static struct child_process *get_helper(struct transport
*transport)
if (data->helper)
return data->helper;
- helper = xcalloc(1, sizeof(*helper));
+ helper = xmalloc(sizeof(*helper));
+ child_process_init(helper);
helper->in = -1;
helper->out = -1;
helper->err = 0;
@@ -395,7 +396,7 @@ static int get_importer(struct transport *transport, struct
child_process *fasti
struct child_process *helper = get_helper(transport);
struct helper_data *data = transport->data;
int cat_blob_fd, code;
- memset(fastimport, 0, sizeof(*fastimport));
+ child_process_init(fastimport);
fastimport->in = helper->out;
argv_array_push(&fastimport->args, "fast-import");
argv_array_push(&fastimport->args, debug ? "--stats" : "--quiet");
--
2.1.0
--
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 4/4] run-command: inline prepare_run_command_v_opt()
Merge prepare_run_command_v_opt() and its only caller. This removes a
pointer indirection and allows to initialize the struct child_process
using CHILD_PROCESS_INIT.
Signed-off-by: Rene Scharfe
---
run-command.c | 24
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/run-command.c b/run-command.c
index 9196ee0..761f0fd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -561,20 +561,6 @@ int run_command(struct child_process *cmd)
return finish_command(cmd);
}
-static void prepare_run_command_v_opt(struct child_process *cmd,
- const char **argv,
- int opt)
-{
- memset(cmd, 0, sizeof(*cmd));
- cmd->argv = argv;
- cmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
- cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
- cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
- cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
- cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
- cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
-}
-
int run_command_v_opt(const char **argv, int opt)
{
return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
@@ -582,8 +568,14 @@ int run_command_v_opt(const char **argv, int opt)
int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir,
const char *const *env)
{
- struct child_process cmd;
- prepare_run_command_v_opt(&cmd, argv, opt);
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ cmd.argv = argv;
+ cmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
+ cmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
+ cmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
+ cmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
+ cmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;
+ cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
cmd.dir = dir;
cmd.env = env;
return run_command(&cmd);
--
2.1.0
--
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] sha1_name: avoid quadratic list insertion in handle_one_ref
Similar to 16445242 (fetch-pack: avoid quadratic list insertion in
mark_complete), sort only after all refs are collected instead of while
inserting. The result is the same, but it's more efficient that way.
The difference will only be measurable in repositories with a large
number of refs.
Signed-off-by: Rene Scharfe
---
sha1_name.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sha1_name.c b/sha1_name.c
index 63ee66f..7098b10 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -839,7 +839,7 @@ static int handle_one_ref(const char *path,
}
if (object->type != OBJ_COMMIT)
return 0;
- commit_list_insert_by_date((struct commit *)object, list);
+ commit_list_insert((struct commit *)object, list);
return 0;
}
@@ -1366,6 +1366,7 @@ static int get_sha1_with_context_1(const char *name,
if (!only_to_die && namelen > 2 && name[1] == '/') {
struct commit_list *list = NULL;
for_each_ref(handle_one_ref, &list);
+ commit_list_sort_by_date(&list);
return get_sha1_oneline(name + 2, sha1, list);
}
if (namelen < 3 ||
--
2.1.0
--
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] walker: avoid quadratic list insertion in mark_complete
Similar to 16445242 (fetch-pack: avoid quadratic list insertion in
mark_complete), sort only after all refs are collected instead of while
inserting. The result is the same, but it's more efficient that way.
The difference will only be measurable in repositories with a large
number of refs.
Signed-off-by: Rene Scharfe
---
walker.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/walker.c b/walker.c
index 0148264..0596e99 100644
--- a/walker.c
+++ b/walker.c
@@ -205,7 +205,7 @@ static int mark_complete(const char *path, const unsigned
char *sha1, int flag,
struct commit *commit = lookup_commit_reference_gently(sha1, 1);
if (commit) {
commit->object.flags |= COMPLETE;
- commit_list_insert_by_date(commit, &complete);
+ commit_list_insert(commit, &complete);
}
return 0;
}
@@ -271,8 +271,10 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
}
}
- if (!walker->get_recover)
+ if (!walker->get_recover) {
for_each_ref(mark_complete, NULL);
+ commit_list_sort_by_date(&complete);
+ }
for (i = 0; i < targets; i++) {
if (interpret_target(walker, target[i], &sha1[20 * i])) {
--
2.1.0
--
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: [BUG] resolved deltas
Am 22.08.2014 um 21:41 schrieb Martin von Gagern: > On 21.08.2014 13:35, Petr Stodulka wrote: >> Hi guys, >> I wanted post you patch here for this bug, but I can't find primary >> source of this problem [0], because I don't understand some ideas in the >> code. >> >> […] >> >> Any next ideas/hints or explanation of these functions? I began study >> source code and mechanisms of the git this week, so don't beat me yet >> please :-) >> >> Regards, >> Petr >> >> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1099919 > > Some pointers to related reports and investigations: > > https://groups.google.com/forum/#!topic/mapsforge-dev/IF6mgmwvZmY > https://groups.google.com/forum/#!topic/mapsforge-dev/f2KvFALlkvo > https://code.google.com/p/support/issues/detail?id=31571 > https://groups.google.com/forum/#!topic/mapsforge-dev/nomzr5dkkqc > http://thread.gmane.org/gmane.comp.version-control.git/254626 > > The last is my own bug report to this mailing list here, which > unfortunately received no reaction yet. In that report, I can confirm > that the commit introducing the assertion is the same commit which > causes things to fail: > https://github.com/git/git/commit/7218a215efc7ae46f7ca8d82442f354e > > In this https://code.google.com/p/mapsforge/ repository, resolve_delta > gets called twice for some delta. The first time, type and real_type are > identical, but by the second pass in find_unresolved_deltas the real > type will have chaned to OBJ_TREE. This caused the old code to simply > scip the object, but the new code aborts instead. > > So far my understanding. I'm not sure whether this kind of duplicate > resolution is something normal or indicates some breakage in the > repository in question. But aborting seems a bad solution in either case. Git 1.7.6 clones the repo without reporting an error or warning, but a check shows that a tree object is missing: $ git clone https://code.google.com/p/mapsforge/ Cloning into mapsforge... remote: Counting objects: 12879, done. Receiving objects: 100% (12879/12879), 10.19 MiB | 2.48 MiB/s, done. Resolving deltas: 100% (4349/4349), done. $ cd mapsforge/ $ git fsck broken link fromtree eb95fb0268c43f512e2ce512e6625072acafbdb5 totree a0155d8d5bb58afb9a5d20459be023899c9a1cef missing tree a0155d8d5bb58afb9a5d20459be023899c9a1cef dangling tree b6f32087526f43205bf8b5e6539936da787ecb1a Git 2.1.0 hits an assertion: $ git clone https://code.google.com/p/mapsforge/ Cloning into 'mapsforge'... remote: Counting objects: 12879, done. Receiving objects: 100% (12879/12879), 10.19 MiB | 2.53 MiB/s, done. git: builtin/index-pack.c:918: find_unresolved_deltas_1: Assertion `child->real_type == OBJ_REF_DELTA' failed. error: index-pack died of signal 6 fatal: index-pack failed The patch below, which makes index-pack ignore objects with unexpected real_type as before, changes the shown error message, but clone still fails: $ git clone https://code.google.com/p/mapsforge/ Cloning into 'mapsforge'... remote: Counting objects: 12879, done. Receiving objects: 100% (12879/12879), 10.19 MiB | 2.43 MiB/s, done. Resolving deltas: 100% (4348/4348), done. fatal: did not receive expected object a0155d8d5bb58afb9a5d20459be023899c9a1cef fatal: index-pack failed Turning that fatal error into a warning get us a bit further: $ git clone https://code.google.com/p/mapsforge/ Cloning into 'mapsforge'... remote: Counting objects: 12879, done. Receiving objects: 100% (12879/12879), 10.19 MiB | 2.38 MiB/s, done. Resolving deltas: 100% (4350/4350), done. warning: did not receive expected object a0155d8d5bb58afb9a5d20459be023899c9a1cef fatal: The same object bc386be34bd4781f71bb68d72a6e0aee3124201e appears twice in the pack fatal: index-pack failed Disabling strict checking (WRITE_IDX_STRICT) as well lets clone succeed, but a check shows that a tree is missing, as wiht git 1.7.6: $ git clone https://code.google.com/p/mapsforge/ Cloning into 'mapsforge'... remote: Counting objects: 12879, done. Receiving objects: 100% (12879/12879), 10.19 MiB | 2.37 MiB/s, done. Resolving deltas: 100% (4349/4349), done. warning: did not receive expected object a0155d8d5bb58afb9a5d20459be023899c9a1cef Checking connectivity... done. $ cd mapsforge/ $ git fsck Checking object directories: 100% (256/256), done. Checking objects: 100% (12879/12879), done. broken link fromtree eb95fb0268c43f512e2ce512e6625072acafbdb5 totree a0155d8d5bb58afb9a5d20459be023899c9a1cef missing tree a0155d8d5bb58afb9a5d20459be023899c9a1cef Cloning the repo with Egit works fine, but git fsck shows the same results as above. https://github.com/applantation/mapsforge has that missing tree object, by the way. OK, what now? It's better to show an error message instead of failing an assertion if a repo is found to be corrupt because the issue is caused by external input. I don't know if the patch below may hav
Re: [BUG] resolved deltas
Am 23.08.2014 um 13:18 schrieb Jeff King:
> On Sat, Aug 23, 2014 at 07:04:59AM -0400, Jeff King wrote:
>
>> On Sat, Aug 23, 2014 at 06:56:40AM -0400, Jeff King wrote:
>>
>>> So I think your patch is doing the right thing.
>>
>> By the way, if you want to add a test to your patch, there is
>> infrastructure in t5308 to create packs with duplicate objects. If I
>> understand the problem correctly, you could trigger this by having a
>> delta object whose base is duplicated, even without the missing object.
>
> This actually turned out to be really easy. The test below fails without
> your patch and passes with it. Please feel free to squash it in.
>
> diff --git a/t/t5308-pack-detect-duplicates.sh
> b/t/t5308-pack-detect-duplicates.sh
> index 9c5a876..50f7a69 100755
> --- a/t/t5308-pack-detect-duplicates.sh
> +++ b/t/t5308-pack-detect-duplicates.sh
> @@ -77,4 +77,19 @@ test_expect_success 'index-pack can reject packs with
> duplicates' '
> test_expect_code 1 git cat-file -e $LO_SHA1
> '
>
> +test_expect_success 'duplicated delta base does not trigger assert' '
> + clear_packs &&
> + {
> + A=01d7713666f4de822776c7622c10f1b07de280dc &&
> + B=e68fe8129b546b101aee9510c5328e7f21ca1d18 &&
> + pack_header 3 &&
> + pack_obj $A $B &&
> + pack_obj $B &&
> + pack_obj $B
> + } >dups.pack &&
> + pack_trailer dups.pack &&
> + git index-pack --stdin + test_must_fail git index-pack --stdin --strict +'
> +
> test_done
Thanks, that looks good. But while preparing the patch I noticed that
the added test sometimes fails. Helgrind pointed outet a race
condition. It is not caused by the patch to turn the asserts into
regular ifs, however -- here's a Helgrind report for the original code
with the new test:
==34949== Helgrind, a thread error detector
==34949== Copyright (C) 2007-2013, and GNU GPL'd, by OpenWorks LLP et al.
==34949== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
==34949== Command: /home/lsr/src/git/t/../bin-wrappers/git index-pack --stdin
==34949==
==34949== Helgrind, a thread error detector
==34949== Copyright (C) 2007-2013, and GNU GPL'd, by OpenWorks LLP et al.
==34949== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
==34949== Command: /home/lsr/src/git/git index-pack --stdin
==34949==
==34949== ---Thread-Announcement--
==34949==
==34949== Thread #3 was created
==34949==at 0x594DF7E: clone (clone.S:74)
==34949==by 0x544A2B9: do_clone.constprop.3 (createthread.c:75)
==34949==by 0x544B762: pthread_create@@GLIBC_2.2.5 (createthread.c:245)
==34949==by 0x4C2D55D: pthread_create_WRK (hg_intercepts.c:269)
==34949==by 0x43ABB8: cmd_index_pack (index-pack.c:1097)
==34949==by 0x405B6A: handle_builtin (git.c:351)
==34949==by 0x404CE8: main (git.c:575)
==34949==
==34949== ---Thread-Announcement--
==34949==
==34949== Thread #2 was created
==34949==at 0x594DF7E: clone (clone.S:74)
==34949==by 0x544A2B9: do_clone.constprop.3 (createthread.c:75)
==34949==by 0x544B762: pthread_create@@GLIBC_2.2.5 (createthread.c:245)
==34949==by 0x4C2D55D: pthread_create_WRK (hg_intercepts.c:269)
==34949==by 0x43ABB8: cmd_index_pack (index-pack.c:1097)
==34949==by 0x405B6A: handle_builtin (git.c:351)
==34949==by 0x404CE8: main (git.c:575)
==34949==
==34949==
==34949==
==34949== Possible data race during read of size 4 at 0x5E15910 by thread #3
==34949== Locks held: none
==34949==at 0x439327: find_unresolved_deltas (index-pack.c:918)
==34949==by 0x439666: threaded_second_pass (index-pack.c:1002)
==34949==by 0x4C2D6F6: mythread_wrapper (hg_intercepts.c:233)
==34949==by 0x544B0A3: start_thread (pthread_create.c:309)
==34949==
==34949== This conflicts with a previous write of size 4 by thread #2
==34949== Locks held: none
==34949==at 0x4390E2: resolve_delta (index-pack.c:865)
==34949==by 0x439340: find_unresolved_deltas (index-pack.c:919)
==34949==by 0x439666: threaded_second_pass (index-pack.c:1002)
==34949==by 0x4C2D6F6: mythread_wrapper (hg_intercepts.c:233)
==34949==by 0x544B0A3: start_thread (pthread_create.c:309)
==34949==
==34949== Address 0x5E15910 is 48 bytes inside a block of size 256 alloc'd
==34949==at 0x4C2A7D0: calloc (vg_replace_malloc.c:618)
==34949==by 0x50CA83: xcalloc (wrapper.c:119)
==34949==by 0x439AF6: cmd_index_pack (index-pack.c:1643)
==34949==by 0x405B6A: handle_builtin (git.c:351)
==34949==by 0x404CE8: main (git.c:575)
==34949==
git: builtin/index-pack.c:918: find_unresolved_deltas_1: Assertion
`child->real_type == OBJ_REF_DELTA' failed.
==34949==
==34949== For counts of detected and suppressed errors, rerun with: -v
==34949== Use --history-level=approx or =none to gain increased speed, at
==34949== the cost of redu
[PATCH] connect: simplify check_ref() using skip_prefix() and starts_with()
Both callers of check_ref() pass in NUL-terminated strings for name.
Remove the len parameter and then use skip_prefix() and starts_with()
instead of memcmp() to check if it starts with certain strings. This
gets rid of several magic string length constants and a strlen() call.
Signed-off-by: Rene Scharfe
---
connect.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/connect.c b/connect.c
index 5047402..34193e5 100644
--- a/connect.c
+++ b/connect.c
@@ -13,28 +13,24 @@
static char *server_capabilities;
static const char *parse_feature_value(const char *, const char *, int *);
-static int check_ref(const char *name, int len, unsigned int flags)
+static int check_ref(const char *name, unsigned int flags)
{
if (!flags)
return 1;
- if (len < 5 || memcmp(name, "refs/", 5))
+ if (!skip_prefix(name, "refs/", &name))
return 0;
- /* Skip the "refs/" part */
- name += 5;
- len -= 5;
-
/* REF_NORMAL means that we don't want the magic fake tag refs */
if ((flags & REF_NORMAL) && check_refname_format(name, 0))
return 0;
/* REF_HEADS means that we want regular branch heads */
- if ((flags & REF_HEADS) && !memcmp(name, "heads/", 6))
+ if ((flags & REF_HEADS) && starts_with(name, "heads/"))
return 1;
/* REF_TAGS means that we want tags */
- if ((flags & REF_TAGS) && !memcmp(name, "tags/", 5))
+ if ((flags & REF_TAGS) && starts_with(name, "tags/"))
return 1;
/* All type bits clear means that we are ok with anything */
@@ -43,7 +39,7 @@ static int check_ref(const char *name, int len, unsigned int
flags)
int check_ref_type(const struct ref *ref, int flags)
{
- return check_ref(ref->name, strlen(ref->name), flags);
+ return check_ref(ref->name, flags);
}
static void die_initial_contact(int got_at_least_one_head)
@@ -167,7 +163,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t
src_len,
continue;
}
- if (!check_ref(name, name_len, flags))
+ if (!check_ref(name, flags))
continue;
ref = alloc_ref(buffer + 41);
hashcpy(ref->old_sha1, old_sha1);
--
2.1.0
--
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] pack-write: simplify index_pack_lockfile using skip_prefix() and xstrfmt()
Get rid of magic string length constants by using skip_prefix() instead
of memcmp() and use xstrfmt() for building a string instead of a
PATH_MAX-sized buffer, snprintf() and xstrdup().
Signed-off-by: Rene Scharfe
---
pack-write.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/pack-write.c b/pack-write.c
index 9ccf804..33293ce 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -288,13 +288,12 @@ char *index_pack_lockfile(int ip_out)
* case, we need it to remove the corresponding .keep file
* later on. If we don't get that then tough luck with it.
*/
- if (read_in_full(ip_out, packname, 46) == 46 && packname[45] == '\n' &&
- memcmp(packname, "keep\t", 5) == 0) {
- char path[PATH_MAX];
+ if (read_in_full(ip_out, packname, 46) == 46 && packname[45] == '\n') {
+ const char *name;
packname[45] = 0;
- snprintf(path, sizeof(path), "%s/pack/pack-%s.keep",
-get_object_directory(), packname + 5);
- return xstrdup(path);
+ if (skip_prefix(packname, "keep\t", &name))
+ return xstrfmt("%s/pack/pack-%s.keep",
+ get_object_directory(), name);
}
return NULL;
}
--
2.1.0
--
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] http-walker: simplify process_alternates_response() using strbuf
Use strbuf to build the new base, which takes care of allocations and
the terminating NUL character automatically.
Signed-off-by: Rene Scharfe
---
http-walker.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index dbddfaa..88da546 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -230,7 +230,6 @@ static void process_alternates_response(void *callback_data)
int okay = 0;
int serverlen = 0;
struct alt_base *newalt;
- char *target = NULL;
if (data[i] == '/') {
/*
* This counts
@@ -287,17 +286,15 @@ static void process_alternates_response(void
*callback_data)
}
/* skip "objects\n" at end */
if (okay) {
- target = xmalloc(serverlen + posn - i - 6);
- memcpy(target, base, serverlen);
- memcpy(target + serverlen, data + i,
- posn - i - 7);
- target[serverlen + posn - i - 7] = 0;
+ struct strbuf target = STRBUF_INIT;
+ strbuf_add(&target, base, serverlen);
+ strbuf_add(&target, data + i, posn - i - 7);
if (walker->get_verbosely)
- fprintf(stderr,
- "Also look at %s\n", target);
+ fprintf(stderr, "Also look at %s\n",
+ target.buf);
newalt = xmalloc(sizeof(*newalt));
newalt->next = NULL;
- newalt->base = target;
+ newalt->base = strbuf_detach(&target, NULL);
newalt->got_indices = 0;
newalt->packs = NULL;
--
2.1.0
--
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 2/2] index-pack: handle duplicate base objects gracefully
Am 30.08.2014 um 15:16 schrieb Jeff King: On Fri, Aug 29, 2014 at 07:59:32PM -0700, Shawn Pearce wrote: I agree it is probably a bug on the sending side, but I think last time this came up we decided to try to be liberal in what we accept. c.f. http://thread.gmane.org/gmane.comp.version-control.git/232305/focus=232310 IIRC they aren't valid pack files to contain duplicates. Once upon a time JGit had a bug and android.googlesource.com returned duplicate objects in a Linux kernel repository. This caused at least some versions of git-core to fail very badly in binary search at object lookup time or something. We had a lot of users angry with us. :) I know Nico said its OK last year, but its really not. I don't think implementations are capable of handling it. We do detect and complain if --strict is given. Should we make it the default instead? I think it is still worthwhile to have a mode that can handle these packs. It may be the only reasonable way to recover the data from such a broken pack (and that broken pack may be the only copy of the data you have, if you are stuck getting it out of a broken implementation on a remote server). Sounds reasonable; being able to extract code from broken repos -- especially in this real-world case -- is beneficial. My only nit with patch 2: Petr Stodulka and Martin von Gagern should be mentioned as bug reporters. René -- 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] config: simplify git_config_include()
Instead of using skip_prefix() to check the first part of the string
and then strcmp() to check the rest, simply use strcmp() to check the
whole string.
Signed-off-by: Rene Scharfe
---
config.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/config.c b/config.c
index 058505c..c558a21 100644
--- a/config.c
+++ b/config.c
@@ -127,7 +127,6 @@ static int handle_path_include(const char *path, struct
config_include_data *inc
int git_config_include(const char *var, const char *value, void *data)
{
struct config_include_data *inc = data;
- const char *type;
int ret;
/*
@@ -138,10 +137,7 @@ int git_config_include(const char *var, const char *value,
void *data)
if (ret < 0)
return ret;
- if (!skip_prefix(var, "include.", &type))
- return ret;
-
- if (!strcmp(type, "path"))
+ if (!strcmp(var, "include.path"))
ret = handle_path_include(value, inc);
return ret;
}
--
2.1.0
--
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] imap-send: simplify v_issue_imap_cmd() and get_cmd_result() using starts_with()
Use starts_with() instead of memcmp() to check if NUL-terminated
strings match prefixes. This gets rid of some magic string length
constants.
Signed-off-by: Rene Scharfe
---
imap-send.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 524fbab..b079a0d 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -524,7 +524,7 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store
*ctx,
if (Verbose) {
if (imap->num_in_progress)
printf("(%d in progress) ", imap->num_in_progress);
- if (memcmp(cmd->cmd, "LOGIN", 5))
+ if (!starts_with(cmd->cmd, "LOGIN"))
printf(">>> %s", buf);
else
printf(">>> %d LOGIN \n", cmd->tag);
@@ -802,7 +802,10 @@ static int get_cmd_result(struct imap_store *ctx, struct
imap_cmd *tcmd)
resp = DRV_OK;
else {
if (!strcmp("NO", arg)) {
- if (cmdp->cb.create && cmd &&
(cmdp->cb.trycreate || !memcmp(cmd, "[TRYCREATE]", 11))) { /* SELECT, APPEND or
UID COPY */
+ if (cmdp->cb.create && cmd &&
+ (cmdp->cb.trycreate ||
+starts_with(cmd, "[TRYCREATE]"))) {
+ /* SELECT, APPEND or UID COPY */
p = strchr(cmdp->cmd, '"');
if (!issue_imap_cmd(ctx, NULL,
"CREATE \"%.*s\"", (int)(strchr(p + 1, '"') - p + 1), p)) {
resp = RESP_BAD;
@@ -827,7 +830,7 @@ static int get_cmd_result(struct imap_store *ctx, struct
imap_cmd *tcmd)
} else /*if (!strcmp("BAD", arg))*/
resp = RESP_BAD;
fprintf(stderr, "IMAP command '%s' returned
response (%s) - %s\n",
-memcmp(cmdp->cmd, "LOGIN", 5) ?
+ !starts_with(cmdp->cmd, "LOGIN") ?
cmdp->cmd : "LOGIN
",
arg, cmd ? cmd : "");
}
--
2.1.0
--
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] merge-tree: remove unused df_conflict arguments
merge_trees_recursive() stores a pointer to its parameter df_conflict in
its struct traverse_info, but it is never actually used. Stop doing
that, remove the parameter and inline the function into merge_trees(),
as the latter is now only passing on its parameters.
Remove the parameter df_conflict from unresolved_directory() as well,
now that there is no way to pass it to merge_trees_recursive() through
that function anymore.
Signed-off-by: Rene Scharfe
---
builtin/merge-tree.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 61cbde4..f9ab485 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -25,7 +25,7 @@ static void add_merge_entry(struct merge_list *entry)
merge_result_end = &entry->next;
}
-static void merge_trees_recursive(struct tree_desc t[3], const char *base, int
df_conflict);
+static void merge_trees(struct tree_desc t[3], const char *base);
static const char *explanation(struct merge_list *entry)
{
@@ -195,8 +195,8 @@ static void resolve(const struct traverse_info *info,
struct name_entry *ours, s
add_merge_entry(final);
}
-static void unresolved_directory(const struct traverse_info *info, struct
name_entry n[3],
-int df_conflict)
+static void unresolved_directory(const struct traverse_info *info,
+struct name_entry n[3])
{
char *newbase;
struct name_entry *p;
@@ -218,7 +218,7 @@ static void unresolved_directory(const struct traverse_info
*info, struct name_e
buf2 = fill_tree_descriptor(t+2, ENTRY_SHA1(n + 2));
#undef ENTRY_SHA1
- merge_trees_recursive(t, newbase, df_conflict);
+ merge_trees(t, newbase);
free(buf0);
free(buf1);
@@ -259,7 +259,7 @@ static void unresolved(const struct traverse_info *info,
struct name_entry n[3])
dirmask |= (1 << i);
}
- unresolved_directory(info, n, dirmask && (dirmask != mask));
+ unresolved_directory(info, n);
if (dirmask == mask)
return;
@@ -335,21 +335,15 @@ static int threeway_callback(int n, unsigned long mask,
unsigned long dirmask, s
return mask;
}
-static void merge_trees_recursive(struct tree_desc t[3], const char *base, int
df_conflict)
+static void merge_trees(struct tree_desc t[3], const char *base)
{
struct traverse_info info;
setup_traverse_info(&info, base);
- info.data = &df_conflict;
info.fn = threeway_callback;
traverse_trees(3, t, &info);
}
-static void merge_trees(struct tree_desc t[3], const char *base)
-{
- merge_trees_recursive(t, base, 0);
-}
-
static void *get_tree_descriptor(struct tree_desc *desc, const char *rev)
{
unsigned char sha1[20];
--
2.1.0
--
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 2/2] index-pack: handle duplicate base objects gracefully
Am 31.08.2014 um 17:17 schrieb Jeff King: On Sat, Aug 30, 2014 at 06:00:59PM +0200, René Scharfe wrote: My only nit with patch 2: Petr Stodulka and Martin von Gagern should be mentioned as bug reporters. Yeah, I agree with that. And actually, you should get a Reported-by: on the first patch. :) However, I think there are some grave implications of this series; see the message I just posted elsewhere in the thread. I think we will end up dropping patch 2, but keep patch 1. OK, but it would still be a good idea to replace the assert()s with die()s and error messages that tell users that the repo is broken, not git. René -- 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] imap-send: simplify v_issue_imap_cmd() and get_cmd_result() using starts_with()
Am 02.09.2014 um 21:23 schrieb Junio C Hamano:
René Scharfe writes:
Use starts_with() instead of memcmp() to check if NUL-terminated
strings match prefixes. This gets rid of some magic string length
constants.
Signed-off-by: Rene Scharfe
---
imap-send.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 524fbab..b079a0d 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -802,7 +802,10 @@ static int get_cmd_result(struct imap_store *ctx, struct
imap_cmd *tcmd)
resp = DRV_OK;
else {
if (!strcmp("NO", arg)) {
- if (cmdp->cb.create && cmd && (cmdp->cb.trycreate
|| !memcmp(cmd, "[TRYCREATE]", 11))) { /* SELECT, APPEND or UID COPY */
+ if (cmdp->cb.create && cmd &&
+ (cmdp->cb.trycreate ||
+starts_with(cmd, "[TRYCREATE]"))) {
+ /* SELECT, APPEND or UID COPY */
p = strchr(cmdp->cmd, '"');
if (!issue_imap_cmd(ctx, NULL, "CREATE
\"%.*s\"", (int)(strchr(p + 1, '"') - p + 1), p)) {
resp = RESP_BAD;
Do we want this hunk, given that it will disappear with the
tf/imap-send-create topic at e0d8e308 (imap-send: create target
mailbox if it is missing, 2014-08-01)?
In that case it's unnecessary; apparently I overlooked the conflicting
hunk while preparing the patch for sending.
René
--
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] parse-options: detect attempt to add a duplicate short option name
Am 03.09.2014 um 21:42 schrieb Junio C Hamano:
> Junio C Hamano writes:
>
>>> diff --git a/builtin/revert.c b/builtin/revert.c
>>> index f9ed5bd..831c2cd 100644
>>> --- a/builtin/revert.c
>>> +++ b/builtin/revert.c
>>> @@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv,
>>> struct replay_opts *opts)
>>> N_("option for merge strategy"), option_parse_x),
>>> { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
>>> N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) ""
>>> },
>>> + OPT_BOOL('n', "no-verify", &opts->no_verify, N_("bypass
>>> pre-commit hook")),
>>
>> I doubt we want this option to squat on '-n'; besides, it is already
>> taken by a more often used "--no-commit".
>>
>> I thought that we added sanity checker for the options[] array to
>> parse-options
>> API. I wonder why it did not kick in...
>
> ... because we didn't, not quite.
>
> Perhaps like this?
>
> -- >8 --
> It is easy to overlook an already assigned single-letter option name
> and try to use it for a new one. Help the developer to catch it
> before such a mistake escapes the lab.
>
> Signed-off-by: Junio C Hamano
> ---
> diff --git a/parse-options.c b/parse-options.c
> index e7dafa8..b7925c5 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -347,12 +347,17 @@ static void check_typos(const char *arg, const struct
> option *options)
> static void parse_options_check(const struct option *opts)
> {
> int err = 0;
> + char short_opts[128];
> +
> + memset(short_opts, '\0', sizeof(short_opts));
>
> for (; opts->type != OPTION_END; opts++) {
> if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&
> (opts->flags & PARSE_OPT_OPTARG))
> err |= optbug(opts, "uses incompatible flags "
> "LASTARG_DEFAULT and OPTARG");
> + if (opts->short_name && short_opts[opts->short_name]++)
> + err |= optbug(opts, "short name already used");
> if (opts->flags & PARSE_OPT_NODASH &&
> ((opts->flags & PARSE_OPT_OPTARG) ||
>!(opts->flags & PARSE_OPT_NOARG) ||
>
Compact and useful, I like it.
You might want to squash in something like this, though. Without it
t1502 fails because -b is defined twice there.
---
t/t1502-rev-parse-parseopt.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 922423e..ebe7c3b 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\END_EXPECT
|-d, --data[=...] short and long option with an optional argument
|
|Argument hints
-|-b short option required argument
+|-B short option required argument
|--bar2 long option required argument
|-e, --fuz
| short and long option required argument
@@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\EOF
|d,data? short and long option with an optional argument
|
| Argument hints
-|b=arg short option required argument
+|B=arg short option required argument
|bar2=arg long option required argument
|e,fuz=with-space short and long option required argument
|s?someshort option optional argument
--
2.1.0
--
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] parse-options: detect attempt to add a duplicate short option name
Am 03.09.2014 um 23:05 schrieb Junio C Hamano:
René Scharfe writes:
Compact and useful, I like it.
You might want to squash in something like this, though. Without it
t1502 fails because -b is defined twice there.
Thanks. I like it to see that the check automatically propagates
even to scripts ;-)
It bugged me enough that we didn't identify which short option
letter we were complaining about
The old code did report the short option. E.g. for t1502 it said:
error: BUG: switch 'b' short name already used
You can leave that to optbug(), no need for the strbuf.
and that opts->short_name is
defined as an "int", which may cause us to overstep char[128],
I ended up doing it this way instead, though. It no longer is so
compact, even though it may still have the same usefulness.
A range check is an additional feature (increased usefulness). I guess
using invalid characters is not that common a mistake, though.
Space is allowed as a short option by the code; intentionally?
We might want to tighten the type of the short_name member to
unsigned char, but I didn't go that far yet, at least in this step.
-- >8 --
Subject: [PATCH] parse-options: detect attempt to add a duplicate short option
name
It is easy to overlook an already assigned single-letter option name
and try to use it for a new one. Help the developer to catch it
before such a mistake escapes the lab.
Helped-by: René Scharfe
Signed-off-by: Junio C Hamano
---
parse-options.c | 15 +++
t/t1502-rev-parse-parseopt.sh | 4 ++--
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index b536896..70227e9 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -345,12 +345,27 @@ static void check_typos(const char *arg, const struct
option *options)
static void parse_options_check(const struct option *opts)
{
int err = 0;
+ char short_opts[128];
+
+ memset(short_opts, '\0', sizeof(short_opts));
for (; opts->type != OPTION_END; opts++) {
if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&
(opts->flags & PARSE_OPT_OPTARG))
err |= optbug(opts, "uses incompatible flags "
"LASTARG_DEFAULT and OPTARG");
+ if (opts->short_name) {
+ struct strbuf errmsg = STRBUF_INIT;
+ if (opts->short_name < ' ' || 0x7F <= opts->short_name)
+ strbuf_addf(&errmsg, "invalid short name
(0x%02x)",
+ opts->short_name);
+ else if (short_opts[opts->short_name]++)
+ strbuf_addf(&errmsg, "short name %c already
used",
+ opts->short_name);
+ if (errmsg.len)
+ err |= optbug(opts, errmsg.buf);
+ strbuf_release(&errmsg);
+ }
if (opts->flags & PARSE_OPT_NODASH &&
((opts->flags & PARSE_OPT_OPTARG) ||
!(opts->flags & PARSE_OPT_NOARG) ||
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 922423e..ebe7c3b 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\END_EXPECT
|-d, --data[=...] short and long option with an optional argument
|
|Argument hints
-|-b short option required argument
+|-B short option required argument
|--bar2 long option required argument
|-e, --fuz
| short and long option required argument
@@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\EOF
|d,data? short and long option with an optional argument
|
| Argument hints
-|b=arg short option required argument
+|B=arg short option required argument
|bar2=arg long option required argument
|e,fuz=with-space short and long option required argument
|s?someshort option optional argument
--
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] parse-options: detect attempt to add a duplicate short option name
Am 04.09.2014 um 00:16 schrieb Junio C Hamano:
> René Scharfe writes:
>
>>> It bugged me enough that we didn't identify which short option
>>> letter we were complaining about
>>
>> The old code did report the short option. E.g. for t1502 it said:
>>
>> error: BUG: switch 'b' short name already used
>>
>> You can leave that to optbug(), no need for the strbuf.
>
> Not quite, as an opt with long name is reported with the long name
> only, which is not very nice when the problem we are reporting is
> about its short variant.
Perhaps something like the patch below helps, here and in general?
>> Space is allowed as a short option by the code; intentionally?
>
> I didn't think of a strong reason to declare either way, so, yes it
> was deliberate that I didn't tighten to disallow.
OK. I don't think it's easy to come up with a usable way for having
space as a short option, but maybe it's possible.
---
parse-options.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/parse-options.c b/parse-options.c
index b7925c5..f1c0b5d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -14,8 +14,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
int optbug(const struct option *opt, const char *reason)
{
- if (opt->long_name)
+ if (opt->long_name) {
+ if (opt->short_name)
+ return error("BUG: switch '%c' (--%s) %s",
+opt->short_name, opt->long_name, reason);
return error("BUG: option '%s' %s", opt->long_name, reason);
+ }
return error("BUG: switch '%c' %s", opt->short_name, reason);
}
--
2.1.0
--
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: "trace.c", line 219: error: identifier redeclared: trace_strbuf
Am 06.09.2014 um 21:26 schrieb dev:
>
>
>
> Build on Solaris 10 of latest git tarball fails thus :
>
> CC tag.o
> CC trace.o
> "trace.c", line 219: error: identifier redeclared: trace_strbuf
> current : function(pointer to const char, pointer to const
> struct strbuf {unsigned long alloc, unsigned long len, pointer to char
> buf}) returning void
> previous: function(pointer to struct trace_key {const pointer to
> const char key, int fd, unsigned int initialized :1, unsigned int
> need_close :1}, pointer to const struct strbuf {unsigned long alloc,
> unsigned long len, pointer to char buf}) returning void : "trace.h",
> line 33
> "trace.c", line 221: warning: argument 003 is incompatible with
> prototype:
> prototype: pointer to struct trace_key {const pointer to const
> char key, int fd, unsigned int initialized :1, unsigned int need_close
> :1} : "trace.c", line 160
> argument : pointer to const char
> "trace.c", line 390: error: reference to static identifier "offset" in
> extern inline function
> "trace.c", line 392: error: reference to static identifier "offset" in
> extern inline function
> "trace.c", line 393: error: reference to static identifier "offset" in
> extern inline function
> "trace.c", line 401: error: reference to static identifier "offset" in
> extern inline function
> "trace.c", line 403: error: reference to static identifier "offset" in
> extern inline function
> cc: acomp failed for trace.c
> gmake: *** [trace.o] Error 2
>
>
>
>
> extracted the tarball and did the following :
>
> $ gmake CFLAGS="$CFLAGS" LDFLAGS="$LD_OPTIONS" NEEDS_LIBICONV=Yes \
>> SHELL_PATH=/usr/local/bin/bash \
>> SANE_TOOL_PATH=/usr/local/bin \
>> USE_LIBPCRE=1 LIBPCREDIR=/usr/local CURLDIR=/usr/local \
>> EXPATDIR=/usr/local NEEDS_LIBINTL_BEFORE_LIBICONV=1 \
>> NEEDS_SOCKET=1 NEEDS_RESOLV=1 USE_NSEC=1 \
>> PERL_PATH=/usr/local/bin/perl \
>> TAR=/usr/bin/tar \
>> NO_PYTHON=1 DEFAULT_PAGER=/usr/xpg4/bin/more \
>> DEFAULT_EDITOR=/usr/local/bin/vim DEFAULT_HELP_FORMAT=man \
>> prefix=/usr/local
> GIT_VERSION = 2.1.0
> * new build flags
> CC credential-store.o
> * new link flags
> CC abspath.o
> CC advice.o
> CC alias.o
> CC alloc.o
> CC archive.o
> CC archive-tar.o
> CC archive-zip.o
> CC argv-array.o
> * new prefix flags
> CC attr.o
> CC base85.o
> CC bisect.o
> CC blob.o
> CC branch.o
> CC bulk-checkin.o
> CC bundle.o
> CC cache-tree.o
> CC color.o
> CC column.o
> CC combine-diff.o
> CC commit.o
> CC compat/obstack.o
> CC compat/terminal.o
> CC config.o
> CC connect.o
> CC connected.o
> CC convert.o
> CC copy.o
> CC credential.o
> CC csum-file.o
> CC ctype.o
> "ctype.c", line 50: warning: initializer does not fit or is out of
> range: 128
> "ctype.c", line 50: warning: initializer does not fit or is out of
> range: 129
> "ctype.c", line 50: warning: initializer does not fit or is out of
> range: 130
> "ctype.c", line 50: warning: initializer does not fit or is out of
> range: 131
> "ctype.c", line 50: warning: initializer does not fit or is out of
> range: 132
> "ctype.c", line 50: warning: initializer does not fit or is out of
> range: 133
> "ctype.c", line 50: warning: initializer does not fit or is out of
> range: 134
> "ctype.c", line 50: warning: initializer does not fit or is out of
> range: 135
> "ctype.c", line 51: warning: initializer does not fit or is out of
> range: 136
> "ctype.c", line 51: warning: initializer does not fit or is out of
> range: 137
> "ctype.c", line 51: warning: initializer does not fit or is out of
> range: 138
> "ctype.c", line 51: warning: initializer does not fit or is out of
> range: 139
> "ctype.c", line 51: warning: initializer does not fit or is out of
> range: 140
> "ctype.c", line 51: warning: initializer does not fit or is out of
> range: 141
> "ctype.c", line 51: warning: initializer does not fit or is out of
> range: 142
> "ctype.c", line 51: warning: initializer does not fit or is out of
> range: 143
> "ctype.c", line 52: warning: initializer does not fit or is out of
> range: 144
> "ctype.c", line 52: warning: initializer does not fit or is out of
> range: 145
> "ctype.c", line 52: warning: initializer does not fit or is out of
> range: 146
> "ctype.c", line 52: warning: initializer does not fit or is out of
> range: 147
> "ctype.c", line 52: warning: initializer does not fit or is out of
> range: 148
> "ctype.c", line 52: warning: initializer does not fit or is out of
> range: 149
> "ctype.c", line 52: warning: initializer does not fit or is out of
> range: 150
> "ctype.c", line 52: warning: initializer does not fit or is out of
> range: 151
> "ctype.c", line 53: warning: initializer does not fit or is out of
> range: 152
> "ctype.c", line 53: warning: initializer does not fit or is out of
> range: 153
> "ctype.c", line 53: warning: initialize
Re: [RFC PATCH 1/2] Makefile: add check-headers target
Am 06.09.2014 um 21:20 schrieb David Aguilar:
This allows us to ensure that each header can be included
individually without needing to include other headers first.
Sounds like a good objective.
Signed-off-by: David Aguilar
---
This patch demonstrates how to verify PATCH 2/2.
Makefile | 6 ++
check-headers.sh | 26 ++
2 files changed, 32 insertions(+)
create mode 100755 check-headers.sh
diff --git a/Makefile b/Makefile
index 30cc622..bc54024 100644
--- a/Makefile
+++ b/Makefile
@@ -2591,6 +2591,12 @@ check-docs::
check-builtins::
./check-builtins.sh
+### Make sure headers include their dependencies
+#
+check-headers::
+ ./check-headers.sh $(CC) $(ALL_CFLAGS)
+
+
### Test suite coverage testing
#
.PHONY: coverage coverage-clean coverage-compile coverage-test coverage-report
diff --git a/check-headers.sh b/check-headers.sh
new file mode 100755
index 000..bf85c41
--- /dev/null
+++ b/check-headers.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+
+exit_code=0
+
+maybe_exit () {
+ status="$1"
+ if test "$status" != 0
+ then
+ exit_code="$status"
+ if test -n "$CHECK_HEADERS_STOP"
+ then
+ exit "$status"
+ fi
+ fi
+}
+
+git ls-files *.h |
This checks all .h files in the top directory. Would it be better to
check all files in LIB_H instead? Or even all .h files in the tree
(using "git ls-files '*.h'")? The latter might be difficult because
some of the files in compat/ #include system-specific headers.
+while read header
+do
+ echo "HEADER $header" &&
+ "$@" -Wno-unused -x c -c -o "$header".bin - <"$header" &&
+ rm "$header".bin ||
+ maybe_exit $?
+done
+
+exit $exit_code
--
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: [RFC PATCH 2/2] headers: include dependent headers
Am 06.09.2014 um 21:20 schrieb David Aguilar: Add dependent headers so that including a header does not require including additional headers. This makes it so that "gcc -c $header" succeeds for each header. Signed-off-by: David Aguilar --- diff --git a/branch.h b/branch.h index 64173ab..a61fd1a 100644 --- a/branch.h +++ b/branch.h @@ -3,6 +3,9 @@ /* Functions for acting on the information about branches. */ +#include "cache.h" +#include "strbuf.h" cache.h includes strbuf.h, so the line above isn't necessary. -- 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: [RFC PATCH v2 2/2] headers: include dependent headers
Am 07.09.2014 um 02:30 schrieb David Aguilar: > Add dependent headers so that including a header does not > require including additional headers. > > This makes it so that "gcc -c $header" succeeds for each header. > > Signed-off-by: David Aguilar > --- > Addresses René's note to not include strbuf.h when cache.h is already > included. Perhaps squash this in in order to catch two more cases and also avoid including git-compat-util.h if we already have cache.h: diff --git a/builtin.h b/builtin.h index df40fce..0419af3 100644 --- a/builtin.h +++ b/builtin.h @@ -1,7 +1,6 @@ #ifndef BUILTIN_H #define BUILTIN_H -#include "git-compat-util.h" #include "cache.h" #include "commit.h" diff --git a/commit.h b/commit.h index 1fe0731..dddc876 100644 --- a/commit.h +++ b/commit.h @@ -3,7 +3,6 @@ #include "object.h" #include "tree.h" -#include "strbuf.h" #include "decorate.h" #include "gpg-interface.h" #include "string-list.h" diff --git a/dir.h b/dir.h index 727160e..6b1 100644 --- a/dir.h +++ b/dir.h @@ -3,7 +3,6 @@ /* See Documentation/technical/api-directory-listing.txt */ -#include "strbuf.h" #include "pathspec.h" #include "cache.h" diff --git a/khash.h b/khash.h index 89f9579..fc8b1bf 100644 --- a/khash.h +++ b/khash.h @@ -26,7 +26,6 @@ #ifndef __AC_KHASH_H #define __AC_KHASH_H -#include "git-compat-util.h" #include "cache.h" #define AC_VERSION_KHASH_H "0.2.8" -- 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 1/2] strbuf: export strbuf_addchars()
Move strbuf_addchars() to strbuf.c, where it belongs, and make it
available for other callers.
Signed-off-by: Rene Scharfe
---
Documentation/technical/api-strbuf.txt | 4
strbuf.c | 7 +++
strbuf.h | 1 +
utf8.c | 7 ---
4 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/Documentation/technical/api-strbuf.txt
b/Documentation/technical/api-strbuf.txt
index 430302c..cca6543 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -160,6 +160,10 @@ then they will free() it.
Add a single character to the buffer.
+`strbuf_addchars`::
+
+ Add a character the specified number of times to the buffer.
+
`strbuf_insert`::
Insert data to the given position of the buffer. The remaining contents
diff --git a/strbuf.c b/strbuf.c
index 4d31443..0346e74 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -204,6 +204,13 @@ void strbuf_adddup(struct strbuf *sb, size_t pos, size_t
len)
strbuf_setlen(sb, sb->len + len);
}
+void strbuf_addchars(struct strbuf *sb, int c, size_t n)
+{
+ strbuf_grow(sb, n);
+ memset(sb->buf + sb->len, c, n);
+ strbuf_setlen(sb, sb->len + n);
+}
+
void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
{
va_list ap;
diff --git a/strbuf.h b/strbuf.h
index 7bdc1da..652b6c4 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -138,6 +138,7 @@ static inline void strbuf_addbuf(struct strbuf *sb, const
struct strbuf *sb2)
strbuf_add(sb, sb2->buf, sb2->len);
}
extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
+extern void strbuf_addchars(struct strbuf *sb, int c, size_t n);
typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder,
void *context);
extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t
fn, void *context);
diff --git a/utf8.c b/utf8.c
index b30790d..6d4d04a 100644
--- a/utf8.c
+++ b/utf8.c
@@ -239,13 +239,6 @@ int is_utf8(const char *text)
return 1;
}
-static void strbuf_addchars(struct strbuf *sb, int c, size_t n)
-{
- strbuf_grow(sb, n);
- memset(sb->buf + sb->len, c, n);
- strbuf_setlen(sb, sb->len + n);
-}
-
static void strbuf_add_indented_text(struct strbuf *buf, const char *text,
int indent, int indent2)
{
--
2.1.0
--
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 2/2] strbuf: use strbuf_addchars() for adding a char multiple times
Signed-off-by: Rene Scharfe
---
graph.c | 5 ++---
merge-recursive.c | 4 +---
pretty.c | 10 +++---
3 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/graph.c b/graph.c
index 6404331..dfb99f6 100644
--- a/graph.c
+++ b/graph.c
@@ -1145,7 +1145,7 @@ int graph_next_line(struct git_graph *graph, struct
strbuf *sb)
static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
{
- int i, j;
+ int i;
if (graph->state != GRAPH_COMMIT) {
graph_next_line(graph, sb);
@@ -1169,8 +1169,7 @@ static void graph_padding_line(struct git_graph *graph,
struct strbuf *sb)
strbuf_addch(sb, ' ');
else {
int num_spaces = ((graph->num_parents - 2) * 2);
- for (j = 0; j < num_spaces; j++)
- strbuf_addch(sb, ' ');
+ strbuf_addchars(sb, ' ', num_spaces);
}
} else {
strbuf_write_column(sb, col, '|');
diff --git a/merge-recursive.c b/merge-recursive.c
index 1d332b8..dd657e6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -163,9 +163,7 @@ static void output(struct merge_options *o, int v, const
char *fmt, ...)
if (!show(o, v))
return;
- strbuf_grow(&o->obuf, o->call_depth * 2 + 2);
- memset(o->obuf.buf + o->obuf.len, ' ', o->call_depth * 2);
- strbuf_setlen(&o->obuf, o->obuf.len + o->call_depth * 2);
+ strbuf_addchars(&o->obuf, ' ', o->call_depth * 2);
va_start(ap, fmt);
strbuf_vaddf(&o->obuf, fmt, ap);
diff --git a/pretty.c b/pretty.c
index 44b9f64..5971415 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1395,9 +1395,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /*
in UTF-8 */
* convert it back to chars
*/
padding = padding - len + local_sb.len;
- strbuf_grow(sb, padding);
- strbuf_setlen(sb, sb_len + padding);
- memset(sb->buf + sb_len, ' ', sb->len - sb_len);
+ strbuf_addchars(sb, ' ', padding);
memcpy(sb->buf + sb_len + offset, local_sb.buf,
local_sb.len);
}
@@ -1672,10 +1670,8 @@ void pp_remainder(struct pretty_print_context *pp,
first = 0;
strbuf_grow(sb, linelen + indent + 20);
- if (indent) {
- memset(sb->buf + sb->len, ' ', indent);
- strbuf_setlen(sb, sb->len + indent);
- }
+ if (indent)
+ strbuf_addchars(sb, ' ', indent);
strbuf_add(sb, line, linelen);
strbuf_addch(sb, '\n');
}
--
2.1.0
--
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 v3 2/2] headers: include dependent headers
Am 07.09.2014 um 11:36 schrieb David Aguilar:
Add dependent headers so that including a header does not
require including additional headers.
This makes it so that "gcc -c $header" succeeds for each header.
diff --git a/cache.h b/cache.h
index 4d5b76c..8b827d7 100644
--- a/cache.h
+++ b/cache.h
@@ -1,7 +1,6 @@
#ifndef CACHE_H
#define CACHE_H
-#include "git-compat-util.h"
#include "strbuf.h"
#include "hashmap.h"
#include "advice.h"
Oh, that's a new change and worth mentioning in the commit message.
diff --git a/color.h b/color.h
index 9a8495b..6b50a0f 100644
--- a/color.h
+++ b/color.h
@@ -1,7 +1,8 @@
#ifndef COLOR_H
#define COLOR_H
-struct strbuf;
+#include "git-compat-util.h"
+#include "strbuf.h"
/* 2 + (2 * num_attrs) + 8 + 1 + 8 + 'm' + NUL */
/* "\033[1;2;4;5;7;38;5;2xx;48;5;2xxm\0" */
I didn't notice this one the first time around. Isn't the forward
declaration of struct strbuf enough?
diff --git a/diff.h b/diff.h
index b4a624d..27f7696 100644
--- a/diff.h
+++ b/diff.h
@@ -6,11 +6,11 @@
#include "tree-walk.h"
#include "pathspec.h"
+#include "strbuf.h"
struct rev_info;
struct diff_options;
struct diff_queue_struct;
-struct strbuf;
struct diff_filespec;
struct userdiff_driver;
struct sha1_array;
Same here.
diff --git a/quote.h b/quote.h
index 71dcc3a..37f857b 100644
--- a/quote.h
+++ b/quote.h
@@ -1,7 +1,8 @@
#ifndef QUOTE_H
#define QUOTE_H
-struct strbuf;
+#include "git-compat-util.h"
+#include "strbuf.h"
/* Help to copy the thing properly quoted for the shell safety.
* any single quote is replaced with '\'', any exclamation point
And here.
> diff --git a/submodule.h b/submodule.h
> index 7beec48..52bb673 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -1,8 +1,10 @@
> #ifndef SUBMODULE_H
> #define SUBMODULE_H
>
> -struct diff_options;
> -struct argv_array;
> +#include "git-compat-util.h"
> +#include "diff.h"
> +#include "argv-array.h"
> +#include "string-list.h"
>
> enum {
>RECURSE_SUBMODULES_ON_DEMAND = -1,
Similarly here with structs diff_options and argv_array.
diff --git a/utf8.c b/utf8.c
index b30790d..fb9f299 100644
--- a/utf8.c
+++ b/utf8.c
@@ -2,13 +2,6 @@
#include "strbuf.h"
#include "utf8.h"
-/* This code is originally from http://www.cl.cam.ac.uk/~mgk25/ucs/ */
-
-struct interval {
- ucs_char_t first;
- ucs_char_t last;
-};
-
size_t display_mode_esc_sequence_len(const char *s)
{
const char *p = s;
diff --git a/utf8.h b/utf8.h
index 65d0e42..af855c5 100644
--- a/utf8.h
+++ b/utf8.h
@@ -1,8 +1,17 @@
#ifndef GIT_UTF8_H
#define GIT_UTF8_H
+#include "strbuf.h"
+
typedef unsigned int ucs_char_t; /* assuming 32bit int */
+/* This code is originally from http://www.cl.cam.ac.uk/~mgk25/ucs/ */
+
+struct interval {
+ ucs_char_t first;
+ ucs_char_t last;
+};
+
size_t display_mode_esc_sequence_len(const char *s);
int utf8_width(const char **start, size_t *remainder_p);
int utf8_strnwidth(const char *string, int len, int skip_ansi);
The move of struct interval was mentioned in the comment section of
the first patch. Perhaps include a note in the commit message?
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index c8b5adb..7fd5364 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -1,8 +1,9 @@
#ifndef FAST_EXPORT_H_
#define FAST_EXPORT_H_
-struct strbuf;
-struct line_buffer;
+#include "git-compat-util.h"
+#include "strbuf.h"
+#include "vcs-svn/line_buffer.h"
void fast_export_init(int fd);
void fast_export_deinit(void);
struct strbuf forward declaration vs. including strbuf.h again.
diff --git a/vcs-svn/repo_tree.h b/vcs-svn/repo_tree.h
index 889c6a3..3a946f7 100644
--- a/vcs-svn/repo_tree.h
+++ b/vcs-svn/repo_tree.h
@@ -1,7 +1,8 @@
#ifndef REPO_TREE_H_
#define REPO_TREE_H_
-struct strbuf;
+#include "git-compat-util.h"
+#include "strbuf.h"
#define REPO_MODE_DIR 004
#define REPO_MODE_BLB 0100644
And again.
diff --git a/vcs-svn/svndiff.h b/vcs-svn/svndiff.h
index 74eb464..d0cbd51 100644
--- a/vcs-svn/svndiff.h
+++ b/vcs-svn/svndiff.h
@@ -1,8 +1,9 @@
#ifndef SVNDIFF_H_
#define SVNDIFF_H_
-struct line_buffer;
-struct sliding_view;
+#include "git-compat-util.h"
+#include "vcs-svn/line_buffer.h"
+#include "vcs-svn/sliding_window.h"
extern int svndiff0_apply(struct line_buffer *delta, off_t delta_len,
struct sliding_view *preimage, FILE *postimage);
Similar issue here with different structs.
--
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: [RFH] renaming strcmp/strncmp-icase
Am 08.09.2014 um 20:52 schrieb Junio C Hamano: There are these two functions in dir.c that has only a handful of callers outside: int strcmp_icase(const char *a, const char *b); int strncmp_icase(const char *a, const char *b, size_t count); How many of you would think these are about comparing two strings in a case-insensitive way? If you raised your hand (like I did), you were wrong. These do comparison case-insensitively only on a case-insensitive filesystem, and hence calling it makes sense only for pathnames you grabbed out of the filesystem via readdir() (or the user gave us, intending to name paths). To avoid confusion, I think they should be renamed to stress the fact that these are about comparing *PATHS*. As I always say, I am bad at naming things and good suggestions are appreciated. pathnamecmp()/pathnamencmp() perhaps? "namen" looks strange to me. How about pathcmp/pathncmp? René -- 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 1/2] strbuf: export strbuf_addchars()
Am 08.09.2014 um 20:32 schrieb Junio C Hamano: René Scharfe writes: Move strbuf_addchars() to strbuf.c, where it belongs, and make it available for other callers. Signed-off-by: Rene Scharfe Wow, fixing up v1.7.0.2~9^2~2? About time, isn't it? ;) Both patches look correct, but I have to wonder where you are drawing these clean-up opportunities from? Almost as if reading through dormant part of the codebase one of your hobbies or something ;-) That, and I'm sitting on a pile of cleanup patches that grew whenever I looked at some piece of code for the first time, e.g. due to bug reports, or when I took a static analyzer for a test drive etc. I'm trying to filter out the good ones and to send them at opportune moments in order to avoid conflicts with real changes. René -- 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: [RFC PATCH 2/2] headers: include dependent headers
Am 08.09.2014 um 19:50 schrieb Junio C Hamano: René Scharfe writes: Am 06.09.2014 um 21:20 schrieb David Aguilar: Add dependent headers so that including a header does not require including additional headers. This makes it so that "gcc -c $header" succeeds for each header. Signed-off-by: David Aguilar --- diff --git a/branch.h b/branch.h index 64173ab..a61fd1a 100644 --- a/branch.h +++ b/branch.h @@ -3,6 +3,9 @@ /* Functions for acting on the information about branches. */ +#include "cache.h" +#include "strbuf.h" cache.h includes strbuf.h, so the line above isn't necessary. True, but is "gcc -c $header" something we want to please in the first place (not an objection, but request for opinions)? It *sounds* useful, but we did without that feature so far. Hmm. It would make it easier to use headers -- any dependency to other files are already taken care of. Since we have less .h files than .c files this seems to make sense. Would it perhaps also make using precompiled headers easier (or possible in the first place)? Do we care? René -- 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 0/7] unpack-trees: plug memory leak for merges
This series adds const to cache_entry pointers in a lot of places, in order to show that we can free them in unpack_nondirectories, which the last patch finally does. First three easy patches for adding const and splitting a function in two: cache: mark cache_entry pointers const read-cache: mark cache_entry pointers const unpack-trees: factor out dup_entry Then a patch that reduces the side effects of merged_entry: unpack-trees: create working copy of merge entry in merged_entry Another easy const patch: diff-lib, read-tree, unpack-trees: mark cache_entry pointers const And patch that introduces "const struct cache_entry * const *", which may look a bit scary at first: diff-lib, read-tree, unpack-trees: mark cache_entry array paramters const Final patch that plugs a memory leak in unpack_nondirectories. unpack-trees: free cache_entry array members for merges It's basically the same one that Stephen tested a while ago (http://thread.gmane.org/gmane.comp.version-control.git/222887). It's not the only leak that affects cherry-pick; expect more (independent) patches. builtin/read-tree.c | 5 +- cache.h | 10 ++-- diff-lib.c | 26 +- read-cache.c| 18 --- unpack-trees.c | 146 unpack-trees.h | 14 +++-- 6 files changed, 131 insertions(+), 88 deletions 1.8.3 -- 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 1/7] cache: mark cache_entry pointers const
Add const for pointers that are only dereferenced for reading by the
inline functions copy_cache_entry and ce_mode_from_stat. This allows
callers to pass in const pointers.
Signed-off-by: René Scharfe
---
cache.h | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 94ca1ac..43a27e7 100644
--- a/cache.h
+++ b/cache.h
@@ -190,7 +190,8 @@ struct cache_entry {
* another. But we never change the name, or the hash state!
*/
#define CE_STATE_MASK (CE_HASHED | CE_UNHASHED)
-static inline void copy_cache_entry(struct cache_entry *dst, struct
cache_entry *src)
+static inline void copy_cache_entry(struct cache_entry *dst,
+ const struct cache_entry *src)
{
unsigned int state = dst->ce_flags & CE_STATE_MASK;
@@ -222,7 +223,8 @@ static inline unsigned int create_ce_mode(unsigned int mode)
return S_IFGITLINK;
return S_IFREG | ce_permissions(mode);
}
-static inline unsigned int ce_mode_from_stat(struct cache_entry *ce, unsigned
int mode)
+static inline unsigned int ce_mode_from_stat(const struct cache_entry *ce,
+unsigned int mode)
{
extern int trust_executable_bit, has_symlinks;
if (!has_symlinks && S_ISREG(mode) &&
--
1.8.3
--
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 2/7] read-cache: mark cache_entry pointers const
ie_match_stat and ie_modified only derefence their struct cache_entry
pointers for reading. Add const to the parameter declaration here and
do the same for the static helper function used by them, as it's the
same there as well. This allows callers to pass in const pointers.
Signed-off-by: René Scharfe
---
cache.h | 4 ++--
read-cache.c | 18 ++
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index 43a27e7..01e8760 100644
--- a/cache.h
+++ b/cache.h
@@ -482,8 +482,8 @@ extern void *read_blob_data_from_index(struct index_state
*, const char *, unsig
#define CE_MATCH_RACY_IS_DIRTY 02
/* do stat comparison even if CE_SKIP_WORKTREE is true */
#define CE_MATCH_IGNORE_SKIP_WORKTREE 04
-extern int ie_match_stat(const struct index_state *, struct cache_entry *,
struct stat *, unsigned int);
-extern int ie_modified(const struct index_state *, struct cache_entry *,
struct stat *, unsigned int);
+extern int ie_match_stat(const struct index_state *, const struct cache_entry
*, struct stat *, unsigned int);
+extern int ie_modified(const struct index_state *, const struct cache_entry *,
struct stat *, unsigned int);
#define PATHSPEC_ONESTAR 1 /* the pathspec pattern sastisfies GFNM_ONESTAR
*/
diff --git a/read-cache.c b/read-cache.c
index 04ed561..e6e0466 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -91,7 +91,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat
*st)
ce_mark_uptodate(ce);
}
-static int ce_compare_data(struct cache_entry *ce, struct stat *st)
+static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
{
int match = -1;
int fd = open(ce->name, O_RDONLY);
@@ -105,7 +105,7 @@ static int ce_compare_data(struct cache_entry *ce, struct
stat *st)
return match;
}
-static int ce_compare_link(struct cache_entry *ce, size_t expected_size)
+static int ce_compare_link(const struct cache_entry *ce, size_t expected_size)
{
int match = -1;
void *buffer;
@@ -126,7 +126,7 @@ static int ce_compare_link(struct cache_entry *ce, size_t
expected_size)
return match;
}
-static int ce_compare_gitlink(struct cache_entry *ce)
+static int ce_compare_gitlink(const struct cache_entry *ce)
{
unsigned char sha1[20];
@@ -143,7 +143,7 @@ static int ce_compare_gitlink(struct cache_entry *ce)
return hashcmp(sha1, ce->sha1);
}
-static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)
+static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st)
{
switch (st->st_mode & S_IFMT) {
case S_IFREG:
@@ -163,7 +163,7 @@ static int ce_modified_check_fs(struct cache_entry *ce,
struct stat *st)
return 0;
}
-static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
+static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)
{
unsigned int changed = 0;
@@ -239,7 +239,8 @@ static int ce_match_stat_basic(struct cache_entry *ce,
struct stat *st)
return changed;
}
-static int is_racy_timestamp(const struct index_state *istate, struct
cache_entry *ce)
+static int is_racy_timestamp(const struct index_state *istate,
+const struct cache_entry *ce)
{
return (!S_ISGITLINK(ce->ce_mode) &&
istate->timestamp.sec &&
@@ -255,7 +256,7 @@ static int is_racy_timestamp(const struct index_state
*istate, struct cache_entr
}
int ie_match_stat(const struct index_state *istate,
- struct cache_entry *ce, struct stat *st,
+ const struct cache_entry *ce, struct stat *st,
unsigned int options)
{
unsigned int changed;
@@ -311,7 +312,8 @@ int ie_match_stat(const struct index_state *istate,
}
int ie_modified(const struct index_state *istate,
- struct cache_entry *ce, struct stat *st, unsigned int options)
+ const struct cache_entry *ce,
+ struct stat *st, unsigned int options)
{
int changed, changed_fs;
--
1.8.3
--
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 7/7] unpack-trees: free cache_entry array members for merges
The merge functions duplicate entries as needed and they don't free
them. Release them in unpack_nondirectories, the same function
where they were allocated, after we're done.
Signed-off-by: René Scharfe
---
unpack-trees.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 2dbc05d..fc0dd5a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -600,9 +600,14 @@ static int unpack_nondirectories(int n, unsigned long mask,
src[i + o->merge] = create_ce_entry(info, names + i, stage);
}
- if (o->merge)
- return call_unpack_fn((const struct cache_entry * const *)src,
- o);
+ if (o->merge) {
+ int rc = call_unpack_fn((const struct cache_entry * const *)src,
+ o);
+ for (i = 1; i <= n; i++)
+ if (src[i] && src[i] != o->df_conflict_entry)
+ free(src[i]);
+ return rc;
+ }
for (i = 0; i < n; i++)
if (src[i] && src[i] != o->df_conflict_entry)
--
1.8.3
--
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 3/7] unpack-trees: factor out dup_entry
While we're add it, mark the struct cache_entry pointer of add_entry
const because we only read from it and this allows callers to pass in
const pointers.
Signed-off-by: René Scharfe
---
unpack-trees.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index ede4299..e8b4cc1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -116,14 +116,20 @@ static void do_add_entry(struct unpack_trees_options *o,
struct cache_entry *ce,
ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
}
-static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
- unsigned int set, unsigned int clear)
+static struct cache_entry *dup_entry(const struct cache_entry *ce)
{
unsigned int size = ce_size(ce);
struct cache_entry *new = xmalloc(size);
memcpy(new, ce, size);
- do_add_entry(o, new, set, clear);
+ return new;
+}
+
+static void add_entry(struct unpack_trees_options *o,
+ const struct cache_entry *ce,
+ unsigned int set, unsigned int clear)
+{
+ do_add_entry(o, dup_entry(ce), set, clear);
}
/*
--
1.8.3
--
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 4/7] unpack-trees: create working copy of merge entry in merged_entry
Duplicate the merge entry right away and work with that instead of
modifying the entry we got and duplicating it only at the end of
the function. Then mark that pointer const to document that we
don't modify the referenced cache_entry.
This change is safe because all existing merge functions call
merged_entry just before returning (or not at all), i.e. they don't
care about changes to the referenced cache_entry after the call.
unpack_nondirectories and unpack_index_entry, which call the merge
functions through call_unpack_fn, aren't interested in such changes
neither.
The change complicates merged_entry a bit because we have to free the
copy if we error out, but allows callers to pass a const pointer.
Signed-off-by: René Scharfe
---
unpack-trees.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index e8b4cc1..2fecef8 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1466,10 +1466,12 @@ static int verify_absent_sparse(struct cache_entry *ce,
return verify_absent_1(ce, orphaned_error, o);
}
-static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
- struct unpack_trees_options *o)
+static int merged_entry(const struct cache_entry *ce,
+ struct cache_entry *old,
+ struct unpack_trees_options *o)
{
int update = CE_UPDATE;
+ struct cache_entry *merge = dup_entry(ce);
if (!old) {
/*
@@ -1487,8 +1489,11 @@ static int merged_entry(struct cache_entry *merge,
struct cache_entry *old,
update |= CE_ADDED;
merge->ce_flags |= CE_NEW_SKIP_WORKTREE;
- if (verify_absent(merge,
ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o))
+ if (verify_absent(merge,
+ ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) {
+ free(merge);
return -1;
+ }
invalidate_ce_path(merge, o);
} else if (!(old->ce_flags & CE_CONFLICTED)) {
/*
@@ -1502,8 +1507,10 @@ static int merged_entry(struct cache_entry *merge,
struct cache_entry *old,
copy_cache_entry(merge, old);
update = 0;
} else {
- if (verify_uptodate(old, o))
+ if (verify_uptodate(old, o)) {
+ free(merge);
return -1;
+ }
/* Migrate old flags over */
update |= old->ce_flags & (CE_SKIP_WORKTREE |
CE_NEW_SKIP_WORKTREE);
invalidate_ce_path(old, o);
@@ -1516,7 +1523,7 @@ static int merged_entry(struct cache_entry *merge, struct
cache_entry *old,
invalidate_ce_path(old, o);
}
- add_entry(o, merge, update, CE_STAGEMASK);
+ do_add_entry(o, merge, update, CE_STAGEMASK);
return 1;
}
--
1.8.3
--
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 6/7] diff-lib, read-tree, unpack-trees: mark cache_entry array paramters const
Change the type merge_fn_t to accept the array of cache_entry pointers
as const pointers to const pointers. This documents the fact that the
merge functions don't modify the cache_entry contents or replace any of
the pointers in the array.
Only a single cast is necessary in unpack_nondirectories because adding
two const modifiers at once is not allowed in C. The cast is safe in
that it doesn't mask any modfication; call_unpack_fn only needs the
array for reading.
Signed-off-by: René Scharfe
---
builtin/read-tree.c | 3 ++-
diff-lib.c | 3 ++-
unpack-trees.c | 21 +
unpack-trees.h | 14 +-
4 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index b847486..0f5d7fe 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -80,7 +80,8 @@ static void debug_stage(const char *label, const struct
cache_entry *ce,
sha1_to_hex(ce->sha1));
}
-static int debug_merge(struct cache_entry **stages, struct
unpack_trees_options *o)
+static int debug_merge(const struct cache_entry * const *stages,
+ struct unpack_trees_options *o)
{
int i;
diff --git a/diff-lib.c b/diff-lib.c
index 83d0cb8..b6f4b21 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -424,7 +424,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
* the fairly complex unpack_trees() semantic requirements, including
* the skipping, the path matching, the type conflict cases etc.
*/
-static int oneway_diff(struct cache_entry **src, struct unpack_trees_options
*o)
+static int oneway_diff(const struct cache_entry * const *src,
+ struct unpack_trees_options *o)
{
const struct cache_entry *idx = src[0];
const struct cache_entry *tree = src[1];
diff --git a/unpack-trees.c b/unpack-trees.c
index c5a40df..2dbc05d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -300,7 +300,8 @@ static int apply_sparse_checkout(struct cache_entry *ce,
struct unpack_trees_opt
return 0;
}
-static inline int call_unpack_fn(struct cache_entry **src, struct
unpack_trees_options *o)
+static inline int call_unpack_fn(const struct cache_entry * const *src,
+struct unpack_trees_options *o)
{
int ret = o->fn(src, o);
if (ret > 0)
@@ -397,7 +398,7 @@ static void add_same_unmerged(struct cache_entry *ce,
static int unpack_index_entry(struct cache_entry *ce,
struct unpack_trees_options *o)
{
- struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
+ const struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
int ret;
src[0] = ce;
@@ -600,7 +601,8 @@ static int unpack_nondirectories(int n, unsigned long mask,
}
if (o->merge)
- return call_unpack_fn(src, o);
+ return call_unpack_fn((const struct cache_entry * const *)src,
+ o);
for (i = 0; i < n; i++)
if (src[i] && src[i] != o->df_conflict_entry)
@@ -1574,7 +1576,8 @@ static void show_stage_entry(FILE *o,
}
#endif
-int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
+int threeway_merge(const struct cache_entry * const *stages,
+ struct unpack_trees_options *o)
{
const struct cache_entry *index;
const struct cache_entry *head;
@@ -1746,7 +1749,8 @@ int threeway_merge(struct cache_entry **stages, struct
unpack_trees_options *o)
* "carry forward" rule, please see .
*
*/
-int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
+int twoway_merge(const struct cache_entry * const *src,
+struct unpack_trees_options *o)
{
const struct cache_entry *current = src[0];
const struct cache_entry *oldtree = src[1];
@@ -1812,8 +1816,8 @@ int twoway_merge(struct cache_entry **src, struct
unpack_trees_options *o)
* Keep the index entries at stage0, collapse stage1 but make sure
* stage0 does not have anything there.
*/
-int bind_merge(struct cache_entry **src,
- struct unpack_trees_options *o)
+int bind_merge(const struct cache_entry * const *src,
+ struct unpack_trees_options *o)
{
const struct cache_entry *old = src[0];
const struct cache_entry *a = src[1];
@@ -1836,7 +1840,8 @@ int bind_merge(struct cache_entry **src,
* The rule is:
* - take the stat information from stage0, take the data from stage1
*/
-int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)
+int oneway_merge(const struct cache_entry * const *src,
+struct unpack_trees_options *o)
{
const struct cache_entry *old = src[0];
const struct cache_entry *a = src[1];
diff --git a/unpack-trees.h b/unpack-trees.h
index 5e432f5..36a73a6 100644
--- a/unpack-trees.
[PATCH 5/7] diff-lib, read-tree, unpack-trees: mark cache_entry pointers const
Add const to struct cache_entry pointers throughout the tree which are
only used for reading. This allows callers to pass in const pointers.
Signed-off-by: René Scharfe
---
builtin/read-tree.c | 2 +-
diff-lib.c | 23 +++---
unpack-trees.c | 91 +
3 files changed, 63 insertions(+), 53 deletions(-)
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 042ac1b..b847486 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -66,7 +66,7 @@ static int exclude_per_directory_cb(const struct option *opt,
const char *arg,
return 0;
}
-static void debug_stage(const char *label, struct cache_entry *ce,
+static void debug_stage(const char *label, const struct cache_entry *ce,
struct unpack_trees_options *o)
{
printf("%s ", label);
diff --git a/diff-lib.c b/diff-lib.c
index f35de0f..83d0cb8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -64,8 +64,9 @@ static int check_removed(const struct cache_entry *ce, struct
stat *st)
* commits, untracked content and/or modified content).
*/
static int match_stat_with_submodule(struct diff_options *diffopt,
- struct cache_entry *ce, struct stat *st,
- unsigned ce_option, unsigned
*dirty_submodule)
+const struct cache_entry *ce,
+struct stat *st, unsigned ce_option,
+unsigned *dirty_submodule)
{
int changed = ce_match_stat(ce, st, ce_option);
if (S_ISGITLINK(ce->ce_mode)) {
@@ -237,7 +238,7 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
/* A file entry went away or appeared */
static void diff_index_show_file(struct rev_info *revs,
const char *prefix,
-struct cache_entry *ce,
+const struct cache_entry *ce,
const unsigned char *sha1, int sha1_valid,
unsigned int mode,
unsigned dirty_submodule)
@@ -246,7 +247,7 @@ static void diff_index_show_file(struct rev_info *revs,
sha1, sha1_valid, ce->name, dirty_submodule);
}
-static int get_stat_data(struct cache_entry *ce,
+static int get_stat_data(const struct cache_entry *ce,
const unsigned char **sha1p,
unsigned int *modep,
int cached, int match_missing,
@@ -283,7 +284,7 @@ static int get_stat_data(struct cache_entry *ce,
}
static void show_new_file(struct rev_info *revs,
- struct cache_entry *new,
+ const struct cache_entry *new,
int cached, int match_missing)
{
const unsigned char *sha1;
@@ -302,8 +303,8 @@ static void show_new_file(struct rev_info *revs,
}
static int show_modified(struct rev_info *revs,
-struct cache_entry *old,
-struct cache_entry *new,
+const struct cache_entry *old,
+const struct cache_entry *new,
int report_missing,
int cached, int match_missing)
{
@@ -362,8 +363,8 @@ static int show_modified(struct rev_info *revs,
* give you the position and number of entries in the index).
*/
static void do_oneway_diff(struct unpack_trees_options *o,
- struct cache_entry *idx,
- struct cache_entry *tree)
+ const struct cache_entry *idx,
+ const struct cache_entry *tree)
{
struct rev_info *revs = o->unpack_data;
int match_missing, cached;
@@ -425,8 +426,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
*/
static int oneway_diff(struct cache_entry **src, struct unpack_trees_options
*o)
{
- struct cache_entry *idx = src[0];
- struct cache_entry *tree = src[1];
+ const struct cache_entry *idx = src[0];
+ const struct cache_entry *tree = src[1];
struct rev_info *revs = o->unpack_data;
/*
diff --git a/unpack-trees.c b/unpack-trees.c
index 2fecef8..c5a40df 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -241,8 +241,11 @@ static int check_updates(struct unpack_trees_options *o)
return errs != 0;
}
-static int verify_uptodate_sparse(struct cache_entry *ce, struct
unpack_trees_options *o);
-static int verify_absent_sparse(struct cache_entry *ce, enum
unpack_trees_error_types, struct unpack_trees_options *o);
+static int verify_uptodate_sparse(const struct cache_entry *ce,
+ struct unpack_trees_options *o);
+static int verify_absent_sparse(const struct cache_entry *ce,
+ enum unpack_trees_error_types,
+
Re: [PATCH 7/7] unpack-trees: free cache_entry array members for merges
Am 30.05.2013 14:04, schrieb Felipe Contreras:
On Thu, May 30, 2013 at 6:34 AM, René Scharfe
wrote:
The merge functions duplicate entries as needed and they don't free
them. Release them in unpack_nondirectories, the same function
where they were allocated, after we're done.
Ah, you beat me to this change, but..
@@ -600,9 +600,14 @@ static int unpack_nondirectories(int n, unsigned long mask,
src[i + o->merge] = create_ce_entry(info, names + i, stage);
}
- if (o->merge)
- return call_unpack_fn((const struct cache_entry * const *)src,
- o);
+ if (o->merge) {
+ int rc = call_unpack_fn((const struct cache_entry * const *)src,
+ o);
+ for (i = 1; i <= n; i++)
+ if (src[i] && src[i] != o->df_conflict_entry)
+ free(src[i]);
Doesn't it make more sense to follow the code above and do src[i + o->merge]?
Not sure I understand. Is the goal to avoid confusion for code readers
by using the same indexing method for allocation and release? Or are
you worried about o->merge having a different value than 1 in that loop?
We'd have to add 1 (== o->merge) to each index variable usage with a
zero-based loop. A one-based loop avoids that, and while it's not
pretty it's also not too complicated, I think.
René
--
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 3/3] unpack-trees: free created cache entries
Am 30.05.2013 15:34, schrieb Felipe Contreras:
We created them, and nobody else is going to destroy them.
Signed-off-by: Felipe Contreras
---
unpack-trees.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index eff2944..9f19d01 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -590,8 +590,16 @@ static int unpack_nondirectories(int n, unsigned long mask,
src[i + o->merge] = create_ce_entry(info, names + i, stage);
}
- if (o->merge)
- return call_unpack_fn(src, o);
+ if (o->merge) {
+ int ret = call_unpack_fn(src, o);
+ for (i = 0; i < n; i++) {
+ struct cache_entry *ce = src[i + o->merge];
+ if (!ce || ce == o->df_conflict_entry)
+ continue;
+ free(ce);
+ }
+ return ret;
+ }
Ah, now I understand what you meant in that other email. That works as
well, of course. It's slightly nicer on the eye, admittedly.
René
--
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 1/3] read-cache: plug a few leaks
Am 30.05.2013 15:34, schrieb Felipe Contreras: > We don't free 'istate->cache' properly. > > Apparently 'initialized' doesn't really mean initialized, but loaded, or > rather 'not-empty', and the cache can be used even if it's not > 'initialized', so we can't rely on this variable to keep track of the > 'istate->cache'. > > So assume it always has data, and free it before overwriting it. > > Signed-off-by: Felipe Contreras > --- > read-cache.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/read-cache.c b/read-cache.c > index 04ed561..e5dc96f 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -1449,6 +1449,7 @@ int read_index_from(struct index_state *istate, const > char *path) > istate->version = ntohl(hdr->hdr_version); > istate->cache_nr = ntohl(hdr->hdr_entries); > istate->cache_alloc = alloc_nr(istate->cache_nr); > + free(istate->cache); With that change, callers of read_index_from need to set ->cache to NULL for uninitialized (on-stack) index_state variables. They only had to set ->initialized to 0 before in that situation. It this chunk safe for all existing callers? Shouldn't the same free in discard_index (added below) be enough? > istate->cache = xcalloc(istate->cache_alloc, sizeof(struct cache_entry > *)); > istate->initialized = 1; > > @@ -1510,6 +1511,9 @@ int discard_index(struct index_state *istate) > > for (i = 0; i < istate->cache_nr; i++) > free(istate->cache[i]); > + free(istate->cache); > + istate->cache = NULL; > + istate->cache_alloc = 0; > resolve_undo_clear_index(istate); > istate->cache_nr = 0; > istate->cache_changed = 0; I was preparing a similar change, looks good. There is a comment at the end of discard_index() that becomes wrong due to that patch, though -- better remove it as well. It was already outdated as it mentioned active_cache, while the function can be used with any index_state. diff --git a/read-cache.c b/read-cache.c index e5dc96f..0f868af 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1522,8 +1522,6 @@ int discard_index(struct index_state *istate) free_name_hash(istate); cache_tree_free(&(istate->cache_tree)); istate->initialized = 0; - - /* no need to throw away allocated active_cache */ return 0; } -- 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 6/7] diff-lib, read-tree, unpack-trees: mark cache_entry array paramters const
Change the type merge_fn_t to accept the array of cache_entry pointers
as const pointers to const pointers. This documents the fact that the
merge functions don't modify the cache_entry contents or replace any of
the pointers in the array.
Only a single cast is necessary in unpack_nondirectories because adding
two const modifiers at once is not allowed in C. The cast is safe in
that it doesn't mask any modfication; call_unpack_fn only needs the
array for reading.
Signed-off-by: René Scharfe
---
builtin/read-tree.c | 3 ++-
diff-lib.c | 3 ++-
unpack-trees.c | 21 +
unpack-trees.h | 14 +-
4 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index b847486..0f5d7fe 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -80,7 +80,8 @@ static void debug_stage(const char *label, const struct
cache_entry *ce,
sha1_to_hex(ce->sha1));
}
-static int debug_merge(struct cache_entry **stages, struct
unpack_trees_options *o)
+static int debug_merge(const struct cache_entry * const *stages,
+ struct unpack_trees_options *o)
{
int i;
diff --git a/diff-lib.c b/diff-lib.c
index 83d0cb8..b6f4b21 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -424,7 +424,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
* the fairly complex unpack_trees() semantic requirements, including
* the skipping, the path matching, the type conflict cases etc.
*/
-static int oneway_diff(struct cache_entry **src, struct unpack_trees_options
*o)
+static int oneway_diff(const struct cache_entry * const *src,
+ struct unpack_trees_options *o)
{
const struct cache_entry *idx = src[0];
const struct cache_entry *tree = src[1];
diff --git a/unpack-trees.c b/unpack-trees.c
index c5a40df..2dbc05d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -300,7 +300,8 @@ static int apply_sparse_checkout(struct cache_entry *ce,
struct unpack_trees_opt
return 0;
}
-static inline int call_unpack_fn(struct cache_entry **src, struct
unpack_trees_options *o)
+static inline int call_unpack_fn(const struct cache_entry * const *src,
+struct unpack_trees_options *o)
{
int ret = o->fn(src, o);
if (ret > 0)
@@ -397,7 +398,7 @@ static void add_same_unmerged(struct cache_entry *ce,
static int unpack_index_entry(struct cache_entry *ce,
struct unpack_trees_options *o)
{
- struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
+ const struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
int ret;
src[0] = ce;
@@ -600,7 +601,8 @@ static int unpack_nondirectories(int n, unsigned long mask,
}
if (o->merge)
- return call_unpack_fn(src, o);
+ return call_unpack_fn((const struct cache_entry * const *)src,
+ o);
for (i = 0; i < n; i++)
if (src[i] && src[i] != o->df_conflict_entry)
@@ -1574,7 +1576,8 @@ static void show_stage_entry(FILE *o,
}
#endif
-int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
+int threeway_merge(const struct cache_entry * const *stages,
+ struct unpack_trees_options *o)
{
const struct cache_entry *index;
const struct cache_entry *head;
@@ -1746,7 +1749,8 @@ int threeway_merge(struct cache_entry **stages, struct
unpack_trees_options *o)
* "carry forward" rule, please see .
*
*/
-int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
+int twoway_merge(const struct cache_entry * const *src,
+struct unpack_trees_options *o)
{
const struct cache_entry *current = src[0];
const struct cache_entry *oldtree = src[1];
@@ -1812,8 +1816,8 @@ int twoway_merge(struct cache_entry **src, struct
unpack_trees_options *o)
* Keep the index entries at stage0, collapse stage1 but make sure
* stage0 does not have anything there.
*/
-int bind_merge(struct cache_entry **src,
- struct unpack_trees_options *o)
+int bind_merge(const struct cache_entry * const *src,
+ struct unpack_trees_options *o)
{
const struct cache_entry *old = src[0];
const struct cache_entry *a = src[1];
@@ -1836,7 +1840,8 @@ int bind_merge(struct cache_entry **src,
* The rule is:
* - take the stat information from stage0, take the data from stage1
*/
-int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)
+int oneway_merge(const struct cache_entry * const *src,
+struct unpack_trees_options *o)
{
const struct cache_entry *old = src[0];
const struct cache_entry *a = src[1];
diff --git a/unpack-trees.h b/unpack-trees.h
index 5e432f5..36a73a6 100644
--- a/unpack-trees.
[PATCH v2 4/7] unpack-trees: create working copy of merge entry in merged_entry
Duplicate the merge entry right away and work with that instead of
modifying the entry we got and duplicating it only at the end of
the function. Then mark that pointer const to document that we
don't modify the referenced cache_entry.
This change is safe because all existing merge functions call
merged_entry just before returning (or not at all), i.e. they don't
care about changes to the referenced cache_entry after the call.
unpack_nondirectories and unpack_index_entry, which call the merge
functions through call_unpack_fn, aren't interested in such changes
neither.
The change complicates merged_entry a bit because we have to free the
copy if we error out, but allows callers to pass a const pointer.
Signed-off-by: René Scharfe
---
unpack-trees.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index e8b4cc1..2fecef8 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1466,10 +1466,12 @@ static int verify_absent_sparse(struct cache_entry *ce,
return verify_absent_1(ce, orphaned_error, o);
}
-static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
- struct unpack_trees_options *o)
+static int merged_entry(const struct cache_entry *ce,
+ struct cache_entry *old,
+ struct unpack_trees_options *o)
{
int update = CE_UPDATE;
+ struct cache_entry *merge = dup_entry(ce);
if (!old) {
/*
@@ -1487,8 +1489,11 @@ static int merged_entry(struct cache_entry *merge,
struct cache_entry *old,
update |= CE_ADDED;
merge->ce_flags |= CE_NEW_SKIP_WORKTREE;
- if (verify_absent(merge,
ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o))
+ if (verify_absent(merge,
+ ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) {
+ free(merge);
return -1;
+ }
invalidate_ce_path(merge, o);
} else if (!(old->ce_flags & CE_CONFLICTED)) {
/*
@@ -1502,8 +1507,10 @@ static int merged_entry(struct cache_entry *merge,
struct cache_entry *old,
copy_cache_entry(merge, old);
update = 0;
} else {
- if (verify_uptodate(old, o))
+ if (verify_uptodate(old, o)) {
+ free(merge);
return -1;
+ }
/* Migrate old flags over */
update |= old->ce_flags & (CE_SKIP_WORKTREE |
CE_NEW_SKIP_WORKTREE);
invalidate_ce_path(old, o);
@@ -1516,7 +1523,7 @@ static int merged_entry(struct cache_entry *merge, struct
cache_entry *old,
invalidate_ce_path(old, o);
}
- add_entry(o, merge, update, CE_STAGEMASK);
+ do_add_entry(o, merge, update, CE_STAGEMASK);
return 1;
}
--
1.8.3
--
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 0/7] unpack-trees: plug memory leak for merges
Changes in v2: Only changed the loop style in patch 7, as suggested by Felipe. This series adds const to cache_entry pointers in a lot of places, in order to show that we can free them in unpack_nondirectories, which the last patch finally does. First three easy patches for adding const and splitting a function in two: cache: mark cache_entry pointers const read-cache: mark cache_entry pointers const unpack-trees: factor out dup_entry Then a patch that reduces the side effects of merged_entry: unpack-trees: create working copy of merge entry in merged_entry Another easy const patch: diff-lib, read-tree, unpack-trees: mark cache_entry pointers const And patch that introduces "const struct cache_entry * const *", which may look a bit scary at first: diff-lib, read-tree, unpack-trees: mark cache_entry array paramters const Final patch that plugs a memory leak in unpack_nondirectories. unpack-trees: free cache_entry array members for merges It's basically the same one that Stephen tested a while ago (http://thread.gmane.org/gmane.comp.version-control.git/222887). It's not the only leak that affects cherry-pick; expect more (independent) patches. builtin/read-tree.c | 5 +- cache.h | 10 ++-- diff-lib.c | 26 - read-cache.c| 18 --- unpack-trees.c | 148 unpack-trees.h | 14 +++-- 6 files changed, 133 insertions(+), 88 deletions(-) -- 1.8.3 -- 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/7] read-cache: mark cache_entry pointers const
ie_match_stat and ie_modified only derefence their struct cache_entry
pointers for reading. Add const to the parameter declaration here and
do the same for the static helper function used by them, as it's the
same there as well. This allows callers to pass in const pointers.
Signed-off-by: René Scharfe
---
cache.h | 4 ++--
read-cache.c | 18 ++
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index 43a27e7..01e8760 100644
--- a/cache.h
+++ b/cache.h
@@ -482,8 +482,8 @@ extern void *read_blob_data_from_index(struct index_state
*, const char *, unsig
#define CE_MATCH_RACY_IS_DIRTY 02
/* do stat comparison even if CE_SKIP_WORKTREE is true */
#define CE_MATCH_IGNORE_SKIP_WORKTREE 04
-extern int ie_match_stat(const struct index_state *, struct cache_entry *,
struct stat *, unsigned int);
-extern int ie_modified(const struct index_state *, struct cache_entry *,
struct stat *, unsigned int);
+extern int ie_match_stat(const struct index_state *, const struct cache_entry
*, struct stat *, unsigned int);
+extern int ie_modified(const struct index_state *, const struct cache_entry *,
struct stat *, unsigned int);
#define PATHSPEC_ONESTAR 1 /* the pathspec pattern sastisfies GFNM_ONESTAR
*/
diff --git a/read-cache.c b/read-cache.c
index 04ed561..e6e0466 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -91,7 +91,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat
*st)
ce_mark_uptodate(ce);
}
-static int ce_compare_data(struct cache_entry *ce, struct stat *st)
+static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
{
int match = -1;
int fd = open(ce->name, O_RDONLY);
@@ -105,7 +105,7 @@ static int ce_compare_data(struct cache_entry *ce, struct
stat *st)
return match;
}
-static int ce_compare_link(struct cache_entry *ce, size_t expected_size)
+static int ce_compare_link(const struct cache_entry *ce, size_t expected_size)
{
int match = -1;
void *buffer;
@@ -126,7 +126,7 @@ static int ce_compare_link(struct cache_entry *ce, size_t
expected_size)
return match;
}
-static int ce_compare_gitlink(struct cache_entry *ce)
+static int ce_compare_gitlink(const struct cache_entry *ce)
{
unsigned char sha1[20];
@@ -143,7 +143,7 @@ static int ce_compare_gitlink(struct cache_entry *ce)
return hashcmp(sha1, ce->sha1);
}
-static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)
+static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st)
{
switch (st->st_mode & S_IFMT) {
case S_IFREG:
@@ -163,7 +163,7 @@ static int ce_modified_check_fs(struct cache_entry *ce,
struct stat *st)
return 0;
}
-static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
+static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)
{
unsigned int changed = 0;
@@ -239,7 +239,8 @@ static int ce_match_stat_basic(struct cache_entry *ce,
struct stat *st)
return changed;
}
-static int is_racy_timestamp(const struct index_state *istate, struct
cache_entry *ce)
+static int is_racy_timestamp(const struct index_state *istate,
+const struct cache_entry *ce)
{
return (!S_ISGITLINK(ce->ce_mode) &&
istate->timestamp.sec &&
@@ -255,7 +256,7 @@ static int is_racy_timestamp(const struct index_state
*istate, struct cache_entr
}
int ie_match_stat(const struct index_state *istate,
- struct cache_entry *ce, struct stat *st,
+ const struct cache_entry *ce, struct stat *st,
unsigned int options)
{
unsigned int changed;
@@ -311,7 +312,8 @@ int ie_match_stat(const struct index_state *istate,
}
int ie_modified(const struct index_state *istate,
- struct cache_entry *ce, struct stat *st, unsigned int options)
+ const struct cache_entry *ce,
+ struct stat *st, unsigned int options)
{
int changed, changed_fs;
--
1.8.3
--
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 7/7] unpack-trees: free cache_entry array members for merges
The merge functions duplicate entries as needed and they don't free
them. Release them in unpack_nondirectories, the same function
where they were allocated, after we're done.
As suggested by Felipe, use the same loop style (zero-based for loop)
for freeing as for allocating.
Improved-by: Felipe Contreras
Signed-off-by: René Scharfe
---
unpack-trees.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 2dbc05d..57b4074 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -600,9 +600,16 @@ static int unpack_nondirectories(int n, unsigned long mask,
src[i + o->merge] = create_ce_entry(info, names + i, stage);
}
- if (o->merge)
- return call_unpack_fn((const struct cache_entry * const *)src,
- o);
+ if (o->merge) {
+ int rc = call_unpack_fn((const struct cache_entry * const *)src,
+ o);
+ for (i = 0; i < n; i++) {
+ struct cache_entry *ce = src[i + o->merge];
+ if (ce != o->df_conflict_entry)
+ free(ce);
+ }
+ return rc;
+ }
for (i = 0; i < n; i++)
if (src[i] && src[i] != o->df_conflict_entry)
--
1.8.3
--
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 5/7] diff-lib, read-tree, unpack-trees: mark cache_entry pointers const
Add const to struct cache_entry pointers throughout the tree which are
only used for reading. This allows callers to pass in const pointers.
Signed-off-by: René Scharfe
---
builtin/read-tree.c | 2 +-
diff-lib.c | 23 +++---
unpack-trees.c | 91 +
3 files changed, 63 insertions(+), 53 deletions(-)
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 042ac1b..b847486 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -66,7 +66,7 @@ static int exclude_per_directory_cb(const struct option *opt,
const char *arg,
return 0;
}
-static void debug_stage(const char *label, struct cache_entry *ce,
+static void debug_stage(const char *label, const struct cache_entry *ce,
struct unpack_trees_options *o)
{
printf("%s ", label);
diff --git a/diff-lib.c b/diff-lib.c
index f35de0f..83d0cb8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -64,8 +64,9 @@ static int check_removed(const struct cache_entry *ce, struct
stat *st)
* commits, untracked content and/or modified content).
*/
static int match_stat_with_submodule(struct diff_options *diffopt,
- struct cache_entry *ce, struct stat *st,
- unsigned ce_option, unsigned
*dirty_submodule)
+const struct cache_entry *ce,
+struct stat *st, unsigned ce_option,
+unsigned *dirty_submodule)
{
int changed = ce_match_stat(ce, st, ce_option);
if (S_ISGITLINK(ce->ce_mode)) {
@@ -237,7 +238,7 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
/* A file entry went away or appeared */
static void diff_index_show_file(struct rev_info *revs,
const char *prefix,
-struct cache_entry *ce,
+const struct cache_entry *ce,
const unsigned char *sha1, int sha1_valid,
unsigned int mode,
unsigned dirty_submodule)
@@ -246,7 +247,7 @@ static void diff_index_show_file(struct rev_info *revs,
sha1, sha1_valid, ce->name, dirty_submodule);
}
-static int get_stat_data(struct cache_entry *ce,
+static int get_stat_data(const struct cache_entry *ce,
const unsigned char **sha1p,
unsigned int *modep,
int cached, int match_missing,
@@ -283,7 +284,7 @@ static int get_stat_data(struct cache_entry *ce,
}
static void show_new_file(struct rev_info *revs,
- struct cache_entry *new,
+ const struct cache_entry *new,
int cached, int match_missing)
{
const unsigned char *sha1;
@@ -302,8 +303,8 @@ static void show_new_file(struct rev_info *revs,
}
static int show_modified(struct rev_info *revs,
-struct cache_entry *old,
-struct cache_entry *new,
+const struct cache_entry *old,
+const struct cache_entry *new,
int report_missing,
int cached, int match_missing)
{
@@ -362,8 +363,8 @@ static int show_modified(struct rev_info *revs,
* give you the position and number of entries in the index).
*/
static void do_oneway_diff(struct unpack_trees_options *o,
- struct cache_entry *idx,
- struct cache_entry *tree)
+ const struct cache_entry *idx,
+ const struct cache_entry *tree)
{
struct rev_info *revs = o->unpack_data;
int match_missing, cached;
@@ -425,8 +426,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
*/
static int oneway_diff(struct cache_entry **src, struct unpack_trees_options
*o)
{
- struct cache_entry *idx = src[0];
- struct cache_entry *tree = src[1];
+ const struct cache_entry *idx = src[0];
+ const struct cache_entry *tree = src[1];
struct rev_info *revs = o->unpack_data;
/*
diff --git a/unpack-trees.c b/unpack-trees.c
index 2fecef8..c5a40df 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -241,8 +241,11 @@ static int check_updates(struct unpack_trees_options *o)
return errs != 0;
}
-static int verify_uptodate_sparse(struct cache_entry *ce, struct
unpack_trees_options *o);
-static int verify_absent_sparse(struct cache_entry *ce, enum
unpack_trees_error_types, struct unpack_trees_options *o);
+static int verify_uptodate_sparse(const struct cache_entry *ce,
+ struct unpack_trees_options *o);
+static int verify_absent_sparse(const struct cache_entry *ce,
+ enum unpack_trees_error_types,
+
[PATCH v2 1/7] cache: mark cache_entry pointers const
Add const for pointers that are only dereferenced for reading by the
inline functions copy_cache_entry and ce_mode_from_stat. This allows
callers to pass in const pointers.
Signed-off-by: René Scharfe
---
cache.h | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 94ca1ac..43a27e7 100644
--- a/cache.h
+++ b/cache.h
@@ -190,7 +190,8 @@ struct cache_entry {
* another. But we never change the name, or the hash state!
*/
#define CE_STATE_MASK (CE_HASHED | CE_UNHASHED)
-static inline void copy_cache_entry(struct cache_entry *dst, struct
cache_entry *src)
+static inline void copy_cache_entry(struct cache_entry *dst,
+ const struct cache_entry *src)
{
unsigned int state = dst->ce_flags & CE_STATE_MASK;
@@ -222,7 +223,8 @@ static inline unsigned int create_ce_mode(unsigned int mode)
return S_IFGITLINK;
return S_IFREG | ce_permissions(mode);
}
-static inline unsigned int ce_mode_from_stat(struct cache_entry *ce, unsigned
int mode)
+static inline unsigned int ce_mode_from_stat(const struct cache_entry *ce,
+unsigned int mode)
{
extern int trust_executable_bit, has_symlinks;
if (!has_symlinks && S_ISREG(mode) &&
--
1.8.3
--
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 3/7] unpack-trees: factor out dup_entry
While we're add it, mark the struct cache_entry pointer of add_entry
const because we only read from it and this allows callers to pass in
const pointers.
Signed-off-by: René Scharfe
---
unpack-trees.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index ede4299..e8b4cc1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -116,14 +116,20 @@ static void do_add_entry(struct unpack_trees_options *o,
struct cache_entry *ce,
ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
}
-static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
- unsigned int set, unsigned int clear)
+static struct cache_entry *dup_entry(const struct cache_entry *ce)
{
unsigned int size = ce_size(ce);
struct cache_entry *new = xmalloc(size);
memcpy(new, ce, size);
- do_add_entry(o, new, set, clear);
+ return new;
+}
+
+static void add_entry(struct unpack_trees_options *o,
+ const struct cache_entry *ce,
+ unsigned int set, unsigned int clear)
+{
+ do_add_entry(o, dup_entry(ce), set, clear);
}
/*
--
1.8.3
--
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 7/7] unpack-trees: free cache_entry array members for merges
Am 02.06.2013 19:25, schrieb Felipe Contreras:
On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
wrote:
+ for (i = 0; i < n; i++) {
+ struct cache_entry *ce = src[i + o->merge];
+ if (ce != o->df_conflict_entry)
It's possible that ce is NULL, but you didn't add that check because
free(NULL) still works? Or because ce cannot be NULL?
If it's the former, I think it's clearer if we check that ce is not
NULL either way.
It is NULL if one tree misses an entry (e.g. a new or removed file).
free handles NULL and we generally avoid duplicating its NULL-check.
You're probably referring to the non-merge case as the example for
checking. That one is different, though, because there we call
do_add_entry instead of free, which does not handle NULL. And it
doesn't have to, as it is mostly called through add_entry, which never
passes NULL.
René
--
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 7/7] unpack-trees: free cache_entry array members for merges
Am 02.06.2013 19:59, schrieb Felipe Contreras:
On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe
wrote:
Am 02.06.2013 19:25, schrieb Felipe Contreras:
On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
wrote:
+ for (i = 0; i < n; i++) {
+ struct cache_entry *ce = src[i + o->merge];
+ if (ce != o->df_conflict_entry)
It's possible that ce is NULL, but you didn't add that check because
free(NULL) still works? Or because ce cannot be NULL?
If it's the former, I think it's clearer if we check that ce is not
NULL either way.
It is NULL if one tree misses an entry (e.g. a new or removed file). free
handles NULL and we generally avoid duplicating its NULL-check.
Yeah, but I can see somebody adding code inside that 'if' clause to
print the cache entry, and see a crash only to wonder what's going on.
And to save what? 5 characters?
The person adding code that depends on ce not being NULL needs to add
that check as well. Let's not worry too much about future changes that
may or (more likely IMHO) may not be done. The test suite covers this
case multiple times, so such a mistake doesn't have a realistic chance
to hit master.
René
--
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 7/7] unpack-trees: free cache_entry array members for merges
Am 03.06.2013 00:38, schrieb Felipe Contreras:
On Sun, Jun 2, 2013 at 3:26 PM, René Scharfe
wrote:
Am 02.06.2013 19:59, schrieb Felipe Contreras:
On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe
wrote:
Am 02.06.2013 19:25, schrieb Felipe Contreras:
On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
wrote:
+ for (i = 0; i < n; i++) {
+ struct cache_entry *ce = src[i + o->merge];
+ if (ce != o->df_conflict_entry)
It's possible that ce is NULL, but you didn't add that check because
free(NULL) still works? Or because ce cannot be NULL?
If it's the former, I think it's clearer if we check that ce is not
NULL either way.
It is NULL if one tree misses an entry (e.g. a new or removed file). free
handles NULL and we generally avoid duplicating its NULL-check.
Yeah, but I can see somebody adding code inside that 'if' clause to
print the cache entry, and see a crash only to wonder what's going on.
And to save what? 5 characters?
The person adding code that depends on ce not being NULL needs to add that
check as well. Let's not worry too much about future changes that may or
(more likely IMHO) may not be done. The test suite covers this case
multiple times, so such a mistake doesn't have a realistic chance to hit
master.
What do we gain by not doing this? 5 less characters?
By following the convention of not checking for NULL when freeing, we
reduce the size of the code slightly and have one less branch
instruction in the object code. That's not particularly exciting in a
single instance but makes a difference if followed throughout the code base.
What do we gain by adding a duplicate check? A few minutes of debugging
time by the person who will add some code and forget the NULL check?
And how likely is that to happen?
René
--
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 7/7] unpack-trees: free cache_entry array members for merges
Am 03.06.2013 01:23, schrieb Felipe Contreras: I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I said we should do 'if (cd && ce != o->df_conflict_entry)' instead of 'if (ce != o->df_conflict_entry)'. I did assume you meant the latter. There's no reason not to. Only the minor ones already mentioned: More text, one more branch in object code, no benefit except for some hypothetical future case that's caught by the test suite anyway -- or by code review. I wonder if we already reached the point where we spent more time discussing this change than the time needed by the envisioned developer to find and fix the NULL check that suddenly became necessary. :) René -- 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 7/7] unpack-trees: free cache_entry array members for merges
Am 03.06.2013 02:04, schrieb Felipe Contreras: On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe wrote: Am 03.06.2013 01:23, schrieb Felipe Contreras: I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I said we should do 'if (cd && ce != o->df_conflict_entry)' instead of 'if (ce != o->df_conflict_entry)'. I did assume you meant the latter. There's no reason not to. Only the minor ones already mentioned: More text, Five characters. one more branch in object code, Which might actually be more optimal. The difference in absolute numbers will most certainly be within the noise for this one case. no benefit except for some hypothetical future case that's caught by the test suite anyway -- or by code review. That's not the benefit, the benefit is that the code is clearer. I don't see that, and I don't like adding a check that I don't expect to be ever needed. Users are safe because the test suite is going to catch a missing check. In general, I think those who introduce dependencies should add the necessary checks. They have to consider the invariants anyway, no matter how many checks you add beforehand for their convenience. I wonder if we already reached the point where we spent more time discussing this change than the time needed by the envisioned developer to find and fix the NULL check that suddenly became necessary. :) Maybe, but if what you want is to avoid the discussion, you could just add the extra five characters and be done with it. Or you could submit a patch on top that adds the check. I'd send it out if you'd supply a commit message. My review comment would be "mostly harmless, but I don't like it very much because it's not needed now and probably won't ever". But I'm more interested in a different way forward: Would it make sense to push the allocations (and frees) into the merge functions? Currently they duplicate one of the cache entries. Would the merge functions become too ugly or too big if they'd have to build them themselves, avoiding duplication? Would it be significantly faster? René -- 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 8/7] unpack-trees: document that pointer ce can be NULL
From: Felipe Contreras
If someone adds code that dereferences ce before it is freed without
checking for NULL it will crash sometimes. Spare that person from
having to wonder about the reason.
Signed-off-by: Felipe Contreras
---
Signoff from http://article.gmane.org/gmane.comp.version-control.git/225972.
No signoff from me because I don't see the point of adding a check for
a developer that probably won't appear.
unpack-trees.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 57b4074..f22bd89 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -605,7 +605,7 @@ static int unpack_nondirectories(int n, unsigned long mask,
o);
for (i = 0; i < n; i++) {
struct cache_entry *ce = src[i + o->merge];
- if (ce != o->df_conflict_entry)
+ if (ce && ce != o->df_conflict_entry)
free(ce);
}
return rc;
--
1.8.3
--
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 v3 2/2] read-cache: plug a few leaks
Am 08.06.2013 00:29, schrieb Felipe Contreras: We are not freeing 'istate->cache' properly. We can't rely on 'initialized' to keep track of the 'istate->cache', because it doesn't really mean it's initialized. So assume it always has data, and free it before overwriting it. That sounds a bit backwards to me. If ->initialized doesn't mean that the index state is initialized then something is fishy. Would it make sense and be sufficient to set ->initialized in add_index_entry? Or to get rid of it and check for ->cache_alloc instead? Signed-off-by: Felipe Contreras --- read-cache.c | 4 1 file changed, 4 insertions(+) diff --git a/read-cache.c b/read-cache.c index 5e30746..a1dd04d 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate, const char *path) istate->version = ntohl(hdr->hdr_version); istate->cache_nr = ntohl(hdr->hdr_entries); istate->cache_alloc = alloc_nr(istate->cache_nr); + free(istate->cache); istate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache)); istate->initialized = 1; You wrote earlier that this change is safe with current callers and that it prevents leaks with the following sequence: discard_cache(); # add entries read_cache(); Do we currently have such a call sequence somewhere? Wouldn't that be a bug, namely forgetting to call discard_cache before read_cache? I've added a "assert(istate->cache_nr == 0);" a few lines above and the test suite still passed. With the hunk below, ->cache is also always NULL and cache_alloc is always 0 at that point. So we don't need that free call there in the cases covered by the test suite at least -- better leave it out. @@ -1512,6 +1513,9 @@ int discard_index(struct index_state *istate) for (i = 0; i < istate->cache_nr; i++) free(istate->cache[i]); + free(istate->cache); + istate->cache = NULL; + istate->cache_alloc = 0; resolve_undo_clear_index(istate); istate->cache_nr = 0; istate->cache_changed = 0; I still like this part, but also would still recommend to remove the now doubly-outdated comment a few lines below that says "no need to throw away allocated active_cache". It was valid back when there was only a single in-memory instance of the index and no istate parameter. René -- 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 v3 2/2] read-cache: plug a few leaks
Am 08.06.2013 14:15, schrieb Felipe Contreras: On Sat, Jun 8, 2013 at 6:32 AM, René Scharfe wrote: diff --git a/read-cache.c b/read-cache.c index 5e30746..a1dd04d 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate, const char *path) istate->version = ntohl(hdr->hdr_version); istate->cache_nr = ntohl(hdr->hdr_entries); istate->cache_alloc = alloc_nr(istate->cache_nr); + free(istate->cache); istate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache)); istate->initialized = 1; You wrote earlier that this change is safe with current callers and that it prevents leaks with the following sequence: discard_cache(); # add entries read_cache(); Do we currently have such a call sequence somewhere? I don't know. Wouldn't that be a bug, namely forgetting to call discard_cache before read_cache? Why would it be a bug? There's nothing in the API that hints there's a problem with that. A comment before read_index_from says "remember to discard_cache() before reading a different cache!". That is probably a reminder that read_index_from does nothing if ->initialized is set. Entries added before calling read_index_from make up a different cache, however, so I think this comment applies for the call sequence above as well. Only read_index_from and add_index_entry allocate ->cache, and only discard_index frees it, so the two are a triple like malloc, realloc and free. Granted, these hints are not part of the API -- that looks like a documentation bug, however. Side note: I wonder why we need to guard against multiple read_index_from calls in a row with ->initialized. Wouldn't it be easier to avoid the duplicate calls in the first place? Finding them now might be not so easy, though. I've added a "assert(istate->cache_nr == 0);" a few lines above and the test suite still passed. With the hunk below, ->cache is also always NULL and cache_alloc is always 0 at that point. So we don't need that free call there in the cases covered by the test suite at least -- better leave it out. Why leave it out? If somebody makes the mistake of doing the above sequence, would you prefer that we leak? Leaking is better than silently cleaning up after a buggy caller because it still allows the underlying bug to be found. Even better would be an assert, but it's important to make sure it doesn't trigger for existing use cases. René -- 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 v3 2/2] read-cache: plug a few leaks
Am 08.06.2013 16:04, schrieb Felipe Contreras: On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe wrote: Am 08.06.2013 14:15, schrieb Felipe Contreras: Why leave it out? If somebody makes the mistake of doing the above sequence, would you prefer that we leak? Leaking is better than silently cleaning up after a buggy caller because it still allows the underlying bug to be found. No, it doesn't. The pointer is replaced and forever lost. How is leaking memory helping anyone to find the bug? Valgrind can tell you where leaked memory was allocated, but not if you free it in the "wrong" place. René -- 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 v3 2/2] read-cache: plug a few leaks
Am 08.06.2013 18:53, schrieb Felipe Contreras: On Sat, Jun 8, 2013 at 10:56 AM, René Scharfe wrote: Am 08.06.2013 16:04, schrieb Felipe Contreras: On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe wrote: Am 08.06.2013 14:15, schrieb Felipe Contreras: Why leave it out? If somebody makes the mistake of doing the above sequence, would you prefer that we leak? Leaking is better than silently cleaning up after a buggy caller because it still allows the underlying bug to be found. No, it doesn't. The pointer is replaced and forever lost. How is leaking memory helping anyone to find the bug? Valgrind can tell you where leaked memory was allocated, but not if you free it in the "wrong" place. It is the correct place to free it. And nobody will ever find it with valgrind, just like nobody has ever tracked down the hundreds of leaks in Git that happen reliably 100% of the time, much less the ones that happen rarely. We could argue about freeing it here or adding a discard_index call somewhere else (which seems more natural to me) once we had a call sequence that actually causes such a leak. The test suite contains none, so I'd say we need more tests first. Lots of the existing leaks were not worth plugging because the process would end right after freeing the memory. Leaving clean-up to the OS was a conscious design choice, simplifying the code. When such code is reused in a library or run multiple times while it was originally meant to be run only a single time (like with cherry-pick --stdin) the original assumption doesn't hold any more and we have a problem. Let's find and fix those leaks by freeing memory in the right places. Freeing memory just in case in places where we can show that no leak is triggered by our test suite doesn't help. René -- 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 v3 2/2] read-cache: plug a few leaks
Am 08.06.2013 19:27, schrieb Felipe Contreras: On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe wrote: Let's find and fix those leaks by freeing memory in the right places. Freeing memory just in case in places where we can show that no leak is triggered by our test suite doesn't help. It helps; it prevents leaks. The real culprit is the bogus API, but I don't see that changing anytime soon, so there are two options when somebody makes a mistake the API allows; leak or don't leak. And you seem to prefer the leak, even though it provides absolutely no advantage. It covers up bugs, which may seem helpful, but isn't, because it's better to fix the actual mistake. What would be a better API? Making discard_index free the array is a good first step; what else is bogus? René -- 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] submodule: remove redundant check for the_index.initialized
read_cache already performs the same check and returns immediately if
the cache has already been loaded.
Signed-off-by: René Scharfe
---
submodule.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/submodule.c b/submodule.c
index e728025..1821a5b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -603,9 +603,8 @@ int fetch_populated_submodules(const struct argv_array
*options,
if (!work_tree)
goto out;
- if (!the_index.initialized)
- if (read_cache() < 0)
- die("index file corrupt");
+ if (read_cache() < 0)
+ die("index file corrupt");
argv_array_push(&argv, "fetch");
for (i = 0; i < options->argc; i++)
--
1.8.3
--
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 v3 2/2] read-cache: plug a few leaks
Am 09.06.2013 04:25, schrieb Felipe Contreras: On Sat, Jun 8, 2013 at 9:11 PM, René Scharfe wrote: Am 08.06.2013 19:27, schrieb Felipe Contreras: On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe wrote: Let's find and fix those leaks by freeing memory in the right places. Freeing memory just in case in places where we can show that no leak is triggered by our test suite doesn't help. It helps; it prevents leaks. The real culprit is the bogus API, but I don't see that changing anytime soon, so there are two options when somebody makes a mistake the API allows; leak or don't leak. And you seem to prefer the leak, even though it provides absolutely no advantage. It covers up bugs, It doesn't. I thought you already silently agreed that nobody would ever find that leak, as they haven't found the hundreds of leaks that plague Git's code. Nah, I explained non-silently that leakage was a design decision for short-running commands that allocate memory, use it and exit. Reusing such code without freeing allocated memory between runs explicitly turns a "good" leak into a "bad" one, as we saw with cherry-pick --stdin. What would be a better API? Making discard_index free the array is a good first step; what else is bogus? 'initialized' for starters; it should be renamed to 'loaded' or removed, but removing it would require many more changes to make sure we don't load twice. Also, when loading cache entries, it might make sense to check if there's already entries that have not been previously discarded properly. Adding diagnostics that help find leaks is a good idea. So, from reading the code, this sequence is OK: discard_cache() // defined starting point read_cache()// reads the cache read_cache()// does nothing And I guess this one is not OK: discard_cache() // defined starting point add_index_entry() // add single entry read_cache()// currently leaks, should warn/die Any more sequences that we need to guard against, or counterexamples? In the meantime, just in case, the only sane thing to do is free the entries rather than leak. I consider not plugging a leak which we don't know how to trigger with existing code even more sane. Yay, circles! ;-) That being said I'm not interested in this patch any more. The patch is good yet after three tries and countless arguments it's still not applied, nor is there any sign of getting there. Let's take it step by step: Once the known leak is plugged we can worry about the unknown ones. I'll send small patches. René -- 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 2/2] read-cache: free cache in discard_index
discard_cache doesn't have to free the array of cache entries, because the next call of read_cache can simply reuse it, as they all operate on the global variable the_index. discard_index on the other hand does have to free it, because it can be used e.g. with index_state variables on the stack, in which case a missing free would cause an unrecoverable leak. This patch releases the memory and removes a comment that was relevant for discard_cache but has become outdated. Since discard_cache is just a wrapper around discard_index nowadays, we lose the optimization that avoids reallocation of that array within loops of read_cache and discard_cache. That doesn't cause a performance regression for me, however (HEAD = this patch, HEAD^ = master + p0002): Test // HEAD^ HEAD ---\\- 0002.1: read_ca// 1000 times 0.62(0.58+0.04) 0.61(0.58+0.02) -1.6% Suggested-by: Felipe Contreras Signed-off-by: René Scharfe --- read-cache.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/read-cache.c b/read-cache.c index 04ed561..4245f8e 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1518,8 +1518,9 @@ int discard_index(struct index_state *istate) free_name_hash(istate); cache_tree_free(&(istate->cache_tree)); istate->initialized = 0; - - /* no need to throw away allocated active_cache */ + free(istate->cache); + istate->cache = NULL; + istate->cache_alloc = 0; return 0; } -- 1.8.3 -- 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 1/2] read-cache: add simple performance test
Add the helper test-read-cache, which can be used to call read_cache and
discard_cache in a loop as well as a performance check based on it.
Signed-off-by: René Scharfe
---
.gitignore | 1 +
Makefile | 1 +
t/perf/p0002-read-cache.sh | 14 ++
test-read-cache.c | 13 +
4 files changed, 29 insertions(+)
create mode 100755 t/perf/p0002-read-cache.sh
create mode 100644 test-read-cache.c
diff --git a/.gitignore b/.gitignore
index 1640c3a..c0e00eb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -191,6 +191,7 @@
/test-mktemp
/test-parse-options
/test-path-utils
+/test-read-cache
/test-regex
/test-revision-walking
/test-run-command
diff --git a/Makefile b/Makefile
index a748133..2e3b4ee 100644
--- a/Makefile
+++ b/Makefile
@@ -572,6 +572,7 @@ TEST_PROGRAMS_NEED_X += test-mergesort
TEST_PROGRAMS_NEED_X += test-mktemp
TEST_PROGRAMS_NEED_X += test-parse-options
TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-read-cache
TEST_PROGRAMS_NEED_X += test-regex
TEST_PROGRAMS_NEED_X += test-revision-walking
TEST_PROGRAMS_NEED_X += test-run-command
diff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh
new file mode 100755
index 000..9180ae9
--- /dev/null
+++ b/t/perf/p0002-read-cache.sh
@@ -0,0 +1,14 @@
+#!/bin/sh
+
+test_description="Tests performance of reading the index"
+
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+count=1000
+test_perf "read_cache/discard_cache $count times" "
+ test-read-cache $count
+"
+
+test_done
diff --git a/test-read-cache.c b/test-read-cache.c
new file mode 100644
index 000..b25bcf1
--- /dev/null
+++ b/test-read-cache.c
@@ -0,0 +1,13 @@
+#include "cache.h"
+
+int main (int argc, char **argv)
+{
+ int i, cnt = 1;
+ if (argc == 2)
+ cnt = strtol(argv[1], NULL, 0);
+ for (i = 0; i < cnt; i++) {
+ read_cache();
+ discard_cache();
+ }
+ return 0;
+}
--
1.8.3
--
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] submodule: remove redundant check for the_index.initialized
Am 09.06.2013 18:44, schrieb Felipe Contreras: On Sun, Jun 9, 2013 at 11:33 AM, René Scharfe wrote: read_cache already performs the same check and returns immediately if the cache has already been loaded. This time I beat you to it first ;) http://article.gmane.org/gmane.comp.version-control.git/226701 Good to see we're agreeing on something for once. ;) René -- 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] match-trees: factor out fill_tree_desc_strict
Deduplicate code by moving tree_desc initialtization into a helper
function, fill_tree_desc_strict. It is like fill_tree_descriptor,
except that it only accepts tree hashes and no tree references (tags,
commits). No functional change.
Signed-off-by: René Scharfe
---
match-trees.c | 44 +++-
1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/match-trees.c b/match-trees.c
index 2bb734d..7873cde 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -47,6 +47,22 @@ static int score_matches(unsigned mode1, unsigned mode2,
const char *path)
return score;
}
+static void *fill_tree_desc_strict(struct tree_desc *desc,
+ const unsigned char *hash)
+{
+ void *buffer;
+ enum object_type type;
+ unsigned long size;
+
+ buffer = read_sha1_file(hash, &type, &size);
+ if (!buffer)
+ die("unable to read tree (%s)", sha1_to_hex(hash));
+ if (type != OBJ_TREE)
+ die("%s is not a tree", sha1_to_hex(hash));
+ init_tree_desc(desc, buffer, size);
+ return buffer;
+}
+
static int base_name_entries_compare(const struct name_entry *a,
const struct name_entry *b)
{
@@ -61,23 +77,10 @@ static int score_trees(const unsigned char *hash1, const
unsigned char *hash2)
{
struct tree_desc one;
struct tree_desc two;
- void *one_buf, *two_buf;
+ void *one_buf = fill_tree_desc_strict(&one, hash1);
+ void *two_buf = fill_tree_desc_strict(&two, hash2);
int score = 0;
- enum object_type type;
- unsigned long size;
- one_buf = read_sha1_file(hash1, &type, &size);
- if (!one_buf)
- die("unable to read tree (%s)", sha1_to_hex(hash1));
- if (type != OBJ_TREE)
- die("%s is not a tree", sha1_to_hex(hash1));
- init_tree_desc(&one, one_buf, size);
- two_buf = read_sha1_file(hash2, &type, &size);
- if (!two_buf)
- die("unable to read tree (%s)", sha1_to_hex(hash2));
- if (type != OBJ_TREE)
- die("%s is not a tree", sha1_to_hex(hash2));
- init_tree_desc(&two, two_buf, size);
for (;;) {
struct name_entry e1, e2;
int got_entry_from_one = tree_entry(&one, &e1);
@@ -124,16 +127,7 @@ static void match_trees(const unsigned char *hash1,
int recurse_limit)
{
struct tree_desc one;
- void *one_buf;
- enum object_type type;
- unsigned long size;
-
- one_buf = read_sha1_file(hash1, &type, &size);
- if (!one_buf)
- die("unable to read tree (%s)", sha1_to_hex(hash1));
- if (type != OBJ_TREE)
- die("%s is not a tree", sha1_to_hex(hash1));
- init_tree_desc(&one, one_buf, size);
+ void *one_buf = fill_tree_desc_strict(&one, hash1);
while (one.size) {
const char *path;
--
1.8.3
--
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
