[RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
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 g...@drmicha.warpmail.net --- builtin/cat-file.c | 9 + t/t8007-cat-file-textconv.sh | 20 +--- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 00528dd..6912dc2 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -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; case 0: if (type_from_string(exp_type) == OBJ_BLOB) { diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh index 78a0085..83c6636 100755 --- a/t/t8007-cat-file-textconv.sh +++ b/t/t8007-cat-file-textconv.sh @@ -22,11 +22,11 @@ test_expect_success 'setup ' ' ' cat expected EOF -fatal: git cat-file --textconv: unable to run textconv on :one.bin +bin: test version 2 EOF test_expect_success 'no filter specified' ' - git cat-file --textconv :one.bin 2result + git cat-file --textconv :one.bin result test_cmp expected result ' @@ -36,10 +36,6 @@ test_expect_success 'setup textconv filters' ' git config diff.test.cachetextconv false ' -cat expected EOF -bin: test version 2 -EOF - test_expect_success 'cat-file without --textconv' ' git cat-file blob :one.bin result test_cmp expected result @@ -73,25 +69,19 @@ test_expect_success 'cat-file --textconv on previous commit' ' ' test_expect_success SYMLINKS 'cat-file without --textconv (symlink)' ' + printf %s one.bin expected git cat-file blob :symlink.bin result - printf %s one.bin expected test_cmp expected result ' test_expect_success SYMLINKS 'cat-file --textconv on index (symlink)' ' - ! git cat-file --textconv :symlink.bin 2result - cat expected \EOF -fatal: git cat-file --textconv: unable to run textconv on :symlink.bin -EOF + git cat-file --textconv :symlink.bin result test_cmp expected result ' test_expect_success SYMLINKS 'cat-file --textconv on HEAD (symlink)' ' - ! git cat-file --textconv HEAD:symlink.bin 2result - cat expected EOF -fatal: git cat-file --textconv: unable to run textconv on HEAD:symlink.bin -EOF + git cat-file --textconv HEAD:symlink.bin result test_cmp expected result ' -- 1.8.1.2.752.g32d147e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
Michael J Gruber g...@drmicha.warpmail.net writes: 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. I am of two minds. Because cat-file is mostly a low-level plumbing, I do not necessarily think it is a bad behaviour for it to error out when it was asked to apply textconv where there is no filter or when the filter fails to produce an output. On the other hand, it certainly makes it more convenient for callers that do not care too deeply, taking textconv as a mere hint just like Porcelains do. Make it behave like the rest of textconv aware commands. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net --- builtin/cat-file.c | 9 + t/t8007-cat-file-textconv.sh | 20 +--- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 00528dd..6912dc2 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -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; Please use the constant string blob_type that is available for all callers including this one. But stepping back a bit. What happens when I say cat-file -c HEAD:Documentation, and what should happen when I do so? I think what we want to see in the ideal world might be: * If we have a textconv for tree objects at that path to format it specially, textconv_object() may be allowed to textualize it (even though it is not a blob, and textconv so far has always been about blobs; it needs to be considered carefully if it makes sense to allow such a usage) and show it; * If we don't, we act as if -c were -p; in other words, we treat the built-in human output implemented by cat-file -p as if that is a textconv. In other words, you may want to fall-thru to case 'p', not case 0 with forced blob type. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
Jeff King p...@peff.net writes: 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. OK, so in that sense, cat-file --textconv HEAD (or HEAD:) should die with Hey, that is not a blob, in other words, Michael's patch does what we want without further tweaks, right? By the way are we sure textconv_object() barfs and dies if fed a non blob? Otherwise the above does not hold, and the semantics become turn on textconv if the object you are showing supports it, otherwise it has to be a blob., I think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
On Wed, Feb 06, 2013 at 02:23:36PM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: 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. OK, so in that sense, cat-file --textconv HEAD (or HEAD:) should die with Hey, that is not a blob, in other words, Michael's patch does what we want without further tweaks, right? Right, it will die because we do not find a path in the object_context. For the same reason that cat-file --textconv $sha1 would die. A more interesting case is cat-file --textconv HEAD:Documentation, which does have a path, but not a blob. And I think that speaks to your point that we want to fall-through to the pretty-print case, not the blob case. By the way are we sure textconv_object() barfs and dies if fed a non blob? Otherwise the above does not hold, and the semantics become turn on textconv if the object you are showing supports it, otherwise it has to be a blob., I think. I'm not sure. The sha1 would get passed all the way down to fill_textconv. I think because sha1_valid is set, it will not try to reuse the working tree file, so we will end up in diff_populate_filespec, and we could actually textconv the tree object itself. So yeah, I think we want a check that makes sure we are working with a blob before we even call that function, and --textconv should just be you must feed me a blob of the form $treeish:$path. In practice nobody wants to do anything else anyway, so let's keep the code paths simple; we can always loosen it later if there is a good reason to do so. -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