From: Michael J Gruber <hidden> Date: 2016-06-15 22:56:03
Recently and not so recently, we made sure that log/grep type operations
use textconv filters when a userfacing diff would do the same:
ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)
b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)
0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)
"git grep" currently does not use textconv filters at all, that is
neither for displaying the match and context nor for the actual grepping.
Introduce a binary mode "--textconv" (in addition to "--text" and "-I")
which makes git grep use any configured textconv filters for grepping
and output purposes.
Signed-off-by: Michael J Gruber <redacted>
---
Notes:
I'm somehow stuck in textconv/filespec/... hell, so I'm sending this out
in request for help. I'm sure there are people for whom it's a breeze to
get this right.
The difficulty is in getting the different cases (blob/sha1 vs.
worktree) right, and in making the changes minimally invasive. It seems
that some more refactoring could help: "git show --textconv" does not
use textconv filters when used on blobs either. (It does for diffs, of
course.) Most existing helper functions are tailored for diffs.
Nota bene: --textconv does not affect "diff --stat" either...
builtin/grep.c | 5 ++++-
grep.c | 47 +++++++++++++++++++++++++++++------------------
grep.h | 3 ++-
3 files changed, 35 insertions(+), 20 deletions(-)
@@ -659,6 +659,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)OPT_SET_INT('I',NULL,&opt.binary,N_("don't match patterns in binary files"),GREP_BINARY_NOMATCH),+OPT_SET_INT(0,"textconv",&opt.binary,+N_("process binary files with textconv filters"),+GREP_BINARY_TEXTCONV),{OPTION_INTEGER,0,"max-depth",&opt.max_depth,N_("depth"),N_("descend at most <depth> levels"),PARSE_OPT_NONEG,NULL,1},
From: Jeff King <hidden> Date: 2016-06-15 22:56:03
On Mon, Feb 04, 2013 at 04:27:31PM +0100, Michael J Gruber wrote:
Recently and not so recently, we made sure that log/grep type operations
use textconv filters when a userfacing diff would do the same:
ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)
b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)
0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)
"git grep" currently does not use textconv filters at all, that is
neither for displaying the match and context nor for the actual grepping.
Introduce a binary mode "--textconv" (in addition to "--text" and "-I")
which makes git grep use any configured textconv filters for grepping
and output purposes.
Sounds like a reasonable goal.
The difficulty is in getting the different cases (blob/sha1 vs.
worktree) right, and in making the changes minimally invasive. It seems
that some more refactoring could help: "git show --textconv" does not
use textconv filters when used on blobs either. (It does for diffs, of
course.) Most existing helper functions are tailored for diffs.
I think "git show" with blobs originally did not because we have no
filename with which to look up the attributes. IIRC, the patches to
support "cat-file --textconv" taught get_sha1_with_context to report the
path at which we found a blob. I suspect it is mostly a matter of
plumbing that information from the revision parser through to
show_blob_object.
Nota bene: --textconv does not affect "diff --stat" either...
Yeah, though I wonder if it should be on by default. The diffstat for a
binary file, unlike the diff, is already useful. The diffstat of the
textconv'd data may _also_ be useful, of course.
quoted hunk
@@ -659,6 +659,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix) OPT_SET_INT('I', NULL, &opt.binary, N_("don't match patterns in binary files"), GREP_BINARY_NOMATCH),+ OPT_SET_INT(0, "textconv", &opt.binary,+ N_("process binary files with textconv filters"),+ GREP_BINARY_TEXTCONV),
Is this really a new form of GREP_BINARY_*? What happens when a file
does not have a textconv filter?
I would expect this to be more like diff's "--textconv" flag, which is
really "allow textconv to be respected". Then you could do:
git grep -I --textconv foo
to grep in the text version of files which support it, and ignore the
rest.
-static int grep_source_load(struct grep_source *gs);
-static int grep_source_is_binary(struct grep_source *gs);
+static int grep_source_load(struct grep_source *gs, struct grep_opt *opt);
+static int grep_source_is_binary(struct grep_source *gs, struct grep_opt *opt);
Hmm. grep_source_load is more or less the analogue of
diff_populate_filespec, which does not know about textconv at all. So I
feel like this might be going in at the wrong layer...
quoted hunk
@@ -1354,14 +1356,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle switch (opt->binary) { case GREP_BINARY_DEFAULT:- if (grep_source_is_binary(gs))+ if (grep_source_is_binary(gs, opt)) binary_match_only = 1; break; case GREP_BINARY_NOMATCH:- if (grep_source_is_binary(gs))+ if (grep_source_is_binary(gs, opt)) return 0; /* Assume unmatch */ break;
The is_binary function learned about "opt" so that it could pass it to
grep_source_load, which might do the textconv for us. But we _know_ that
we will not here, because we see that we have other GREP_BINARY flags.
And when we do have _TEXTCONV:
case GREP_BINARY_TEXT:
+ case GREP_BINARY_TEXTCONV:
break;
So here we do the textconv for the sha1 case. But what about file
sources?
This is why I think the layer is wrong; you want the fill_textconv
function to call your abstract _load function (in the diff_filespec
world, it is diff_filespec_populate, but it is the moral equivalent).
And you want it to hold off as long as possible in case we can pull the
value from cache, or feed the working tree version of a file straight to
the filter.
-void grep_source_load_driver(struct grep_source *gs)
+void grep_source_load_driver(struct grep_source *gs, struct grep_opt *opt)
{
if (gs->driver)
return;
- grep_attr_lock();
+ grep_attr_lock(); //TODO
+ printf("Looking up userdiff driver for: %s", gs->path);
if (gs->path)
gs->driver = userdiff_find_by_path(gs->path);
if (!gs->driver)
gs->driver = userdiff_find_by_name("default");
+ if (opt->binary == GREP_BINARY_TEXTCONV)
+ gs->driver = userdiff_get_textconv(gs->driver);
grep_attr_unlock();
}
This is wrong. The point of userdiff_get_textconv is that it will return
NULL when we are not doing textconv for this path. So you can use it
like:
struct userdiff_driver *textconv = userdiff_get_textconv(gs->driver);
if (textconv) {
/* ok, we are doing textconv. Call our fill_textconv
* equivalent. */
}
else {
/* nope, plain old file. */
}
But by assigning it on top of gs->driver, you're going to end up with a
NULL driver sometimes. And the post-condition of the load_driver
function is that gs->driver always points to a valid driver (even if it
is the default one). I wouldn't be surprised if this causes segfaults.
So I would do it more like the patch below. Only lightly tested by me.
There are some refactoring opportunities if you want to bring
grep_source and diff_filespec closer together.
---
@@ -659,6 +659,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)OPT_SET_INT('I',NULL,&opt.binary,N_("don't match patterns in binary files"),GREP_BINARY_NOMATCH),+OPT_BOOL(0,"textconv",&opt.allow_textconv,+N_("process binary files with textconv filters")),{OPTION_INTEGER,0,"max-depth",&opt.max_depth,N_("depth"),N_("descend at most <depth> levels"),PARSE_OPT_NONEG,NULL,1},
From: Michael J Gruber <hidden> Date: 2016-06-15 22:56:03
Jeff King venit, vidit, dixit 05.02.2013 12:13:
quoted hunk
On Mon, Feb 04, 2013 at 04:27:31PM +0100, Michael J Gruber wrote:
quoted
Recently and not so recently, we made sure that log/grep type operations
use textconv filters when a userfacing diff would do the same:
ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)
b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)
0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)
"git grep" currently does not use textconv filters at all, that is
neither for displaying the match and context nor for the actual grepping.
Introduce a binary mode "--textconv" (in addition to "--text" and "-I")
which makes git grep use any configured textconv filters for grepping
and output purposes.
Sounds like a reasonable goal.
quoted
The difficulty is in getting the different cases (blob/sha1 vs.
worktree) right, and in making the changes minimally invasive. It seems
that some more refactoring could help: "git show --textconv" does not
use textconv filters when used on blobs either. (It does for diffs, of
course.) Most existing helper functions are tailored for diffs.
I think "git show" with blobs originally did not because we have no
filename with which to look up the attributes. IIRC, the patches to
support "cat-file --textconv" taught get_sha1_with_context to report the
path at which we found a blob. I suspect it is mostly a matter of
plumbing that information from the revision parser through to
show_blob_object.
quoted
Nota bene: --textconv does not affect "diff --stat" either...
Yeah, though I wonder if it should be on by default. The diffstat for a
binary file, unlike the diff, is already useful. The diffstat of the
textconv'd data may _also_ be useful, of course.
quoted
@@ -659,6 +659,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix) OPT_SET_INT('I', NULL, &opt.binary, N_("don't match patterns in binary files"), GREP_BINARY_NOMATCH),+ OPT_SET_INT(0, "textconv", &opt.binary,+ N_("process binary files with textconv filters"),+ GREP_BINARY_TEXTCONV),
Is this really a new form of GREP_BINARY_*? What happens when a file
does not have a textconv filter?
I would expect this to be more like diff's "--textconv" flag, which is
really "allow textconv to be respected". Then you could do:
git grep -I --textconv foo
to grep in the text version of files which support it, and ignore the
rest.
quoted
-static int grep_source_load(struct grep_source *gs);
-static int grep_source_is_binary(struct grep_source *gs);
+static int grep_source_load(struct grep_source *gs, struct grep_opt *opt);
+static int grep_source_is_binary(struct grep_source *gs, struct grep_opt *opt);
Hmm. grep_source_load is more or less the analogue of
diff_populate_filespec, which does not know about textconv at all. So I
feel like this might be going in at the wrong layer...
quoted
@@ -1354,14 +1356,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle switch (opt->binary) { case GREP_BINARY_DEFAULT:- if (grep_source_is_binary(gs))+ if (grep_source_is_binary(gs, opt)) binary_match_only = 1; break; case GREP_BINARY_NOMATCH:- if (grep_source_is_binary(gs))+ if (grep_source_is_binary(gs, opt)) return 0; /* Assume unmatch */ break;
The is_binary function learned about "opt" so that it could pass it to
grep_source_load, which might do the textconv for us. But we _know_ that
we will not here, because we see that we have other GREP_BINARY flags.
And when we do have _TEXTCONV:
quoted
case GREP_BINARY_TEXT:
+ case GREP_BINARY_TEXTCONV:
break;
So here we do the textconv for the sha1 case. But what about file
sources?
This is why I think the layer is wrong; you want the fill_textconv
function to call your abstract _load function (in the diff_filespec
world, it is diff_filespec_populate, but it is the moral equivalent).
And you want it to hold off as long as possible in case we can pull the
value from cache, or feed the working tree version of a file straight to
the filter.
quoted
-void grep_source_load_driver(struct grep_source *gs)
+void grep_source_load_driver(struct grep_source *gs, struct grep_opt *opt)
{
if (gs->driver)
return;
- grep_attr_lock();
+ grep_attr_lock(); //TODO
+ printf("Looking up userdiff driver for: %s", gs->path);
if (gs->path)
gs->driver = userdiff_find_by_path(gs->path);
if (!gs->driver)
gs->driver = userdiff_find_by_name("default");
+ if (opt->binary == GREP_BINARY_TEXTCONV)
+ gs->driver = userdiff_get_textconv(gs->driver);
grep_attr_unlock();
}
This is wrong. The point of userdiff_get_textconv is that it will return
NULL when we are not doing textconv for this path. So you can use it
like:
struct userdiff_driver *textconv = userdiff_get_textconv(gs->driver);
if (textconv) {
/* ok, we are doing textconv. Call our fill_textconv
* equivalent. */
}
else {
/* nope, plain old file. */
}
But by assigning it on top of gs->driver, you're going to end up with a
NULL driver sometimes. And the post-condition of the load_driver
function is that gs->driver always points to a valid driver (even if it
is the default one). I wouldn't be surprised if this causes segfaults.
So I would do it more like the patch below. Only lightly tested by me.
There are some refactoring opportunities if you want to bring
grep_source and diff_filespec closer together.
---
@@ -659,6 +659,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)OPT_SET_INT('I',NULL,&opt.binary,N_("don't match patterns in binary files"),GREP_BINARY_NOMATCH),+OPT_BOOL(0,"textconv",&opt.allow_textconv,+N_("process binary files with textconv filters")),{OPTION_INTEGER,0,"max-depth",&opt.max_depth,N_("depth"),N_("descend at most <depth> levels"),PARSE_OPT_NONEG,NULL,1},
Thanks Jeff, that helps a lot! It covers "grep expr" and "grep expr rev
-- path" just fine. I'll look into "grep expr rev:path" which does not
work yet because of an empty driver.
I also have "show --textconv" covered and a suggestion for "cat-file
--textconv" (to work without a textconv filter).
Expect a mini-series soon :)
Michael
From: Jeff King <hidden> Date: 2016-06-15 22:56:04
On Tue, Feb 05, 2013 at 05:21:18PM +0100, Michael J Gruber wrote:
Thanks Jeff, that helps a lot! It covers "grep expr" and "grep expr rev
-- path" just fine. I'll look into "grep expr rev:path" which does not
work yet because of an empty driver.
I also have "show --textconv" covered and a suggestion for "cat-file
--textconv" (to work without a textconv filter).
Expect a mini-series soon :)
Cool, I'm glad it helped. It would be great if diff_filespec and
grep_source could grow together into a unified object. One of the gross
things about the patch I posted is that we will now sometimes read the
file/blob data via grep_source_load, and sometimes via
diff_populate_filespec. They _should_ be equivalent, but in an ideal
world, they would be the same code path.
That may be too much to tackle for your series, though (I wanted to do
it when I factored out grep_source, but backed off for the same reason).
The "grep expr rev:path" fix should look something like this:
@@ -820,13 +820,17 @@ int cmd_grep(int argc, const char **argv, const char *prefix)for(i=0;i<argc;i++){constchar*arg=argv[i];unsignedcharsha1[20];+structobject_contextoc;/* Is it a rev? */-if(!get_sha1(arg,sha1)){+if(!get_sha1_with_context(arg,sha1,&oc)){structobject*object=parse_object(sha1);if(!object)die(_("bad object %s"),arg);if(!seen_dashdash)verify_non_filename(prefix,arg);+/* oops, we need something that will remember oc.path+*here,sothatwecanpassitalongto+*grep_source_init*/add_object_array(object,arg,&list);continue;}
But you'll have to replace the object_array with something more
featureful, I think.
-Peff
From: Michael J Gruber <hidden> Date: 2016-06-15 22:56:04
This min series aims at completing the textconv support of user facing
commands. It is RFC for lack of documentation and tests, to check
whether we really want to go in that direction (I do).
1/4 covers the missing textconv support in the "blob case" of "git
show", which should be (and then is) analogous to "cat-file" and "diff".
2/4 remedies an oddity of "git cat-file --textconv", which errored out
when there is no textconv filter rather than giving an unfiltered blob
(like every other textconv aware command).
3/4 implements "--textconv" for "git grep" sans the blob case; the code
is all Jeff's.
4/4 adds blob support to 3/4 (the "rev:path" case).
3 and 4 can be squashed, of course.
Now I'm quite a happy differ/shower/grepper with my latin1 and OO files ;)
Jeff King (1):
grep: allow to use textconv filters
Michael J Gruber (3):
show: obey --textconv for blobs
cat-file: do not die on --textconv without textconv filters
grep: obey --textconv for the case rev:path
builtin/cat-file.c | 9 ++--
builtin/grep.c | 13 +++---
builtin/log.c | 24 +++++++++--
grep.c | 100 +++++++++++++++++++++++++++++++++++++------
grep.h | 1 +
object.c | 26 ++++++++---
object.h | 2 +
t/t8007-cat-file-textconv.sh | 20 +++------
8 files changed, 148 insertions(+), 47 deletions(-)
--
1.8.1.2.752.g32d147e
From: Michael J Gruber <hidden> Date: 2016-06-15 22:56:04
Currently, "diff" and "cat-file" for blobs obey "--textconv" options
(with the former defaulting to "--textconv" and the latter to
"--no-textconv") whereas "show" does not obey this option, even though
it takes diff options.
Make "show" on blobs behave like "diff", i.e. obey "--textconv" by
default and "--no-textconv" when given.
Signed-off-by: Michael J Gruber <redacted>
---
builtin/log.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
@@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)strbuf_release(&out);}-staticintshow_blob_object(constunsignedchar*sha1,structrev_info*rev)+staticintshow_blob_object(constunsignedchar*sha1,structrev_info*rev,constchar*obj_name){+unsignedcharsha1c[20];+structobject_contextobj_context;+char*buf;+unsignedlongsize;+fflush(stdout);-returnstream_blob_to_fd(1,sha1,NULL,0);+if(!DIFF_OPT_TST(&rev->diffopt,ALLOW_TEXTCONV))+returnstream_blob_to_fd(1,sha1,NULL,0);++if(get_sha1_with_context(obj_name,0,sha1c,&obj_context))+die("Not a valid object name %s",obj_name);+if(!obj_context.path[0]||+!textconv_object(obj_context.path,obj_context.mode,sha1c,1,&buf,&size))+returnstream_blob_to_fd(1,sha1,NULL,0);++if(!buf)+die("git show %s: bad file",obj_name);++write_or_die(1,buf,size);+return0;}staticintshow_tag_object(constunsignedchar*sha1,structrev_info*rev)
From: Michael J Gruber <hidden> Date: 2016-06-15 22:56:04
When a command is supposed to use textconv filters (by default or with
"--textconv") and none are configured then the blob is output without
conversion; the only exception to this rule is "cat-file --textconv".
Make it behave like the rest of textconv aware commands.
Signed-off-by: Michael J Gruber <redacted>
---
builtin/cat-file.c | 9 +++++----
t/t8007-cat-file-textconv.sh | 20 +++++---------------
2 files changed, 10 insertions(+), 19 deletions(-)
@@ -146,10 +146,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)die("git cat-file --textconv %s: <object> must be <sha1:path>",obj_name);-if(!textconv_object(obj_context.path,obj_context.mode,sha1,1,&buf,&size))-die("git cat-file --textconv: unable to run textconv on %s",-obj_name);-break;+if(textconv_object(obj_context.path,obj_context.mode,sha1,1,&buf,&size))+break;++/* otherwise expect a blob */+exp_type="blob";case0:if(type_from_string(exp_type)==OBJ_BLOB){
@@ -73,25 +69,19 @@ test_expect_success 'cat-file --textconv on previous commit' '' test_expect_successSYMLINKS'cat-file without --textconv (symlink)''+printf"%s""one.bin">expected&&gitcat-fileblob:symlink.bin>result&&-printf"%s""one.bin">expectedtest_cmpexpectedresult' test_expect_successSYMLINKS'cat-file --textconv on index (symlink)''-!gitcat-file--textconv:symlink.bin2>result&&-cat>expected<<\EOF&&-fatal:gitcat-file--textconv:unabletoruntextconvon:symlink.bin-EOF+gitcat-file--textconv:symlink.bin>result&&test_cmpexpectedresult' test_expect_successSYMLINKS'cat-file --textconv on HEAD (symlink)''-!gitcat-file--textconvHEAD:symlink.bin2>result&&-cat>expected<<EOF&&-fatal:gitcat-file--textconv:unabletoruntextconvonHEAD:symlink.bin-EOF+gitcat-file--textconvHEAD:symlink.bin>result&&test_cmpexpectedresult'
From: Michael J Gruber <hidden> Date: 2016-06-15 22:56:04
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 <redacted>
---
builtin/grep.c | 11 ++++++-----
object.c | 26 ++++++++++++++++++++------
object.h | 2 ++
3 files changed, 28 insertions(+), 11 deletions(-)
@@ -820,14 +820,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)for(i=0;i<argc;i++){constchar*arg=argv[i];unsignedcharsha1[20];+structobject_contextoc;/* Is it a rev? */-if(!get_sha1(arg,sha1)){+if(!get_sha1_with_context(arg,0,sha1,&oc)){structobject*object=parse_object(sha1);if(!object)die(_("bad object %s"),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(structobject_context)));continue;}if(!strcmp(arg,"--")){
From: Michael J Gruber <hidden> Date: 2016-06-15 22:56:04
From: Jeff King <redacted>
Recently and not so recently, we made sure that log/grep type operations
use textconv filters when a userfacing diff would do the same:
ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)
b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)
0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)
"git grep" currently does not use textconv filters at all, that is
neither for displaying the match and context nor for the actual grepping.
Introduce an option "--textconv" which makes git grep use any configured
textconv filters for grepping and output purposes. It is off by default.
Signed-off-by: Michael J Gruber <redacted>
---
builtin/grep.c | 2 ++
grep.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++--------
grep.h | 1 +
3 files changed, 89 insertions(+), 14 deletions(-)
@@ -659,6 +659,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)OPT_SET_INT('I',NULL,&opt.binary,N_("don't match patterns in binary files"),GREP_BINARY_NOMATCH),+OPT_BOOL(0,"textconv",&opt.allow_textconv,+N_("process binary files with textconv filters")),{OPTION_INTEGER,0,"max-depth",&opt.max_depth,N_("depth"),N_("descend at most <depth> levels"),PARSE_OPT_NONEG,NULL,1},
Should this maybe just take the whole object_array_entry as a cleanup?
{
+ unsigned char sha1c[20];
+ struct object_context obj_context;
+ char *buf;
+ unsigned long size;
+
fflush(stdout);
- return stream_blob_to_fd(1, sha1, NULL, 0);
+ if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
+ return stream_blob_to_fd(1, sha1, NULL, 0);
+
+ if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
+ die("Not a valid object name %s", obj_name);
It seems a little hacky that we have to look up the sha1 again. What
should happen in the off chance that "hashcmp(sha1, sha1c) != 0" due to
a race with a simultaneous update of a ref?
Would it be better if object_array_entry replaced its "mode" member with
an object_context? The only downside I see is that we might waste a
significant amount of memory (each context has a PATH_MAX buffer in it).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:56:04
On Wed, Feb 06, 2013 at 04:08:51PM +0100, Michael J Gruber wrote:
When a command is supposed to use textconv filters (by default or with
"--textconv") and none are configured then the blob is output without
conversion; the only exception to this rule is "cat-file --textconv".
Make it behave like the rest of textconv aware commands.
Makes sense.
- if (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))
- die("git cat-file --textconv: unable to run textconv on %s",
- obj_name);
- break;
+ if (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))
+ break;
The implication here is that textconv_object should be handling its own
errors and dying, and the return is always "yes, I converted" or "no, I
did not". Which I think is the case.
+
+ /* otherwise expect a blob */
+ exp_type = "blob";
case 0:
if (type_from_string(exp_type) == OBJ_BLOB) {
I wondered at first why we needed to set exp_type here; shouldn't we
already be expecting a blob if we are doing textconv? But then I see
this is really about the fall-through in the switch (which we might want
an explicit comment for).
Which made me wonder: what happens with:
git cat-file --textconv HEAD
It looks like we die just before textconv-ing, because we have no
obj_context.path. But that is also unlike all of the other --textconv
switches, which mean "turn on textconv if you are showing a blob that
supports it" and not "the specific operation is --textconv, apply it to
this blob". I don't know if that is worth changing or not.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:56:04
On Wed, Feb 06, 2013 at 04:08:52PM +0100, Michael J Gruber wrote:
From: Jeff King <redacted>
Recently and not so recently, we made sure that log/grep type operations
use textconv filters when a userfacing diff would do the same:
ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)
b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)
0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)
"git grep" currently does not use textconv filters at all, that is
neither for displaying the match and context nor for the actual grepping.
Introduce an option "--textconv" which makes git grep use any configured
textconv filters for grepping and output purposes. It is off by default.
Signed-off-by: Michael J Gruber <redacted>
Signed-off-by: Jeff King <redacted>
I'd really love to see the refactoring I talked about in my earlier
message. But as I'm not willing to devote the time to do it right now,
and I do not think this patch has any particular bugs, I think it is OK
as it gets the job done, and does not make the later refactoring any
harder.
The one ugliness that still remains is:
+ if (opt->allow_textconv) {
+ grep_source_load_driver(gs);
+ /*
+ * We might set up the shared textconv cache data here, which
+ * is not thread-safe.
+ */
+ grep_attr_lock();
+ textconv = userdiff_get_textconv(gs->driver);
+ grep_attr_unlock();
+ }
We lock/unlock the grep_attr_lock twice here: once in
grep_source_load_driver, and then immediately again to call
userdiff_get_textconv. I don't know if it is worth doing the two under
the same lock or not (I guess it should not increase lock contention,
since we do the same amount of work, so it is really just the extra lock
instructions).
-Peff
This seems a little gross. Who is responsible for allocating the
context? Who frees it? It looks like we duplicate it in cmd_grep. Which
I think is OK, but it means all of this context infrastructure in
object.[ch] is just bolted-on junk waiting for somebody to use it wrong
or get confused. It does not get set, for example, by the regular
setup_revisions code path.
It would be nice if we could just always have the context available,
then setup_revisions could set it up by default (and replace the "mode"
parameter entirely). But we'd need to do something to avoid the
PATH_MAX-sized buffer for each entry, as some code paths may have a
large number of pending objects.
-Peff
Should this maybe just take the whole object_array_entry as a cleanup?
It's just a question of one or two/three pointers (I can't count), but
yes, that would be possible.
quoted
{
+ unsigned char sha1c[20];
+ struct object_context obj_context;
+ char *buf;
+ unsigned long size;
+
fflush(stdout);
- return stream_blob_to_fd(1, sha1, NULL, 0);
+ if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
+ return stream_blob_to_fd(1, sha1, NULL, 0);
+
+ if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
+ die("Not a valid object name %s", obj_name);
It seems a little hacky that we have to look up the sha1 again. What
should happen in the off chance that "hashcmp(sha1, sha1c) != 0" due to
a race with a simultaneous update of a ref?
I thought about a check here but didn't bother to because I knew the
refactoring would come up again...
Would it be better if object_array_entry replaced its "mode" member with
an object_context?
Do all callers/users want to deal with object_context?
I'm wondering why o_c has a mode at all, since it is mostly used in
conjunction with an object, isn't it?
The only downside I see is that we might waste a
significant amount of memory (each context has a PATH_MAX buffer in it).
That's why I used a reference to the struct, see my other reply.
Michael
This seems a little gross. Who is responsible for allocating the
context? Who frees it? It looks like we duplicate it in cmd_grep. Which
Well, who is responsible for allocating and freeing name and item? I
didn't want to introduce a new member which is a struct when all other
complex members are pointers. Wouldn't that be confusing?
I think is OK, but it means all of this context infrastructure in
object.[ch] is just bolted-on junk waiting for somebody to use it wrong
or get confused. It does not get set, for example, by the regular
setup_revisions code path.
Sure, it's NULL when there is no context info, just like in many other
cases.
It would be nice if we could just always have the context available,
then setup_revisions could set it up by default (and replace the "mode"
parameter entirely). But we'd need to do something to avoid the
PATH_MAX-sized buffer for each entry, as some code paths may have a
large number of pending objects.
If the information is always available even if we don't need it then it
always takes space. The only way out would be pointing into a pool of
path names rather having a copy in each entry. It's not like I hadn't
talked about providing virtual (blob) objects for path names keyed by
their sha1 before... It's just that I want my grep --textconv now ;)
Michael
From: Jeff King <hidden> Date: 2016-06-15 22:56:05
On Thu, Feb 07, 2013 at 10:05:26AM +0100, Michael J Gruber wrote:
quoted
Would it be better if object_array_entry replaced its "mode" member with
an object_context?
Do all callers/users want to deal with object_context?
Wouldn't it just mean replacing "entry->mode" with "entry->oc.mode" at
each user?
I'm wondering why o_c has a mode at all, since it is mostly used in
conjunction with an object, isn't it?
Just as we record the path from the surrounding tree, we record the
mode. It's that mode which gets put into the pending object list by the
revision parser (see the very end of handle_revision_arg). Storing an
object_context instead of the mode would be a strict superset of what we
store now (right now we just throw the rest away).
-Peff
This seems a little gross. Who is responsible for allocating the
context? Who frees it? It looks like we duplicate it in cmd_grep. Which
Well, who is responsible for allocating and freeing name and item? I
didn't want to introduce a new member which is a struct when all other
complex members are pointers. Wouldn't that be confusing?
We cheat on those two. "item" is always a pointer to a "struct object",
which lasts forever and never gets freed. When "name" is set by
setup_revisions, it comes from the argv list, which is assumed to last
forever (and when we add pending blobs for a "--objects" traversal, it
is the empty string (literal).
I'd be OK if we had an exterior object_context that could be handled
in the same way. But how do we tell setup_revisions that we are
interested in seeing the object_context from each parsed item, where
does the allocation come from (is it malloc'd by setup_revisions?), and
who is responsible for freeing it when we pop pending objects in
get_revisions and similar?
I don't think it's as clear cut.
I wonder, though...what we really care about here is just the pathname.
But if it is a pending object that comes from a blob revision argument,
won't it always be of the form "treeish:path"? Could we not even resolve
the sha1 again, but instead just parse out the ":path" bit?
That is sort of like what the repeated call to get_sha1_with_context
does in your first patch. Except that we do not actually want to lookup
the sha1, and it is harmful to do so (e.g., if the ref had moved on to a
new tree that does not have that path, get_sha1 would fail, but we do
not even care what is in the tree; we only want the parsing side effects
of get_sha1).
Hmm.
-Peff
PS By the way, while looking at the object_array code (which I have not
really used much before), I noticed that add_pending_commit_list sets
the "name" field to the result of sha1_to_hex. Which means that it is
likely to be completely bogus by the time you read it. I'm not even
sure where it gets read or if this matters. And obviously it's
completely unrelated to what we were discussing; just something I
noticed.
From: Michael J Gruber <hidden> Date: 2016-06-15 22:56:05
Jeff King venit, vidit, dixit 07.02.2013 10:11:
On Thu, Feb 07, 2013 at 10:05:26AM +0100, Michael J Gruber wrote:
quoted
quoted
Would it be better if object_array_entry replaced its "mode" member with
an object_context?
Do all callers/users want to deal with object_context?
Wouldn't it just mean replacing "entry->mode" with "entry->oc.mode" at
each user?
Yes, I meant at the time of creation, i.e. when someone has to create
and pass an o_a_e and maybe only knows a mode, and thus would have to
set the path to NULL or "".
quoted
I'm wondering why o_c has a mode at all, since it is mostly used in
conjunction with an object, isn't it?
Just as we record the path from the surrounding tree, we record the
mode. It's that mode which gets put into the pending object list by the
revision parser (see the very end of handle_revision_arg). Storing an
object_context instead of the mode would be a strict superset of what we
store now (right now we just throw the rest away).
Sure. But why does object_context have a mode member at all? Maybe it is
not alway used together with another struct which has the mode already,
then that's a reason.
Michael
From: Jeff King <hidden> Date: 2016-06-15 22:56:05
On Thu, Feb 07, 2013 at 10:34:26AM +0100, Michael J Gruber wrote:
quoted
Just as we record the path from the surrounding tree, we record the
mode. It's that mode which gets put into the pending object list by the
revision parser (see the very end of handle_revision_arg). Storing an
object_context instead of the mode would be a strict superset of what we
store now (right now we just throw the rest away).
Sure. But why does object_context have a mode member at all? Maybe it is
not alway used together with another struct which has the mode already,
then that's a reason.
Exactly. It's purely for pulling information out of
get_sha1_with_context, and does not know that you are going to put its
output into an object_array_entry (and many call sites do not).
-Peff
This seems a little gross. Who is responsible for allocating the
context? Who frees it? It looks like we duplicate it in cmd_grep. Which
Well, who is responsible for allocating and freeing name and item? I
didn't want to introduce a new member which is a struct when all other
complex members are pointers. Wouldn't that be confusing?
We cheat on those two. "item" is always a pointer to a "struct object",
which lasts forever and never gets freed. When "name" is set by
setup_revisions, it comes from the argv list, which is assumed to last
forever (and when we add pending blobs for a "--objects" traversal, it
is the empty string (literal).
I see, so they are really different.
I'd be OK if we had an exterior object_context that could be handled
in the same way. But how do we tell setup_revisions that we are
interested in seeing the object_context from each parsed item, where
does the allocation come from (is it malloc'd by setup_revisions?), and
who is responsible for freeing it when we pop pending objects in
get_revisions and similar?
Do we really need all of tree, path and mode in object_context (I mean
not just here, but other users), or only the path? I'd try and resurrect
the virtual path name objects then, they would be just like "item"
storage-wise.
I don't think it's as clear cut.
I wonder, though...what we really care about here is just the pathname.
But if it is a pending object that comes from a blob revision argument,
won't it always be of the form "treeish:path"? Could we not even resolve
the sha1 again, but instead just parse out the ":path" bit?
Do we have that, and in what form (e.g. magic expanded etc.)?
That is sort of like what the repeated call to get_sha1_with_context
does in your first patch. Except that we do not actually want to lookup
the sha1, and it is harmful to do so (e.g., if the ref had moved on to a
new tree that does not have that path, get_sha1 would fail, but we do
not even care what is in the tree; we only want the parsing side effects
of get_sha1).
Hmm.
-Peff
PS By the way, while looking at the object_array code (which I have not
really used much before), I noticed that add_pending_commit_list sets
the "name" field to the result of sha1_to_hex. Which means that it is
likely to be completely bogus by the time you read it. I'm not even
sure where it gets read or if this matters. And obviously it's
completely unrelated to what we were discussing; just something I
noticed.
Another thing I noted is that our path mangling at least for grep has
some issues:
(cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)
../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01
Taking everything right of ":" could still work.
Michael
From: Jeff King <hidden> Date: 2016-06-15 22:56:05
On Thu, Feb 07, 2013 at 10:47:55AM +0100, Michael J Gruber wrote:
quoted
I'd be OK if we had an exterior object_context that could be handled
in the same way. But how do we tell setup_revisions that we are
interested in seeing the object_context from each parsed item, where
does the allocation come from (is it malloc'd by setup_revisions?), and
who is responsible for freeing it when we pop pending objects in
get_revisions and similar?
Do we really need all of tree, path and mode in object_context (I mean
not just here, but other users), or only the path? I'd try and resurrect
the virtual path name objects then, they would be just like "item"
storage-wise.
We need at least mode, since that is how the mode parameter of
object_array_entry gets set. I do not know off-hand who uses "tree". I
suspect the intent was to do .gitattributes lookups inside that tree,
but I do not think we actually do in-tree lookups currently.
quoted
I don't think it's as clear cut.
I wonder, though...what we really care about here is just the pathname.
But if it is a pending object that comes from a blob revision argument,
won't it always be of the form "treeish:path"? Could we not even resolve
the sha1 again, but instead just parse out the ":path" bit?
Do we have that, and in what form (e.g. magic expanded etc.)?
Ah, I should have mentioned that. :) We should have the original rev
name in the object_array_entry's name field, shouldn't we? It's just a
matter of re-parsing it.
Another thing I noted is that our path mangling at least for grep has
some issues:
(cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)
../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01
From: Michael J Gruber <hidden> Date: 2016-06-15 22:56:05
Jeff King venit, vidit, dixit 07.02.2013 10:55:
On Thu, Feb 07, 2013 at 10:47:55AM +0100, Michael J Gruber wrote:
quoted
quoted
I'd be OK if we had an exterior object_context that could be handled
in the same way. But how do we tell setup_revisions that we are
interested in seeing the object_context from each parsed item, where
does the allocation come from (is it malloc'd by setup_revisions?), and
who is responsible for freeing it when we pop pending objects in
get_revisions and similar?
Do we really need all of tree, path and mode in object_context (I mean
not just here, but other users), or only the path? I'd try and resurrect
the virtual path name objects then, they would be just like "item"
storage-wise.
We need at least mode, since that is how the mode parameter of
object_array_entry gets set. I do not know off-hand who uses "tree". I
suspect the intent was to do .gitattributes lookups inside that tree,
but I do not think we actually do in-tree lookups currently.
quoted
quoted
I don't think it's as clear cut.
I wonder, though...what we really care about here is just the pathname.
But if it is a pending object that comes from a blob revision argument,
won't it always be of the form "treeish:path"? Could we not even resolve
the sha1 again, but instead just parse out the ":path" bit?
Do we have that, and in what form (e.g. magic expanded etc.)?
Ah, I should have mentioned that. :) We should have the original rev
name in the object_array_entry's name field, shouldn't we? It's just a
matter of re-parsing it.
quoted
Another thing I noted is that our path mangling at least for grep has
some issues:
(cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)
../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01
Yuck.
And even more yuck:
(cd t && git grep --full-name GET_SHA1_QUIETLY HEAD:../cache.h)
HEAD:../cache.h:#define GET_SHA1_QUIETLY 01
Someone does not expect a "rev:" to be in there, it seems ;)
Michael