Re: [PATCH 6/6] grep: obey --textconv for the case rev:path
On Sat, Apr 20, 2013 at 04:42:49PM +0200, Michael J Gruber wrote: > > And this mass of almost-the-same functions is gross, too, especially > > given that the object_context contains a mode itself. > > Well, it's just providing different ways to call into the one and only > function, in order to satisfy different callers' needs. It's not unheard > of (or rather: unseen) in our code, is it? No, we have instances of it already. And they're ugly, too. :) I think when we hit more than 2 or 3 wrappers it is time to start thinking whether they can be consolidated. I think it is mostly the overlap in context and mode that makes me find this one particularly ugly. But it's probably not solvable without some pretty heavy refactoring. > I vaguely seem to recall we had some more general framework cooking but > I may be wrong (I was offline due to sickness for a while). It was about > attaching some additional info to something. Yes, I said "vaguely" ... Yeah, I really wanted to keep the context inside the object_array, but it means either wasting a lot of space (due to over-large buffers) or having the array elements be variable-sized (with a flex-array for the pathname). And object_array entries already have a memory-leak problem from the "name" field, which I think we just punt on elsewhere. So I think this is probably the lesser of the possible evils. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] grep: obey --textconv for the case rev:path
Jeff King venit, vidit, dixit 20.04.2013 06:24:
> On Fri, Apr 19, 2013 at 06:44:49PM +0200, Michael J Gruber wrote:
>
>> @@ -820,12 +820,13 @@ int cmd_grep(int argc, const char **argv, const char
>> *prefix)
>> for (i = 0; i < argc; i++) {
>> const char *arg = argv[i];
>> unsigned char sha1[20];
>> +struct object_context oc;
>> /* Is it a rev? */
>> -if (!get_sha1(arg, sha1)) {
>> +if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
>> struct object *object = parse_object_or_die(sha1, arg);
>> if (!seen_dashdash)
>> verify_non_filename(prefix, arg);
>> -add_object_array(object, arg, &list);
>> +add_object_array_with_context(object, arg, &list,
>> xmemdupz(&oc, sizeof(struct object_context)));
>
> Hrm. I'm not excited about the extra allocation here. Who frees it?
>
>> +void add_object_array(struct object *obj, const char *name, struct
>> object_array *array)
>> +{
>> +add_object_array_with_mode(obj, name, array, S_IFINVALID);
>> +}
>> +
>> +void add_object_array_with_mode(struct object *obj, const char *name,
>> struct object_array *array, unsigned mode)
>> +{
>> +add_object_array_with_mode_context(obj, name, array, mode, NULL);
>> +}
>> +
>> +void add_object_array_with_context(struct object *obj, const char *name,
>> struct object_array *array, struct object_context *context)
>> +{
>> +if (context)
>> +add_object_array_with_mode_context(obj, name, array,
>> context->mode, context);
>> +else
>> +add_object_array_with_mode_context(obj, name, array,
>> S_IFINVALID, context);
>> +}
>
> And this mass of almost-the-same functions is gross, too, especially
> given that the object_context contains a mode itself.
Well, it's just providing different ways to call into the one and only
function, in order to satisfy different callers' needs. It's not unheard
of (or rather: unseen) in our code, is it?
> Unfortunately, I'm not sure if I have a more pleasant suggestion. I seem
> to recall wrestling with this issue during the last round, too.
Yes, I think that's what we ended up with. At least it's just one
context struct per argument to grep, so it's not that bad after all.
I vaguely seem to recall we had some more general framework cooking but
I may be wrong (I was offline due to sickness for a while). It was about
attaching some additional info to something. Yes, I said "vaguely" ...
Michael
--
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 6/6] grep: obey --textconv for the case rev:path
On Fri, Apr 19, 2013 at 06:44:49PM +0200, Michael J Gruber wrote:
> @@ -820,12 +820,13 @@ int cmd_grep(int argc, const char **argv, const char
> *prefix)
> for (i = 0; i < argc; i++) {
> const char *arg = argv[i];
> unsigned char sha1[20];
> + struct object_context oc;
> /* Is it a rev? */
> - if (!get_sha1(arg, sha1)) {
> + if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
> struct object *object = parse_object_or_die(sha1, arg);
> if (!seen_dashdash)
> verify_non_filename(prefix, arg);
> - add_object_array(object, arg, &list);
> + add_object_array_with_context(object, arg, &list,
> xmemdupz(&oc, sizeof(struct object_context)));
Hrm. I'm not excited about the extra allocation here. Who frees it?
> +void add_object_array(struct object *obj, const char *name, struct
> object_array *array)
> +{
> + add_object_array_with_mode(obj, name, array, S_IFINVALID);
> +}
> +
> +void add_object_array_with_mode(struct object *obj, const char *name, struct
> object_array *array, unsigned mode)
> +{
> + add_object_array_with_mode_context(obj, name, array, mode, NULL);
> +}
> +
> +void add_object_array_with_context(struct object *obj, const char *name,
> struct object_array *array, struct object_context *context)
> +{
> + if (context)
> + add_object_array_with_mode_context(obj, name, array,
> context->mode, context);
> + else
> + add_object_array_with_mode_context(obj, name, array,
> S_IFINVALID, context);
> +}
And this mass of almost-the-same functions is gross, too, especially
given that the object_context contains a mode itself.
Unfortunately, I'm not sure if I have a more pleasant suggestion. I seem
to recall wrestling with this issue during the last round, too.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/6] grep: obey --textconv for the case rev:path
Make "grep" obey the "--textconv" option also for the object case, i.e.
when used with an argument "rev:path".
Signed-off-by: Michael J Gruber
---
builtin/grep.c | 11 ++-
object.c | 26 --
object.h | 2 ++
t/t7008-grep-binary.sh | 6 --
4 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 00ee57d..bb7f970 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -458,10 +458,10 @@ static int grep_tree(struct grep_opt *opt, const struct
pathspec *pathspec,
}
static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
- struct object *obj, const char *name)
+ struct object *obj, const char *name, struct
object_context *oc)
{
if (obj->type == OBJ_BLOB)
- return grep_sha1(opt, obj->sha1, name, 0, NULL);
+ return grep_sha1(opt, obj->sha1, name, 0, oc ? oc->path : NULL);
if (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) {
struct tree_desc tree;
void *data;
@@ -503,7 +503,7 @@ static int grep_objects(struct grep_opt *opt, const struct
pathspec *pathspec,
for (i = 0; i < nr; i++) {
struct object *real_obj;
real_obj = deref_tag(list->objects[i].item, NULL, 0);
- if (grep_object(opt, pathspec, real_obj,
list->objects[i].name)) {
+ if (grep_object(opt, pathspec, real_obj, list->objects[i].name,
list->objects[i].context)) {
hit = 1;
if (opt->status_only)
break;
@@ -820,12 +820,13 @@ int cmd_grep(int argc, const char **argv, const char
*prefix)
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
unsigned char sha1[20];
+ struct object_context oc;
/* Is it a rev? */
- if (!get_sha1(arg, sha1)) {
+ if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
struct object *object = parse_object_or_die(sha1, arg);
if (!seen_dashdash)
verify_non_filename(prefix, arg);
- add_object_array(object, arg, &list);
+ add_object_array_with_context(object, arg, &list,
xmemdupz(&oc, sizeof(struct object_context)));
continue;
}
if (!strcmp(arg, "--")) {
diff --git a/object.c b/object.c
index 20703f5..c8ffc9e 100644
--- a/object.c
+++ b/object.c
@@ -255,12 +255,7 @@ int object_list_contains(struct object_list *list, struct
object *obj)
return 0;
}
-void add_object_array(struct object *obj, const char *name, struct
object_array *array)
-{
- add_object_array_with_mode(obj, name, array, S_IFINVALID);
-}
-
-void add_object_array_with_mode(struct object *obj, const char *name, struct
object_array *array, unsigned mode)
+static void add_object_array_with_mode_context(struct object *obj, const char
*name, struct object_array *array, unsigned mode, struct object_context
*context)
{
unsigned nr = array->nr;
unsigned alloc = array->alloc;
@@ -275,9 +270,28 @@ void add_object_array_with_mode(struct object *obj, const
char *name, struct obj
objects[nr].item = obj;
objects[nr].name = name;
objects[nr].mode = mode;
+ objects[nr].context = context;
array->nr = ++nr;
}
+void add_object_array(struct object *obj, const char *name, struct
object_array *array)
+{
+ add_object_array_with_mode(obj, name, array, S_IFINVALID);
+}
+
+void add_object_array_with_mode(struct object *obj, const char *name, struct
object_array *array, unsigned mode)
+{
+ add_object_array_with_mode_context(obj, name, array, mode, NULL);
+}
+
+void add_object_array_with_context(struct object *obj, const char *name,
struct object_array *array, struct object_context *context)
+{
+ if (context)
+ add_object_array_with_mode_context(obj, name, array,
context->mode, context);
+ else
+ add_object_array_with_mode_context(obj, name, array,
S_IFINVALID, context);
+}
+
void object_array_remove_duplicates(struct object_array *array)
{
unsigned int ref, src, dst;
diff --git a/object.h b/object.h
index 97d384b..695847d 100644
--- a/object.h
+++ b/object.h
@@ -13,6 +13,7 @@ struct object_array {
struct object *item;
const char *name;
unsigned mode;
+ struct object_context *context;
} *objects;
};
@@ -85,6 +86,7 @@ int object_list_contains(struct object_list *list, struct
object *obj);
/* Object array handling .. */
void add_object_array(struct object *obj, const char *name, struct
object_array *array);
void add_object_array_with_mode(struct object *obj, const char *name, struct
object_array *array
