[RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters

[Michael J Gruber]

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

[Junio C Hamano]

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

[Jeff King]

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

[Junio C Hamano]

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

[Jeff King]

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