Re: [PATCH] daemon: use strbuf for hostname info
Am 07.03.2015 um 02:08 schrieb Jeff King: On Sat, Mar 07, 2015 at 01:20:22AM +0100, René Scharfe wrote: Not a big deal, but do we want to rename sanitize_client_strbuf to sanitize_client? It only had the unwieldy name to distinguish it from this one. A patch would look like this. The result is shorter, but no win in terms of vertical space (number of lines). IMHO this is an improvement, though whether it is enough to merit the code churn I dunno. So I'm in favor, but don't mind dropping it if others disagree. I don't think the change justifies a separate patch, but we can squash it in. :) 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/2] daemon: use strbuf for hostname info
Convert hostname, canon_hostname, ip_address and tcp_port to strbuf.
This allows to get rid of the helpers strbuf_addstr_or_null() and STRARG
because a strbuf always represents a valid (initially empty) string.
sanitize_client() is not needed anymore and sanitize_client_strbuf()
takes its place and name.
Helped-by: Jeff King p...@peff.net
Signed-off-by: Rene Scharfe l@web.de
---
This one is the first patch + s/_strbuf// + s/_reset/_release/.
daemon.c | 98 +++-
1 file changed, 41 insertions(+), 57 deletions(-)
diff --git a/daemon.c b/daemon.c
index c3edd96..265c188 100644
--- a/daemon.c
+++ b/daemon.c
@@ -56,10 +56,10 @@ static const char *user_path;
static unsigned int timeout;
static unsigned int init_timeout;
-static char *hostname;
-static char *canon_hostname;
-static char *ip_address;
-static char *tcp_port;
+static struct strbuf hostname = STRBUF_INIT;
+static struct strbuf canon_hostname = STRBUF_INIT;
+static struct strbuf ip_address = STRBUF_INIT;
+static struct strbuf tcp_port = STRBUF_INIT;
static int hostname_lookup_done;
@@ -68,13 +68,13 @@ static void lookup_hostname(void);
static const char *get_canon_hostname(void)
{
lookup_hostname();
- return canon_hostname;
+ return canon_hostname.buf;
}
static const char *get_ip_address(void)
{
lookup_hostname();
- return ip_address;
+ return ip_address.buf;
}
static void logreport(int priority, const char *err, va_list params)
@@ -122,12 +122,6 @@ static void NORETURN daemon_die(const char *err, va_list
params)
exit(1);
}
-static void strbuf_addstr_or_null(struct strbuf *sb, const char *s)
-{
- if (s)
- strbuf_addstr(sb, s);
-}
-
struct expand_path_context {
const char *directory;
};
@@ -138,22 +132,22 @@ static size_t expand_path(struct strbuf *sb, const char
*placeholder, void *ctx)
switch (placeholder[0]) {
case 'H':
- strbuf_addstr_or_null(sb, hostname);
+ strbuf_addbuf(sb, hostname);
return 1;
case 'C':
if (placeholder[1] == 'H') {
- strbuf_addstr_or_null(sb, get_canon_hostname());
+ strbuf_addstr(sb, get_canon_hostname());
return 2;
}
break;
case 'I':
if (placeholder[1] == 'P') {
- strbuf_addstr_or_null(sb, get_ip_address());
+ strbuf_addstr(sb, get_ip_address());
return 2;
}
break;
case 'P':
- strbuf_addstr_or_null(sb, tcp_port);
+ strbuf_addbuf(sb, tcp_port);
return 1;
case 'D':
strbuf_addstr(sb, context-directory);
@@ -301,16 +295,14 @@ static int run_access_hook(struct daemon_service
*service, const char *dir, cons
char *eol;
int seen_errors = 0;
-#define STRARG(x) ((x) ? (x) : )
*arg++ = access_hook;
*arg++ = service-name;
*arg++ = path;
- *arg++ = STRARG(hostname);
- *arg++ = STRARG(get_canon_hostname());
- *arg++ = STRARG(get_ip_address());
- *arg++ = STRARG(tcp_port);
+ *arg++ = hostname.buf;
+ *arg++ = get_canon_hostname();
+ *arg++ = get_ip_address();
+ *arg++ = tcp_port.buf;
*arg = NULL;
-#undef STRARG
child.use_shell = 1;
child.argv = argv;
@@ -542,7 +534,7 @@ static void parse_host_and_port(char *hostport, char **host,
* trailing and leading dots, which means that the client cannot escape
* our base path via .. traversal.
*/
-static void sanitize_client_strbuf(struct strbuf *out, const char *in)
+static void sanitize_client(struct strbuf *out, const char *in)
{
for (; *in; in++) {
if (*in == '/')
@@ -556,23 +548,14 @@ static void sanitize_client_strbuf(struct strbuf *out,
const char *in)
strbuf_setlen(out, out-len - 1);
}
-static char *sanitize_client(const char *in)
-{
- struct strbuf out = STRBUF_INIT;
- sanitize_client_strbuf(out, in);
- return strbuf_detach(out, NULL);
-}
-
/*
* Like sanitize_client, but we also perform any canonicalization
* to make life easier on the admin.
*/
-static char *canonicalize_client(const char *in)
+static void canonicalize_client(struct strbuf *out, const char *in)
{
- struct strbuf out = STRBUF_INIT;
- sanitize_client_strbuf(out, in);
- strbuf_tolower(out);
- return strbuf_detach(out, NULL);
+ sanitize_client(out, in);
+ strbuf_tolower(out);
}
/*
@@ -595,11 +578,11 @@ static void parse_host_arg(char *extra_args, int buflen)
char *port;
parse_host_and_port(val, host, port);
if (port) {
-
[PATCH v2 2/2] daemon: deglobalize hostname information
Move the variables related to the client-supplied hostname into its own
struct, let execute() own an instance of that instead of storing the
information in global variables and pass the struct to any function that
needs to access it as a parameter.
The lifetime of the variables is easier to see this way. Allocated
memory is released within execute(). The strbufs don't have to be reset
anymore because they are written to only once at most: parse_host_arg()
is only called once by execute() and lookup_hostname() guards against
being called twice using hostname_lookup_done.
Signed-off-by: Rene Scharfe l@web.de
---
daemon.c | 133 +++
1 file changed, 74 insertions(+), 59 deletions(-)
diff --git a/daemon.c b/daemon.c
index 265c188..9ee2187 100644
--- a/daemon.c
+++ b/daemon.c
@@ -43,9 +43,6 @@ static const char *base_path;
static const char *interpolated_path;
static int base_path_relaxed;
-/* Flag indicating client sent extra args. */
-static int saw_extended_args;
-
/* If defined, ~user notation is allowed and the string is inserted
* after ~user/. E.g. a request to git://host/~alice/frotz would
* go to /home/alice/pub_git/frotz with --user-path=pub_git.
@@ -56,25 +53,27 @@ static const char *user_path;
static unsigned int timeout;
static unsigned int init_timeout;
-static struct strbuf hostname = STRBUF_INIT;
-static struct strbuf canon_hostname = STRBUF_INIT;
-static struct strbuf ip_address = STRBUF_INIT;
-static struct strbuf tcp_port = STRBUF_INIT;
-
-static int hostname_lookup_done;
+struct hostinfo {
+ struct strbuf hostname;
+ struct strbuf canon_hostname;
+ struct strbuf ip_address;
+ struct strbuf tcp_port;
+ unsigned int hostname_lookup_done:1;
+ unsigned int saw_extended_args:1;
+};
-static void lookup_hostname(void);
+static void lookup_hostname(struct hostinfo *hi);
-static const char *get_canon_hostname(void)
+static const char *get_canon_hostname(struct hostinfo *hi)
{
- lookup_hostname();
- return canon_hostname.buf;
+ lookup_hostname(hi);
+ return hi-canon_hostname.buf;
}
-static const char *get_ip_address(void)
+static const char *get_ip_address(struct hostinfo *hi)
{
- lookup_hostname();
- return ip_address.buf;
+ lookup_hostname(hi);
+ return hi-ip_address.buf;
}
static void logreport(int priority, const char *err, va_list params)
@@ -124,30 +123,32 @@ static void NORETURN daemon_die(const char *err, va_list
params)
struct expand_path_context {
const char *directory;
+ struct hostinfo *hostinfo;
};
static size_t expand_path(struct strbuf *sb, const char *placeholder, void
*ctx)
{
struct expand_path_context *context = ctx;
+ struct hostinfo *hi = context-hostinfo;
switch (placeholder[0]) {
case 'H':
- strbuf_addbuf(sb, hostname);
+ strbuf_addbuf(sb, hi-hostname);
return 1;
case 'C':
if (placeholder[1] == 'H') {
- strbuf_addstr(sb, get_canon_hostname());
+ strbuf_addstr(sb, get_canon_hostname(hi));
return 2;
}
break;
case 'I':
if (placeholder[1] == 'P') {
- strbuf_addstr(sb, get_ip_address());
+ strbuf_addstr(sb, get_ip_address(hi));
return 2;
}
break;
case 'P':
- strbuf_addbuf(sb, tcp_port);
+ strbuf_addbuf(sb, hi-tcp_port);
return 1;
case 'D':
strbuf_addstr(sb, context-directory);
@@ -156,7 +157,7 @@ static size_t expand_path(struct strbuf *sb, const char
*placeholder, void *ctx)
return 0;
}
-static const char *path_ok(const char *directory)
+static const char *path_ok(const char *directory, struct hostinfo *hi)
{
static char rpath[PATH_MAX];
static char interp_path[PATH_MAX];
@@ -192,11 +193,12 @@ static const char *path_ok(const char *directory)
dir = rpath;
}
}
- else if (interpolated_path saw_extended_args) {
+ else if (interpolated_path hi-saw_extended_args) {
struct strbuf expanded_path = STRBUF_INIT;
struct expand_path_context context;
context.directory = directory;
+ context.hostinfo = hi;
if (*dir != '/') {
/* Allow only absolute */
@@ -286,7 +288,8 @@ static int daemon_error(const char *dir, const char *msg)
static const char *access_hook;
-static int run_access_hook(struct daemon_service *service, const char *dir,
const char *path)
+static int run_access_hook(struct daemon_service *service, const char *dir,
+ const char *path, struct hostinfo *hi)
{
Re: [PATCH] daemon: use strbuf for hostname info
Am 06.03.2015 um 22:06 schrieb Jeff King:
On Fri, Mar 06, 2015 at 09:57:22AM +0100, René Scharfe wrote:
if (port) {
- free(tcp_port);
- tcp_port = sanitize_client(port);
+ strbuf_reset(tcp_port);
+ sanitize_client_strbuf(tcp_port, port);
The equivalent of free() is strbuf_release(). I think it is reasonable
to strbuf_reset here, since we are about to write into it again anyway
(though I doubt it happens much in practice, since that would imply
multiple `host=` segments sent by the client). But later...
- free(hostname);
- free(canon_hostname);
- free(ip_address);
- free(tcp_port);
- hostname = canon_hostname = ip_address = tcp_port = NULL;
+ strbuf_reset(hostname);
+ strbuf_reset(canon_hostname);
+ strbuf_reset(ip_address);
+ strbuf_reset(tcp_port);
These probably want to all be strbuf_release(). Again, I doubt it
matters much because this is a forked daemon serving only a single
request (so they'll get freed by the OS soon anyway), but I think
freeing the memory here follows the original intent.
Using a static strbuf means (in my mind) don't worry about freeing,
a memory leak won't happen anyway because we reuse allocations.
The new code adds recycling of allocations, which I somehow expect
in connection with static allocations where possible. You're right
that using strbuf_release() would match the original code more
strictly.
But this block is a no-op anyway because it's the first thing we do
to these (initially empty) variables. That's not immediately
obvious and should be addressed in a separate 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
Re: [PATCH] daemon: use strbuf for hostname info
Am 06.03.2015 um 22:06 schrieb Jeff King:
On Fri, Mar 06, 2015 at 09:57:22AM +0100, René Scharfe wrote:
Convert hostname, canon_hostname, ip_address and tcp_port to strbuf.
This allows to get rid of the helpers strbuf_addstr_or_null() and STRARG
because a strbuf always represents a valid (initially empty) string.
sanitize_client() becomes unused and is removed as well.
Makes sense. I had a feeling that we might have cared about NULL versus
the empty string somewhere, but I did not see it in the patch below, so
I think it is fine.
-static char *sanitize_client(const char *in)
-{
-struct strbuf out = STRBUF_INIT;
-sanitize_client_strbuf(out, in);
-return strbuf_detach(out, NULL);
-}
Not a big deal, but do we want to rename sanitize_client_strbuf to
sanitize_client? It only had the unwieldy name to distinguish it from
this one.
A patch would look like this. The result is shorter, but no win in
terms of vertical space (number of lines).
-- 8 --
Subject: daemon: drop _strbuf suffix of sanitize and canonicalize functions
Now that only the strbuf variants of the functions remain, remove the
_strbuf part from their names.
Suggested-by: Jeff King p...@peff.net
Signed-off-by: Rene Scharfe l@web.de
---
daemon.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/daemon.c b/daemon.c
index c04315e..0412f8c 100644
--- a/daemon.c
+++ b/daemon.c
@@ -534,7 +534,7 @@ static void parse_host_and_port(char *hostport, char **host,
* trailing and leading dots, which means that the client cannot escape
* our base path via .. traversal.
*/
-static void sanitize_client_strbuf(struct strbuf *out, const char *in)
+static void sanitize_client(struct strbuf *out, const char *in)
{
for (; *in; in++) {
if (*in == '/')
@@ -549,12 +549,12 @@ static void sanitize_client_strbuf(struct strbuf *out,
const char *in)
}
/*
- * Like sanitize_client_strbuf, but we also perform any canonicalization
+ * Like sanitize_client, but we also perform any canonicalization
* to make life easier on the admin.
*/
-static void canonicalize_client_strbuf(struct strbuf *out, const char *in)
+static void canonicalize_client(struct strbuf *out, const char *in)
{
- sanitize_client_strbuf(out, in);
+ sanitize_client(out, in);
strbuf_tolower(out);
}
@@ -579,10 +579,10 @@ static void parse_host_arg(char *extra_args, int buflen)
parse_host_and_port(val, host, port);
if (port) {
strbuf_reset(tcp_port);
- sanitize_client_strbuf(tcp_port, port);
+ sanitize_client(tcp_port, port);
}
strbuf_reset(hostname);
- canonicalize_client_strbuf(hostname, host);
+ canonicalize_client(hostname, host);
hostname_lookup_done = 0;
}
@@ -620,8 +620,8 @@ static void lookup_hostname(void)
strbuf_reset(canon_hostname);
if (ai-ai_canonname)
- sanitize_client_strbuf(canon_hostname,
- ai-ai_canonname);
+ sanitize_client(canon_hostname,
+ ai-ai_canonname);
else
strbuf_addbuf(canon_hostname, ip_address);
@@ -645,7 +645,7 @@ static void lookup_hostname(void)
addrbuf, sizeof(addrbuf));
strbuf_reset(canon_hostname);
- sanitize_client_strbuf(canon_hostname, hent-h_name);
+ sanitize_client(canon_hostname, hent-h_name);
strbuf_reset(ip_address);
strbuf_addstr(ip_address, addrbuf);
}
--
2.3.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
Re: [PATCH] [GSoC][MICRO] Forbid log --graph --no-walk
Am 06.03.2015 um 09:55 schrieb Dongcan Jiang:
Because --graph is about connected history while --no-walk is about discrete
points.
revision.c: Judge whether --graph and --no-walk come together when running
git-log.
buildin/log.c: Set git-log cmd flag.
Documentation/rev-list-options.txt: Add specification on the forbidden usage.
Signed-off-by: Dongcan Jiang dongcan.ji...@gmail.com
---
Documentation/rev-list-options.txt | 2 ++
builtin/log.c | 1 +
revision.c | 4
revision.h | 3 +++
4 files changed, 10 insertions(+)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 4ed8587..eea2c0a 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -679,6 +679,7 @@ endif::git-rev-list[]
given on the command line. Otherwise (if `sorted` or no argument
was given), the commits are shown in reverse chronological order
by commit time.
+ Cannot be combined with `--graph` when running git-log.
--do-walk::
Overrides a previous `--no-walk`.
@@ -781,6 +782,7 @@ you would get an output like this:
on the left hand side of the output. This may cause extra lines
to be printed in between commits, in order for the graph history
to be drawn properly.
+ Cannot be combined with `--no-walk` when running git-log.
+
This enables parent rewriting, see 'History Simplification' below.
+
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..7bf5adb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
git_config(git_log_config, NULL);
init_revisions(rev, prefix);
+ rev.cmd_is_log = 1;
rev.always_show_header = 1;
memset(opt, 0, sizeof(opt));
opt.def = HEAD;
diff --git a/revision.c b/revision.c
index 66520c6..5f62c89 100644
--- a/revision.c
+++ b/revision.c
@@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char
*prefix)
revs-commit_format = CMIT_FMT_DEFAULT;
+ revs-cmd_is_log = 0;
+
init_grep_defaults();
grep_init(revs-grep_filter, prefix);
revs-grep_filter.status_only = 1;
@@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
if (revs-reflog_info revs-graph)
die(cannot combine --walk-reflogs with --graph);
+ if (revs-no_walk revs-graph revs-cmd_is_log)
+ die(cannot combine --no-walk with --graph when running
git-log);
Why only for git log? Doesn't the justification given in the commit
message above apply in general?
if (!revs-reflog_info revs-grep_filter.use_reflog_filter)
die(cannot use --grep-reflog without --walk-reflogs);
diff --git a/revision.h b/revision.h
index 0ea8b4e..255982a 100644
--- a/revision.h
+++ b/revision.h
@@ -146,6 +146,9 @@ struct rev_info {
track_first_time:1,
linear:1;
+ /* cmd type */
+ unsigned int cmd_is_log:1;
+
enum date_mode date_mode;
unsigned intabbrev;
--
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] daemon: use strbuf for hostname info
Convert hostname, canon_hostname, ip_address and tcp_port to strbuf.
This allows to get rid of the helpers strbuf_addstr_or_null() and STRARG
because a strbuf always represents a valid (initially empty) string.
sanitize_client() becomes unused and is removed as well.
Signed-off-by: Rene Scharfe l@web.de
---
daemon.c | 98 +++-
1 file changed, 41 insertions(+), 57 deletions(-)
diff --git a/daemon.c b/daemon.c
index c3edd96..c04315e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -56,10 +56,10 @@ static const char *user_path;
static unsigned int timeout;
static unsigned int init_timeout;
-static char *hostname;
-static char *canon_hostname;
-static char *ip_address;
-static char *tcp_port;
+static struct strbuf hostname = STRBUF_INIT;
+static struct strbuf canon_hostname = STRBUF_INIT;
+static struct strbuf ip_address = STRBUF_INIT;
+static struct strbuf tcp_port = STRBUF_INIT;
static int hostname_lookup_done;
@@ -68,13 +68,13 @@ static void lookup_hostname(void);
static const char *get_canon_hostname(void)
{
lookup_hostname();
- return canon_hostname;
+ return canon_hostname.buf;
}
static const char *get_ip_address(void)
{
lookup_hostname();
- return ip_address;
+ return ip_address.buf;
}
static void logreport(int priority, const char *err, va_list params)
@@ -122,12 +122,6 @@ static void NORETURN daemon_die(const char *err, va_list
params)
exit(1);
}
-static void strbuf_addstr_or_null(struct strbuf *sb, const char *s)
-{
- if (s)
- strbuf_addstr(sb, s);
-}
-
struct expand_path_context {
const char *directory;
};
@@ -138,22 +132,22 @@ static size_t expand_path(struct strbuf *sb, const char
*placeholder, void *ctx)
switch (placeholder[0]) {
case 'H':
- strbuf_addstr_or_null(sb, hostname);
+ strbuf_addbuf(sb, hostname);
return 1;
case 'C':
if (placeholder[1] == 'H') {
- strbuf_addstr_or_null(sb, get_canon_hostname());
+ strbuf_addstr(sb, get_canon_hostname());
return 2;
}
break;
case 'I':
if (placeholder[1] == 'P') {
- strbuf_addstr_or_null(sb, get_ip_address());
+ strbuf_addstr(sb, get_ip_address());
return 2;
}
break;
case 'P':
- strbuf_addstr_or_null(sb, tcp_port);
+ strbuf_addbuf(sb, tcp_port);
return 1;
case 'D':
strbuf_addstr(sb, context-directory);
@@ -301,16 +295,14 @@ static int run_access_hook(struct daemon_service
*service, const char *dir, cons
char *eol;
int seen_errors = 0;
-#define STRARG(x) ((x) ? (x) : )
*arg++ = access_hook;
*arg++ = service-name;
*arg++ = path;
- *arg++ = STRARG(hostname);
- *arg++ = STRARG(get_canon_hostname());
- *arg++ = STRARG(get_ip_address());
- *arg++ = STRARG(tcp_port);
+ *arg++ = hostname.buf;
+ *arg++ = get_canon_hostname();
+ *arg++ = get_ip_address();
+ *arg++ = tcp_port.buf;
*arg = NULL;
-#undef STRARG
child.use_shell = 1;
child.argv = argv;
@@ -556,23 +548,14 @@ static void sanitize_client_strbuf(struct strbuf *out,
const char *in)
strbuf_setlen(out, out-len - 1);
}
-static char *sanitize_client(const char *in)
-{
- struct strbuf out = STRBUF_INIT;
- sanitize_client_strbuf(out, in);
- return strbuf_detach(out, NULL);
-}
-
/*
- * Like sanitize_client, but we also perform any canonicalization
+ * Like sanitize_client_strbuf, but we also perform any canonicalization
* to make life easier on the admin.
*/
-static char *canonicalize_client(const char *in)
+static void canonicalize_client_strbuf(struct strbuf *out, const char *in)
{
- struct strbuf out = STRBUF_INIT;
- sanitize_client_strbuf(out, in);
- strbuf_tolower(out);
- return strbuf_detach(out, NULL);
+ sanitize_client_strbuf(out, in);
+ strbuf_tolower(out);
}
/*
@@ -595,11 +578,11 @@ static void parse_host_arg(char *extra_args, int buflen)
char *port;
parse_host_and_port(val, host, port);
if (port) {
- free(tcp_port);
- tcp_port = sanitize_client(port);
+ strbuf_reset(tcp_port);
+ sanitize_client_strbuf(tcp_port, port);
}
- free(hostname);
- hostname = canonicalize_client(host);
+ strbuf_reset(hostname);
+
[PATCH] zlib: initialize git_zstream in git_deflate_init{,_gzip,_raw}
Clear the git_zstream variable at the start of git_deflate_init() etc.
so that callers don't have to do that.
Signed-off-by: Rene Scharfe l@web.de
---
archive-zip.c | 2 --
builtin/index-pack.c | 1 -
builtin/pack-objects.c | 2 --
bulk-checkin.c | 1 -
diff.c | 1 -
fast-import.c | 3 ---
http-push.c| 1 -
remote-curl.c | 1 -
sha1_file.c| 1 -
zlib.c | 2 ++
10 files changed, 2 insertions(+), 13 deletions(-)
diff --git a/archive-zip.c b/archive-zip.c
index 4bde019..1a54e1b 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -120,7 +120,6 @@ static void *zlib_deflate_raw(void *data, unsigned long
size,
void *buffer;
int result;
- memset(stream, 0, sizeof(stream));
git_deflate_init_raw(stream, compression_level);
maxsize = git_deflate_bound(stream, size);
buffer = xmalloc(maxsize);
@@ -349,7 +348,6 @@ static int write_zip_entry(struct archiver_args *args,
size_t out_len;
unsigned char compressed[STREAM_BUFFER_SIZE * 2];
- memset(zstream, 0, sizeof(zstream));
git_deflate_init_raw(zstream, args-compression_level);
compressed_size = 0;
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 4632117..cf654df 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1204,7 +1204,6 @@ static int write_compressed(struct sha1file *f, void *in,
unsigned int size)
int status;
unsigned char outbuf[4096];
- memset(stream, 0, sizeof(stream));
git_deflate_init(stream, zlib_compression_level);
stream.next_in = in;
stream.avail_in = size;
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d816587..c3a7516 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -125,7 +125,6 @@ static unsigned long do_compress(void **pptr, unsigned long
size)
void *in, *out;
unsigned long maxsize;
- memset(stream, 0, sizeof(stream));
git_deflate_init(stream, pack_compression_level);
maxsize = git_deflate_bound(stream, size);
@@ -153,7 +152,6 @@ static unsigned long write_large_blob_data(struct
git_istream *st, struct sha1fi
unsigned char obuf[1024 * 16];
unsigned long olen = 0;
- memset(stream, 0, sizeof(stream));
git_deflate_init(stream, pack_compression_level);
for (;;) {
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 0c4b8a7..8d157eb 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -105,7 +105,6 @@ static int stream_to_pack(struct bulk_checkin_state *state,
int write_object = (flags HASH_WRITE_OBJECT);
off_t offset = 0;
- memset(s, 0, sizeof(s));
git_deflate_init(s, pack_compression_level);
hdrlen = encode_in_pack_object_header(type, size, obuf);
diff --git a/diff.c b/diff.c
index d1bd534..dad875c 100644
--- a/diff.c
+++ b/diff.c
@@ -2093,7 +2093,6 @@ static unsigned char *deflate_it(char *data,
unsigned char *deflated;
git_zstream stream;
- memset(stream, 0, sizeof(stream));
git_deflate_init(stream, zlib_compression_level);
bound = git_deflate_bound(stream, size);
deflated = xmalloc(bound);
diff --git a/fast-import.c b/fast-import.c
index aac2c24..77fb2ff 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1062,7 +1062,6 @@ static int store_object(
} else
delta = NULL;
- memset(s, 0, sizeof(s));
git_deflate_init(s, pack_compression_level);
if (delta) {
s.next_in = delta;
@@ -1090,7 +1089,6 @@ static int store_object(
free(delta);
delta = NULL;
- memset(s, 0, sizeof(s));
git_deflate_init(s, pack_compression_level);
s.next_in = (void *)dat-buf;
s.avail_in = dat-len;
@@ -1190,7 +1188,6 @@ static void stream_blob(uintmax_t len, unsigned char
*sha1out, uintmax_t mark)
crc32_begin(pack_file);
- memset(s, 0, sizeof(s));
git_deflate_init(s, pack_compression_level);
hdrlen = encode_in_pack_object_header(OBJ_BLOB, len, out_buf);
diff --git a/http-push.c b/http-push.c
index 0beb7ab..bfb1c96 100644
--- a/http-push.c
+++ b/http-push.c
@@ -365,7 +365,6 @@ static void start_put(struct transfer_request *request)
hdrlen = sprintf(hdr, %s %lu, typename(type), len) + 1;
/* Set it up */
- memset(stream, 0, sizeof(stream));
git_deflate_init(stream, zlib_compression_level);
size = git_deflate_bound(stream, len + hdrlen);
strbuf_init(request-buffer.buf, size);
diff --git a/remote-curl.c b/remote-curl.c
index deb4bfe..af7b678 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -567,7 +567,6 @@ retry:
git_zstream stream;
int ret;
-
Re: [PATCH] archive-zip: add --text parameter
Am 04.03.2015 um 22:13 schrieb René Scharfe:
diff --git a/archive-zip.c b/archive-zip.c
index 4bde019..3767940 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -5,6 +5,7 @@
#include archive.h
#include streaming.h
#include utf8.h
+#include xdiff-interface.h
static int zip_date;
static int zip_time;
@@ -210,6 +211,7 @@ static int write_zip_entry(struct archiver_args *args,
struct git_istream *stream = NULL;
unsigned long flags = 0;
unsigned long size;
+ int is_binary = -1;
crc = crc32(0, NULL, 0);
@@ -238,8 +240,14 @@ static int write_zip_entry(struct archiver_args *args,
method = 0;
attr2 = S_ISLNK(mode) ? ((mode | 0777) 16) :
(mode 0111) ? ((mode) 16) : 0;
- if (S_ISREG(mode) args-compression_level != 0 size 0)
- method = 8;
+ if (S_ISREG(mode)) {
+ if (args-compression_level != 0 size 0)
+ method = 8;
+ if (args-text == ARCHIVE_TEXT_ALL)
+ is_binary = 0;
+ else if (args-text == ARCHIVE_TEXT_NONE)
+ is_binary = 1;
+ }
if (S_ISREG(mode) type == OBJ_BLOB !args-convert
size big_file_threshold) {
@@ -256,6 +264,8 @@ static int write_zip_entry(struct archiver_args *args,
return error(cannot read %s,
sha1_to_hex(sha1));
crc = crc32(crc, buffer, size);
+ if (is_binary 0)
+ is_binary = buffer_is_binary(buffer, size);
out = buffer;
}
compressed_size = (method == 0) ? size : 0;
@@ -300,7 +310,6 @@ static int write_zip_entry(struct archiver_args *args,
copy_le16(dirent.extra_length, ZIP_EXTRA_MTIME_SIZE);
copy_le16(dirent.comment_length, 0);
copy_le16(dirent.disk, 0);
- copy_le16(dirent.attr1, 0);
copy_le32(dirent.attr2, attr2);
copy_le32(dirent.offset, zip_offset);
@@ -328,6 +337,8 @@ static int write_zip_entry(struct archiver_args *args,
if (readlen = 0)
break;
crc = crc32(crc, buf, readlen);
+ if (is_binary 0)
+ is_binary = buffer_is_binary(buffer, size);
buffer is NULL here, so this crashes. buf and readlen have to be used
instead. This code path is only used for entries that are too big to be
compressed in one go.
write_or_die(1, buf, readlen);
}
close_istream(stream);
@@ -361,6 +372,8 @@ static int write_zip_entry(struct archiver_args *args,
if (readlen = 0)
break;
crc = crc32(crc, buf, readlen);
+ if (is_binary 0)
+ is_binary = buffer_is_binary(buffer, size);
Same here.
zstream.next_in = buf;
zstream.avail_in = readlen;
@@ -405,6 +418,8 @@ static int write_zip_entry(struct archiver_args *args,
free(deflated);
free(buffer);
+ copy_le16(dirent.attr1, !is_binary);
+
memcpy(zip_dir + zip_dir_offset, dirent, ZIP_DIR_HEADER_SIZE);
zip_dir_offset += ZIP_DIR_HEADER_SIZE;
memcpy(zip_dir + zip_dir_offset, path, pathlen);
@@ -466,7 +481,7 @@ static int write_zip_archive(const struct archiver *ar,
static struct archiver zip_archiver = {
zip,
write_zip_archive,
- ARCHIVER_WANT_COMPRESSION_LEVELS|ARCHIVER_REMOTE
+ ARCHIVER_WANT_COMPRESSION_LEVELS|ARCHIVER_REMOTE|ARCHIVER_TEXT_ATTRIBUTE
};
void init_zip_archiver(void)
--
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] archive-zip: add --text parameter
Am 05.03.2015 um 03:16 schrieb Junio C Hamano: René Scharfe l@web.de writes: No sign-off, yet, because I'm not sure we really need another option. E.g. --text=all doesn't seem to be actually useful, but it was easy to implement. Info-ZIP's zip always creates archives like --text=auto does, so perhaps we should make that our default behavior as well? My knee-jerk reaction is yeah, why not? what are the downsides, other than the result will not be bit-for-bit identical to the output from older Git. I am sure I am missing something as I do not regularly use this format. AFAICS there won't be any other downsides. And archive stability is harder to achieve for ZIP anyway because it depends on compression level and (more fundamentally) on libz version. @@ -256,6 +264,8 @@ static int write_zip_entry(struct archiver_args *args, return error(cannot read %s, sha1_to_hex(sha1)); crc = crc32(crc, buffer, size); + if (is_binary 0) + is_binary = buffer_is_binary(buffer, size); In this codepath, do you have the path of the thing the buffer contents came from? I am wondering if consulting the attributes system is a better idea. Anything that is explicitly marked as binary or -diff is definitely binary, and anything that is not marked as binary is text to us for all practical purposes, no? Yes, attributes can help, especially to allow users to correct wrong guesses of the heuristic. Offering automatic detection of binary files by default like git diff and git grep is still a good idea, I think. buffer_is_binary() doesn't add a lot of overhead since it only looks at the first few bytes of the buffer. 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] archive-zip: mark text files in archives
Set the text flag for ZIP archive entries that look like text files so
that unzip -a can be used to perform end-of-line conversions. Info-ZIP
zip does the same.
Detect binary files the same way as git diff and git grep do, namely by
checking for the attribute diff and its negation -diff, and if none
is found by falling back to checking for the presence of NUL bytes in
the first few bytes of the file contents.
7-Zip, Windows' built-in ZIP functionality and Info-ZIP unzip without
the switch -a are not affected by the change and still extract text
files without doing any end-of-line conversions.
NB: The actual end-of-line style used in the archive entries doesn't
matter to unzip -a, as it converts any CR, CRLF and LF to the line end
characters appropriate for the platform it is running on.
Suggested-by: Ulrike Fischer lua...@nililand.de
Signed-off-by: Rene Scharfe l@web.de
---
archive-zip.c | 25 -
t/t5003-archive-zip.sh | 47 ++-
2 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/archive-zip.c b/archive-zip.c
index 4bde019..0f9e87f 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -5,6 +5,8 @@
#include archive.h
#include streaming.h
#include utf8.h
+#include userdiff.h
+#include xdiff-interface.h
static int zip_date;
static int zip_time;
@@ -189,6 +191,16 @@ static int has_only_ascii(const char *s)
}
}
+static int entry_is_binary(const char *path, const void *buffer, size_t size)
+{
+ struct userdiff_driver *driver = userdiff_find_by_path(path);
+ if (!driver)
+ driver = userdiff_find_by_name(default);
+ if (driver-binary != -1)
+ return driver-binary;
+ return buffer_is_binary(buffer, size);
+}
+
#define STREAM_BUFFER_SIZE (1024 * 16)
static int write_zip_entry(struct archiver_args *args,
@@ -210,6 +222,8 @@ static int write_zip_entry(struct archiver_args *args,
struct git_istream *stream = NULL;
unsigned long flags = 0;
unsigned long size;
+ int is_binary = -1;
+ const char *path_without_prefix = path + args-baselen;
crc = crc32(0, NULL, 0);
@@ -256,6 +270,8 @@ static int write_zip_entry(struct archiver_args *args,
return error(cannot read %s,
sha1_to_hex(sha1));
crc = crc32(crc, buffer, size);
+ is_binary = entry_is_binary(path_without_prefix,
+ buffer, size);
out = buffer;
}
compressed_size = (method == 0) ? size : 0;
@@ -300,7 +316,6 @@ static int write_zip_entry(struct archiver_args *args,
copy_le16(dirent.extra_length, ZIP_EXTRA_MTIME_SIZE);
copy_le16(dirent.comment_length, 0);
copy_le16(dirent.disk, 0);
- copy_le16(dirent.attr1, 0);
copy_le32(dirent.attr2, attr2);
copy_le32(dirent.offset, zip_offset);
@@ -328,6 +343,9 @@ static int write_zip_entry(struct archiver_args *args,
if (readlen = 0)
break;
crc = crc32(crc, buf, readlen);
+ if (is_binary == -1)
+ is_binary = entry_is_binary(path_without_prefix,
+ buf, readlen);
write_or_die(1, buf, readlen);
}
close_istream(stream);
@@ -361,6 +379,9 @@ static int write_zip_entry(struct archiver_args *args,
if (readlen = 0)
break;
crc = crc32(crc, buf, readlen);
+ if (is_binary == -1)
+ is_binary = entry_is_binary(path_without_prefix,
+ buf, readlen);
zstream.next_in = buf;
zstream.avail_in = readlen;
@@ -405,6 +426,8 @@ static int write_zip_entry(struct archiver_args *args,
free(deflated);
free(buffer);
+ copy_le16(dirent.attr1, !is_binary);
+
memcpy(zip_dir + zip_dir_offset, dirent, ZIP_DIR_HEADER_SIZE);
zip_dir_offset += ZIP_DIR_HEADER_SIZE;
memcpy(zip_dir + zip_dir_offset, path, pathlen);
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index c929db5..14744b2 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -33,6 +33,37 @@ check_zip() {
test_expect_success UNZIP validate file contents
diff -r a ${dir_with_prefix}a
+
+ dir=eol_$1
+ dir_with_prefix=$dir/$2
+ extracted=${dir_with_prefix}a
+ original=a
+
+ test_expect_success UNZIP extract ZIP archive with EOL conversion '
+ (mkdir $dir cd $dir $GIT_UNZIP -a ../$zipfile)
+ '
+
+
[PATCH] archive-zip: add --text parameter
Entries in a ZIP file can be marked as text files. Extractors can use
that flag to apply end-of-line conversions. An example is unzip -a.
git archive currently marks all ZIP file entries as binary files. This
patch adds the new option --text that can be used to mark non-binary
files or all files as text files, thus enabling the use of unzip -a.
No sign-off, yet, because I'm not sure we really need another option.
E.g. --text=all doesn't seem to be actually useful, but it was easy to
implement. Info-ZIP's zip always creates archives like --text=auto
does, so perhaps we should make that our default behavior as well?
Changing the default behavior would cause newer versions of git archive
to create different ZIP files than older ones, of course. This can
break caching and signature checking.
The last time we did that was in 2012 when we added an extended mtime
field (227bf5980), I think. I don't remember any fallout from that
change, but there was a recent discussion about the stability of
generated tar files, so I'm a bit cautious:
http://thread.gmane.org/gmane.comp.version-control.git/258516
---
Documentation/git-archive.txt | 5
archive-zip.c | 23 ++
archive.c | 18 ++
archive.h | 7 ++
t/t5003-archive-zip.sh| 56 +++
5 files changed, 105 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index cfa1e4e..684ca36 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt
@@ -93,6 +93,11 @@ zip
Highest and slowest compression level. You can specify any
number from 1 to 9 to adjust compression speed and ratio.
+--text=which::
+ Mark the specfied entries as text files so that `unzip -a`
+ converts end-of-line characters while extracting. The value
+ must be either 'all', 'auto', or 'none' (the default).
+
CONFIGURATION
-
diff --git a/archive-zip.c b/archive-zip.c
index 4bde019..3767940 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -5,6 +5,7 @@
#include archive.h
#include streaming.h
#include utf8.h
+#include xdiff-interface.h
static int zip_date;
static int zip_time;
@@ -210,6 +211,7 @@ static int write_zip_entry(struct archiver_args *args,
struct git_istream *stream = NULL;
unsigned long flags = 0;
unsigned long size;
+ int is_binary = -1;
crc = crc32(0, NULL, 0);
@@ -238,8 +240,14 @@ static int write_zip_entry(struct archiver_args *args,
method = 0;
attr2 = S_ISLNK(mode) ? ((mode | 0777) 16) :
(mode 0111) ? ((mode) 16) : 0;
- if (S_ISREG(mode) args-compression_level != 0 size 0)
- method = 8;
+ if (S_ISREG(mode)) {
+ if (args-compression_level != 0 size 0)
+ method = 8;
+ if (args-text == ARCHIVE_TEXT_ALL)
+ is_binary = 0;
+ else if (args-text == ARCHIVE_TEXT_NONE)
+ is_binary = 1;
+ }
if (S_ISREG(mode) type == OBJ_BLOB !args-convert
size big_file_threshold) {
@@ -256,6 +264,8 @@ static int write_zip_entry(struct archiver_args *args,
return error(cannot read %s,
sha1_to_hex(sha1));
crc = crc32(crc, buffer, size);
+ if (is_binary 0)
+ is_binary = buffer_is_binary(buffer, size);
out = buffer;
}
compressed_size = (method == 0) ? size : 0;
@@ -300,7 +310,6 @@ static int write_zip_entry(struct archiver_args *args,
copy_le16(dirent.extra_length, ZIP_EXTRA_MTIME_SIZE);
copy_le16(dirent.comment_length, 0);
copy_le16(dirent.disk, 0);
- copy_le16(dirent.attr1, 0);
copy_le32(dirent.attr2, attr2);
copy_le32(dirent.offset, zip_offset);
@@ -328,6 +337,8 @@ static int write_zip_entry(struct archiver_args *args,
if (readlen = 0)
break;
crc = crc32(crc, buf, readlen);
+ if (is_binary 0)
+ is_binary = buffer_is_binary(buffer, size);
write_or_die(1, buf, readlen);
}
close_istream(stream);
@@ -361,6 +372,8 @@ static int write_zip_entry(struct archiver_args *args,
if (readlen = 0)
break;
crc = crc32(crc, buf, readlen);
+ if (is_binary 0)
+ is_binary = buffer_is_binary(buffer, size);
Re: zip files created with git archive flags text files as binaries
Am 23.02.2015 um 20:30 schrieb René Scharfe: Am 23.02.2015 um 14:58 schrieb Ulrike Fischer: The zip contained four text files and a pdf. The CTAN maintainers informed me that all files in the zip are flagged as binaries and this makes it difficult for them to process them further (they want to correct line feeds of the text files: http://mirror.ctan.org/tex-archive/help/ctan/CTAN-upload-addendum.html#crlf) By the way, a workaround for CTAN could be to extract the files using unzip and zip them again using Info-ZIP zip (one of the good zip programs mentioned on that website). The result will be a ZIP file with text files flagged appropriately. Just saying. Would it be possible to correct the zip-backend so that it flags text files correctly? Or alternativly could one configure git archive to use another zip programm? We would have to detect the line ending format (DOS, Unix, Macintosh, etc.) of each file, then set the attribute t (text) and the host system. The detection would slow down archive creation a bit and the resulting files would be different, of course, so this feature should only be enabled by a new command line option. I'll take a look. Actually its easier than that. unzip -a (with end-of-line conversion) doesn't care for the actual format, it simply converts all occurrences of CR, CRLF and LF to the appropriate newline chars for the platform it is running on if the text flag is set. Files with mixed line endings are normalized, i.e. they have a consistent end-of-line style afterward. I'll send a first patch shortly. 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: Copyright on wildmatch.c
Am 24.02.2015 um 13:34 schrieb Guilherme: This is just an email to all the people i have written in private about relicensing the files in need in TSS so they can reply to this email and it be recorded in the mailing list. The files are part of ctypes.c hex.c git-compat-util.h. On Tue, Feb 24, 2015 at 1:22 PM, Guilherme guibuf...@gmail.com wrote: Hello, I'm writing to you in regards to the files ctypes.c which you have modified part of in the git project. I'm currently working on integrating gitignore pattern matching into the_sivler_searcher(http://github.com/ggreer/the_silver_searcher). PR https://github.com/ggreer/the_silver_searcher/pull/614 For that i needed wildmatch.c and it's dependencies. That is parts of ctypes.c lines 8 to 31. Unfortunately TSS is Apache licensed wheras git is GPL, those licenses are incompatible and thus i ask if you agree that your portion if the code is also licensed under Apache2 for the use in TSS. That's fine with me. 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: zip files created with git archive flags text files as binaries
Am 23.02.2015 um 14:58 schrieb Ulrike Fischer: I'm using git on windows 7. $ git --version git version 1.9.4.msysgit.0 Git's code for ZIP file creation hasn't changed since then. Some days ago I uploaded a latex package to CTAN (www.ctan.org). I created the zip-file with git archive --format=zip --prefix=citeall/ --output=zip/citeall_2015-02-20.zip HEAD The zip contained four text files and a pdf. The CTAN maintainers informed me that all files in the zip are flagged as binaries and this makes it difficult for them to process them further (they want to correct line feeds of the text files: http://mirror.ctan.org/tex-archive/help/ctan/CTAN-upload-addendum.html#crlf) I wouldn't have expected that this ZIP file attribute is actually used in the wild. unzip -Z reports for my zip: $ unzip -Z citeall_2015_02_20.zip Archive: citeall_2015_02_20.zip Zip file size: 105509 bytes, number of entries: 6 drwx--- 0.0 fat0 bx stor 15-Feb-20 17:07 citeall/ -rw 0.0 fat 458 bx defN 15-Feb-20 17:07 citeall/README -rw 0.0 fat 102244 bx defN 15-Feb-20 17:07 citeall/citeall.pdf -rw 0.0 fat 3431 bx defN 15-Feb-20 17:07 citeall/citeall.sty -rw 0.0 fat 3971 bx defN 15-Feb-20 17:07 citeall/citeall.tex -rw 0.0 fat 557 bx defN 15-Feb-20 17:07 citeall/examples-citeall.bib 6 files, 110661 bytes uncompressed, 104669 bytes compressed: 5.4% The problem are all the bx entries. The x is due to the extra mtime header and unrelated to your issue. b (binary) is set unconditionally to ensure that line endings are kept intact by unpackers on all platforms. fat is used as lowest common denominator for regular files and directories; unx is used for symbolic links. When I zip all the files with the standard windows zip-tool I get this: $ unzip -Z citeall-win.zip Archive: citeall-win.zip Zip file size: 105275 bytes, number of entries: 5 -rw 2.0 fat 102244 b- defN 15-Feb-20 17:07 citeall/citeall.pdf -rw 2.0 fat 3431 t- defN 15-Feb-20 17:07 citeall/citeall.sty -rw 2.0 fat 3971 t- defN 15-Feb-20 17:07 citeall/citeall.tex -rw 2.0 fat 557 t- defN 15-Feb-20 17:07 citeall/examples-citeall.bib -rw 2.0 fat 458 t- defN 15-Feb-20 17:07 citeall/README 5 files, 110661 bytes uncompressed, 104675 bytes compressed: 5.4% Here the text files have a correct t flag. I don't know if it the problem exists also with zips created with git archive on non-windows OS. Yes, git archive creates the same ZIP files everywhere. Would it be possible to correct the zip-backend so that it flags text files correctly? Or alternativly could one configure git archive to use another zip programm? We would have to detect the line ending format (DOS, Unix, Macintosh, etc.) of each file, then set the attribute t (text) and the host system. The detection would slow down archive creation a bit and the resulting files would be different, of course, so this feature should only be enabled by a new command line option. I'll take a look. 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] sha1_name: use strlcpy() to copy strings
Am 22.02.2015 um 21:00 schrieb Junio C Hamano: René Scharfe l@web.de writes: Use strlcpy() instead of calling strncpy() and then setting the last byte of the target buffer to NUL explicitly. This shortens and simplifies the code a bit. Thanks. It makes me wonder if the longer term direction should be not to use a bound buffer for oc-path, though. That's a good idea in general, but a bit more involved since we'd need to introduce a cleanup function that releases the memory allocated by the new version of get_sha1_with_context() first and call it from the appropriate places. Would that be a good micro-project for GSoC or is it too simple? 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] for-each-ref: use skip_prefix() to avoid duplicate string comparison
Use skip_prefix() to get the part after color: (if present) and only
compare it with reset instead of comparing the whole string again.
This gets rid of the duplicate color: part of the string constant.
Signed-off-by: Rene Scharfe l@web.de
---
builtin/for-each-ref.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 19be78a..83f9cf9 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -178,11 +178,10 @@ static const char *find_next(const char *cp)
static int verify_format(const char *format)
{
const char *cp, *sp;
- static const char color_reset[] = color:reset;
need_color_reset_at_eol = 0;
for (cp = format; *cp (sp = find_next(cp)); ) {
- const char *ep = strchr(sp, ')');
+ const char *color, *ep = strchr(sp, ')');
int at;
if (!ep)
@@ -191,8 +190,8 @@ static int verify_format(const char *format)
at = parse_atom(sp + 2, ep);
cp = ep + 1;
- if (starts_with(used_atom[at], color:))
- need_color_reset_at_eol = !!strcmp(used_atom[at],
color_reset);
+ if (skip_prefix(used_atom[at], color:, color))
+ need_color_reset_at_eol = !!strcmp(color, reset);
}
return 0;
}
--
2.3.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] pretty: use starts_with() to check for a prefix
Simplify the code and avoid duplication by using starts_with() instead
of strlen() and strncmp() to check if a line starts with encoding .
Signed-off-by: Rene Scharfe l@web.de
---
pretty.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pretty.c b/pretty.c
index 9d34d02..7b49304 100644
--- a/pretty.c
+++ b/pretty.c
@@ -567,7 +567,7 @@ static char *replace_encoding_header(char *buf, const char
*encoding)
char *cp = buf;
/* guess if there is an encoding header before a \n\n */
- while (strncmp(cp, encoding , strlen(encoding ))) {
+ while (!starts_with(cp, encoding )) {
cp = strchr(cp, '\n');
if (!cp || *++cp == '\n')
return buf;
--
2.3.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: use strlcpy() to copy strings
Use strlcpy() instead of calling strncpy() and then setting the last byte of the target buffer to NUL explicitly. This shortens and simplifies the code a bit. Signed-of-by: Rene Scharfe l@web.de --- sha1_name.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/sha1_name.c b/sha1_name.c index cf2a83b..95f9f8f 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -1391,9 +1391,7 @@ static int get_sha1_with_context_1(const char *name, namelen = strlen(cp); } - strncpy(oc-path, cp, - sizeof(oc-path)); - oc-path[sizeof(oc-path)-1] = '\0'; + strlcpy(oc-path, cp, sizeof(oc-path)); if (!active_cache) read_cache(); @@ -1443,9 +1441,7 @@ static int get_sha1_with_context_1(const char *name, name, len); } hashcpy(oc-tree, tree_sha1); - strncpy(oc-path, filename, - sizeof(oc-path)); - oc-path[sizeof(oc-path)-1] = '\0'; + strlcpy(oc-path, filename, sizeof(oc-path)); free(new_filename); return ret; -- 2.3.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] connect: use strcmp() for string comparison
Get rid of magic string length constants and simply compare the strings
using strcmp(). This makes the intent of the code a bit clearer.
Signed-off-by: Rene Scharfe l@web.de
---
connect.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/connect.c b/connect.c
index 062e133..2a5c400 100644
--- a/connect.c
+++ b/connect.c
@@ -157,8 +157,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t
src_len,
server_capabilities = xstrdup(name + name_len + 1);
}
- if (extra_have
- name_len == 5 !memcmp(.have, name, 5)) {
+ if (extra_have !strcmp(name, .have)) {
sha1_array_append(extra_have, old_sha1);
continue;
}
--
2.3.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 1/2] daemon: look up client-supplied hostname lazily
Look up canonical hostname and IP address using getaddrinfo(3) or
gethostbyname(3) only if --interpolated-path or --access-hook were
specified.
Do that by introducing getter functions for canon_hostname and
ip_address and using them for all read accesses. These wrappers call
the new helper lookup_hostname(), which sets the variables only at its
first call.
Signed-off-by: Rene Scharfe l@web.de
---
daemon.c | 37 +
1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/daemon.c b/daemon.c
index 54a03bd..ef41943 100644
--- a/daemon.c
+++ b/daemon.c
@@ -61,6 +61,22 @@ static char *canon_hostname;
static char *ip_address;
static char *tcp_port;
+static int hostname_lookup_done;
+
+static void lookup_hostname(void);
+
+static const char *get_canon_hostname(void)
+{
+ lookup_hostname();
+ return canon_hostname;
+}
+
+static const char *get_ip_address(void)
+{
+ lookup_hostname();
+ return ip_address;
+}
+
static void logreport(int priority, const char *err, va_list params)
{
if (log_syslog) {
@@ -147,8 +163,8 @@ static const char *path_ok(const char *directory)
struct strbuf_expand_dict_entry dict[6];
dict[0].placeholder = H; dict[0].value = hostname;
- dict[1].placeholder = CH; dict[1].value = canon_hostname;
- dict[2].placeholder = IP; dict[2].value = ip_address;
+ dict[1].placeholder = CH; dict[1].value =
get_canon_hostname();
+ dict[2].placeholder = IP; dict[2].value = get_ip_address();
dict[3].placeholder = P; dict[3].value = tcp_port;
dict[4].placeholder = D; dict[4].value = directory;
dict[5].placeholder = NULL; dict[5].value = NULL;
@@ -254,8 +270,8 @@ static int run_access_hook(struct daemon_service *service,
const char *dir, cons
*arg++ = service-name;
*arg++ = path;
*arg++ = STRARG(hostname);
- *arg++ = STRARG(canon_hostname);
- *arg++ = STRARG(ip_address);
+ *arg++ = STRARG(get_canon_hostname());
+ *arg++ = STRARG(get_ip_address());
*arg++ = STRARG(tcp_port);
*arg = NULL;
#undef STRARG
@@ -509,6 +525,7 @@ static void parse_host_arg(char *extra_args, int buflen)
}
free(hostname);
hostname = xstrdup_tolower(host);
+ hostname_lookup_done = 0;
}
/* On to the next one */
@@ -517,11 +534,14 @@ static void parse_host_arg(char *extra_args, int buflen)
if (extra_args end *extra_args)
die(Invalid request);
}
+}
- /*
-* Locate canonical hostname and its IP address.
-*/
- if (hostname) {
+/*
+ * Locate canonical hostname and its IP address.
+ */
+static void lookup_hostname(void)
+{
+ if (!hostname_lookup_done hostname) {
#ifndef NO_IPV6
struct addrinfo hints;
struct addrinfo *ai;
@@ -569,6 +589,7 @@ static void parse_host_arg(char *extra_args, int buflen)
ip_address = xstrdup(addrbuf);
}
#endif
+ hostname_lookup_done = 1;
}
}
--
2.3.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] .clang-format: introduce the use of clang-format
Am 17.01.2015 um 22:30 schrieb Ramkumar Ramachandra:
Instead of manually eyeballing style in reviews, just ask all
contributors to run their patches through [git-]clang-format.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
The idea is to introduce the community to this new toy I found called
clang-format. Whether or not it's actually going to be used doesn't
bother me too much.
I'm not 100% sure of the style, but I'll leave you to tweak that
using http://clang.llvm.org/docs/ClangFormatStyleOptions.html
The current code isn't terribly conformant, but I suppose that'll
change with time.
.clang-format | 7 +++
1 file changed, 7 insertions(+)
create mode 100644 .clang-format
diff --git a/.clang-format b/.clang-format
new file mode 100644
index 000..63a53e0
--- /dev/null
+++ b/.clang-format
@@ -0,0 +1,7 @@
+BasedOnStyle: LLVM
+IndentWidth: 8
+UseTab: Always
+BreakBeforeBraces: Linux
+AllowShortBlocksOnASingleLine: false
+AllowShortIfStatementsOnASingleLine: false
+IndentCaseLabels: false
\ No newline at end of file
Why no newline on the last line?
These one would be needed as well to match our style, I think:
AllowShortFunctionsOnASingleLine: None
ContinuationIndentWidth: 8
And probably this one:
Cpp11BracedListStyle: false
However, even then struct declarations that are combined with variable
declaration and initialization get mangled:
struct a {
int n;
const char *s;
} arr[] = {
{ 1, one },
{ 2, two }
};
becomes:
struct a
{
int n;
const char *s;
} arr[] = { { 1, one }, { 2, two } };
It gets formatted better if arr is declared separately.
And this one helps get rid of the added line break between struct a and
the following brace:
BreakBeforeBraces: Stroustrup
--
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] remote: Remove -v/--verbose option from git remote show synopsis
Am 08.01.2015 um 18:57 schrieb Alexander Kuleshov:
git remote show doesn't use -v/--verbose option
Hmm, but it does?
$ git version
git version 2.2.1
$ git remote show
origin
$ git remote -v show
origin git://git.kernel.org/pub/scm/git/git.git (fetch)
origin git://git.kernel.org/pub/scm/git/git.git (push)
Perhaps you meant the following variant? The changed line documents the
one above, though (-v before show).
$ git remote show -v
error: unknown switch `v'
usage: git remote show [options] name
-ndo not query remotes
Signed-off-by: Alexander Kuleshov kuleshovm...@gmail.com
---
builtin/remote.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 46ecfd9..978c645 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -14,7 +14,7 @@ static const char * const builtin_remote_usage[] = {
N_(git remote rename old new),
N_(git remote remove name),
N_(git remote set-head name (-a | --auto | -d | --delete
|branch)),
- N_(git remote [-v | --verbose] show [-n] name),
+ N_(git remote show [-n] name),
N_(git remote prune [-n | --dry-run] name),
N_(git remote [-v | --verbose] update [-p | --prune] [(group |
remote)...]),
N_(git remote set-branches [--add] name branch...),
--
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: release strbuf after use in suggest_conflicts()
Signed-off-by: Rene Scharfe l@web.de --- builtin/merge.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/merge.c b/builtin/merge.c index 215d485..d722889 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -894,6 +894,7 @@ static int suggest_conflicts(void) append_conflicts_hint(msgbuf); fputs(msgbuf.buf, fp); + strbuf_release(msgbuf); fclose(fp); rerere(allow_rerere_auto); printf(_(Automatic merge failed; -- 2.2.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] commit-tree: simplify parsing of option -S using skip_prefix()
Signed-off-by: Rene Scharfe l@web.de
---
builtin/commit-tree.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 8a66c74..25aa2cd 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -66,10 +66,8 @@ int cmd_commit_tree(int argc, const char **argv, const char
*prefix)
continue;
}
- if (!memcmp(arg, -S, 2)) {
- sign_commit = arg + 2;
+ if (skip_prefix(arg, -S, sign_commit))
continue;
- }
if (!strcmp(arg, --no-gpg-sign)) {
sign_commit = NULL;
--
2.2.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] transport: simplify duplicating a substring in transport_get() using xmemdupz()
Signed-off-by: Rene Scharfe l@web.de
---
transport.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/transport.c b/transport.c
index 70d38e4..08bcd3a 100644
--- a/transport.c
+++ b/transport.c
@@ -971,9 +971,7 @@ struct transport *transport_get(struct remote *remote,
const char *url)
} else {
/* Unknown protocol in URL. Pass to external handler. */
int len = external_specification_len(url);
- char *handler = xmalloc(len + 1);
- handler[len] = 0;
- strncpy(handler, url, len);
+ char *handler = xmemdupz(url, len);
transport_helper_init(ret, handler);
}
--
2.2.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] refs: release strbuf after use in check_refname_component()
Signed-off-by: Rene Scharfe l@web.de --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 5fcacc6..ed3b2cb 100644 --- a/refs.c +++ b/refs.c @@ -2334,7 +2334,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, struct strbuf err = STRBUF_INIT; unable_to_lock_message(ref_file, errno, err); error(%s, err.buf); - strbuf_reset(err); + strbuf_release(err); goto error_return; } } -- 2.2.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] use strbuf_complete_line() for adding a newline if needed
Call strbuf_complete_line() instead of open-coding it. Also remove
surrounding comments indicating the intent to complete a line since
this information is already included in the function name.
Signed-off-by: Rene Scharfe l@web.de
---
builtin/fmt-merge-msg.c | 3 +--
notes-utils.c | 3 +--
trace.c | 4 +---
3 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 37177c6..af7919e 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -216,8 +216,7 @@ static void add_branch_desc(struct strbuf *out, const char
*name)
strbuf_addf(out, : %.*s, (int)(ep - bp), bp);
bp = ep;
}
- if (out-buf[out-len - 1] != '\n')
- strbuf_addch(out, '\n');
+ strbuf_complete_line(out);
}
strbuf_release(desc);
}
diff --git a/notes-utils.c b/notes-utils.c
index b64dc1b..ccbf073 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -44,8 +44,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
/* Prepare commit message and reflog message */
strbuf_addstr(buf, msg);
- if (buf.buf[buf.len - 1] != '\n')
- strbuf_addch(buf, '\n'); /* Make sure msg ends with newline */
+ strbuf_complete_line(buf);
create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);
strbuf_insert(buf, 0, notes: , 7); /* commit message starts at index
7 */
diff --git a/trace.c b/trace.c
index 4778608..f6f9f3a 100644
--- a/trace.c
+++ b/trace.c
@@ -122,9 +122,7 @@ static int prepare_trace_line(const char *file, int line,
static void print_trace_line(struct trace_key *key, struct strbuf *buf)
{
- /* append newline if missing */
- if (buf-len buf-buf[buf-len - 1] != '\n')
- strbuf_addch(buf, '\n');
+ strbuf_complete_line(buf);
write_or_whine_pipe(get_trace_fd(key), buf-buf, buf-len, err_msg);
strbuf_release(buf);
--
2.2.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] use labs() for variables of type long instead of abs()
Using abs() on long values can cause truncation, so use labs() instead.
Reported by Clang 3.5 (-Wabsolute-value, enabled by -Wall).
Signed-off-by: Rene Scharfe l@web.de
---
builtin/receive-pack.c | 2 +-
config.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32fc540..e908d07 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -431,7 +431,7 @@ static const char *check_nonce(const char *buf, size_t len)
nonce_stamp_slop = (long)ostamp - (long)stamp;
if (nonce_stamp_slop_limit
- abs(nonce_stamp_slop) = nonce_stamp_slop_limit) {
+ labs(nonce_stamp_slop) = nonce_stamp_slop_limit) {
/*
* Pretend as if the received nonce (which passes the
* HMAC check, so it is not a forged by third-party)
diff --git a/config.c b/config.c
index 15a2983..ae1398f 100644
--- a/config.c
+++ b/config.c
@@ -506,9 +506,9 @@ static int git_parse_signed(const char *value, intmax_t
*ret, intmax_t max)
errno = EINVAL;
return 0;
}
- uval = abs(val);
+ uval = labs(val);
uval *= factor;
- if (uval max || abs(val) uval) {
+ if (uval max || labs(val) uval) {
errno = ERANGE;
return 0;
}
--
2.1.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] run-command: use void to declare that functions take no parameters
Explicitly declare that git_atexit_dispatch() and git_atexit_clear()
take no parameters instead of leaving their parameter list empty and
thus unspecified.
Signed-off-by: Rene Scharfe l@web.de
---
run-command.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/run-command.c b/run-command.c
index 79a0a76..a476999 100644
--- a/run-command.c
+++ b/run-command.c
@@ -636,7 +636,7 @@ static struct {
static int git_atexit_installed;
-static void git_atexit_dispatch()
+static void git_atexit_dispatch(void)
{
size_t i;
@@ -644,7 +644,7 @@ static void git_atexit_dispatch()
git_atexit_hdlrs.handlers[i-1]();
}
-static void git_atexit_clear()
+static void git_atexit_clear(void)
{
free(git_atexit_hdlrs.handlers);
memset(git_atexit_hdlrs, 0, sizeof(git_atexit_hdlrs));
--
2.1.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] use child_process_init() to initialize struct child_process variables
Am 29.10.2014 um 18:21 schrieb Jeff King:
On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote:
diff --git a/trailer.c b/trailer.c
index 8514566..7ff036c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -237,7 +237,7 @@ static const char *apply_command(const char *command,
const char *arg)
strbuf_replace(cmd, TRAILER_ARG_STRING, arg);
argv[0] = cmd.buf;
-memset(cp, 0, sizeof(cp));
+child_process_init(cp);
cp.argv = argv;
cp.env = local_repo_env;
cp.no_stdin = 1;
I think this one can use CHILD_PROCESS_INIT in the declaration. I guess
it is debatable whether that is actually preferable, but I tend to think
it is cleaner and less error-prone.
Agreed, thanks.
-- 8 --
Subject: [PATCH] trailer: use CHILD_PROCESS_INIT in apply_command()
Initialize the struct child_process variable cp at declaration time.
This is shorter, saves a function call and prevents using the variable
before initialization by mistake.
Suggested-by: Jeff King p...@peff.net
Signed-off-by: Rene Scharfe l@web.de
---
trailer.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/trailer.c b/trailer.c
index 7ff036c..6ae7865 100644
--- a/trailer.c
+++ b/trailer.c
@@ -228,7 +228,7 @@ static const char *apply_command(const char *command, const
char *arg)
{
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
- struct child_process cp;
+ struct child_process cp = CHILD_PROCESS_INIT;
const char *argv[] = {NULL, NULL};
const char *result;
@@ -237,7 +237,6 @@ static const char *apply_command(const char *command, const
char *arg)
strbuf_replace(cmd, TRAILER_ARG_STRING, arg);
argv[0] = cmd.buf;
- child_process_init(cp);
cp.argv = argv;
cp.env = local_repo_env;
cp.no_stdin = 1;
--
2.1.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 2/2] use env_array member of struct child_process
Am 20.10.2014 um 11:19 schrieb Jeff King:
On Sun, Oct 19, 2014 at 01:14:20PM +0200, René Scharfe wrote:
--- a/wt-status.c
+++ b/wt-status.c
@@ -726,14 +726,14 @@ static void wt_status_print_changed(struct wt_status
*s)
static void wt_status_print_submodule_summary(struct wt_status *s, int
uncommitted)
{
struct child_process sm_summary = CHILD_PROCESS_INIT;
-struct argv_array env = ARGV_ARRAY_INIT;
struct argv_array argv = ARGV_ARRAY_INIT;
I don't think it belongs in this patch, but a follow-on 3/2 might be to
give argv the same treatment.
Yes, good idea.
-- 8 --
Subject: [PATCH] use args member of struct child_process
Convert users of struct child_process to using the managed argv_array
args instead of providing their own. This shortens the code a bit and
ensures that the allocated memory is released automatically after use.
Suggested-by: Jeff King p...@peff.net
Signed-off-by: Rene Scharfe l@web.de
---
builtin/repack.c | 47 ++-
wt-status.c | 17 +++--
2 files changed, 29 insertions(+), 35 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 2845620..83e91c7 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -135,7 +135,6 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
};
struct child_process cmd = CHILD_PROCESS_INIT;
struct string_list_item *item;
- struct argv_array cmd_args = ARGV_ARRAY_INIT;
struct string_list names = STRING_LIST_INIT_DUP;
struct string_list rollback = STRING_LIST_INIT_NODUP;
struct string_list existing_packs = STRING_LIST_INIT_DUP;
@@ -202,56 +201,55 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
sigchain_push_common(remove_pack_on_signal);
- argv_array_push(cmd_args, pack-objects);
- argv_array_push(cmd_args, --keep-true-parents);
+ argv_array_push(cmd.args, pack-objects);
+ argv_array_push(cmd.args, --keep-true-parents);
if (!pack_kept_objects)
- argv_array_push(cmd_args, --honor-pack-keep);
- argv_array_push(cmd_args, --non-empty);
- argv_array_push(cmd_args, --all);
- argv_array_push(cmd_args, --reflog);
- argv_array_push(cmd_args, --indexed-objects);
+ argv_array_push(cmd.args, --honor-pack-keep);
+ argv_array_push(cmd.args, --non-empty);
+ argv_array_push(cmd.args, --all);
+ argv_array_push(cmd.args, --reflog);
+ argv_array_push(cmd.args, --indexed-objects);
if (window)
- argv_array_pushf(cmd_args, --window=%s, window);
+ argv_array_pushf(cmd.args, --window=%s, window);
if (window_memory)
- argv_array_pushf(cmd_args, --window-memory=%s,
window_memory);
+ argv_array_pushf(cmd.args, --window-memory=%s,
window_memory);
if (depth)
- argv_array_pushf(cmd_args, --depth=%s, depth);
+ argv_array_pushf(cmd.args, --depth=%s, depth);
if (max_pack_size)
- argv_array_pushf(cmd_args, --max-pack-size=%s,
max_pack_size);
+ argv_array_pushf(cmd.args, --max-pack-size=%s,
max_pack_size);
if (no_reuse_delta)
- argv_array_pushf(cmd_args, --no-reuse-delta);
+ argv_array_pushf(cmd.args, --no-reuse-delta);
if (no_reuse_object)
- argv_array_pushf(cmd_args, --no-reuse-object);
+ argv_array_pushf(cmd.args, --no-reuse-object);
if (write_bitmaps)
- argv_array_push(cmd_args, --write-bitmap-index);
+ argv_array_push(cmd.args, --write-bitmap-index);
if (pack_everything ALL_INTO_ONE) {
get_non_kept_pack_filenames(existing_packs);
if (existing_packs.nr delete_redundant) {
if (unpack_unreachable)
- argv_array_pushf(cmd_args,
+ argv_array_pushf(cmd.args,
--unpack-unreachable=%s,
unpack_unreachable);
else if (pack_everything LOOSEN_UNREACHABLE)
- argv_array_push(cmd_args,
+ argv_array_push(cmd.args,
--unpack-unreachable);
}
} else {
- argv_array_push(cmd_args, --unpacked);
- argv_array_push(cmd_args, --incremental);
+ argv_array_push(cmd.args, --unpacked);
+ argv_array_push(cmd.args, --incremental);
}
if (local)
- argv_array_push(cmd_args, --local);
+ argv_array_push(cmd.args, --local);
if (quiet)
- argv_array_push(cmd_args, --quiet);
+ argv_array_push(cmd.args, --quiet);
if (delta_base_offset
Re: [PATCH][RFC] grep: add color.grep.matchcontext and color.grep.matchselected
Am 28.10.2014 um 00:32 schrieb Zoltan Klinger:
I like René's approach, too. It's more flexible, supports the old
behaviour and it scratches my itch as well.
Don't mind if you dropped my patch and used René's instead.
Good. :) And here's the t/ part of your patch, slightly changed to
exercise the new config options.
---
t/t7810-grep.sh | 93 +
1 file changed, 93 insertions(+)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 40615de..5d3e161 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1202,4 +1202,97 @@ test_expect_success LIBPCRE 'grep -P ^ ' '
test_cmp expected actual
'
+cat expected EOF
+space-line without leading space1
+space: line REDwith RESETleading space1
+space: line REDwith RESETleading REDspace2RESET
+space: line REDwith RESETleading space3
+space:line without leading REDspace2RESET
+EOF
+
+test_expect_success 'grep --color -e A -e B with context' '
+ test_config color.grep.context normal
+ test_config color.grep.filename normal
+ test_config color.grep.function normal
+ test_config color.grep.linenumber normal
+ test_config color.grep.matchred
+ test_config color.grep.selected normal
+ test_config color.grep.separatornormal
+
+ git grep --color=always -C2 -e with -e space2 space |
+ test_decode_color actual
+ test_cmp expected actual
+'
+
+cat expected EOF
+space-line without leading space1
+space- line GREENwith RESETleading space1
+space: line REDwith RESETleading REDspace2RESET
+space- line GREENwith RESETleading space3
+space-line without leading GREENspace2RESET
+EOF
+
+test_expect_success 'grep --color -e A --and -e B with context' '
+ test_config color.grep.context normal
+ test_config color.grep.filename normal
+ test_config color.grep.function normal
+ test_config color.grep.linenumber normal
+ test_config color.grep.matchContext green
+ test_config color.grep.matchSelectedred
+ test_config color.grep.selected normal
+ test_config color.grep.separatornormal
+
+ git grep --color=always -C2 -e with --and -e space2 space |
+ test_decode_color actual
+ test_cmp expected actual
+'
+
+cat expected EOF
+space-line without leading space1
+space: line REDwith RESETleading space1
+space- line GREENwith RESETleading GREENspace2RESET
+space: line REDwith RESETleading space3
+space-line without leading GREENspace2RESET
+EOF
+
+test_expect_success 'grep --color -e A --and --not -e B with context' '
+ test_config color.grep.context normal
+ test_config color.grep.filename normal
+ test_config color.grep.function normal
+ test_config color.grep.linenumber normal
+ test_config color.grep.matchContext green
+ test_config color.grep.matchSelectedred
+ test_config color.grep.selected normal
+ test_config color.grep.separatornormal
+
+ git grep --color=always -C2 -e with --and --not -e space2 space |
+ test_decode_color actual
+ test_cmp expected actual
+'
+
+cat expected EOF
+hello.c-#include stdio.h
+hello.c=GREENintRESET main(GREENintRESET argc, const char **argv)
+hello.c-{
+hello.c: prREDintRESETf(REDHelloRESET world.\n);
+hello.c- return 0;
+hello.c- /* char ?? */
+hello.c-}
+EOF
+
+test_expect_success 'grep --color -e A --and -e B -p with context' '
+ test_config color.grep.context normal
+ test_config color.grep.filename normal
+ test_config color.grep.function normal
+ test_config color.grep.linenumber normal
+ test_config color.grep.matchContext green
+ test_config color.grep.matchSelectedred
+ test_config color.grep.selected normal
+ test_config color.grep.separatornormal
+
+ git grep --color=always -p -C3 -e int --and -e Hello --no-index hello.c
|
+ test_decode_color actual
+ test_cmp expected actual
+'
+
test_done
--
2.1.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] receive-pack: avoid minor leak in case start_async() fails
If the asynchronous start of copy_to_sideband() fails, then any
env_array entries added to struct child_process proc by
prepare_push_cert_sha1() are leaked. Call the latter function only
after start_async() succeeded so that the allocated entries are
cleaned up automatically by start_command() or finish_command().
Signed-off-by: Rene Scharfe l@web.de
---
builtin/receive-pack.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index fc03937..32fc540 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -527,8 +527,6 @@ static int run_and_feed_hook(const char *hook_name, feed_fn
feed, void *feed_sta
proc.in = -1;
proc.stdout_to_stderr = 1;
- prepare_push_cert_sha1(proc);
-
if (use_sideband) {
memset(muxer, 0, sizeof(muxer));
muxer.proc = copy_to_sideband;
@@ -539,6 +537,8 @@ static int run_and_feed_hook(const char *hook_name, feed_fn
feed, void *feed_sta
proc.err = muxer.in;
}
+ prepare_push_cert_sha1(proc);
+
code = start_command(proc);
if (code) {
if (use_sideband)
--
2.1.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] use child_process_init() to initialize struct child_process variables
Call child_process_init() instead of zeroing the memory of variables of type struct child_process by hand before use because the former is both clearer and shorter. Signed-off-by: Rene Scharfe l@web.de --- bundle.c | 2 +- column.c | 2 +- trailer.c | 2 +- transport-helper.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bundle.c b/bundle.c index fa67057..c846092 100644 --- a/bundle.c +++ b/bundle.c @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path, write_or_die(bundle_fd, \n, 1); /* write pack */ - memset(rls, 0, sizeof(rls)); + child_process_init(rls); argv_array_pushl(rls.args, pack-objects, --all-progress-implied, --stdout, --thin, --delta-base-offset, diff --git a/column.c b/column.c index 8082a94..786abe6 100644 --- a/column.c +++ b/column.c @@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts) if (fd_out != -1) return -1; - memset(column_process, 0, sizeof(column_process)); + child_process_init(column_process); argv = column_process.args; argv_array_push(argv, column); diff --git a/trailer.c b/trailer.c index 8514566..7ff036c 100644 --- a/trailer.c +++ b/trailer.c @@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg) strbuf_replace(cmd, TRAILER_ARG_STRING, arg); argv[0] = cmd.buf; - memset(cp, 0, sizeof(cp)); + child_process_init(cp); cp.argv = argv; cp.env = local_repo_env; cp.no_stdin = 1; diff --git a/transport-helper.c b/transport-helper.c index 6cd9dd1..0224687 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -414,7 +414,7 @@ static int get_exporter(struct transport *transport, struct child_process *helper = get_helper(transport); int i; - memset(fastexport, 0, sizeof(*fastexport)); + child_process_init(fastexport); /* we need to duplicate helper-in because we want to use it after * fastexport is done with it. */ -- 2.1.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] api-run-command: add missing list item marker
Signed-off-by: Rene Scharfe l@web.de --- Documentation/technical/api-run-command.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt index 3f12fcd..a9fdb45 100644 --- a/Documentation/technical/api-run-command.txt +++ b/Documentation/technical/api-run-command.txt @@ -13,7 +13,7 @@ produces in the caller in order to process it. Functions - -`child_process_init` +`child_process_init`:: Initialize a struct child_process variable. -- 2.1.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][RFC] grep: add color.grep.matchcontext and color.grep.matchselected
The config option color.grep.match can be used to specify the highlighting
color for matching strings. Add the options matchContext and matchSelected
to allow different colors to be specified for matching strings in the
context vs. in selected lines. This is similar to the ms and mc specifiers
in GNU grep's environment variable GREP_COLORS.
Signed-off-by: Rene Scharfe l@web.de
---
Only *very* lightly tested, and a test for t/is missing anyway. Just
wanted to quickly show what I meant. You'd set color.grep.matchContext=
to turn off highlighting in context lines. What do you think?
Documentation/config.txt | 6 +-
grep.c | 29 ++---
grep.h | 3 ++-
3 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8b49813..78832ae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -885,7 +885,11 @@ color.grep.slot::
`linenumber`;;
line number prefix (when using `-n`)
`match`;;
- matching text
+ matching text (same as setting `matchContext` and `matchSelected`)
+`matchContext`;;
+ matching text in context lines
+`matchSelected`;;
+ matching text in selected lines
`selected`;;
non-matching text in selected lines
`separator`;;
diff --git a/grep.c b/grep.c
index 4dc31ea..6e085f8 100644
--- a/grep.c
+++ b/grep.c
@@ -35,7 +35,8 @@ void init_grep_defaults(void)
strcpy(opt-color_filename, );
strcpy(opt-color_function, );
strcpy(opt-color_lineno, );
- strcpy(opt-color_match, GIT_COLOR_BOLD_RED);
+ strcpy(opt-color_match_context, GIT_COLOR_BOLD_RED);
+ strcpy(opt-color_match_selected, GIT_COLOR_BOLD_RED);
strcpy(opt-color_selected, );
strcpy(opt-color_sep, GIT_COLOR_CYAN);
opt-color = -1;
@@ -101,12 +102,22 @@ int grep_config(const char *var, const char *value, void
*cb)
color = opt-color_function;
else if (!strcmp(var, color.grep.linenumber))
color = opt-color_lineno;
- else if (!strcmp(var, color.grep.match))
- color = opt-color_match;
+ else if (!strcmp(var, color.grep.matchcontext))
+ color = opt-color_match_context;
+ else if (!strcmp(var, color.grep.matchselected))
+ color = opt-color_match_selected;
else if (!strcmp(var, color.grep.selected))
color = opt-color_selected;
else if (!strcmp(var, color.grep.separator))
color = opt-color_sep;
+ else if (!strcmp(var, color.grep.match)) {
+ int rc = 0;
+ if (!value)
+ return config_error_nonbool(var);
+ rc |= color_parse(value, opt-color_match_context);
+ rc |= color_parse(value, opt-color_match_selected);
+ return rc;
+ }
if (color) {
if (!value)
@@ -144,7 +155,8 @@ void grep_init(struct grep_opt *opt, const char *prefix)
strcpy(opt-color_filename, def-color_filename);
strcpy(opt-color_function, def-color_function);
strcpy(opt-color_lineno, def-color_lineno);
- strcpy(opt-color_match, def-color_match);
+ strcpy(opt-color_match_context, def-color_match_context);
+ strcpy(opt-color_match_selected, def-color_match_selected);
strcpy(opt-color_selected, def-color_selected);
strcpy(opt-color_sep, def-color_sep);
}
@@ -1084,7 +1096,7 @@ static void show_line(struct grep_opt *opt, char *bol,
char *eol,
const char *name, unsigned lno, char sign)
{
int rest = eol - bol;
- char *line_color = NULL;
+ const char *match_color, *line_color = NULL;
if (opt-file_break opt-last_shown == 0) {
if (opt-show_hunk_mark)
@@ -1123,6 +1135,10 @@ static void show_line(struct grep_opt *opt, char *bol,
char *eol,
int eflags = 0;
if (sign == ':')
+ match_color = opt-color_match_selected;
+ else
+ match_color = opt-color_match_context;
+ if (sign == ':')
line_color = opt-color_selected;
else if (sign == '-')
line_color = opt-color_context;
@@ -1135,8 +1151,7 @@ static void show_line(struct grep_opt *opt, char *bol,
char *eol,
output_color(opt, bol, match.rm_so, line_color);
output_color(opt, bol + match.rm_so,
-match.rm_eo - match.rm_so,
-opt-color_match);
+match.rm_eo - match.rm_so, match_color);
bol += match.rm_eo;
rest -= match.rm_eo;
eflags = REG_NOTBOL;
diff --git a/grep.h b/grep.h
index eaaced1..95f197a 100644
--- a/grep.h
+++ b/grep.h
Re: [PATCH] grep: fix match highlighting for combined patterns with context lines
Am 21.10.2014 um 07:56 schrieb Zoltan Klinger: When git grep is run with combined patterns such as '-e p1 --and -e p2' and surrounding context lines are requested, the output contains incorrectly highlighted matches. Consider the following output (highlighted matches are surrounded by '*' characters): $ cat testfile foo a foo b foo bar baz bar foo bar x bar y $ git grep -n -C2 -e foo --and -e bar testfile testfile-1-*foo* a testfile-2-*foo* b testfile:3:*foo* *bar* testfile:4:baz *bar* *foo* testfile-5-*bar* x testfile-6-*bar* y Lines 1, 2, 5 and 6 do not match the combined patterns, they only contain incorrectly highlighted 'false positives'. The old code highlights all search terms, anywhere. I wouldn't call the ones in the context lines false positives. The user might be interested in those occurrences as well (I know I am ;). GNU grep allows coloring to be configured in much greater detail with its GREP_COLORS variable. I didn't think that level of tuning is desirable until now. What your patch does is equivalent to change the default of ms=01;31:mc=01;31 (color matching string in selected lines and context lines) to ms=01;31:mc= (color matching string in selected lines). The difference is only visible with -v or git grep's --not and --and. So, if you really don't want matching string in context lines to be colored, perhaps it's time to add a color.grep.contextmatch for matching text in context lines? 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: Sources for 3.18-rc1 not uploaded
Am 23.10.2014 um 03:09 schrieb brian m. carlson: On Wed, Oct 22, 2014 at 11:42:48AM +0200, Michael J Gruber wrote: Junio C Hamano schrieb am 21.10.2014 um 20:14: Michael J Gruber g...@drmicha.warpmail.net writes: Unfortunately, the git archive doc clearly says that the umask is applied to all archive entries. Is an extended pax header an archive entry? I doubt it, and the above is not relevant. The mode bits for the archive entry that it applies to does not come from there. The problem seem to be old tar versions which mis-take the extensions for archive entries, aren't they? Yes. POSIX isn't clear on how unknown entries are to be handled. I've seen some Windows tar implementations extract GNU longlink extensions as files, which leads to a lot of pain. That's by design -- extended headers are meant to be extracted as plain files by implementations that do not understand them. http://pubs.opengroup.org/onlinepubs/009695399/utilities/pax.html says: If a particular implementation does not recognize the type, or the user does not have appropriate privilege to create that type, the file shall be extracted as if it were a regular file if the file type is defined to have a meaning for the size field that could cause data logical records to be written on the medium [...]. My question to Brian still stands which existing users he was trying to cater for with his patch. If there indeed are no existing affected users besides the KUP users (as you seem to assume) it's a clear case. Pun intended ;) The pax format is an extension of the tar format. All of the pax implementations I've seen on Linux (OpenBSD's and MirBSD's) don't actually understand the pax headers and emit them as files. 7zip does as well. I expect there are other Unix systems where tar itself doesn't understand pax headers, although I don't have access to anything other than Linux and FreeBSD. NetBSD's tar does as well. It's surprising and sad to see *pax* implementations not supporting pax extended headers in 2014, though. It seems long file names etc. are not common enough. Or perhaps pax is simply not used that much. Since it's very common to extract tar archives in /tmp, I didn't want to leave world-writable files in /tmp (or anywhere else someone might get to them). While the contents probably aren't sensitive, a malicious user might fill someone's quota by helpfully appending /dev/zero to the file. And yes, users do these things. The extracted files are only world-writable if umask 2 == 0 or if -p (preserve permissions) has been used, no? 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 1/2] run-command: add env_array, an optional argv_array for env
Similar to args, add a struct argv_array member to struct child_process
that simplifies specifying the environment for children. It is freed
automatically by finish_command() or if start_command() encounters an
error.
Suggested-by: Jeff King p...@peff.net
Signed-off-by: Rene Scharfe l@web.de
---
Documentation/technical/api-run-command.txt | 5 +
run-command.c | 6 ++
run-command.h | 3 ++-
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/technical/api-run-command.txt
b/Documentation/technical/api-run-command.txt
index 842b838..3f12fcd 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -169,6 +169,11 @@ string pointers (NULL terminated) in .env:
. If the string does not contain '=', it names an environment
variable that will be removed from the child process's environment.
+If the .env member is NULL, `start_command` will point it at the
+.env_array `argv_array` (so you may use one or the other, but not both).
+The memory in .env_array will be cleaned up automatically during
+`finish_command` (or during `start_command` when it is unsuccessful).
+
To specify a new initial working directory for the sub-process,
specify it in the .dir member.
diff --git a/run-command.c b/run-command.c
index 761f0fd..46be513 100644
--- a/run-command.c
+++ b/run-command.c
@@ -12,6 +12,7 @@ void child_process_init(struct child_process *child)
{
memset(child, 0, sizeof(*child));
argv_array_init(child-args);
+ argv_array_init(child-env_array);
}
struct child_to_clean {
@@ -287,6 +288,8 @@ int start_command(struct child_process *cmd)
if (!cmd-argv)
cmd-argv = cmd-args.argv;
+ if (!cmd-env)
+ cmd-env = cmd-env_array.argv;
/*
* In case of errors we must keep the promise to close FDs
@@ -338,6 +341,7 @@ fail_pipe:
error(cannot create %s pipe for %s: %s,
str, cmd-argv[0], strerror(failed_errno));
argv_array_clear(cmd-args);
+ argv_array_clear(cmd-env_array);
errno = failed_errno;
return -1;
}
@@ -524,6 +528,7 @@ fail_pipe:
else if (cmd-err)
close(cmd-err);
argv_array_clear(cmd-args);
+ argv_array_clear(cmd-env_array);
errno = failed_errno;
return -1;
}
@@ -550,6 +555,7 @@ int finish_command(struct child_process *cmd)
{
int ret = wait_or_whine(cmd-pid, cmd-argv[0]);
argv_array_clear(cmd-args);
+ argv_array_clear(cmd-env_array);
return ret;
}
diff --git a/run-command.h b/run-command.h
index 1b135d1..2137315 100644
--- a/run-command.h
+++ b/run-command.h
@@ -10,6 +10,7 @@
struct child_process {
const char **argv;
struct argv_array args;
+ struct argv_array env_array;
pid_t pid;
/*
* Using .in, .out, .err:
@@ -44,7 +45,7 @@ struct child_process {
unsigned clean_on_exit:1;
};
-#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT }
+#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
void child_process_init(struct child_process *);
int start_command(struct child_process *);
--
2.1.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] receive-pack: plug minor memory leak in unpack()
Am 14.10.2014 um 11:16 schrieb Jeff King: On Mon, Oct 13, 2014 at 12:08:09PM -0700, Junio C Hamano wrote: I wonder if run-command should provide a managed env array similar to the args array. That's a good idea. I took a look at a few of them: I took a brief look, too. I had hoped we could just all it env and everybody would be happy using it instead of bare pointers. But quite a few callers assign local_repo_env to to child_process.env. We could copy it into an argv_array, of course, but that really feels like working around the interface. So I think we would prefer to keep both forms available. We could add a flag (clear_local_repo_env?) and reference local_repo_env only in run-command.c for these cases. But some other cases remain that are better off providing their own array, like in daemon.c. That raises the question: what should it be called? The argv_array form of argv is called args. The more I see it, the more I hate that name, as the two are easily confused. We could have: const char **argv; struct argv_array argv_array; const char **env; struct argv_array env_array; though argv_array is a lot to type when you have a string of argv_array_pushf() calls (which are already by themselves kind of verbose). Maybe that's not too big a deal, though. I actually like args and argv. :) Mixing them up is noticed by the compiler, so any confusion is cleared up rather quickly. We could flip it to give the managed version the short name (and calling the unmanaged version env_ptr or something). That would require munging the existing callers, but the tweak would be simple. Perhaps, but I'm a but skeptical of big renames. Let's start small and add env_array, and see how far we get with that. - daemon.c::handle() uses a static set of environment variables that are not built with argv_array(). Same for argv. Most of the callers you mentioned are good candidates. This one is tricky. The argv array gets malloc'd and set up by the parent git-daemon process. Then each time we get a client, we create a new struct child_process that references it. So using the managed argv-array would actually be a bit more complicated (and not save any memory; we just always point to the single copy for each child). For the environment, we build it in a function-local buffer, point the child_process's env field at it, start the child, and then copy the child_process into our global list of children. When we notice a child is dead (by linearly going through the list with waitpid), we free the list entry. So there are a few potentially bad things here: 1. We memcpy the child_process to put it on the list. Which does work, though it feels a little like we are violating the abstraction barrier. 2. The child_process in the list points to the local env buffer that is no longer valid. There's no bug because we don't ever look at it. Moving to a managed env would fix that. But I have to wonder if we even want to be keeping the struct child_process around in the first place (all we really care about is the pid). 3. If we do move to a managed env, then we expect it to get cleaned up in finish_command. But we never call that; we just free the list memory containing the child_process. We would want to call finish_command. Except that we will have reaped the process already with our call to waitpid() from check_dead_children. So we'd need a new call to do just the cleanup without the wait, I guess. 4. For every loop on the listen socket, we call waitpid() for each living child, which is a bit wasteful. We'd probably do better to call waitpid(0, status, WNOHANG), and then look up the resulting pid in a hashmap or sorted list when we actually see something that died. I don't know that this is a huge problem in practice. We use git-daemon pretty heavily on our backend servers at GitHub, and it seems to use about 5% of a CPU constantly on each machine. Which is kind of lame, given that it isn't doing anything at all, but is hardly earth-shattering. So I'm not sure if it is worth converting to a managed env. There's a bit of ugliness, but none of it is causing any problems, and doing so opens a can of worms. The most interesting thing to fix (to me, anyway) is number 4, but that has nothing to do with the env in the first place. :) Trickiness makes me nervous, especially in daemon.c. And 5% CPU usage just for waiting sounds awful. Using waitpid(0, ...) is not supported by the current implementation in compat/mingw.c, however. I agree that env handling should only be changed after the wait loop has been improved. By the way, does getaddrinfo(3) show up in your profiles much? Recently I looked into calling it only on demand instead of for all incoming connections because doing that unconditional with a user-supplied (tainted) hostname just felt wrong.
[PATCH 2/2] use env_array member of struct child_process
Convert users of struct child_process to using the managed env_array for
specifying environment variables instead of supplying an array on the
stack or bringing their own argv_array. This shortens and simplifies
the code and ensures automatically that the allocated memory is freed
after use.
Signed-off-by: Rene Scharfe l@web.de
---
builtin/receive-pack.c | 23 ++-
http-backend.c | 9 +++--
pager.c| 15 ---
transport-helper.c | 10 ++
wt-status.c| 6 ++
5 files changed, 25 insertions(+), 38 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f2f6c67..7593861 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -453,7 +453,6 @@ leave:
static void prepare_push_cert_sha1(struct child_process *proc)
{
static int already_done;
- struct argv_array env = ARGV_ARRAY_INIT;
if (!push_cert.len)
return;
@@ -487,20 +486,26 @@ static void prepare_push_cert_sha1(struct child_process
*proc)
nonce_status = check_nonce(push_cert.buf, bogs);
}
if (!is_null_sha1(push_cert_sha1)) {
- argv_array_pushf(env, GIT_PUSH_CERT=%s,
sha1_to_hex(push_cert_sha1));
- argv_array_pushf(env, GIT_PUSH_CERT_SIGNER=%s,
+ argv_array_pushf(proc-env_array, GIT_PUSH_CERT=%s,
+sha1_to_hex(push_cert_sha1));
+ argv_array_pushf(proc-env_array, GIT_PUSH_CERT_SIGNER=%s,
sigcheck.signer ? sigcheck.signer : );
- argv_array_pushf(env, GIT_PUSH_CERT_KEY=%s,
+ argv_array_pushf(proc-env_array, GIT_PUSH_CERT_KEY=%s,
sigcheck.key ? sigcheck.key : );
- argv_array_pushf(env, GIT_PUSH_CERT_STATUS=%c,
sigcheck.result);
+ argv_array_pushf(proc-env_array, GIT_PUSH_CERT_STATUS=%c,
+sigcheck.result);
if (push_cert_nonce) {
- argv_array_pushf(env, GIT_PUSH_CERT_NONCE=%s,
push_cert_nonce);
- argv_array_pushf(env, GIT_PUSH_CERT_NONCE_STATUS=%s,
nonce_status);
+ argv_array_pushf(proc-env_array,
+GIT_PUSH_CERT_NONCE=%s,
+push_cert_nonce);
+ argv_array_pushf(proc-env_array,
+GIT_PUSH_CERT_NONCE_STATUS=%s,
+nonce_status);
if (nonce_status == NONCE_SLOP)
- argv_array_pushf(env,
GIT_PUSH_CERT_NONCE_SLOP=%ld,
+ argv_array_pushf(proc-env_array,
+GIT_PUSH_CERT_NONCE_SLOP=%ld,
nonce_stamp_slop);
}
- proc-env = env.argv;
}
}
diff --git a/http-backend.c b/http-backend.c
index 404e682..e3e0dda 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -314,7 +314,6 @@ static void run_service(const char **argv)
const char *encoding = getenv(HTTP_CONTENT_ENCODING);
const char *user = getenv(REMOTE_USER);
const char *host = getenv(REMOTE_ADDR);
- struct argv_array env = ARGV_ARRAY_INIT;
int gzipped_request = 0;
struct child_process cld = CHILD_PROCESS_INIT;
@@ -329,13 +328,12 @@ static void run_service(const char **argv)
host = (none);
if (!getenv(GIT_COMMITTER_NAME))
- argv_array_pushf(env, GIT_COMMITTER_NAME=%s, user);
+ argv_array_pushf(cld.env_array, GIT_COMMITTER_NAME=%s, user);
if (!getenv(GIT_COMMITTER_EMAIL))
- argv_array_pushf(env, GIT_COMMITTER_EMAIL=%s@http.%s,
-user, host);
+ argv_array_pushf(cld.env_array,
+GIT_COMMITTER_EMAIL=%s@http.%s, user, host);
cld.argv = argv;
- cld.env = env.argv;
if (gzipped_request)
cld.in = -1;
cld.git_cmd = 1;
@@ -350,7 +348,6 @@ static void run_service(const char **argv)
if (finish_command(cld))
exit(1);
- argv_array_clear(env);
}
static int show_text_ref(const char *name, const unsigned char *sha1,
diff --git a/pager.c b/pager.c
index b2b805a..f6e8c33 100644
--- a/pager.c
+++ b/pager.c
@@ -74,17 +74,10 @@ void setup_pager(void)
pager_process.use_shell = 1;
pager_process.argv = pager_argv;
pager_process.in = -1;
- if (!getenv(LESS) || !getenv(LV)) {
- static const char *env[3];
- int i = 0;
-
- if (!getenv(LESS))
- env[i++] = LESS=FRX;
- if (!getenv(LV))
- env[i++] = LV=-c;
- env[i] = NULL;
-
[PATCH] receive-pack: plug minor memory leak in unpack()
The argv_array used in unpack() is never freed. Instead of adding
explicit calls to argv_array_clear() use the args member of struct
child_process and let run_command() and friends clean up for us.
Signed-off-by: Rene Scharfe l@web.de
---
builtin/receive-pack.c | 18 --
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a51846c..443dd37 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1220,7 +1220,6 @@ static const char *pack_lockfile;
static const char *unpack(int err_fd, struct shallow_info *si)
{
struct pack_header hdr;
- struct argv_array av = ARGV_ARRAY_INIT;
const char *hdr_err;
int status;
char hdr_arg[38];
@@ -1243,16 +1242,16 @@ static const char *unpack(int err_fd, struct
shallow_info *si)
if (si-nr_ours || si-nr_theirs) {
alt_shallow_file = setup_temporary_shallow(si-shallow);
- argv_array_pushl(av, --shallow-file, alt_shallow_file, NULL);
+ argv_array_push(child.args, --shallow-file);
+ argv_array_push(child.args, alt_shallow_file);
}
if (ntohl(hdr.hdr_entries) unpack_limit) {
- argv_array_pushl(av, unpack-objects, hdr_arg, NULL);
+ argv_array_pushl(child.args, unpack-objects, hdr_arg, NULL);
if (quiet)
- argv_array_push(av, -q);
+ argv_array_push(child.args, -q);
if (fsck_objects)
- argv_array_push(av, --strict);
- child.argv = av.argv;
+ argv_array_push(child.args, --strict);
child.no_stdout = 1;
child.err = err_fd;
child.git_cmd = 1;
@@ -1267,13 +1266,12 @@ static const char *unpack(int err_fd, struct
shallow_info *si)
if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
strcpy(keep_arg + s, localhost);
- argv_array_pushl(av, index-pack,
+ argv_array_pushl(child.args, index-pack,
--stdin, hdr_arg, keep_arg, NULL);
if (fsck_objects)
- argv_array_push(av, --strict);
+ argv_array_push(child.args, --strict);
if (fix_thin)
- argv_array_push(av, --fix-thin);
- child.argv = av.argv;
+ argv_array_push(child.args, --fix-thin);
child.out = -1;
child.err = err_fd;
child.git_cmd = 1;
--
2.1.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] use skip_prefix() to avoid more magic numbers
Am 07.10.2014 um 20:23 schrieb Junio C Hamano:
René Scharfe l@web.de writes:
@@ -335,20 +337,18 @@ static int append_ref(const char *refname, const
unsigned char *sha1, int flags,
static struct {
int kind;
const char *prefix;
-int pfxlen;
} ref_kind[] = {
-{ REF_LOCAL_BRANCH, refs/heads/, 11 },
-{ REF_REMOTE_BRANCH, refs/remotes/, 13 },
+{ REF_LOCAL_BRANCH, refs/heads/ },
+{ REF_REMOTE_BRANCH, refs/remotes/ },
};
/* Detect kind */
for (i = 0; i ARRAY_SIZE(ref_kind); i++) {
prefix = ref_kind[i].prefix;
-if (strncmp(refname, prefix, ref_kind[i].pfxlen))
-continue;
-kind = ref_kind[i].kind;
-refname += ref_kind[i].pfxlen;
-break;
+if (skip_prefix(refname, prefix, refname)) {
+kind = ref_kind[i].kind;
+break;
+}
This certainly makes it easier to read.
I suspect that the original was done as a (possibly premature)
optimization to avoid having to do strlen(3) on a variable that
points at constant strings for each and every ref we iterate with
for_each_rawref(), and it is somewhat sad to see it lost because
skip_prefix() assumes that the caller never knows the length of the
prefix, though.
I didn't think much about the performance implications. skip_prefix()
doesn't call strlen(3), though. Your reply made me curious. The
synthetic test program below can be used to call the old and the new
code numerous times. I called it like this:
for a in strncmp skip_prefix
do
for b in refs/heads/x refs/remotes/y refs/of/the/third/kind
do
time ./test-prefix $a $b
done
done
And got the following results:
1x strncmp for refs/heads/x, which is a local branch
real0m2.423s
user0m2.420s
sys 0m0.000s
1x strncmp for refs/remotes/y, which is a remote branch
real0m4.331s
user0m4.328s
sys 0m0.000s
1x strncmp for refs/of/the/third/kind, which is no branch
real0m3.878s
user0m3.872s
sys 0m0.000s
1x skip_prefix for refs/heads/x, which is a local branch
real0m0.891s
user0m0.888s
sys 0m0.000s
1x skip_prefix for refs/remotes/y, which is a remote branch
real0m1.345s
user0m1.340s
sys 0m0.000s
1x skip_prefix for refs/of/the/third/kind, which is no branch
real0m1.080s
user0m1.076s
sys 0m0.000s
The code handles millions of ref strings per second before and after
the change, and with the change it's faster. I hope the results are
reproducible and make it easier to say goodbye to pfxlen. :)
René
---
.gitignore| 1 +
Makefile | 1 +
test-prefix.c | 87 +++
3 files changed, 89 insertions(+)
create mode 100644 test-prefix.c
diff --git a/.gitignore b/.gitignore
index 5bfb234..8416c5e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -193,6 +193,7 @@
/test-mktemp
/test-parse-options
/test-path-utils
+/test-prefix
/test-prio-queue
/test-read-cache
/test-regex
diff --git a/Makefile b/Makefile
index f34a2d4..c09b59e 100644
--- a/Makefile
+++ b/Makefile
@@ -561,6 +561,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-prefix
TEST_PROGRAMS_NEED_X += test-prio-queue
TEST_PROGRAMS_NEED_X += test-read-cache
TEST_PROGRAMS_NEED_X += test-regex
diff --git a/test-prefix.c b/test-prefix.c
new file mode 100644
index 000..ddc63af
--- /dev/null
+++ b/test-prefix.c
@@ -0,0 +1,87 @@
+#include cache.h
+
+#define ROUNDS 1
+
+#define REF_LOCAL_BRANCH0x01
+#define REF_REMOTE_BRANCH 0x02
+
+static int test_skip_prefix(const char *refname)
+{
+ int kind, i;
+ const char *prefix;
+ static struct {
+ int kind;
+ const char *prefix;
+ } ref_kind[] = {
+ { REF_LOCAL_BRANCH, refs/heads/ },
+ { REF_REMOTE_BRANCH, refs/remotes/ },
+ };
+
+ for (i = 0; i ARRAY_SIZE(ref_kind); i++) {
+ prefix = ref_kind[i].prefix;
+ if (skip_prefix(refname, prefix, refname)) {
+ kind = ref_kind[i].kind;
+ break;
+ }
+ }
+ if (ARRAY_SIZE(ref_kind) = i)
+ return 0;
+ return kind;
+}
+
+static int test_strncmp(const char *refname)
+{
+ int kind, i;
+ const char *prefix;
+ static struct {
+ int kind;
+ const char *prefix;
+ int pfxlen;
+ } ref_kind[] = {
+ { REF_LOCAL_BRANCH, refs/heads/, 11 },
+ { REF_REMOTE_BRANCH, refs/remotes/, 13 },
+ };
+
+ for (i = 0; i ARRAY_SIZE(ref_kind); i
Re: [PATCH 12/16] sha1_file: add for_each iterators for loose and packed objects
Am 03.10.2014 um 22:32 schrieb Jeff King:
We typically iterate over the reachable objects in a
repository by starting at the tips and walking the graph.
There's no easy way to iterate over all of the objects,
including unreachable ones. Let's provide a way of doing so.
Signed-off-by: Jeff King p...@peff.net
---
cache.h | 11 +++
sha1_file.c | 62 +
2 files changed, 73 insertions(+)
diff --git a/cache.h b/cache.h
index 7abe7f6..3826b4b 100644
--- a/cache.h
+++ b/cache.h
@@ -1270,6 +1270,17 @@ int for_each_loose_file_in_objdir(const char *path,
each_loose_subdir_fn subdir_cb,
void *data);
+/*
+ * Iterate over loose and packed objects in both the local
+ * repository and any alternates repositories.
+ */
+typedef int each_packed_object_fn(const unsigned char *sha1,
+ struct packed_git *pack,
+ uint32_t pos,
+ void *data);
+extern int for_each_loose_object(each_loose_object_fn, void *);
+extern int for_each_packed_object(each_packed_object_fn, void *);
+
struct object_info {
/* Request */
enum object_type *typep;
diff --git a/sha1_file.c b/sha1_file.c
index 9fdad47..d017289 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3313,3 +3313,65 @@ int for_each_loose_file_in_objdir(const char *path,
strbuf_release(buf);
return r;
}
+
+struct loose_alt_odb_data {
+ each_loose_object_fn *cb;
+ void *data;
+};
+
+static int loose_from_alt_odb(struct alternate_object_database *alt,
+ void *vdata)
+{
+ struct loose_alt_odb_data *data = vdata;
+ return for_each_loose_file_in_objdir(alt-base,
+data-cb, NULL, NULL,
+data-data);
+}
+
+int for_each_loose_object(each_loose_object_fn cb, void *data)
+{
+ struct loose_alt_odb_data alt;
+ int r;
+
+ r = for_each_loose_file_in_objdir(get_object_directory(),
+ cb, NULL, NULL, data);
+ if (r)
+ return r;
+
+ alt.cb = cb;
+ alt.data = data;
+ return foreach_alt_odb(loose_from_alt_odb, alt);
+}
+
+int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn cb,
void *data)
Should this one be declared static? It seems to be used only in
sha1_file.c.
+{
+ uint32_t i;
+ int r = 0;
+
+ for (i = 0; i p-num_objects; i++) {
+ const unsigned char *sha1 = nth_packed_object_sha1(p, i);
+
+ if (!sha1)
+ return error(unable to get sha1 of object %u in %s,
+i, p-pack_name);
+
+ r = cb(sha1, p, i, data);
+ if (r)
+ break;
+ }
+ return r;
+}
+
+int for_each_packed_object(each_packed_object_fn cb, void *data)
+{
+ struct packed_git *p;
+ int r = 0;
+
+ prepare_packed_git();
+ for (p = packed_git; p; p = p-next) {
+ r = for_each_object_in_pack(p, cb, data);
+ if (r)
+ break;
+ }
+ return 0;
+}
Perhaps return r instead here?
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 16/16] write_sha1_file: freshen existing objects
Am 03.10.2014 um 22:41 schrieb Jeff King:
When we try to write a loose object file, we first check
whether that object already exists. If so, we skip the
write as an optimization. However, this can interfere with
prune's strategy of using mtimes to mark files in progress.
For example, if a branch contains a particular tree object
and is deleted, that tree object may become unreachable, and
have an old mtime. If a new operation then tries to write
the same tree, this ends up as a noop; we notice we
already have the object and do nothing. A prune running
simultaneously with this operation will see the object as
old, and may delete it.
We can solve this by freshening objects that we avoid
writing by updating their mtime. The algorithm for doing so
is essentially the same as that of has_sha1_file. Therefore
we provide a new (static) interface check_and_freshen,
which finds and optionally freshens the object. It's trivial
to implement freshening and simple checking by tweaking a
single parameter.
Signed-off-by: Jeff King p...@peff.net
---
sha1_file.c| 51 +++---
t/t6501-freshen-objects.sh | 27
2 files changed, 71 insertions(+), 7 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index d017289..d263b38 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -442,27 +442,53 @@ void prepare_alt_odb(void)
read_info_alternates(get_object_directory(), 0);
}
-static int has_loose_object_local(const unsigned char *sha1)
+static int freshen_file(const char *fn)
{
- return !access(sha1_file_name(sha1), F_OK);
+ struct utimbuf t;
+ t.actime = t.modtime = time(NULL);
+ return utime(fn, t);
}
Mental note: freshen_file() returns 0 on success.
-int has_loose_object_nonlocal(const unsigned char *sha1)
+static int check_and_freshen_file(const char *fn, int freshen)
+{
+ if (access(fn, F_OK))
+ return 0;
+ if (freshen freshen_file(fn))
+ return 0;
+ return 1;
+}
Returns 1 on success.
+
+static int check_and_freshen_local(const unsigned char *sha1, int freshen)
+{
+ return check_and_freshen_file(sha1_file_name(sha1), freshen);
+}
Returns 1 on success.
+
+static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)
{
struct alternate_object_database *alt;
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt-next) {
fill_sha1_path(alt-name, sha1);
- if (!access(alt-base, F_OK))
+ if (check_and_freshen_file(alt-base, freshen))
return 1;
}
return 0;
}
Returns 1 on success.
+static int check_and_freshen(const unsigned char *sha1, int freshen)
+{
+ return check_and_freshen_local(sha1, freshen) ||
+ check_and_freshen_nonlocal(sha1, freshen);
+}
Returns 1 on success.
+
+int has_loose_object_nonlocal(const unsigned char *sha1)
+{
+ return check_and_freshen_nonlocal(sha1, 0);
+}
+
static int has_loose_object(const unsigned char *sha1)
{
- return has_loose_object_local(sha1) ||
- has_loose_object_nonlocal(sha1);
+ return check_and_freshen(sha1, 0);
}
static unsigned int pack_used_ctr;
@@ -2949,6 +2975,17 @@ static int write_loose_object(const unsigned char *sha1,
char *hdr, int hdrlen,
return move_temp_to_file(tmp_file, filename);
}
+static int freshen_loose_object(const unsigned char *sha1)
+{
+ return check_and_freshen(sha1, 1);
+}
Returns 1 on success.
+
+static int freshen_packed_object(const unsigned char *sha1)
+{
+ struct pack_entry e;
+ return find_pack_entry(sha1, e) freshen_file(e.p-pack_name);
+}
Returns 1 if a pack entry is found and freshen_file() fails, and 0 if no
entry is found or freshen_file() succeeds.
It should be !freshen(...) instead, no?
Or better, let freshen_file() return 1 on success as the other functions
here.
+
int write_sha1_file(const void *buf, unsigned long len, const char *type,
unsigned char *returnsha1)
{
unsigned char sha1[20];
@@ -2961,7 +2998,7 @@ int write_sha1_file(const void *buf, unsigned long len,
const char *type, unsign
write_sha1_file_prepare(buf, len, type, sha1, hdr, hdrlen);
if (returnsha1)
hashcpy(returnsha1, sha1);
- if (has_sha1_file(sha1))
+ if (freshen_loose_object(sha1) || freshen_packed_object(sha1))
return 0;
return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
}
diff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh
index e25c47d..157f3f9 100755
--- a/t/t6501-freshen-objects.sh
+++ b/t/t6501-freshen-objects.sh
@@ -100,6 +100,33 @@ for repack in '' true; do
test_expect_success repository passes fsck ($title) '
git fsck
'
+
+ test_expect_success abandon objects again ($title) '
+ git reset --hard
Re: [PATCH 0/16] make prune mtime-checking more careful
Am 05.10.2014 um 00:22 schrieb Junio C Hamano: Jeff King p...@peff.net writes: There's quite a lot of patches here, but most of them are preparatory cleanups. The meat is in patches 13, 15, and 16. [01/16]: foreach_alt_odb: propagate return value from callback [02/16]: isxdigit: cast input to unsigned char [03/16]: object_array: factor out slopbuf-freeing logic [04/16]: object_array: add a clear function [05/16]: clean up name allocation in prepare_revision_walk [06/16]: reachable: clear pending array after walking it [07/16]: t5304: use test_path_is_* instead of test -f [08/16]: t5304: use helper to report failure of test foo = bar [09/16]: prune: factor out loose-object directory traversal [10/16]: count-objects: do not use xsize_t when counting object size [11/16]: count-objects: use for_each_loose_file_in_objdir [12/16]: sha1_file: add for_each iterators for loose and packed objects [13/16]: prune: keep objects reachable from recent objects [14/16]: pack-objects: refactor unpack-unreachable expiration check [15/16]: pack-objects: match prune logic for discarding objects [16/16]: write_sha1_file: freshen existing objects Somebody please help me out here. This applied on top of 'maint' (which does have c40fdd01) makes the test #9 (prune: do not prune detached HEAD with no reflog) fail. The test passes if the return value of freshen_file() is negated in freshen_packed_object() (see my reply to patch 16). If we merge 'dt/cache-tree-repair' (which in turn needs 'jc/reopen-lock-file') to 'maint' and then apply these on top, the said test passes. But I do not see an apparent reason why X-. Didn't check this interaction, but it sounds strange. 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] mailsplit: remove unnecessary unlink(2) call
The output file hasn't been created at this point, yet, so there is no
need to delete it when exiting early.
Suggested-by: Jeff King p...@peff.net
Signed-off-by: Rene Scharfe l@web.de
---
Original thread: http://thread.gmane.org/gmane.comp.version-control.git/255140
builtin/mailsplit.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 763cda0..8e02ea1 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -59,7 +59,6 @@ static int split_one(FILE *mbox, const char *name, int
allow_bare)
int is_bare = !is_from_line(buf.buf, buf.len);
if (is_bare !allow_bare) {
- unlink(name);
fprintf(stderr, corrupt mailbox\n);
exit(1);
}
--
2.1.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] use skip_prefix() to avoid more magic numbers
Continue where ae021d87 (use skip_prefix to avoid magic numbers) left off
and use skip_prefix() in more places for determining the lengths of prefix
strings to avoid using dependent constants and other indirect methods.
Signed-off-by: Rene Scharfe l@web.de
---
builtin/apply.c | 2 +-
builtin/branch.c| 29 +++
builtin/cat-file.c | 5 ++--
builtin/checkout.c | 6 ++---
builtin/clean.c | 7 +++---
builtin/commit.c| 18 +++
builtin/get-tar-commit-id.c | 5 ++--
builtin/log.c | 6 +++--
builtin/remote-ext.c| 10
pretty.c| 56 +
10 files changed, 69 insertions(+), 75 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 8714a88..97f7e8e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -435,7 +435,7 @@ static unsigned long linelen(const char *buffer, unsigned
long size)
static int is_dev_null(const char *str)
{
- return !memcmp(/dev/null, str, 9) isspace(str[9]);
+ return skip_prefix(str, /dev/null, str) isspace(*str);
}
#define TERM_SPACE 1
diff --git a/builtin/branch.c b/builtin/branch.c
index 9e4666f..6785097 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int ofs)
static int git_branch_config(const char *var, const char *value, void *cb)
{
+ const char *slot_name;
+
if (starts_with(var, column.))
return git_column_config(var, value, branch, colopts);
if (!strcmp(var, color.branch)) {
branch_use_color = git_config_colorbool(var, value);
return 0;
}
- if (starts_with(var, color.branch.)) {
- int slot = parse_branch_color_slot(var, 13);
+ if (skip_prefix(var, color.branch., slot_name)) {
+ int slot = parse_branch_color_slot(var, slot_name - var);
if (slot 0)
return 0;
if (!value)
@@ -335,20 +337,18 @@ static int append_ref(const char *refname, const unsigned
char *sha1, int flags,
static struct {
int kind;
const char *prefix;
- int pfxlen;
} ref_kind[] = {
- { REF_LOCAL_BRANCH, refs/heads/, 11 },
- { REF_REMOTE_BRANCH, refs/remotes/, 13 },
+ { REF_LOCAL_BRANCH, refs/heads/ },
+ { REF_REMOTE_BRANCH, refs/remotes/ },
};
/* Detect kind */
for (i = 0; i ARRAY_SIZE(ref_kind); i++) {
prefix = ref_kind[i].prefix;
- if (strncmp(refname, prefix, ref_kind[i].pfxlen))
- continue;
- kind = ref_kind[i].kind;
- refname += ref_kind[i].pfxlen;
- break;
+ if (skip_prefix(refname, prefix, refname)) {
+ kind = ref_kind[i].kind;
+ break;
+ }
}
if (ARRAY_SIZE(ref_kind) = i)
return 0;
@@ -872,13 +872,10 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
head = resolve_refdup(HEAD, head_sha1, 0, NULL);
if (!head)
die(_(Failed to resolve HEAD as a valid ref.));
- if (!strcmp(head, HEAD)) {
+ if (!strcmp(head, HEAD))
detached = 1;
- } else {
- if (!starts_with(head, refs/heads/))
- die(_(HEAD not found below refs/heads!));
- head += 11;
- }
+ else if (!skip_prefix(head, refs/heads/, head))
+ die(_(HEAD not found below refs/heads!));
hashcpy(merge_filter_ref, head_sha1);
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 7073304..f8d8129 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -82,8 +82,9 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name)
enum object_type type;
unsigned long size;
char *buffer = read_sha1_file(sha1, type,
size);
- if (memcmp(buffer, object , 7) ||
- get_sha1_hex(buffer + 7, blob_sha1))
+ const char *target;
+ if (!skip_prefix(buffer, object , target) ||
+ get_sha1_hex(target, blob_sha1))
die(%s not a valid tag,
sha1_to_hex(sha1));
free(buffer);
} else
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8afdf2b..cef1996 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1150,10 +1150,8 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
const char *argv0 = argv[0];
[PATCH] bundle: plug minor memory leak in is_tag_in_date_range()
Free the buffer returned by read_sha1_file() even if no valid tagger line is found. Signed-off-by: Rene Scharfe l@web.de --- bundle.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/bundle.c b/bundle.c index b2b89fe..9ed865c 100644 --- a/bundle.c +++ b/bundle.c @@ -211,24 +211,28 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) enum object_type type; char *buf, *line, *lineend; unsigned long date; + int result = 1; if (revs-max_age == -1 revs-min_age == -1) - return 1; + goto out; buf = read_sha1_file(tag-sha1, type, size); if (!buf) - return 1; + goto out; line = memmem(buf, size, \ntagger , 8); if (!line++) - return 1; + goto out_free; lineend = memchr(line, '\n', buf + size - line); line = memchr(line, '', lineend ? lineend - line : buf + size - line); if (!line++) - return 1; + goto out_free; date = strtoul(line, NULL, 10); - free(buf); - return (revs-max_age == -1 || revs-max_age date) + result = (revs-max_age == -1 || revs-max_age date) (revs-min_age == -1 || revs-min_age date); +out_free: + free(buf); +out: + return result; } int create_bundle(struct bundle_header *header, const char *path, -- 2.1.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 01/16] foreach_alt_odb: propagate return value from callback
Am 03.10.2014 um 22:21 schrieb Jeff King:
We check the return value of the callback and stop iterating
if it is non-zero. However, we do not make the non-zero
return value available to the caller, so they have no way of
knowing whether the operation succeeded or not (technically
they can keep their own error flag in the callback data, but
that is unlike our other for_each functions).
Signed-off-by: Jeff King p...@peff.net
---
cache.h | 2 +-
sha1_file.c | 12
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 8206039..cd16e25 100644
--- a/cache.h
+++ b/cache.h
@@ -1143,7 +1143,7 @@ extern void prepare_alt_odb(void);
extern void read_info_alternates(const char * relative_base, int depth);
extern void add_to_alternates_file(const char *reference);
typedef int alt_odb_fn(struct alternate_object_database *, void *);
-extern void foreach_alt_odb(alt_odb_fn, void*);
+extern int foreach_alt_odb(alt_odb_fn, void*);
struct pack_window {
struct pack_window *next;
diff --git a/sha1_file.c b/sha1_file.c
index c08c0cb..bae1c15 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -412,14 +412,18 @@ void add_to_alternates_file(const char *reference)
link_alt_odb_entries(alt, strlen(alt), '\n', NULL, 0);
}
-void foreach_alt_odb(alt_odb_fn fn, void *cb)
+int foreach_alt_odb(alt_odb_fn fn, void *cb)
{
struct alternate_object_database *ent;
+ int r = 0;
prepare_alt_odb();
- for (ent = alt_odb_list; ent; ent = ent-next)
- if (fn(ent, cb))
- return;
+ for (ent = alt_odb_list; ent; ent = ent-next) {
+ int r = fn(ent, cb);
+ if (r)
+ break;
+ }
+ return r;
This will always return zero. You probably shadowed r unintentionally
inside the loop, right?
}
void prepare_alt_odb(void)
--
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] sha1-array: add test-sha1-array and basic tests
Signed-off-by: Rene Scharfe l@web.de
---
.gitignore| 1 +
Makefile | 1 +
t/t0064-sha1-array.sh | 64 +++
test-sha1-array.c | 34 +++
4 files changed, 100 insertions(+)
create mode 100755 t/t0064-sha1-array.sh
create mode 100644 test-sha1-array.c
diff --git a/.gitignore b/.gitignore
index 5bfb234..9ec40fa 100644
--- a/.gitignore
+++ b/.gitignore
@@ -199,6 +199,7 @@
/test-revision-walking
/test-run-command
/test-sha1
+/test-sha1-array
/test-sigchain
/test-string-list
/test-subprocess
diff --git a/Makefile b/Makefile
index f34a2d4..356feb5 100644
--- a/Makefile
+++ b/Makefile
@@ -568,6 +568,7 @@ TEST_PROGRAMS_NEED_X += test-revision-walking
TEST_PROGRAMS_NEED_X += test-run-command
TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
TEST_PROGRAMS_NEED_X += test-sha1
+TEST_PROGRAMS_NEED_X += test-sha1-array
TEST_PROGRAMS_NEED_X += test-sigchain
TEST_PROGRAMS_NEED_X += test-string-list
TEST_PROGRAMS_NEED_X += test-subprocess
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
new file mode 100755
index 000..bd68789
--- /dev/null
+++ b/t/t0064-sha1-array.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='basic tests for the SHA1 array implementation'
+. ./test-lib.sh
+
+echo20() {
+ prefix=$1
+ shift
+ while test $# -gt 0
+ do
+ echo $prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1
+ shift
+ done
+}
+
+test_expect_success 'ordered enumeration' '
+ echo20 44 55 88 aa expect
+ {
+ echo20 append 88 44 aa 55
+ echo for_each_unique
+ } | test-sha1-array actual
+ test_cmp expect actual
+'
+
+test_expect_success 'ordered enumeration with duplicate suppression' '
+ echo20 44 55 88 aa expect
+ {
+ echo20 append 88 44 aa 55
+ echo20 append 88 44 aa 55
+ echo for_each_unique
+ } | test-sha1-array actual
+ test_cmp expect actual
+'
+
+test_expect_success 'lookup' '
+ {
+ echo20 append 88 44 aa 55
+ echo20 lookup 55
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -eq 1
+'
+
+test_expect_success 'lookup non-existing entry' '
+ {
+ echo20 append 88 44 aa 55
+ echo20 lookup 33
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -lt 0
+'
+
+test_expect_success 'lookup with duplicates' '
+ {
+ echo20 append 88 44 aa 55
+ echo20 append 88 44 aa 55
+ echo20 lookup 55
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -ge 2
+ test $n -le 3
+'
+
+test_done
diff --git a/test-sha1-array.c b/test-sha1-array.c
new file mode 100644
index 000..ddc491e
--- /dev/null
+++ b/test-sha1-array.c
@@ -0,0 +1,34 @@
+#include cache.h
+#include sha1-array.h
+
+static void print_sha1(const unsigned char sha1[20], void *data)
+{
+ puts(sha1_to_hex(sha1));
+}
+
+int main(int argc, char **argv)
+{
+ struct sha1_array array = SHA1_ARRAY_INIT;
+ struct strbuf line = STRBUF_INIT;
+
+ while (strbuf_getline(line, stdin, '\n') != EOF) {
+ const char *arg;
+ unsigned char sha1[20];
+
+ if (skip_prefix(line.buf, append , arg)) {
+ if (get_sha1_hex(arg, sha1))
+ die(not a hexadecimal SHA1: %s, arg);
+ sha1_array_append(array, sha1);
+ } else if (skip_prefix(line.buf, lookup , arg)) {
+ if (get_sha1_hex(arg, sha1))
+ die(not a hexadecimal SHA1: %s, arg);
+ printf(%d\n, sha1_array_lookup(array, sha1));
+ } else if (!strcmp(line.buf, clear))
+ sha1_array_clear(array);
+ else if (!strcmp(line.buf, for_each_unique))
+ sha1_array_for_each_unique(array, print_sha1, NULL);
+ else
+ die(unknown command: %s, line.buf);
+ }
+ return 0;
+}
--
2.1.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 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()
If the first 18 bytes of the SHA1's of all entries are the same then
sha1_pos() dies and reports that the lower and upper limits of the
binary search were the same that this wasn't supposed to happen. This
is wrong because the remaining two bytes could still differ.
Furthermore: It wouldn't be a problem if they actually were the same,
i.e. if all entries have the same SHA1. The code already handles
duplicates just fine otherwise. Simply remove the erroneous check.
Signed-off-by: Rene Scharfe l@web.de
---
sha1-lookup.c | 2 --
t/t0064-sha1-array.sh | 20
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/sha1-lookup.c b/sha1-lookup.c
index 2dd8515..5f06921 100644
--- a/sha1-lookup.c
+++ b/sha1-lookup.c
@@ -84,8 +84,6 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t
nr,
die(BUG: assertion failed in binary search);
}
}
- if (18 = ofs)
- die(cannot happen -- lo and hi are identical);
}
do {
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
index bd68789..3fcb8d8 100755
--- a/t/t0064-sha1-array.sh
+++ b/t/t0064-sha1-array.sh
@@ -61,4 +61,24 @@ test_expect_success 'lookup with duplicates' '
test $n -le 3
'
+test_expect_success 'lookup with almost duplicate values' '
+ {
+ echo append
+ echo append 555f
+ echo20 lookup 55
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -eq 0
+'
+
+test_expect_success 'lookup with single duplicate value' '
+ {
+ echo20 append 55 55
+ echo20 lookup 55
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -ge 0
+ test $n -le 1
+'
+
test_done
--
2.1.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 1/3] daemon: handle gethostbyname() error
If the user-supplied hostname can't be found then we should not use it.
We already avoid doing that in the non-NO_IPV6 case by checking if the
return value of getaddrinfo() is zero (success). Do the same in the
NO_IPV6 case and make sure the return value of gethostbyname() isn't
NULL before dereferencing this pointer.
Signed-off-by: Rene Scharfe l@web.de
---
Most lines are just re-indented, not actually changed.
daemon.c | 27 ++-
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/daemon.c b/daemon.c
index 4dcfff9..a6f467e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -553,20 +553,21 @@ static void parse_host_arg(char *extra_args, int buflen)
static char addrbuf[HOST_NAME_MAX + 1];
hent = gethostbyname(hostname);
+ if (hent) {
+ ap = hent-h_addr_list;
+ memset(sa, 0, sizeof sa);
+ sa.sin_family = hent-h_addrtype;
+ sa.sin_port = htons(0);
+ memcpy(sa.sin_addr, *ap, hent-h_length);
+
+ inet_ntop(hent-h_addrtype, sa.sin_addr,
+ addrbuf, sizeof(addrbuf));
- ap = hent-h_addr_list;
- memset(sa, 0, sizeof sa);
- sa.sin_family = hent-h_addrtype;
- sa.sin_port = htons(0);
- memcpy(sa.sin_addr, *ap, hent-h_length);
-
- inet_ntop(hent-h_addrtype, sa.sin_addr,
- addrbuf, sizeof(addrbuf));
-
- free(canon_hostname);
- canon_hostname = xstrdup(hent-h_name);
- free(ip_address);
- ip_address = xstrdup(addrbuf);
+ free(canon_hostname);
+ canon_hostname = xstrdup(hent-h_name);
+ free(ip_address);
+ ip_address = xstrdup(addrbuf);
+ }
#endif
}
}
--
2.1.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 2/3] daemon: fix error message after bind()
Signed-off-by: Rene Scharfe l@web.de
---
daemon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/daemon.c b/daemon.c
index a6f467e..090f6a4 100644
--- a/daemon.c
+++ b/daemon.c
@@ -924,7 +924,7 @@ static int setup_named_sock(char *listen_addr, int
listen_port, struct socketlis
}
if ( bind(sockfd, (struct sockaddr *)sin, sizeof sin) 0 ) {
- logerror(Could not listen to %s: %s,
+ logerror(Could not bind to %s: %s,
ip2str(AF_INET, (struct sockaddr *)sin, sizeof(sin)),
strerror(errno));
close(sockfd);
--
2.1.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 3/3] daemon: remove write-only variable maxfd
It became unused when 6573faff (NO_IPV6 support for git daemon) replaced
select() with poll().
Signed-off-by: Rene Scharfe l@web.de
---
daemon.c | 4
1 file changed, 4 deletions(-)
diff --git a/daemon.c b/daemon.c
index 090f6a4..54a03bd 100644
--- a/daemon.c
+++ b/daemon.c
@@ -815,7 +815,6 @@ static const char *ip2str(int family, struct sockaddr *sin,
socklen_t len)
static int setup_named_sock(char *listen_addr, int listen_port, struct
socketlist *socklist)
{
int socknum = 0;
- int maxfd = -1;
char pbuf[NI_MAXSERV];
struct addrinfo hints, *ai0, *ai;
int gai;
@@ -883,9 +882,6 @@ static int setup_named_sock(char *listen_addr, int
listen_port, struct socketlis
ALLOC_GROW(socklist-list, socklist-nr + 1, socklist-alloc);
socklist-list[socklist-nr++] = sockfd;
socknum++;
-
- if (maxfd sockfd)
- maxfd = sockfd;
}
freeaddrinfo(ai0);
--
2.1.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 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()
Am 01.10.2014 um 12:50 schrieb Jeff King:
On Wed, Oct 01, 2014 at 11:43:21AM +0200, René Scharfe wrote:
If the first 18 bytes of the SHA1's of all entries are the same then
sha1_pos() dies and reports that the lower and upper limits of the
binary search were the same that this wasn't supposed to happen. This
is wrong because the remaining two bytes could still differ.
Furthermore: It wouldn't be a problem if they actually were the same,
i.e. if all entries have the same SHA1. The code already handles
duplicates just fine otherwise. Simply remove the erroneous check.
Yeah, I agree that assertion is just wrong.
Regarding duplicates: in sha1_entry_pos, we had to handle the not
found case specially, because we may have found the left-hand or
right-hand side of a run of duplicates, and we want to return the
correct slot where the new item would go (see the comment added by
171bdac). I think we don't have to deal with that here, because we are
just dealing with the initial mi selection. The actual binary search
is plain-vanilla, which handles that case just fine.
I wonder if it is worth adding a test (you test only that not found
produces a negative index, but not which index). Like:
api-sha1-array.txt says about sha1_array_lookup: If not found, returns
a negative integer, and that's what the test checks.
I actually like that the value is not specified for that case because no
existing caller actually uses it and it leaves room to implement the
function e.g. using bsearch(3).
I agree that adding a lookup non-existing entry with duplicates test
would make t0064 more complete, though.
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
index 3fcb8d8..7781129 100755
--- a/t/t0064-sha1-array.sh
+++ b/t/t0064-sha1-array.sh
@@ -42,12 +42,12 @@ test_expect_success 'lookup' '
'
test_expect_success 'lookup non-existing entry' '
+ echo -1 expect
{
echo20 append 88 44 aa 55
echo20 lookup 33
} | test-sha1-array actual
- n=$(cat actual)
- test $n -lt 0
+ test_cmp expect actual
'
test_expect_success 'lookup with duplicates' '
@@ -61,6 +61,17 @@ test_expect_success 'lookup with duplicates' '
test $n -le 3
'
+test_expect_success 'lookup non-existing entry with duplicates' '
+ echo -5 expect
+ {
+ echo20 append 88 44 aa 55
+ echo20 append 88 44 aa 55
+ echo20 lookup 66
+ } | test-sha1-array actual
+ test_cmp expect actual
+'
+
+
test_expect_success 'lookup with almost duplicate values' '
{
echo append
--
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 v7 09/38] lock_file(): always initialize and register lock_file object
Am 01.10.2014 um 12:28 schrieb Michael Haggerty: The purpose of this patch is to make the state diagram for lock_file objects simpler and deterministic. If locking fails, lock_file() sometimes leaves the lock_file object partly initialized, but sometimes not. It sometimes registers the object in lock_file_list, but sometimes not. This makes the state diagram for lock_file objects effectively indeterministic and hard to reason about. A future patch will also change the filename field into a strbuf, which needs more involved initialization, so it will become even more important that the state of a lock_file object is well-defined after a failed attempt to lock. The ambiguity doesn't currently have any ill effects, because lock_file objects cannot be removed from the lock_file_list anyway. But to make it easier to document and reason about the code, make this behavior inconsistent: *always* initialize the lock_file object and s/incon/con/, certainly? *always* register it in lock_file_list the first time it is used, regardless of whether an error occurs. While we're at it, make sure that all of the lock_file fields are initialized to values appropriate for an unlocked object; the caller is only responsible for making sure that on_list is set to zero before the first time it is used. -- 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] sha1-array: add test-sha1-array and basic tests
Am 01.10.2014 um 16:11 schrieb Eric Sunshine:
On Wed, Oct 1, 2014 at 5:40 AM, René Scharfe l@web.de wrote:
Signed-off-by: Rene Scharfe l@web.de
---
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
new file mode 100755
index 000..bd68789
--- /dev/null
+++ b/t/t0064-sha1-array.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='basic tests for the SHA1 array implementation'
+. ./test-lib.sh
+
+echo20() {
+ prefix=$1
+ shift
+ while test $# -gt 0
+ do
+ echo $prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1
Each caller of echo20() manually includes a space at the end of
$prefix. Would it make sense to instead have echo20() do this on
behalf of the caller?
echo $prefix $1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1
That wouldn't work if the prefix is the empty string; we don't want a
space in that case (see the next echo20 call below).
But ${prefix:+$prefix } would do the trick. Thanks for the idea. :)
+ shift
+ done
+}
+
+test_expect_success 'ordered enumeration' '
+ echo20 44 55 88 aa expect
+ {
+ echo20 append 88 44 aa 55
Which would slightly reduce the burden on the caller and make it read
(very slightly) nicer:
echo20 append 88 44 aa 55
+ echo for_each_unique
+ } | test-sha1-array actual
+ test_cmp expect actual
+'
+
+test_expect_success 'ordered enumeration with duplicate suppression' '
+ echo20 44 55 88 aa expect
+ {
+ echo20 append 88 44 aa 55
+ echo20 append 88 44 aa 55
+ echo for_each_unique
+ } | test-sha1-array actual
+ test_cmp expect actual
+'
+
+test_expect_success 'lookup' '
+ {
+ echo20 append 88 44 aa 55
+ echo20 lookup 55
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -eq 1
+'
+
+test_expect_success 'lookup non-existing entry' '
+ {
+ echo20 append 88 44 aa 55
+ echo20 lookup 33
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -lt 0
+'
+
+test_expect_success 'lookup with duplicates' '
+ {
+ echo20 append 88 44 aa 55
+ echo20 append 88 44 aa 55
+ echo20 lookup 55
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -ge 2
+ test $n -le 3
+'
+
+test_done
--
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/2] sha1-array: add test-sha1-array and basic tests
Helped-by: Jeff King p...@peff.net
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Rene Scharfe l@web.de
---
Added a test for looking up a non-existing entry in an array that
contains duplicates, as suggested by Jeff. Changed echo20() to add
a space after the prefix as needed, as suggested by Eric.
.gitignore| 1 +
Makefile | 1 +
t/t0064-sha1-array.sh | 74 +++
test-sha1-array.c | 34 +++
4 files changed, 110 insertions(+)
create mode 100755 t/t0064-sha1-array.sh
create mode 100644 test-sha1-array.c
diff --git a/.gitignore b/.gitignore
index 5bfb234..9ec40fa 100644
--- a/.gitignore
+++ b/.gitignore
@@ -199,6 +199,7 @@
/test-revision-walking
/test-run-command
/test-sha1
+/test-sha1-array
/test-sigchain
/test-string-list
/test-subprocess
diff --git a/Makefile b/Makefile
index f34a2d4..356feb5 100644
--- a/Makefile
+++ b/Makefile
@@ -568,6 +568,7 @@ TEST_PROGRAMS_NEED_X += test-revision-walking
TEST_PROGRAMS_NEED_X += test-run-command
TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
TEST_PROGRAMS_NEED_X += test-sha1
+TEST_PROGRAMS_NEED_X += test-sha1-array
TEST_PROGRAMS_NEED_X += test-sigchain
TEST_PROGRAMS_NEED_X += test-string-list
TEST_PROGRAMS_NEED_X += test-subprocess
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
new file mode 100755
index 000..3f26e11
--- /dev/null
+++ b/t/t0064-sha1-array.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='basic tests for the SHA1 array implementation'
+. ./test-lib.sh
+
+echo20() {
+ prefix=${1:+$1 }
+ shift
+ while test $# -gt 0
+ do
+ echo $prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1
+ shift
+ done
+}
+
+test_expect_success 'ordered enumeration' '
+ echo20 44 55 88 aa expect
+ {
+ echo20 append 88 44 aa 55
+ echo for_each_unique
+ } | test-sha1-array actual
+ test_cmp expect actual
+'
+
+test_expect_success 'ordered enumeration with duplicate suppression' '
+ echo20 44 55 88 aa expect
+ {
+ echo20 append 88 44 aa 55
+ echo20 append 88 44 aa 55
+ echo for_each_unique
+ } | test-sha1-array actual
+ test_cmp expect actual
+'
+
+test_expect_success 'lookup' '
+ {
+ echo20 append 88 44 aa 55
+ echo20 lookup 55
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -eq 1
+'
+
+test_expect_success 'lookup non-existing entry' '
+ {
+ echo20 append 88 44 aa 55
+ echo20 lookup 33
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -lt 0
+'
+
+test_expect_success 'lookup with duplicates' '
+ {
+ echo20 append 88 44 aa 55
+ echo20 append 88 44 aa 55
+ echo20 lookup 55
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -ge 2
+ test $n -le 3
+'
+
+test_expect_success 'lookup non-existing entry with duplicates' '
+ {
+ echo20 append 88 44 aa 55
+ echo20 append 88 44 aa 55
+ echo20 lookup 66
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -lt 0
+'
+
+test_done
diff --git a/test-sha1-array.c b/test-sha1-array.c
new file mode 100644
index 000..ddc491e
--- /dev/null
+++ b/test-sha1-array.c
@@ -0,0 +1,34 @@
+#include cache.h
+#include sha1-array.h
+
+static void print_sha1(const unsigned char sha1[20], void *data)
+{
+ puts(sha1_to_hex(sha1));
+}
+
+int main(int argc, char **argv)
+{
+ struct sha1_array array = SHA1_ARRAY_INIT;
+ struct strbuf line = STRBUF_INIT;
+
+ while (strbuf_getline(line, stdin, '\n') != EOF) {
+ const char *arg;
+ unsigned char sha1[20];
+
+ if (skip_prefix(line.buf, append , arg)) {
+ if (get_sha1_hex(arg, sha1))
+ die(not a hexadecimal SHA1: %s, arg);
+ sha1_array_append(array, sha1);
+ } else if (skip_prefix(line.buf, lookup , arg)) {
+ if (get_sha1_hex(arg, sha1))
+ die(not a hexadecimal SHA1: %s, arg);
+ printf(%d\n, sha1_array_lookup(array, sha1));
+ } else if (!strcmp(line.buf, clear))
+ sha1_array_clear(array);
+ else if (!strcmp(line.buf, for_each_unique))
+ sha1_array_for_each_unique(array, print_sha1, NULL);
+ else
+ die(unknown command: %s, line.buf);
+ }
+ return 0;
+}
--
2.1.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/2] sha1-lookup: handle duplicates in sha1_pos()
If the first 18 bytes of the SHA1's of all entries are the same then
sha1_pos() dies and reports that the lower and upper limits of the
binary search were the same that this wasn't supposed to happen. This
is wrong because the remaining two bytes could still differ.
Furthermore: It wouldn't be a problem if they actually were the same,
i.e. if all entries have the same SHA1. The code already handles
duplicates just fine. Simply remove the erroneous check.
Signed-off-by: Rene Scharfe l@web.de
---
sha1-lookup.c | 2 --
t/t0064-sha1-array.sh | 20
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/sha1-lookup.c b/sha1-lookup.c
index 2dd8515..5f06921 100644
--- a/sha1-lookup.c
+++ b/sha1-lookup.c
@@ -84,8 +84,6 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t
nr,
die(BUG: assertion failed in binary search);
}
}
- if (18 = ofs)
- die(cannot happen -- lo and hi are identical);
}
do {
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
index 3f26e11..dbbe2e9 100755
--- a/t/t0064-sha1-array.sh
+++ b/t/t0064-sha1-array.sh
@@ -71,4 +71,24 @@ test_expect_success 'lookup non-existing entry with
duplicates' '
test $n -lt 0
'
+test_expect_success 'lookup with almost duplicate values' '
+ {
+ echo append
+ echo append 555f
+ echo20 lookup 55
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -eq 0
+'
+
+test_expect_success 'lookup with single duplicate value' '
+ {
+ echo20 append 55 55
+ echo20 lookup 55
+ } | test-sha1-array actual
+ n=$(cat actual)
+ test $n -ge 0
+ test $n -le 1
+'
+
test_done
--
2.1.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] t0090: avoid passing empty string to printf %d
FreeBSD's printf(1) doesn't accept empty strings for numerical format
specifiers:
$ printf %d\n /dev/null; echo $?
printf: : expected numeric value
1
Initialize the AWK variable c to make sure the shell variable
subtree_count always contains a numerical value, in order to keep the
subsequently called printf happy.
Signed-off-by: Rene Scharfe l@web.de
---
t/t0090-cache-tree.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index f9648a8..158cf4f 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -22,7 +22,7 @@ generate_expected_cache_tree_rec () {
# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
# We want to count only foo because it's the only direct child
subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq)
- subtree_count=$(echo $subtrees|awk '$1 {++c} END {print c}')
+ subtree_count=$(echo $subtrees|awk -v c=0 '$1 {++c} END {print c}')
entries=$(git ls-files|wc -l)
printf SHA $dir (%d entries, %d subtrees)\n $entries
$subtree_count
for subtree in $subtrees
--
2.1.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
Re: [PATCH 1/2] add macro REALLOC_ARRAY
Am 24.09.2014 um 09:32 schrieb Michael Haggerty: Is there a reason that ALLOC_GROW and REALLOC_ARRAY are defined in two separate header files (cache.h and git-compat-util.h, respectively)? It seems to me that they are close siblings and therefore I find it surprising that they are not defined right next to each other. REALLOC_ARRAY is more like xrealloc and it's used in places that only #include git-compat-util.h and not cache.h, so the first header was the right place. ALLOC_GROW could be moved there in a separate patch, but I'm not sure it's worth it. 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 2/2] use REALLOC_ARRAY for changing the allocation size of arrays
Am 24.09.2014 um 20:47 schrieb Jonathan Nieder: René Scharfe wrote: --- a/khash.h +++ b/khash.h (not really about this patch) Where did this file come from? Do we want to be able to sync with upstream to get later bugfixes (e.g., https://github.com/attractivechaos/klib/commit/000f0890)? If so, it might make sense to avoid unnecessary changes to the file so it's easier to see when it's safe to replace the file wholesale with a new version. True. For this patch we're safe, I think, because it already called xrealloc before I touched it (and has been doing that since it was imported into 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
[PATCH] remote: simplify match_name_with_pattern() using strbuf
Make the code simpler and shorter by avoiding repetitive use of
string length variables and leaving memory allocation to strbuf
functions.
Signed-off-by: Rene Scharfe l@web.de
---
remote.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/remote.c b/remote.c
index 35e62ee..ce785f8 100644
--- a/remote.c
+++ b/remote.c
@@ -862,21 +862,14 @@ static int match_name_with_pattern(const char *key, const
char *name,
ret = !strncmp(name, key, klen) namelen = klen + ksuffixlen
!memcmp(name + namelen - ksuffixlen, kstar + 1, ksuffixlen);
if (ret value) {
+ struct strbuf sb = STRBUF_INIT;
const char *vstar = strchr(value, '*');
- size_t vlen;
- size_t vsuffixlen;
if (!vstar)
die(Value '%s' of pattern has no '*', value);
- vlen = vstar - value;
- vsuffixlen = strlen(vstar + 1);
- *result = xmalloc(vlen + vsuffixlen +
- strlen(name) -
- klen - ksuffixlen + 1);
- strncpy(*result, value, vlen);
- strncpy(*result + vlen,
- name + klen, namelen - klen - ksuffixlen);
- strcpy(*result + vlen + namelen - klen - ksuffixlen,
- vstar + 1);
+ strbuf_add(sb, value, vstar - value);
+ strbuf_add(sb, name + klen, namelen - klen - ksuffixlen);
+ strbuf_addstr(sb, vstar + 1);
+ *result = strbuf_detach(sb, NULL);
}
return ret;
}
--
2.1.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 1/2] graph: simplify graph_padding_line()
Deduplicate code common to both branches of if statements.
Signed-off-by: Rene Scharfe l@web.de
---
graph.c | 17 -
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/graph.c b/graph.c
index dfb99f6..52605e4 100644
--- a/graph.c
+++ b/graph.c
@@ -1161,20 +1161,11 @@ static void graph_padding_line(struct git_graph *graph,
struct strbuf *sb)
*/
for (i = 0; i graph-num_columns; i++) {
struct column *col = graph-columns[i];
- struct commit *col_commit = col-commit;
- if (col_commit == graph-commit) {
- strbuf_write_column(sb, col, '|');
-
- if (graph-num_parents 3)
- strbuf_addch(sb, ' ');
- else {
- int num_spaces = ((graph-num_parents - 2) * 2);
- strbuf_addchars(sb, ' ', num_spaces);
- }
- } else {
- strbuf_write_column(sb, col, '|');
+ strbuf_write_column(sb, col, '|');
+ if (col-commit == graph-commit graph-num_parents 2)
+ strbuf_addchars(sb, ' ', (graph-num_parents - 2) * 2);
+ else
strbuf_addch(sb, ' ');
- }
}
graph_pad_horizontally(graph, sb, graph-num_columns);
--
2.1.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
Re: [PATCH 1/2] graph: simplify graph_padding_line()
Am 20.09.2014 um 20:29 schrieb René Scharfe: Deduplicate code common to both branches of if statements. There is no 2/2, this patch is the only one at this time. 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] add macro REALLOCARRAY
Am 16.09.2014 um 05:04 schrieb Junio C Hamano: On Sun, Sep 14, 2014 at 9:55 AM, René Scharfe l@web.de wrote: +#define REALLOCARRAY(x, alloc) x = xrealloc((x), (alloc) * sizeof(*(x))) I have been wondering if x could be an expression that has an operator that binds weaker than the assignment '='. That may necessitate the LHS of the assignment to be somehow marked as bound the tightest, i.e. #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x))) Or am I being overly silly? ALLOC_GROW did well without that. I can't think of a good use case for a complex expression on the right-hand side. That said, I think I still have a spare matching pair of parentheses lying around here somewhere, so let's play it safe and use them. :) The added underscore is a good idea 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
[PATCH 1/2] add macro REALLOC_ARRAY
The macro ALLOC_GROW manages several aspects of dynamic memory
allocations for arrays: It performs overprovisioning in order to avoid
reallocations in future calls, updates the allocation size variable,
multiplies the item size and thus allows users to simply specify the
item count, performs the reallocation and updates the array pointer.
Sometimes this is too much. Add the macro REALLOC_ARRAY, which only
takes care of the latter three points and allows users to specfiy the
number of items the array can store. It can increase and also decrease
the size. Using the macro avoid duplicating the variable name and
takes care of the item sizes automatically.
Signed-off-by: Rene Scharfe l@web.de
---
Documentation/technical/api-allocation-growing.txt | 3 +++
git-compat-util.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/Documentation/technical/api-allocation-growing.txt
b/Documentation/technical/api-allocation-growing.txt
index 542946b..5a59b54 100644
--- a/Documentation/technical/api-allocation-growing.txt
+++ b/Documentation/technical/api-allocation-growing.txt
@@ -34,3 +34,6 @@ item[nr++] = value you like;
You are responsible for updating the `nr` variable.
+
+If you need to specify the number of elements to allocate explicitly
+then use the macro `REALLOC_ARRAY(item, alloc)` instead of `ALLOC_GROW`.
diff --git a/git-compat-util.h b/git-compat-util.h
index 4e7e3f8..5a15b53 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -626,6 +626,8 @@ 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);
+#define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
+
static inline size_t xsize_t(off_t len)
{
if (len (size_t) len)
--
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] use REALLOC_ARRAY for changing the allocation size of arrays
Signed-off-by: Rene Scharfe l@web.de --- attr.c | 3 +-- builtin/apply.c| 2 +- builtin/for-each-ref.c | 9 +++-- builtin/index-pack.c | 4 +--- builtin/log.c | 2 +- builtin/merge.c| 2 +- builtin/mv.c | 8 builtin/pack-objects.c | 3 +-- builtin/show-branch.c | 2 +- cache.h| 2 +- column.c | 6 ++ commit-slab.h | 3 +-- fast-import.c | 2 +- git.c | 3 +-- graph.c| 14 -- khash.h| 12 line-log.c | 2 +- object.c | 2 +- pack-bitmap-write.c| 3 +-- pack-bitmap.c | 6 ++ pack-objects.c | 3 +-- revision.c | 2 +- sh-i18n--envsubst.c| 5 + shallow.c | 3 +-- string-list.c | 3 +-- walker.c | 4 ++-- 26 files changed, 40 insertions(+), 70 deletions(-) diff --git a/attr.c b/attr.c index 734222d..cd54697 100644 --- a/attr.c +++ b/attr.c @@ -97,8 +97,7 @@ static struct git_attr *git_attr_internal(const char *name, int len) a-attr_nr = attr_nr++; git_attr_hash[pos] = a; - check_all_attr = xrealloc(check_all_attr, - sizeof(*check_all_attr) * attr_nr); + REALLOC_ARRAY(check_all_attr, attr_nr); check_all_attr[a-attr_nr].attr = a; check_all_attr[a-attr_nr].value = ATTR__UNKNOWN; return a; diff --git a/builtin/apply.c b/builtin/apply.c index f204cca..8714a88 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -2626,7 +2626,7 @@ static void update_image(struct image *img, * NOTE: this knows that we never call remove_first_line() * on anything other than pre/post image. */ - img-line = xrealloc(img-line, nr * sizeof(*img-line)); + REALLOC_ARRAY(img-line, nr); img-line_allocated = img-line; } if (preimage_limit != postimage-nr) diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 47bd624..7f55e68 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -138,10 +138,8 @@ static int parse_atom(const char *atom, const char *ep) /* Add it in, including the deref prefix */ at = used_atom_cnt; used_atom_cnt++; - used_atom = xrealloc(used_atom, -(sizeof *used_atom) * used_atom_cnt); - used_atom_type = xrealloc(used_atom_type, - (sizeof(*used_atom_type) * used_atom_cnt)); + REALLOC_ARRAY(used_atom, used_atom_cnt); + REALLOC_ARRAY(used_atom_type, used_atom_cnt); used_atom[at] = xmemdupz(atom, ep - atom); used_atom_type[at] = valid_atom[i].cmp_type; if (*atom == '*') @@ -870,8 +868,7 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f ref-flag = flag; cnt = cb-grab_cnt; - cb-grab_array = xrealloc(cb-grab_array, - sizeof(*cb-grab_array) * (cnt + 1)); + REALLOC_ARRAY(cb-grab_array, cnt + 1); cb-grab_array[cnt++] = ref; cb-grab_cnt = cnt; return 0; diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 5568a5b..783623d 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1140,9 +1140,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha int nr_objects_initial = nr_objects; if (nr_unresolved = 0) die(_(confusion beyond insanity)); - objects = xrealloc(objects, - (nr_objects + nr_unresolved + 1) - * sizeof(*objects)); + REALLOC_ARRAY(objects, nr_objects + nr_unresolved + 1); memset(objects + nr_objects + 1, 0, nr_unresolved * sizeof(*objects)); f = sha1fd(output_fd, curr_pack); diff --git a/builtin/log.c b/builtin/log.c index e4d8122..7643396 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1440,7 +1440,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) continue; nr++; - list = xrealloc(list, nr * sizeof(list[0])); + REALLOC_ARRAY(list, nr); list[nr - 1] = commit; } if (nr == 0) diff --git a/builtin/merge.c b/builtin/merge.c index 9da9e30..cb9af1e 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -556,7 +556,7 @@ static void parse_branch_merge_options(char *bmo) if (argc 0) die(_(Bad branch.%s.mergeoptions string: %s), branch, split_cmdline_strerror(argc)); - argv = xrealloc(argv, sizeof(*argv) * (argc + 2)); + REALLOC_ARRAY(argv, argc + 2); memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
[PATCH 1/2] add macro REALLOCARRAY
The macro ALLOC_GROW manages several aspects of dynamic memory
allocations for arrays: It performs overprovisioning in order to avoid
reallocations in future calls, updates the allocation size variable,
multiplies the item size and thus allows users to simply specify the
item count, performs the reallocation and updates the array pointer.
Sometimes this is too much. Add the macro REALLOCARRAY, which only
takes care of the latter three points and allows users to specify the
number of items an array can store directly. It can increase and
also decrease its size. Using this macro avoids duplicating the
array pointer name and takes care of item sizes automatically.
Signed-off-by: Rene Scharfe l@web.de
---
Documentation/technical/api-allocation-growing.txt | 3 +++
git-compat-util.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/Documentation/technical/api-allocation-growing.txt
b/Documentation/technical/api-allocation-growing.txt
index 542946b..4b5f049 100644
--- a/Documentation/technical/api-allocation-growing.txt
+++ b/Documentation/technical/api-allocation-growing.txt
@@ -34,3 +34,6 @@ item[nr++] = value you like;
You are responsible for updating the `nr` variable.
+
+If you need to specify the number of elements to allocate explicitly
+then use the macro `REALLOCARRAY(item, alloc)` instead of `ALLOC_GROW`.
diff --git a/git-compat-util.h b/git-compat-util.h
index 4e7e3f8..d926e4c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -626,6 +626,8 @@ 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);
+#define REALLOCARRAY(x, alloc) x = xrealloc((x), (alloc) * sizeof(*(x)))
+
static inline size_t xsize_t(off_t len)
{
if (len (size_t) len)
--
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] use REALLOCARRAY for changing the allocation size of arrays
Signed-off-by: Rene Scharfe l@web.de --- attr.c | 3 +-- builtin/apply.c| 2 +- builtin/for-each-ref.c | 9 +++-- builtin/index-pack.c | 4 +--- builtin/log.c | 2 +- builtin/merge.c| 2 +- builtin/mv.c | 8 builtin/pack-objects.c | 3 +-- builtin/show-branch.c | 2 +- cache.h| 2 +- column.c | 6 ++ commit-slab.h | 3 +-- fast-import.c | 2 +- git.c | 3 +-- graph.c| 14 -- khash.h| 12 line-log.c | 2 +- object.c | 2 +- pack-bitmap-write.c| 3 +-- pack-bitmap.c | 6 ++ pack-objects.c | 3 +-- revision.c | 2 +- sh-i18n--envsubst.c| 5 + shallow.c | 3 +-- string-list.c | 3 +-- walker.c | 4 ++-- 26 files changed, 40 insertions(+), 70 deletions(-) diff --git a/attr.c b/attr.c index 734222d..e89be50 100644 --- a/attr.c +++ b/attr.c @@ -97,8 +97,7 @@ static struct git_attr *git_attr_internal(const char *name, int len) a-attr_nr = attr_nr++; git_attr_hash[pos] = a; - check_all_attr = xrealloc(check_all_attr, - sizeof(*check_all_attr) * attr_nr); + REALLOCARRAY(check_all_attr, attr_nr); check_all_attr[a-attr_nr].attr = a; check_all_attr[a-attr_nr].value = ATTR__UNKNOWN; return a; diff --git a/builtin/apply.c b/builtin/apply.c index f204cca..ae6ac6b 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -2626,7 +2626,7 @@ static void update_image(struct image *img, * NOTE: this knows that we never call remove_first_line() * on anything other than pre/post image. */ - img-line = xrealloc(img-line, nr * sizeof(*img-line)); + REALLOCARRAY(img-line, nr); img-line_allocated = img-line; } if (preimage_limit != postimage-nr) diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 47bd624..c5a06f3 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -138,10 +138,8 @@ static int parse_atom(const char *atom, const char *ep) /* Add it in, including the deref prefix */ at = used_atom_cnt; used_atom_cnt++; - used_atom = xrealloc(used_atom, -(sizeof *used_atom) * used_atom_cnt); - used_atom_type = xrealloc(used_atom_type, - (sizeof(*used_atom_type) * used_atom_cnt)); + REALLOCARRAY(used_atom, used_atom_cnt); + REALLOCARRAY(used_atom_type, used_atom_cnt); used_atom[at] = xmemdupz(atom, ep - atom); used_atom_type[at] = valid_atom[i].cmp_type; if (*atom == '*') @@ -870,8 +868,7 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f ref-flag = flag; cnt = cb-grab_cnt; - cb-grab_array = xrealloc(cb-grab_array, - sizeof(*cb-grab_array) * (cnt + 1)); + REALLOCARRAY(cb-grab_array, cnt + 1); cb-grab_array[cnt++] = ref; cb-grab_cnt = cnt; return 0; diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 5568a5b..5d03e00 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1140,9 +1140,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha int nr_objects_initial = nr_objects; if (nr_unresolved = 0) die(_(confusion beyond insanity)); - objects = xrealloc(objects, - (nr_objects + nr_unresolved + 1) - * sizeof(*objects)); + REALLOCARRAY(objects, nr_objects + nr_unresolved + 1); memset(objects + nr_objects + 1, 0, nr_unresolved * sizeof(*objects)); f = sha1fd(output_fd, curr_pack); diff --git a/builtin/log.c b/builtin/log.c index e4d8122..3852a63 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1440,7 +1440,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) continue; nr++; - list = xrealloc(list, nr * sizeof(list[0])); + REALLOCARRAY(list, nr); list[nr - 1] = commit; } if (nr == 0) diff --git a/builtin/merge.c b/builtin/merge.c index 9da9e30..92e376f 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -556,7 +556,7 @@ static void parse_branch_merge_options(char *bmo) if (argc 0) die(_(Bad branch.%s.mergeoptions string: %s), branch, split_cmdline_strerror(argc)); - argv = xrealloc(argv, sizeof(*argv) * (argc + 2)); + REALLOCARRAY(argv, argc + 2); memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
Re: [PATCH 2/3] make update-server-info more robust
Am 13.09.2014 um 22:19 schrieb Jeff King: Since git update-server-info may be called automatically as part of a push or a gc --auto, we should be robust against two processes trying to update it simultaneously. However, we currently use a fixed tempfile, which means that two simultaneous writers may step on each other's toes and end up renaming junk into place. Let's instead switch to using a unique tempfile via mkstemp. We do not want to use a lockfile here, because it's OK for two writers to simultaneously update (one will win the rename race, but that's OK; they should be writing the same information). While we're there, let's clean up a few other things: 1. Detect write errors. Report them and abort the update if any are found. 2. Free path memory rather than leaking it (and clean up the tempfile when necessary). 3. Use the pathdup functions consistently rather than static buffers or manually calculated lengths. This last one fixes a potential overflow of infofile in update_info_packs (e.g., by putting large junk into $GIT_OBJECT_DIRECTORY). However, this overflow was probably not an interesting attack vector for two reasons: a. The attacker would need to control the environment to do this, in which case it was already game-over. b. During its setup phase, git checks that the directory actually exists, which means it is probably shorter than PATH_MAX anyway. Because both update_info_refs and update_info_packs share these same failings (and largely duplicate each other), this patch factors out the improved error-checking version into a helper function. Signed-off-by: Jeff King p...@peff.net --- I guess point (b) may not apply on systems that have a really small PATH_MAX that does not reflect what you can actually create in the filesystem (Windows?). It's the other way around: PATH_MAX is an actual limit basically only on Windows [1] unless you avoid using the Windows API [2]. Regardless of the security implications, getting rid of more PATH_MAX buffers is a good move. And I looked only briefly at your patch, but I like the three bullet points above. :) René [1] http://insanecoding.blogspot.de/2007/11/pathmax-simply-isnt.html [2] http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx -- 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] repack: call prune_packed_objects() and update_server_info() directly
Call the functions behind git prune-packed and git update-server-info
directly instead of using run_command(). This is shorter, easier and
quicker.
Signed-off-by: Rene Scharfe l@web.de
---
builtin/repack.c | 23 ++-
1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index fc088db..2aae05d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -377,6 +377,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
/* End of pack replacement. */
if (delete_redundant) {
+ int opts = 0;
sort_string_list(names);
for_each_string_list_item(item, existing_packs) {
char *sha1;
@@ -387,25 +388,13 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
if (!string_list_has_string(names, sha1))
remove_redundant_pack(packdir, item-string);
}
- argv_array_push(cmd_args, prune-packed);
- if (quiet)
- argv_array_push(cmd_args, --quiet);
-
- memset(cmd, 0, sizeof(cmd));
- cmd.argv = cmd_args.argv;
- cmd.git_cmd = 1;
- run_command(cmd);
- argv_array_clear(cmd_args);
+ if (!quiet isatty(2))
+ opts |= PRUNE_PACKED_VERBOSE;
+ prune_packed_objects(opts);
}
- if (!no_update_server_info) {
- argv_array_push(cmd_args, update-server-info);
- memset(cmd, 0, sizeof(cmd));
- cmd.argv = cmd_args.argv;
- cmd.git_cmd = 1;
- run_command(cmd);
- argv_array_clear(cmd_args);
- }
+ if (!no_update_server_info)
+ update_server_info(0);
remove_temporary_files();
string_list_clear(names, 0);
string_list_clear(rollback, 0);
--
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: [RFC PATCH 2/2] headers: include dependent headers
Am 08.09.2014 um 19:50 schrieb Junio C Hamano: René Scharfe l@web.de 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 dav...@gmail.com --- 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
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 l@web.de writes: Move strbuf_addchars() to strbuf.c, where it belongs, and make it available for other callers. Signed-off-by: Rene Scharfe l@web.de 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 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 dav...@gmail.com --- 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 l@web.de
---
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 l@web.de
---
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: 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: initializer does not fit or is out of
range: 154
ctype.c, line 53: warning: initializer does not fit or is out of
range: 155
ctype.c, line 53: warning: initializer does not fit or is out of
range: 156
ctype.c, line 53: warning:
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 dav...@gmail.com
---
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 dav...@gmail.com --- 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: [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 pstod...@redhat.com and Martin von Gagern martin.vgag...@gmx.net 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: [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 dups.pack
+ test_must_fail git index-pack --stdin --strict dups.pack
+'
+
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 reduced accuracy of conflicting-access
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 have any bad side effects, though, so no sign-off at this
[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 l@web.de
---
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 l@web.de
---
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
[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 j...@kdbg.org
Signed-off-by: Rene Scharfe l@web.de
---
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
---
[PATCH v2 3/4] run-command: call run_command_v_opt_cd_env() instead of duplicating it
Signed-off-by: Rene Scharfe l@web.de
---
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 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 l@web.de
---
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
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] 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 l@web.de
---
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 l@web.de
---
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 l@web.de
---
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] 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 stefanbel...@gmail.com --- 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] 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 avd...@math.vsu.ru
Signed-off-by: Rene Scharfe l@web.de
---
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: Bug report about symlinks
Am 03.08.2014 um 19:19 schrieb Junio C Hamano: René Scharfe l@web.de 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
Re: Bug report about symlinks
Am 01.08.2014 um 18:23 schrieb Junio C Hamano:
René Scharfe l@web.de 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 file... to update what will be committed)
(use git checkout -- file... to discard changes in working directory)
deleted:b/different
Untracked files:
(use git add file... 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
