* [PATCH 3/9] grep: refactor the concept of "grep source" into an object
From: Jeff King @ 2012-02-02 8:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>
The main interface to the low-level grep code is
grep_buffer, which takes a pointer to a buffer and a size.
This is convenient and flexible (we use it to grep commit
bodies, files on disk, and blobs by sha1), but it makes it
hard to pass extra information about what we are grepping
(either for correctness, like overriding binary
auto-detection, or for optimizations, like lazily loading
blob contents).
Instead, let's encapsulate the idea of a "grep source",
including the buffer, its size, and where the data is coming
from. This is similar to the diff_filespec structure used by
the diff code (unsurprising, since future patches will
implement some of the same optimizations found there).
The diffstat is slightly scarier than the actual patch
content. Most of the modified lines are simply replacing
access to raw variables with their counterparts that are now
in a "struct grep_source". Most of the added lines were
taken from builtin/grep.c, which partially abstracted the
idea of grep sources (for file vs sha1 sources).
Instead of dropping the now-redundant code, this patch
leaves builtin/grep.c using the traditional grep_buffer
interface (which now wraps the grep_source interface). That
makes it easy to test that there is no change of behavior
(yet).
Signed-off-by: Jeff King <peff@peff.net>
---
grep.c | 198 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
grep.h | 22 +++++++
2 files changed, 186 insertions(+), 34 deletions(-)
diff --git a/grep.c b/grep.c
index db58a29..8204ca2 100644
--- a/grep.c
+++ b/grep.c
@@ -837,13 +837,13 @@ pthread_mutex_t grep_read_mutex;
#define grep_attr_unlock()
#endif
-static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)
+static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)
{
xdemitconf_t *xecfg = opt->priv;
if (xecfg && !xecfg->find_func) {
struct userdiff_driver *drv;
grep_attr_lock();
- drv = userdiff_find_by_path(name);
+ drv = userdiff_find_by_path(gs->name);
grep_attr_unlock();
if (drv && drv->funcname.pattern) {
const struct userdiff_funcname *pe = &drv->funcname;
@@ -866,33 +866,33 @@ static int match_funcname(struct grep_opt *opt, const char *name, char *bol, cha
return 0;
}
-static void show_funcname_line(struct grep_opt *opt, const char *name,
- char *buf, char *bol, unsigned lno)
+static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
+ char *bol, unsigned lno)
{
- while (bol > buf) {
+ while (bol > gs->buf) {
char *eol = --bol;
- while (bol > buf && bol[-1] != '\n')
+ while (bol > gs->buf && bol[-1] != '\n')
bol--;
lno--;
if (lno <= opt->last_shown)
break;
- if (match_funcname(opt, name, bol, eol)) {
- show_line(opt, bol, eol, name, lno, '=');
+ if (match_funcname(opt, gs, bol, eol)) {
+ show_line(opt, bol, eol, gs->name, lno, '=');
break;
}
}
}
-static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,
+static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
char *bol, char *end, unsigned lno)
{
unsigned cur = lno, from = 1, funcname_lno = 0;
int funcname_needed = !!opt->funcname;
- if (opt->funcbody && !match_funcname(opt, name, bol, end))
+ if (opt->funcbody && !match_funcname(opt, gs, bol, end))
funcname_needed = 2;
if (opt->pre_context < lno)
@@ -901,14 +901,14 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,
from = opt->last_shown + 1;
/* Rewind. */
- while (bol > buf &&
+ while (bol > gs->buf &&
cur > (funcname_needed == 2 ? opt->last_shown + 1 : from)) {
char *eol = --bol;
- while (bol > buf && bol[-1] != '\n')
+ while (bol > gs->buf && bol[-1] != '\n')
bol--;
cur--;
- if (funcname_needed && match_funcname(opt, name, bol, eol)) {
+ if (funcname_needed && match_funcname(opt, gs, bol, eol)) {
funcname_lno = cur;
funcname_needed = 0;
}
@@ -916,7 +916,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,
/* We need to look even further back to find a function signature. */
if (opt->funcname && funcname_needed)
- show_funcname_line(opt, name, buf, bol, cur);
+ show_funcname_line(opt, gs, bol, cur);
/* Back forward. */
while (cur < lno) {
@@ -924,7 +924,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,
while (*eol != '\n')
eol++;
- show_line(opt, bol, eol, name, cur, sign);
+ show_line(opt, bol, eol, gs->name, cur, sign);
bol = eol + 1;
cur++;
}
@@ -991,11 +991,10 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)
fwrite(buf, size, 1, stdout);
}
-static int grep_buffer_1(struct grep_opt *opt, const char *name,
- char *buf, unsigned long size, int collect_hits)
+static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)
{
- char *bol = buf;
- unsigned long left = size;
+ char *bol;
+ unsigned long left;
unsigned lno = 1;
unsigned last_hit = 0;
int binary_match_only = 0;
@@ -1023,13 +1022,16 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
}
opt->last_shown = 0;
+ if (grep_source_load(gs) < 0)
+ return 0;
+
switch (opt->binary) {
case GREP_BINARY_DEFAULT:
- if (buffer_is_binary(buf, size))
+ if (buffer_is_binary(gs->buf, gs->size))
binary_match_only = 1;
break;
case GREP_BINARY_NOMATCH:
- if (buffer_is_binary(buf, size))
+ if (buffer_is_binary(gs->buf, gs->size))
return 0; /* Assume unmatch */
break;
case GREP_BINARY_TEXT:
@@ -1043,6 +1045,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
try_lookahead = should_lookahead(opt);
+ bol = gs->buf;
+ left = gs->size;
while (left) {
char *eol, ch;
int hit;
@@ -1091,14 +1095,14 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
if (opt->status_only)
return 1;
if (opt->name_only) {
- show_name(opt, name);
+ show_name(opt, gs->name);
return 1;
}
if (opt->count)
goto next_line;
if (binary_match_only) {
opt->output(opt, "Binary file ", 12);
- output_color(opt, name, strlen(name),
+ output_color(opt, gs->name, strlen(gs->name),
opt->color_filename);
opt->output(opt, " matches\n", 9);
return 1;
@@ -1107,23 +1111,23 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
* pre-context lines, we would need to show them.
*/
if (opt->pre_context || opt->funcbody)
- show_pre_context(opt, name, buf, bol, eol, lno);
+ show_pre_context(opt, gs, bol, eol, lno);
else if (opt->funcname)
- show_funcname_line(opt, name, buf, bol, lno);
- show_line(opt, bol, eol, name, lno, ':');
+ show_funcname_line(opt, gs, bol, lno);
+ show_line(opt, bol, eol, gs->name, lno, ':');
last_hit = lno;
if (opt->funcbody)
show_function = 1;
goto next_line;
}
- if (show_function && match_funcname(opt, name, bol, eol))
+ if (show_function && match_funcname(opt, gs, bol, eol))
show_function = 0;
if (show_function ||
(last_hit && lno <= last_hit + opt->post_context)) {
/* If the last hit is within the post context,
* we need to show this line.
*/
- show_line(opt, bol, eol, name, lno, '-');
+ show_line(opt, bol, eol, gs->name, lno, '-');
}
next_line:
@@ -1141,7 +1145,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
return 0;
if (opt->unmatch_name_only) {
/* We did not see any hit, so we want to show this */
- show_name(opt, name);
+ show_name(opt, gs->name);
return 1;
}
@@ -1155,7 +1159,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
*/
if (opt->count && count) {
char buf[32];
- output_color(opt, name, strlen(name), opt->color_filename);
+ output_color(opt, gs->name, strlen(gs->name), opt->color_filename);
output_sep(opt, ':');
snprintf(buf, sizeof(buf), "%u\n", count);
opt->output(opt, buf, strlen(buf));
@@ -1190,23 +1194,149 @@ static int chk_hit_marker(struct grep_expr *x)
}
}
-int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)
+int grep_source(struct grep_opt *opt, struct grep_source *gs)
{
/*
* we do not have to do the two-pass grep when we do not check
* buffer-wide "all-match".
*/
if (!opt->all_match)
- return grep_buffer_1(opt, name, buf, size, 0);
+ return grep_source_1(opt, gs, 0);
/* Otherwise the toplevel "or" terms hit a bit differently.
* We first clear hit markers from them.
*/
clr_hit_marker(opt->pattern_expression);
- grep_buffer_1(opt, name, buf, size, 1);
+ grep_source_1(opt, gs, 1);
if (!chk_hit_marker(opt->pattern_expression))
return 0;
- return grep_buffer_1(opt, name, buf, size, 0);
+ return grep_source_1(opt, gs, 0);
+}
+
+int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)
+{
+ struct grep_source gs;
+ int r;
+
+ grep_source_init(&gs, GREP_SOURCE_BUF, name, NULL);
+ gs.buf = buf;
+ gs.size = size;
+
+ r = grep_source(opt, &gs);
+
+ grep_source_clear(&gs);
+ return r;
+}
+
+void grep_source_init(struct grep_source *gs, enum grep_source_type type,
+ const char *name, const void *identifier)
+{
+ gs->type = type;
+ gs->name = name ? xstrdup(name) : NULL;
+ gs->buf = NULL;
+ gs->size = 0;
+
+ switch (type) {
+ case GREP_SOURCE_FILE:
+ gs->identifier = xstrdup(identifier);
+ break;
+ case GREP_SOURCE_SHA1:
+ gs->identifier = xmalloc(20);
+ memcpy(gs->identifier, identifier, 20);
+ break;
+ case GREP_SOURCE_BUF:
+ gs->identifier = NULL;
+ }
+}
+
+void grep_source_clear(struct grep_source *gs)
+{
+ free(gs->name);
+ gs->name = NULL;
+ free(gs->identifier);
+ gs->identifier = NULL;
+ grep_source_clear_data(gs);
+}
+
+void grep_source_clear_data(struct grep_source *gs)
+{
+ switch (gs->type) {
+ case GREP_SOURCE_FILE:
+ case GREP_SOURCE_SHA1:
+ free(gs->buf);
+ gs->buf = NULL;
+ gs->size = 0;
+ break;
+ case GREP_SOURCE_BUF:
+ /* leave user-provided buf intact */
+ break;
+ }
+}
+
+static int grep_source_load_sha1(struct grep_source *gs)
+{
+ enum object_type type;
+
+ grep_read_lock();
+ gs->buf = read_sha1_file(gs->identifier, &type, &gs->size);
+ grep_read_unlock();
+
+ if (!gs->buf)
+ return error(_("'%s': unable to read %s"),
+ gs->name,
+ sha1_to_hex(gs->identifier));
+ return 0;
+}
+
+static int grep_source_load_file(struct grep_source *gs)
+{
+ const char *filename = gs->identifier;
+ struct stat st;
+ char *data;
+ size_t size;
+ int i;
+
+ if (lstat(filename, &st) < 0) {
+ err_ret:
+ if (errno != ENOENT)
+ error(_("'%s': %s"), filename, strerror(errno));
+ return -1;
+ }
+ if (!S_ISREG(st.st_mode))
+ return -1;
+ size = xsize_t(st.st_size);
+ i = open(filename, O_RDONLY);
+ if (i < 0)
+ goto err_ret;
+ data = xmalloc(size + 1);
+ if (st.st_size != read_in_full(i, data, size)) {
+ error(_("'%s': short read %s"), filename, strerror(errno));
+ close(i);
+ free(data);
+ return -1;
+ }
+ close(i);
+ data[size] = 0;
+
+ gs->buf = data;
+ gs->size = size;
+ return 0;
+}
+
+int grep_source_load(struct grep_source *gs)
+{
+ if (gs->buf)
+ return 0;
+
+ switch (gs->type) {
+ case GREP_SOURCE_FILE:
+ return grep_source_load_file(gs);
+ case GREP_SOURCE_SHA1:
+ return grep_source_load_sha1(gs);
+ case GREP_SOURCE_BUF:
+ return gs->buf ? 0 : -1;
+ }
+ die("BUG: invalid grep_source type");
}
diff --git a/grep.h b/grep.h
index 4f1b025..e386ca4 100644
--- a/grep.h
+++ b/grep.h
@@ -129,6 +129,28 @@ extern void compile_grep_patterns(struct grep_opt *opt);
extern void free_grep_patterns(struct grep_opt *opt);
extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);
+struct grep_source {
+ char *name;
+
+ enum grep_source_type {
+ GREP_SOURCE_SHA1,
+ GREP_SOURCE_FILE,
+ GREP_SOURCE_BUF,
+ } type;
+ void *identifier;
+
+ char *buf;
+ unsigned long size;
+};
+
+void grep_source_init(struct grep_source *gs, enum grep_source_type type,
+ const char *name, const void *identifier);
+int grep_source_load(struct grep_source *gs);
+void grep_source_clear_data(struct grep_source *gs);
+void grep_source_clear(struct grep_source *gs);
+
+int grep_source(struct grep_opt *opt, struct grep_source *gs);
+
extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);
extern int grep_threads_ok(const struct grep_opt *opt);
--
1.7.9.3.gc3fce1.dirty
^ permalink raw reply related
* [PATCH 4/9] convert git-grep to use grep_source interface
From: Jeff King @ 2012-02-02 8:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>
The grep_source interface (as opposed to grep_buffer) will
eventually gives us a richer interface for telling the
low-level grep code about our buffers. Eventually this will
lead to things like better binary-file handling. For now, it
lets us drop a lot of now-redundant code.
The conversion is mostly straight-forward. One thing to note
is that the memory ownership rules for "struct grep_source"
are different than the "struct work_item" found here (the
former will copy things like the filename, rather than
taking ownership). Therefore you will also see some slight
tweaking of when filename buffers are released.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 142 +++++++++-----------------------------------------------
1 files changed, 23 insertions(+), 119 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index f4402fa..bc85a20 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -29,25 +29,12 @@ static int use_threads = 1;
#define THREADS 8
static pthread_t threads[THREADS];
-static void *load_sha1(const unsigned char *sha1, unsigned long *size,
- const char *name);
-static void *load_file(const char *filename, size_t *sz);
-
-enum work_type {WORK_SHA1, WORK_FILE};
-
/* We use one producer thread and THREADS consumer
* threads. The producer adds struct work_items to 'todo' and the
* consumers pick work items from the same array.
*/
struct work_item {
- enum work_type type;
- char *name;
-
- /* if type == WORK_SHA1, then 'identifier' is a SHA1,
- * otherwise type == WORK_FILE, and 'identifier' is a NUL
- * terminated filename.
- */
- void *identifier;
+ struct grep_source source;
char done;
struct strbuf out;
};
@@ -98,7 +85,8 @@ static pthread_cond_t cond_result;
static int skip_first_line;
-static void add_work(enum work_type type, char *name, void *id)
+static void add_work(enum grep_source_type type, const char *name,
+ const void *id)
{
grep_lock();
@@ -106,9 +94,7 @@ static void add_work(enum work_type type, char *name, void *id)
pthread_cond_wait(&cond_write, &grep_mutex);
}
- todo[todo_end].type = type;
- todo[todo_end].name = name;
- todo[todo_end].identifier = id;
+ grep_source_init(&todo[todo_end].source, type, name, id);
todo[todo_end].done = 0;
strbuf_reset(&todo[todo_end].out);
todo_end = (todo_end + 1) % ARRAY_SIZE(todo);
@@ -136,21 +122,6 @@ static struct work_item *get_work(void)
return ret;
}
-static void grep_sha1_async(struct grep_opt *opt, char *name,
- const unsigned char *sha1)
-{
- unsigned char *s;
- s = xmalloc(20);
- memcpy(s, sha1, 20);
- add_work(WORK_SHA1, name, s);
-}
-
-static void grep_file_async(struct grep_opt *opt, char *name,
- const char *filename)
-{
- add_work(WORK_FILE, name, xstrdup(filename));
-}
-
static void work_done(struct work_item *w)
{
int old_done;
@@ -177,8 +148,7 @@ static void work_done(struct work_item *w)
write_or_die(1, p, len);
}
- free(w->name);
- free(w->identifier);
+ grep_source_clear(&w->source);
}
if (old_done != todo_done)
@@ -201,25 +171,8 @@ static void *run(void *arg)
break;
opt->output_priv = w;
- if (w->type == WORK_SHA1) {
- unsigned long sz;
- void* data = load_sha1(w->identifier, &sz, w->name);
-
- if (data) {
- hit |= grep_buffer(opt, w->name, data, sz);
- free(data);
- }
- } else if (w->type == WORK_FILE) {
- size_t sz;
- void* data = load_file(w->identifier, &sz);
- if (data) {
- hit |= grep_buffer(opt, w->name, data, sz);
- free(data);
- }
- } else {
- assert(0);
- }
-
+ hit |= grep_source(opt, &w->source);
+ grep_source_clear_data(&w->source);
work_done(w);
}
free_grep_patterns(arg);
@@ -365,23 +318,10 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type
return data;
}
-static void *load_sha1(const unsigned char *sha1, unsigned long *size,
- const char *name)
-{
- enum object_type type;
- void *data = lock_and_read_sha1_file(sha1, &type, size);
-
- if (!data)
- error(_("'%s': unable to read %s"), name, sha1_to_hex(sha1));
-
- return data;
-}
-
static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
const char *filename, int tree_name_len)
{
struct strbuf pathbuf = STRBUF_INIT;
- char *name;
if (opt->relative && opt->prefix_length) {
quote_path_relative(filename + tree_name_len, -1, &pathbuf,
@@ -391,87 +331,51 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
strbuf_addstr(&pathbuf, filename);
}
- name = strbuf_detach(&pathbuf, NULL);
-
#ifndef NO_PTHREADS
if (use_threads) {
- grep_sha1_async(opt, name, sha1);
+ add_work(GREP_SOURCE_SHA1, pathbuf.buf, sha1);
+ strbuf_release(&pathbuf);
return 0;
} else
#endif
{
+ struct grep_source gs;
int hit;
- unsigned long sz;
- void *data = load_sha1(sha1, &sz, name);
- if (!data)
- hit = 0;
- else
- hit = grep_buffer(opt, name, data, sz);
- free(data);
- free(name);
- return hit;
- }
-}
+ grep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, sha1);
+ strbuf_release(&pathbuf);
+ hit = grep_source(opt, &gs);
-static void *load_file(const char *filename, size_t *sz)
-{
- struct stat st;
- char *data;
- int i;
-
- if (lstat(filename, &st) < 0) {
- err_ret:
- if (errno != ENOENT)
- error(_("'%s': %s"), filename, strerror(errno));
- return NULL;
- }
- if (!S_ISREG(st.st_mode))
- return NULL;
- *sz = xsize_t(st.st_size);
- i = open(filename, O_RDONLY);
- if (i < 0)
- goto err_ret;
- data = xmalloc(*sz + 1);
- if (st.st_size != read_in_full(i, data, *sz)) {
- error(_("'%s': short read %s"), filename, strerror(errno));
- close(i);
- free(data);
- return NULL;
+ grep_source_clear(&gs);
+ return hit;
}
- close(i);
- data[*sz] = 0;
- return data;
}
static int grep_file(struct grep_opt *opt, const char *filename)
{
struct strbuf buf = STRBUF_INIT;
- char *name;
if (opt->relative && opt->prefix_length)
quote_path_relative(filename, -1, &buf, opt->prefix);
else
strbuf_addstr(&buf, filename);
- name = strbuf_detach(&buf, NULL);
#ifndef NO_PTHREADS
if (use_threads) {
- grep_file_async(opt, name, filename);
+ add_work(GREP_SOURCE_FILE, buf.buf, filename);
+ strbuf_release(&buf);
return 0;
} else
#endif
{
+ struct grep_source gs;
int hit;
- size_t sz;
- void *data = load_file(filename, &sz);
- if (!data)
- hit = 0;
- else
- hit = grep_buffer(opt, name, data, sz);
- free(data);
- free(name);
+ grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename);
+ strbuf_release(&buf);
+ hit = grep_source(opt, &gs);
+
+ grep_source_clear(&gs);
return hit;
}
}
--
1.7.9.3.gc3fce1.dirty
^ permalink raw reply related
* [PATCH 5/9] grep: drop grep_buffer's "name" parameter
From: Jeff King @ 2012-02-02 8:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>
Before the grep_source interface existed, grep_buffer was
used by two types of callers:
1. Ones which pulled a file into a buffer, and then wanted
to supply the file's name for the output (i.e.,
git grep).
2. Ones which really just wanted to grep a buffer (i.e.,
git log --grep).
Callers in set (1) should now be using grep_source. Callers
in set (2) always pass NULL for the "name" parameter of
grep_buffer. We can therefore get rid of this now-useless
parameter.
Signed-off-by: Jeff King <peff@peff.net>
---
This one isn't necessary, obviously, but I think it's a nice clean-up
after the last two patches.
grep.c | 4 ++--
grep.h | 2 +-
revision.c | 1 -
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/grep.c b/grep.c
index 8204ca2..2a3fe7c 100644
--- a/grep.c
+++ b/grep.c
@@ -1215,12 +1215,12 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs)
return grep_source_1(opt, gs, 0);
}
-int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)
+int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)
{
struct grep_source gs;
int r;
- grep_source_init(&gs, GREP_SOURCE_BUF, name, NULL);
+ grep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL);
gs.buf = buf;
gs.size = size;
diff --git a/grep.h b/grep.h
index e386ca4..8bf3001 100644
--- a/grep.h
+++ b/grep.h
@@ -127,7 +127,7 @@ extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const cha
extern void append_header_grep_pattern(struct grep_opt *, enum grep_header_field, const char *);
extern void compile_grep_patterns(struct grep_opt *opt);
extern void free_grep_patterns(struct grep_opt *opt);
-extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);
+extern int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size);
struct grep_source {
char *name;
diff --git a/revision.c b/revision.c
index c97d834..819ff01 100644
--- a/revision.c
+++ b/revision.c
@@ -2149,7 +2149,6 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
return 1;
return grep_buffer(&opt->grep_filter,
- NULL, /* we say nothing, not even filename */
commit->buffer, strlen(commit->buffer));
}
--
1.7.9.3.gc3fce1.dirty
^ permalink raw reply related
* [PATCH 6/9] grep: cache userdiff_driver in grep_source
From: Jeff King @ 2012-02-02 8:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>
Right now, grep only uses the userdiff_driver for one thing:
looking up funcname patterns for "-p" and "-W". As new uses
for userdiff drivers are added to the grep code, we want to
minimize attribute lookups, which can be expensive.
It might seem at first that this would also optimize multiple
lookups when the funcname pattern for a file is needed
multiple times. However, the compiled funcname pattern is
already cached in struct grep_opt's "priv" member, so
multiple lookups are already suppressed.
Signed-off-by: Jeff King <peff@peff.net>
---
grep.c | 22 ++++++++++++++++------
grep.h | 4 ++++
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/grep.c b/grep.c
index 2a3fe7c..bb18569 100644
--- a/grep.c
+++ b/grep.c
@@ -841,12 +841,9 @@ static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bo
{
xdemitconf_t *xecfg = opt->priv;
if (xecfg && !xecfg->find_func) {
- struct userdiff_driver *drv;
- grep_attr_lock();
- drv = userdiff_find_by_path(gs->name);
- grep_attr_unlock();
- if (drv && drv->funcname.pattern) {
- const struct userdiff_funcname *pe = &drv->funcname;
+ grep_source_load_driver(gs);
+ if (gs->driver->funcname.pattern) {
+ const struct userdiff_funcname *pe = &gs->driver->funcname;
xdiff_set_find_func(xecfg, pe->pattern, pe->cflags);
} else {
xecfg = opt->priv = NULL;
@@ -1237,6 +1234,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
gs->name = name ? xstrdup(name) : NULL;
gs->buf = NULL;
gs->size = 0;
+ gs->driver = NULL;
switch (type) {
case GREP_SOURCE_FILE:
@@ -1340,3 +1338,15 @@ int grep_source_load(struct grep_source *gs)
}
die("BUG: invalid grep_source type");
}
+
+void grep_source_load_driver(struct grep_source *gs)
+{
+ if (gs->driver)
+ return;
+
+ grep_attr_lock();
+ gs->driver = userdiff_find_by_path(gs->name);
+ if (!gs->driver)
+ gs->driver = userdiff_find_by_name("default");
+ grep_attr_unlock();
+}
diff --git a/grep.h b/grep.h
index 8bf3001..73b28c2 100644
--- a/grep.h
+++ b/grep.h
@@ -9,6 +9,7 @@ typedef int pcre_extra;
#endif
#include "kwset.h"
#include "thread-utils.h"
+#include "userdiff.h"
enum grep_pat_token {
GREP_PATTERN,
@@ -141,6 +142,8 @@ struct grep_source {
char *buf;
unsigned long size;
+
+ struct userdiff_driver *driver;
};
void grep_source_init(struct grep_source *gs, enum grep_source_type type,
@@ -148,6 +151,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
int grep_source_load(struct grep_source *gs);
void grep_source_clear_data(struct grep_source *gs);
void grep_source_clear(struct grep_source *gs);
+void grep_source_load_driver(struct grep_source *gs);
int grep_source(struct grep_opt *opt, struct grep_source *gs);
--
1.7.9.3.gc3fce1.dirty
^ permalink raw reply related
* [PATCH 7/9] grep: respect diff attributes for binary-ness
From: Jeff King @ 2012-02-02 8:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>
There is currently no way for users to tell git-grep that a
particular path is or is not a binary file; instead, grep
always relies on its auto-detection (or the user specifying
"-a" to treat all binary-looking files like text).
This patch teaches git-grep to use the same attribute lookup
that is used by git-diff. We could add a new "grep" flag,
but that is unnecessarily complex and unlikely to be useful.
Despite the name, the "-diff" attribute (or "diff=foo" and
the associated diff.foo.binary config option) are really
about describing the contents of the path. It's simply
historical that diff was the only thing that cared about
these attributes in the past.
And if this simple approach turns out to be insufficient, we
still have a backwards-compatible path forward: we can add a
separate "grep" attribute, and fall back to respecting
"diff" if it is unset.
Signed-off-by: Jeff King <peff@peff.net>
---
grep.c | 16 ++++++++++++++--
grep.h | 1 +
t/t7008-grep-binary.sh | 24 ++++++++++++++++++++++++
3 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/grep.c b/grep.c
index bb18569..a50d161 100644
--- a/grep.c
+++ b/grep.c
@@ -1024,11 +1024,11 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
switch (opt->binary) {
case GREP_BINARY_DEFAULT:
- if (buffer_is_binary(gs->buf, gs->size))
+ if (grep_source_is_binary(gs))
binary_match_only = 1;
break;
case GREP_BINARY_NOMATCH:
- if (buffer_is_binary(gs->buf, gs->size))
+ if (grep_source_is_binary(gs))
return 0; /* Assume unmatch */
break;
case GREP_BINARY_TEXT:
@@ -1350,3 +1350,15 @@ void grep_source_load_driver(struct grep_source *gs)
gs->driver = userdiff_find_by_name("default");
grep_attr_unlock();
}
+
+int grep_source_is_binary(struct grep_source *gs)
+{
+ grep_source_load_driver(gs);
+ if (gs->driver->binary != -1)
+ return gs->driver->binary;
+
+ if (!grep_source_load(gs))
+ return buffer_is_binary(gs->buf, gs->size);
+
+ return 0;
+}
diff --git a/grep.h b/grep.h
index 73b28c2..36e49d8 100644
--- a/grep.h
+++ b/grep.h
@@ -152,6 +152,7 @@ int grep_source_load(struct grep_source *gs);
void grep_source_clear_data(struct grep_source *gs);
void grep_source_clear(struct grep_source *gs);
void grep_source_load_driver(struct grep_source *gs);
+int grep_source_is_binary(struct grep_source *gs);
int grep_source(struct grep_opt *opt, struct grep_source *gs);
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index 917a264..fd6410f 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -99,4 +99,28 @@ test_expect_success 'git grep y<NUL>x a' "
test_must_fail git grep -f f a
"
+test_expect_success 'grep respects binary diff attribute' '
+ echo text >t &&
+ git add t &&
+ echo t:text >expect &&
+ git grep text t >actual &&
+ test_cmp expect actual &&
+ echo "t -diff" >.gitattributes &&
+ echo "Binary file t matches" >expect &&
+ git grep text t >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep respects not-binary diff attribute' '
+ echo binQary | q_to_nul >b &&
+ git add b &&
+ echo "Binary file b matches" >expect &&
+ git grep bin b >actual &&
+ test_cmp expect actual &&
+ echo "b diff" >.gitattributes &&
+ echo "b:binQary" >expect &&
+ git grep bin b | nul_to_q >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.9.3.gc3fce1.dirty
^ permalink raw reply related
* [PATCH 8/9] grep: load file data after checking binary-ness
From: Jeff King @ 2012-02-02 8:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>
Usually we load each file to grep into memory, check whether
it's binary, and then either grep it (the default) or not
(if "-I" was given).
In the "-I" case, we can skip loading the file entirely if
it is marked as binary via gitattributes. On my giant
3-gigabyte media repository, doing "git grep -I foo" went
from:
real 0m0.712s
user 0m0.044s
sys 0m4.780s
to:
real 0m0.026s
user 0m0.016s
sys 0m0.020s
Obviously this is an extreme example. The repo is almost
entirely binary files, and you can see that we spent all of
our time asking the kernel to read() the data. However, with
a cold disk cache, even avoiding a few binary files can have
an impact.
Signed-off-by: Jeff King <peff@peff.net>
---
grep.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/grep.c b/grep.c
index a50d161..3821400 100644
--- a/grep.c
+++ b/grep.c
@@ -1019,9 +1019,6 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
}
opt->last_shown = 0;
- if (grep_source_load(gs) < 0)
- return 0;
-
switch (opt->binary) {
case GREP_BINARY_DEFAULT:
if (grep_source_is_binary(gs))
@@ -1042,6 +1039,9 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
try_lookahead = should_lookahead(opt);
+ if (grep_source_load(gs) < 0)
+ return 0;
+
bol = gs->buf;
left = gs->size;
while (left) {
--
1.7.9.3.gc3fce1.dirty
^ permalink raw reply related
* [PATCH 9/9] grep: pre-load userdiff drivers when threaded
From: Jeff King @ 2012-02-02 8:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>
The low-level grep_source code will automatically load the
userdiff driver to see whether a file is binary. However,
when we are threaded, it will load the drivers in a
non-deterministic order, handling each one as its assigned
thread happens to be scheduled.
Meanwhile, the attribute lookup code (which underlies the
userdiff driver lookup) is optimized to handle paths in
sequential order (because they tend to share the same
gitattributes files). Multi-threading the lookups destroys
the locality and makes this optimization less effective.
We can fix this by pre-loading the userdiff driver in the
main thread, before we hand off the file to a worker thread.
My best-of-five for "git grep foo" on the linux-2.6
repository went from:
real 0m0.391s
user 0m1.708s
sys 0m0.584s
to:
real 0m0.360s
user 0m1.576s
sys 0m0.572s
Not a huge speedup, but it's quite easy to do. The only
trick is that we shouldn't perform this optimization if "-a"
was used, in which case we won't bother checking whether
the files are binary at all.
Signed-off-by: Jeff King <peff@peff.net>
---
The speedup is especially unimpressive when you consider that it won't
grow as the grep load grows. This is a pretty fast grep. If you used a
real regex, the whole thing would take even longer, and you will still
only be shaving off a few tens of milliseconds. So I wouldn't be
heart-broken if this patch was dropped. I included it because it's easy
to do, and maybe somebody with a slower machine would find the absolute
time difference more noticeable.
builtin/grep.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index bc85a20..9fc3e95 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -85,8 +85,8 @@ static pthread_cond_t cond_result;
static int skip_first_line;
-static void add_work(enum grep_source_type type, const char *name,
- const void *id)
+static void add_work(struct grep_opt *opt, enum grep_source_type type,
+ const char *name, const void *id)
{
grep_lock();
@@ -95,6 +95,8 @@ static void add_work(enum grep_source_type type, const char *name,
}
grep_source_init(&todo[todo_end].source, type, name, id);
+ if (opt->binary != GREP_BINARY_TEXT)
+ grep_source_load_driver(&todo[todo_end].source);
todo[todo_end].done = 0;
strbuf_reset(&todo[todo_end].out);
todo_end = (todo_end + 1) % ARRAY_SIZE(todo);
@@ -333,7 +335,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
#ifndef NO_PTHREADS
if (use_threads) {
- add_work(GREP_SOURCE_SHA1, pathbuf.buf, sha1);
+ add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, sha1);
strbuf_release(&pathbuf);
return 0;
} else
@@ -362,7 +364,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
#ifndef NO_PTHREADS
if (use_threads) {
- add_work(GREP_SOURCE_FILE, buf.buf, filename);
+ add_work(opt, GREP_SOURCE_FILE, buf.buf, filename);
strbuf_release(&buf);
return 0;
} else
--
1.7.9.3.gc3fce1.dirty
^ permalink raw reply related
* Re: [PATCH 0/9] respect binary attribute in grep
From: Jeff King @ 2012-02-02 8:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>
On Thu, Feb 02, 2012 at 03:17:47AM -0500, Jeff King wrote:
> I implemented all of the other optimizations I mentioned except the
> "only stream the first few bytes when auto-detecting binary-ness" one.
> However, it should be easy to do on top of these changes. I need to
> re-visit the similar change to diff_filespec_is_binary, and I'll do both
> at the same time.
Oh, and I didn't even think about implementing streaming grep. The
context-finding code relies on being able to backtrack through the file
in memory. We _could_ implement streaming only for binary files (i.e.,
when we will just print "Binary file foo matches"). However, I suspect
people with big binary files will want to be using "-I" anyway, so as to
avoid even pulling the data from disk at all.
We might eventually want to add a config-option version of "-I" for
people who have repositories of mixed source code and large binary
assets.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02 8:34 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, SZEDER Gábor, Shawn O. Pearce
In-Reply-To: <20120202081622.GB3823@burratino>
On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> And yet another bug in zsh[1] causes a mismatch; zsh seems to have
>> problem emulating wordspliting, but only on the command substitution.
>
> Patches didn't hit the list again. Any idea why?
No. A bug in list software?
I didn't get any warning or error.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02 8:48 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <1328145320-14071-1-git-send-email-felipe.contreras@gmail.com>
Hi,
Felipe Contreras wrote:
> Felipe Contreras (4):
> completion: be nicer with zsh
Since I can't find this patch in the mail archive, I'll reply here.
Luckily the most important bit is above already.
I think I mentioned before that this subject line is what will appear
in the shortlog and the shortlog is all that some people will see of
the changelog, so it should include a self-contained description of
the impact of the patch.
However, clearly I did not say it clearly enough. :) I guess it's
better to take a cue from storytellers and show rather than tell.
(Please don't take this as a precedent --- I will not always be doing
the style fixes myself, and sometimes will consider a patch to scratch
someone else's itch not worth the trouble and work on something else.)
-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Thu, 2 Feb 2012 03:15:17 +0200
Subject: completion: avoid default value assignment on : true command
zsh versions from 4.3.0 to present (4.3.15) do not correctly propagate
the SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)}
expressions. For example, after running
emulate sh
fn () {
var='one two'
printf '%s\n' $var
}
x=$(fn)
: ${y=$(fn)}
printing "$x" results in two lines as expected, but printing "$y"
results in a single line because $var is expanded as a single word
when evaluating fn to compute y.
So avoid the construct, and use an explicit 'test -n "$foo" ||
foo=$(bar)' instead. This fixes a bug tht caused all commands to be
treated as porcelain and show up in "git <TAB><TAB>" completion,
because the list of all commands was treated as a single word in
__git_list_porcelain_commands and did not match any of the patterns
that would usually cause plumbing to be excluded.
[jn: clarified commit message, indentation style fix]
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
contrib/completion/git-completion.bash | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 78be1958..d7965daf 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -676,7 +676,8 @@ __git_merge_strategies=
# is needed.
__git_compute_merge_strategies ()
{
- : ${__git_merge_strategies:=$(__git_list_merge_strategies)}
+ [ -n "$__git_merge_strategies" ] ||
+ __git_merge_strategies=$(__git_list_merge_strategies)
}
__git_complete_revlist_file ()
@@ -854,7 +855,8 @@ __git_list_all_commands ()
__git_all_commands=
__git_compute_all_commands ()
{
- : ${__git_all_commands:=$(__git_list_all_commands)}
+ [ -n "$__git_all_commands" ] ||
+ __git_all_commands=$(__git_list_all_commands)
}
__git_list_porcelain_commands ()
@@ -947,7 +949,8 @@ __git_porcelain_commands=
__git_compute_porcelain_commands ()
{
__git_compute_all_commands
- : ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}
+ [ -n "$__git_porcelain_commands" ] ||
+ __git_porcelain_commands=$(__git_list_porcelain_commands)
}
__git_pretty_aliases ()
--
1.7.9
^ permalink raw reply related
* Re: How to find and analyze bad merges?
From: norbert.nemec @ 2012-02-02 9:01 UTC (permalink / raw)
To: git
In-Reply-To: <7vd39xy7it.fsf@alter.siamese.dyndns.org>
Am 02.02.12 09:16, schrieb Junio C Hamano:
> "norbert.nemec"<norbert.nemec@native-instruments.de> writes:
>
>> a colleague of mine happened to produce a bad merge by unintenionally
>> picking the version of the remote branch ("R") for all conflicting
>> files. Effectively, he eliminated a whole bunch of bugfixes that were
>> already on his local branch ("L").
>>
>> Obviously this was a mistake on his side, but hey: everyone makes
>> mistakes. The real problem is to find this problem afterwards,
>> possibly weeks later, when you suddenly realize that a bug that you
>> had fixed suddenly reappears.
>
> Bisect?
This is not the point: My colleague knew exactly which commit contained
the bugfix. The trouble was finding out why this bugfix disappeared even
though everything indicated that it was cleanly merged into the current
branch.
^ permalink raw reply
* Re: [PATCH v3 2/4] completion: simplify __git_remotes
From: Jonathan Nieder @ 2012-02-02 9:05 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <1328145320-14071-1-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras wrote:
> completion: simplify __git_remotes
> completion: remove unused code
In the same spirit of stealing potential shortlog lines from Junio:
-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Subject: completion: use ls -1 instead of rolling a loop to do that ourselves
This simplifies the code a great deal. In particular, it allows us to
get rid of __git_shopt, which is used only in this fuction to enable
'nullglob' in zsh.
[jn: squashed with a patch that actually gets rid of __git_shopt]
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The diffstat says it all.
contrib/completion/git-completion.bash | 37 +-------------------------------
1 files changed, 1 insertions(+), 36 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 78be1958..4612dde9 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -644,12 +644,7 @@ __git_refs_remotes ()
__git_remotes ()
{
local i ngoff IFS=$'\n' d="$(__gitdir)"
- __git_shopt -q nullglob || ngoff=1
- __git_shopt -s nullglob
- for i in "$d/remotes"/*; do
- echo ${i#$d/remotes/}
- done
- [ "$ngoff" ] && __git_shopt -u nullglob
+ test -d "$d/remotes" && ls -1 "$d/remotes"
for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
i="${i#remote.}"
echo "${i/.url*/}"
@@ -2733,33 +2728,3 @@ if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
|| complete -o default -o nospace -F _git git.exe
fi
-
-if [[ -n ${ZSH_VERSION-} ]]; then
- __git_shopt () {
- local option
- if [ $# -ne 2 ]; then
- echo "USAGE: $0 (-q|-s|-u) <option>" >&2
- return 1
- fi
- case "$2" in
- nullglob)
- option="$2"
- ;;
- *)
- echo "$0: invalid option: $2" >&2
- return 1
- esac
- case "$1" in
- -q) setopt | grep -q "$option" ;;
- -u) unsetopt "$option" ;;
- -s) setopt "$option" ;;
- *)
- echo "$0: invalid flag: $1" >&2
- return 1
- esac
- }
-else
- __git_shopt () {
- shopt "$@"
- }
-fi
--
1.7.9
^ permalink raw reply related
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02 9:10 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor
In-Reply-To: <CAMP44s3FxUmnpQevoV2ARJpWK9CJ16zXDmpJRDOLHNW6RdSc5Q@mail.gmail.com>
(dropping Shawn from cc list, since I don't think he's touched the
completion code for years)
Felipe Contreras wrote:
> On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Patches didn't hit the list again. Any idea why?
>
> No. A bug in list software?
>
> I didn't get any warning or error.
Except in response to HTML attachments, I've never seen vger return any
explanation when it decides a message is spam. It just discards them.
See [1] for details. If there's no obvious explanation, I'd suggest
contacting the postmaster.
Hope that helps, and sorry for the fuss,
Jonathan
[1] http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02 9:38 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, SZEDER Gábor
In-Reply-To: <20120202091059.GE3823@burratino>
On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (dropping Shawn from cc list, since I don't think he's touched the
> completion code for years)
> Felipe Contreras wrote:
>> On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> Patches didn't hit the list again. Any idea why?
>>
>> No. A bug in list software?
>>
>> I didn't get any warning or error.
>
> Except in response to HTML attachments, I've never seen vger return any
> explanation when it decides a message is spam. It just discards them.
Which is wrong, because when it discards them wrongly nobody knows why.
> See [1] for details. If there's no obvious explanation, I'd suggest
> contacting the postmaster.
But there's nothing like the taboo words in the mail:
http://vger.kernel.org/majordomo-taboos.txt
--
Felipe Contreras
^ permalink raw reply
* Re: General support for ! in git-config values
From: demerphq @ 2012-02-02 9:44 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <20120202023857.GA11745@sigill.intra.peff.net>
On 2 February 2012 03:38, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 02, 2012 at 02:57:14AM +0100, demerphq wrote:
>
>> > Not really.
>> >
>> > I do not think whatever "utility" value outweighs the hassle of having to
>> > think through the ramifications (including but not limited to security) of
>> > running arbitrary user command every time a value is looked up.
>>
>> Why is that your problem? If I have to enable it then isn't that my choice?
>
> From a security perspective, you want to make sure that people who
> aren't interested in your feature don't accidentally trigger it. E.g.,
> imagine I currently run a locked-down git repo but execute some commands
> on your behalf, and I allow you to set a few "known safe" config options
> like user.email. Even though I am not interested in your feature,
> respecting "!rm -rf /" in the user.email you give me would be a bad
> thing.
Like I said, I do not think this should be enabled by default, I think
it should be possible to enable it config wide. So unless this
scenario involves getting the owner of the locked down repo to enable
a config option they know nothing about, in which case I would say
there are easier attacks -- someone that stupid probably could be
talked into telling you their root password. :-)
> It's not an insurmountable problem. There could be options to turn it
> on, or turn it off, or whatever.
My thought exactly. Anyone paranoid about security would never enable
this feature. Those who are comfortable with the security issues
could.
> Or we could shrug and say that config
> is already dangerous to let other people set (which it is already, but
> only for some options).
I think that since it could be set up to be determined by the user,
that it would be no more dangerous than any other option.
> But those are the sorts of ramifications that
> need to be thought through.
I understand that. All I can say is $work uses git on a pretty large
scale, 100+ devs etc, and we use it to manage our rollout processes
which we use a lot (I cant say how often but a lot). So if it would be
useful to us it probably would be useful to others.
> (Another one is that with our current strategy, we actually read and
> parse the config files multiple times. Should your program get run many
> times?).
Again I would say this is not git's problem. If it should not be run
multiple times it is up to the user to figure out an alternative.
The general design of git seems to me to be based around providing
building blocks that people can use to build new and interesting tools
on top of, and so it seems counter to that philosophy to reject an
feature based on speculative security issues that really can't be
decided in advance but must instead be decided on a case by case
basis.
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02 9:46 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor
In-Reply-To: <CAMP44s0nD4p9=fwpLwchmGJ123onLmRaSPmOL+cYpTFCJ-jwXw@mail.gmail.com>
Felipe Contreras wrote:
> On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> See [1] for details. If there's no obvious explanation, I'd suggest
>> contacting the postmaster.
>
> But there's nothing like the taboo words in the mail:
> http://vger.kernel.org/majordomo-taboos.txt
Why are you telling me? I am not the postmaster. I can't do anything
to investigate it or fix it.
^ permalink raw reply
* Re: General support for ! in git-config values
From: Jeff King @ 2012-02-02 9:54 UTC (permalink / raw)
To: demerphq; +Cc: Junio C Hamano, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <CANgJU+X2dRP__PFAywGEisDS3xyF7fSszSQG6BO61j2TMKL3Qg@mail.gmail.com>
On Thu, Feb 02, 2012 at 10:44:05AM +0100, demerphq wrote:
> The general design of git seems to me to be based around providing
> building blocks that people can use to build new and interesting tools
> on top of, and so it seems counter to that philosophy to reject an
> feature based on speculative security issues that really can't be
> decided in advance but must instead be decided on a case by case
> basis.
I can't speak for Junio, but I am certainly not rejecting it. Only
saying that it needs to be thought through, and the utility weighed
against the costs. So far I haven't seen an actual patch to comment on
(or even a proposed syntax beyond starting a string with "!", which I
think is a non-starter due to conflicting with existing uses), nor have
I seen a concrete use case (you mentioned pulling the name/email from
ldap, but you also mentioned that there are lots of other ways of
solving that particular problem, so it's not especially compelling).
I'd be happy to hear a more concrete proposal.
-Peff
^ permalink raw reply
* Re: [PATCH] i18n: po for zh_cn
From: Frederik Schwarzer @ 2012-02-02 10:04 UTC (permalink / raw)
To: git
Am Donnerstag, 2. Februar 2012, 09:15:38 schrieben Sie:
(damn, my mail programme always answers to the sender only ...)
Hi,
> Jiang Xin <worldhello.net@gmail.com> writes:
> > 2012/2/2 Junio C Hamano <gitster@pobox.com>:
> > ...
> >
> >> (6) From time to time, the l10n coordinator will pull from
> >> "git.git" when
> >>
> >> meaningful number of changes are made to the translatable
> >> strings. I hope this would happen much less often than
> >> once per week, preferably much less frequently. The l10n
> >> coordinator updates po/git.pot and makes a commit, and
> >> notifies the l10n teams.
> >
> > notifies the l10n teams using another mailing list maybe.
>
> I personally think using the regular git@vger.kernel.org list would
> be preferrable for this. People who translate would want to learn
> the reasoning that led to the final phrasing of the messages to
> come up with usable translation, and following the main list would
> be one way to do so. Having to follow two separate list will be an
> unnecessary burden.
>
> But the choice of how to coordinate his or her work with the l10n
> teams is entirely up to the l10n coordinator.
As I see it there are two kinds of people doing translations.
Developers who also translate and people who want to translate
software but are not involved in the development.
Translations of the former group are in many cases suboptimal. But the
latter group will not follow this mailing list. They are not
interested in all the details. They live in their room, get a piece of
paper and translate it. In case something is unclear, they aks. But as
it is now, it would look something like this:
Translators receive 100+ emails per day and are supposed to filter out
the one email every 10 days that carries useful information for their
work. In practice I guess interested Translators (who are not
interested in every code detail) will unsubscribe after a few days and
then miss all the fun.
A git-i18n mailing list could coordinate that. It would not be a list
for l10n teams to do their internal coordination, but for the i18n
coordinator to notify l10n teams about updated POT files (he might
even merge PO files) and for l10n teams to ask about strings they are
unsure about. These questions would then be digested by the i18n
coordinator and brought to the attention of the developers if needed.
How does that sound?
Regards
PS: I would have even put all the POT/PO stuff in an extra Git
repository. Working with translation is decoupled from development
cleanly by the msginit/msgmerge process but well, it might not be woth
that separation for a single project. :)
^ permalink raw reply
* Re: How to find and analyze bad merges?
From: norbert.nemec @ 2012-02-02 10:05 UTC (permalink / raw)
To: git
In-Reply-To: <jgdgcv$h8n$1@dough.gmane.org>
Thinking about a possible solution:
Is there a way to re-do a merge-commit and diff the result against the
recorded merge without touching the working tree? This would be the
killer-feature to analyze a recorded merge-commit.
Am 02.02.12 09:10, schrieb norbert.nemec:
> Hi there,
>
> a colleague of mine happened to produce a bad merge by unintenionally
> picking the version of the remote branch ("R") for all conflicting
> files. Effectively, he eliminated a whole bunch of bugfixes that were
> already on his local branch ("L").
>
> Obviously this was a mistake on his side, but hey: everyone makes
> mistakes. The real problem is to find this problem afterwards, possibly
> weeks later, when you suddenly realize that a bug that you had fixed
> suddenly reappears.
>
> A "git log" on the whole repository shows both branches R and L.
> A "git show" on the bugfix commit shows the bugfix as you expect it.
>
> BUT:
> A "git log" on the file itself shows neither the problematic merge nor
> the bugfix commit. Git considers the merge of this file trivial because
> the content is identical to that of parent R. Therefore, whatever
> happened on branch L is not considered relevant history of the file.
>
> FURTHERMORE:
> A "git show" of the merge itself does not show the conflicting file
> either. Obviously, "git show" on a merge decides which files are
> relevant not based on conflicts but based on resolutions.
>
> To sort out what happened, you first need to have a suspicion and then
> dig fairly deep in the manuals to set the correct options to show what
> happened.
>
> I think, both "git log" and "git show" should by default be a bit more
> conservative in hiding "insignificant" merges:
> * In "git log" a branch should only be hidden if it never touched the file.
> * In "git show" a merge should display all files that did have a
> conflict independent of the resolution. (I am open to discuss whether
> auto-resolvable conflicts should be displayed by default. Non-trivial
> conflicts definitely should)
>
> Greetings,
> Norbert
>
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02 10:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <20120202084859.GC3823@burratino>
On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> Felipe Contreras (4):
>> completion: be nicer with zsh
>
> Since I can't find this patch in the mail archive, I'll reply here.
> Luckily the most important bit is above already.
>
> I think I mentioned before that this subject line is what will appear
> in the shortlog and the shortlog is all that some people will see of
> the changelog, so it should include a self-contained description of
> the impact of the patch.
Exactly, and "completion: avoid default value assignment on : true
command" tells *nothing* to most people. Why is this patch needed? Do
I care about it?
OTOH "completion: be nicer with zsh" explains the purpose of the patch
and people that don't care about zsh can happily ignore it if they
want, and the ones that care about zsh might want to back port it, or
whatever.
Also, if you are looking at a shortlog and looking for patches related
to zsh, you would easily see this.
"completion: avoid default value assignment on : true command" almost
ensures that the people reading that summary would need to read
further to see *why*.
> -- >8 --
> From: Felipe Contreras <felipe.contreras@gmail.com>
> Date: Thu, 2 Feb 2012 03:15:17 +0200
> Subject: completion: avoid default value assignment on : true command
No useful information provided yet. Should I care about this? I guess
I need to open the mail and see.
> zsh versions from 4.3.0 to present (4.3.15) do not correctly propagate
> the SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)}
> expressions. For example, after running
Ok, a bug in zsh, I can skip this. Or if I care about zsh, then I
still have to read further, because I still don't know what's the
problem.
And BTW, this is extremely detailed. Where's the evidence? Is there a
bug report? How can I follow this to find out when it's fixed. (hint:
missing link)
> emulate sh
> fn () {
> var='one two'
> printf '%s\n' $var
> }
> x=$(fn)
> : ${y=$(fn)}
>
> printing "$x" results in two lines as expected, but printing "$y"
> results in a single line because $var is expanded as a single word
> when evaluating fn to compute y.
Ok, still reading.
> So avoid the construct, and use an explicit 'test -n "$foo" ||
> foo=$(bar)' instead.
Yeah...
> This fixes a bug tht caused all commands to be
> treated as porcelain and show up in "git <TAB><TAB>" completion,
> because the list of all commands was treated as a single word in
> __git_list_porcelain_commands and did not match any of the patterns
> that would usually cause plumbing to be excluded.
Aha! Finally we arrive to the important part.
I guess we have different styles of writing. Personally, I don't see
the point of forcing the readers to go through the whole thing.
Compare this to mine:
| Subject: [PATCH v3 1/4] completion: be nicer with zsh
At this point most people can skip this.
| And yet another bug in zsh[1] causes a mismatch; zsh seems to have
| problem emulating wordspliting, but only on the command substitution.
|
| Let's avoid it.
A bug in zsh in certain situations... got it.
| I found this issue because __git_compute_porcelain_commands listed all
| commands (not only porcelain).
At this point it might have been better to mention "git <TAB><TAB>"
instead, as you did. But not a big deal, the problem is clear; all
commands are listed.
Almost all people would stop at this point; either they don't care
about zsh, or they care, and they want the patch.
| This is because in zsh the following code:
|
| for i in $__git_all_commands
|
| would evaluate $__git_all_commands as a single word (with spaces),
| ${=__git_all_commands} should be used to do word splitting expansion
| (unless SH_WORD_SPLIT is used).
|
| sh emulation should take care of that, but the command expantion is
| messing up with that.
And more details for the skeptics, or the ones that want to learn more.
| tl;dr: $__git_porcelain_commands = $__git_all_commands
Wrapping it up, to make clear what happens.
| [1] http://article.gmane.org/gmane.comp.shells.zsh.devel/24296
And then a link to further details, discussion, and possible fixes.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02 10:18 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, SZEDER Gábor
In-Reply-To: <20120202094624.GF3823@burratino>
On Thu, Feb 2, 2012 at 11:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> See [1] for details. If there's no obvious explanation, I'd suggest
>>> contacting the postmaster.
>>
>> But there's nothing like the taboo words in the mail:
>> http://vger.kernel.org/majordomo-taboos.txt
>
> Why are you telling me? I am not the postmaster. I can't do anything
> to investigate it or fix it.
I'm not telling you, this is a conversation on a public mailing list.
Other people might know something about, or they might hit the same
issue in the future. It's good to state things on record.
And FTR, I had already contacted the postmaster, which has been not
exactly helpful in previous occasions, but lets see.
--
Felipe Contreras
^ permalink raw reply
* Re: General support for ! in git-config values
From: demerphq @ 2012-02-02 10:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <20120202095432.GA19356@sigill.intra.peff.net>
On 2 February 2012 10:54, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 02, 2012 at 10:44:05AM +0100, demerphq wrote:
>
>> The general design of git seems to me to be based around providing
>> building blocks that people can use to build new and interesting tools
>> on top of, and so it seems counter to that philosophy to reject an
>> feature based on speculative security issues that really can't be
>> decided in advance but must instead be decided on a case by case
>> basis.
>
> I can't speak for Junio, but I am certainly not rejecting it. Only
> saying that it needs to be thought through, and the utility weighed
> against the costs.
Of course. I totally understand. I have written mails saying stuff
like this myself. :-)
> So far I haven't seen an actual patch to comment on
> (or even a proposed syntax beyond starting a string with "!", which I
> think is a non-starter due to conflicting with existing uses),
I understand. I think we will probably use backtick quoting in git-deploy. So
deploy.prefix=`cat /etc/SERVER_ROLE`
will execute cat /etc/SERVER_ROLE and use the results as the value of
the config option.
> nor have
> I seen a concrete use case (you mentioned pulling the name/email from
> ldap, but you also mentioned that there are lots of other ways of
> solving that particular problem, so it's not especially compelling).
One place that it would be useful for us in git-deploy would be to
detect the tag prefix for the rollout we are doing. Every staging
server already has a file that contains this value. We would like to
make it easy for people to configure the tool to either use the value
provided, or to use something like `cat /etc/SERVER_ROLE` instead.
Anyway, from that POV I could totally understand "so do that in
git-deploy". Since the tool is written in perl we have to wrap
git-config anyway, so it easy to add a special case for ourselves.
But I still think the general idea is pretty useful, the ldap example
is IMO a cleaner solution than the alternatives, and a variant that I
think is much harder to do currently come to mind right away: setting
the user.email automatically depending on where in your tree a git
repo was located, so that when I work on repo underneath /CPAN/ it
uses my CPAN address, and when I work in my /work/ tree it uses my
$work address, etc, without me having to configure it repo by repo.
(This has bitten more than once in the past)
> I'd be happy to hear a more concrete proposal.
I will be mostly afk the next week so I will leave that to Avar if he
wants to pursue it.
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Thomas Rast @ 2012-02-02 10:35 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Jonathan Nieder, git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <CAMP44s0w1eXWWaMT3WAfLxFHPQvc9dp33cyJ=T2im6g7rsrKhw@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Exactly, and "completion: avoid default value assignment on : true
> command" tells *nothing* to most people. Why is this patch needed? Do
> I care about it?
>
> OTOH "completion: be nicer with zsh" explains the purpose of the patch
> and people that don't care about zsh can happily ignore it if they
> want, and the ones that care about zsh might want to back port it, or
> whatever.
Perhaps you could compromise on
completion: work around zsh word splitting bug in : ${foo:=$(bar)}
?
> | tl;dr: $__git_porcelain_commands = $__git_all_commands
>
> Wrapping it up, to make clear what happens.
I think this is not good style for a commit message. Apart from the
very trendy use of tl;dr, it doesn't even properly summarize the cause
*or* the user-visible symptom. It just states how the confusion
propagates somewhere in the middle of the code.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* [PATCH/RFC 0/3] Re: [PATCH] vcs-svn: Fix some compiler warnings
From: Jonathan Nieder @ 2012-02-02 10:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, David Barr, GIT Mailing-list, Dmitry Ivankov
In-Reply-To: <7vipjpzxav.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> I'd hold the branch in 'next' for now, until this gets resolved (one
> possible resolution is to declare Ramsey's patch is good enough for now,
> and do the follow-up later).
Sounds sensible. How about the following? Intended to replace
Ramsay's patch. Not well tested yet.
By the way, wouldn't the (p->field < 0) test in grep.c line 330
trigger the same compiler bug?
Jonathan Nieder (2):
vcs-svn: allow import of > 4GiB files
vcs-svn: suppress a -Wtype-limits warning
Ramsay Allan Jones (1):
vcs-svn: rename check_overflow arguments for clarity
vcs-svn/fast_export.c | 15 +++++++++------
vcs-svn/fast_export.h | 4 ++--
vcs-svn/sliding_window.c | 10 +++++-----
vcs-svn/svndump.c | 21 +++++++++++++++------
4 files changed, 31 insertions(+), 19 deletions(-)
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02 10:50 UTC (permalink / raw)
To: Thomas Rast; +Cc: Felipe Contreras, git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <8739at8qw6.fsf@thomas.inf.ethz.ch>
Thomas Rast wrote:
> Perhaps you could compromise on
>
> completion: work around zsh word splitting bug in : ${foo:=$(bar)}
Thanks, that looks like a good subject line to me. It gives a hint of
what the patch is trying to do, and it does not try to fool me into
thinking that if I use bash the patch does not affect me.
Felipe, I'm not going to respond to the rest of your message. Perhaps
someone more patient than I am will, or if someone has specific
questions for me, I'll be glad to help.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox