threads / patch / 43877

patch, 4 partscat-file --textconv/--filters: allow specifying the path separately

Subject: [PATCH 3/4] cat-file --textconv/--filters: allow specifying the path separately

## tl;dr

66 messages between Aug 18, 2016 and Sep 11, 2016. Diffs are folded; open one to read it.

replies: 65people: 4as markdown or json

Johannes Schindelin· Aug 18, 2016, 12:46 UTC · lore

[PATCH 0/4] cat-file: optionally convert to worktree version

When third-party tools need to access to contents of blobs in the database, they might be more interested in the worktree version than in the "clean" version of said contents.

This branch introduces the --filters option to make that happen, the --use-path option to provide the path separately if the blob name rather than the tree name is availale, and offers batch support in which case it expects the object names and the path on its input lines, separated by white space.

The new --filters option is an obvious sibling of --textconv, and shares the peculiar feature that the drivers (and end-of-line convention) are determined from the current worktree, not from the attributes stored in the revision that may have been part of the object name.

As --textconv is so similar to --filters, it was taught to understand the --use-path option and it was made compatible with batch mode, too.

I briefly considered teaching the batch mode to extract the path from object names if they are specified as <tree-ish>:<path>. The changes would be quite intrusive, though, and uglify the code substanitially. So I decided against that.

Johannes Schindelin (4):
  cat-file: fix a grammo in the man page
  cat-file: introduce the --filters option
  cat-file --textconv/--filters: allow specifying the path separately
  cat-file: support --textconv/--filters in batch mode
 Documentation/git-cat-file.txt |  40 ++++++++++++----
 builtin/cat-file.c             | 106 +++++++++++++++++++++++++++++++++++++----
 t/t8010-cat-file-filters.sh    |  57 ++++++++++++++++++++++
 3 files changed, 185 insertions(+), 18 deletions(-)
 create mode 100755 t/t8010-cat-file-filters.sh
Published-As: https://github.com/dscho/git/releases/tag/cat-file-filters-v1
Fetch-It-Via: git fetch https://github.com/dscho/git cat-file-filters-v1
-- 
2.9.2.691.g78954f3

base-commit: d63263a4dee8fc7da9b97bbdedf9c0d1f33024d4
Johannes Schindelin· Aug 18, 2016, 12:46 UTC · re: Johannes Schindelin · lore

There are circumstances when it is relatively easy to figure out the object name for a given path, but not the revision. For example, when looking at a diff generated by Git, the object names are recorded, but not the revision. As a matter of fact, the revisions from which the diff was generated may not even exist locally.

In such a case, the user would have to generate a fake revision just to be able to use --textconv or --filters.

Let's simplify this dramatically, because we do not really need that revision at all: all we care about is that we know the path. In the scenario described above, we do know the path, and we just want to specify it separately from the object name.

Example usage:
	git cat-file --textconv --use-path=main.c 0f1937fd
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt |  7 ++++++-
 builtin/cat-file.c             | 22 +++++++++++++++++-----
 t/t8010-cat-file-filters.sh    | 13 +++++++++++++
 3 files changed, 36 insertions(+), 6 deletions(-)
Show changes to 3 files +36 −6

Documentation/git-cat-file.txt, builtin/cat-file.c, t/t8010-cat-file-filters.sh

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 7d48735..59a3c37 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,7 +9,7 @@ git-cat-file - Provide content or type and size information for repository objec
 SYNOPSIS
 --------
 [verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) <object>
+'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--use-path=<path>] <object>
 'git cat-file' (--batch | --batch-check) [--follow-symlinks]
 
 DESCRIPTION
@@ -64,6 +64,11 @@ OPTIONS
 	end-of-line conversion, etc). In this case, <object> has to be of
 	the form <tree-ish>:<path>, or :<path>.
 
+--use-path=<path>::
+	For use with --textconv or --filters, to allow specifying an object
+	name and a path separately, e.g. when it is difficult to figure out
+	the revision from which the blob came.
+
 --batch::
 --batch=<format>::
 	Print object information and contents for each object provided
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0b74afa..5ff58b3 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,6 +20,8 @@ struct batch_options {
 	const char *format;
 };
 
+static const char *force_path;
+
 static int filter_object(const char *path, unsigned mode,
 			 const unsigned char *sha1,
 			 char **buf, unsigned long *size)
@@ -89,21 +91,24 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		return !has_sha1_file(sha1);
 
 	case 'w':
-		if (!obj_context.path[0])
+		if (!force_path && !obj_context.path[0])
 			die("git cat-file --filters %s: <object> must be "
 			    "<sha1:path>", obj_name);
 
-		if (filter_object(obj_context.path, obj_context.mode,
+		if (filter_object(force_path ? force_path : obj_context.path,
+				  force_path ? 0100644 : obj_context.mode,
 				  sha1, &buf, &size))
 			return -1;
 		break;
 
 	case 'c':
-		if (!obj_context.path[0])
+		if (!force_path && !obj_context.path[0])
 			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))
+		if (textconv_object(force_path ? force_path : obj_context.path,
+				    force_path ? 0100644 : obj_context.mode,
+				    sha1, 1, &buf, &size))
 			break;
 
 	case 'p':
@@ -477,7 +482,7 @@ static int batch_objects(struct batch_options *opt)
 }
 
 static const char * const cat_file_usage[] = {
-	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) <object>"),
+	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) [--use-path=<path>] <object>"),
 	N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
 	NULL
 };
@@ -525,6 +530,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 			    N_("for blob objects, run textconv on object's content"), 'c'),
 		OPT_CMDMODE(0, "filters", &opt,
 			    N_("for blob objects, run filters on object's content"), 'w'),
+		OPT_STRING(0, "use-path", &force_path, N_("blob"),
+			   N_("use a specific path for --textconv/--filters")),
 		OPT_BOOL(0, "allow-unknown-type", &unknown_type,
 			  N_("allow -s and -t to work with broken/corrupt objects")),
 		OPT_BOOL(0, "buffer", &batch.buffer_output, N_("buffer --batch output")),
@@ -567,6 +574,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		usage_with_options(cat_file_usage, options);
 	}
 
+	if (force_path && opt != 'c' && opt != 'w') {
+		error("--use-path=<path> needs --textconv or --filters");
+		usage_with_options(cat_file_usage, options);
+	}
+
 	if (batch.buffer_output < 0)
 		batch.buffer_output = batch.all_objects;
 
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index e466634..fd17159 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -31,4 +31,17 @@ test_expect_success 'cat-file --filters converts to worktree version' '
 	has_cr actual
 '
 
+test_expect_success 'cat-file --filters --use-path=<path> works' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	git cat-file --filters --use-path=world.txt $sha1 >actual &&
+	has_cr actual
+'
+
+test_expect_success 'cat-file --textconv --use-path=<path> works' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
+	git cat-file --textconv --use-path=hello.txt $sha1 >rot13 &&
+	test uryyb = "$(cat rot13 | remove_cr)"
+'
+
 test_done
-- 
2.9.2.691.g78954f3
Junio C Hamano· Aug 18, 2016, 16:52 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/4] cat-file --textconv/--filters: allow specifying the path separately

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> There are circumstances when it is relatively easy to figure out the
> object name for a given path, but not the revision. For example, when
revision of the containing tree, I presume?
> Let's simplify this dramatically, because we do not really need that
> revision at all: all we care about is that we know the path. In the
> scenario described above, we do know the path, and we just want to
> specify it separately from the object name.

I wouldn't call it "simplifying dramatically". It is just to support two use cases that is different from the use case where you want to use "<tree>:<path>".

We already have a precedent in "hash-object --path=<path>" for these two different uses cases from the primary one. That form can be used when we know the contents but we do not know where the contents came from. It can also be used when we want to pretend that contents came from a specific path, that may be different from the file we are hashing.

Let's be consistent and (1) call it "--path", and (2) explain it as a feature to allow you to tell the path separately or allow you to pretend that the content is at a path different from reality.

After all, you would want to ignore <path2> part in this construct:
	git cat-file --filter --path=<path1> HEAD:<path2>
for the purpose of filtering, right?
Show 26 quoted lines
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 0b74afa..5ff58b3 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -20,6 +20,8 @@ struct batch_options {
>  	const char *format;
>  };
>  
> +static const char *force_path;
> +
>  static int filter_object(const char *path, unsigned mode,
>  			 const unsigned char *sha1,
>  			 char **buf, unsigned long *size)
> @@ -89,21 +91,24 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>  		return !has_sha1_file(sha1);
>  
>  	case 'w':
> -		if (!obj_context.path[0])
> +		if (!force_path && !obj_context.path[0])
>  			die("git cat-file --filters %s: <object> must be "
>  			    "<sha1:path>", obj_name);
>  
> -		if (filter_object(obj_context.path, obj_context.mode,
> +		if (filter_object(force_path ? force_path : obj_context.path,
> +				  force_path ? 0100644 : obj_context.mode,
>  				  sha1, &buf, &size))

The mode override is somewhat questionable. Wouldn't you want to reject

	git cat-file --filter --path=Makefile HEAD:RelNotes

when HEAD:RelNotes blob is known to be a symbolic link? After all, you would reject this

	git cat-file --filter --path=Makefile HEAD:t/

and it is quite similar, no? I think this can be argued both ways, but I have a feeling that obj_context.mode, if available, should be honored with or without force_path.

Show 20 quoted lines
> diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
> index e466634..fd17159 100755
> --- a/t/t8010-cat-file-filters.sh
> +++ b/t/t8010-cat-file-filters.sh
> @@ -31,4 +31,17 @@ test_expect_success 'cat-file --filters converts to worktree version' '
>  	has_cr actual
>  '
>  
> +test_expect_success 'cat-file --filters --use-path=<path> works' '
> +	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> +	git cat-file --filters --use-path=world.txt $sha1 >actual &&
> +	has_cr actual
> +'
> +
> +test_expect_success 'cat-file --textconv --use-path=<path> works' '
> +	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> +	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
> +	git cat-file --textconv --use-path=hello.txt $sha1 >rot13 &&
> +	test uryyb = "$(cat rot13 | remove_cr)"
> +'

I think I saw some code to ensure "when giving this option you need that option in effect, too"; they should be tested here, too, no?

Johannes Schindelin· Aug 19, 2016, 14:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/4] cat-file --textconv/--filters: allow specifying the path separately

Hi Junio,
On Thu, 18 Aug 2016, Junio C Hamano wrote:
Show 6 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > There are circumstances when it is relatively easy to figure out the
> > object name for a given path, but not the revision. For example, when
> 
> revision of the containing tree, I presume?
name of containing tree, actually.
Show 8 quoted lines
> > Let's simplify this dramatically, because we do not really need that
> > revision at all: all we care about is that we know the path. In the
> > scenario described above, we do know the path, and we just want to
> > specify it separately from the object name.
> 
> I wouldn't call it "simplifying dramatically".  It is just to
> support two use cases that is different from the use case where you
> want to use "<tree>:<path>".

"this" was not the code in question. "this" is the use case. Just put my shoes on for a moment. As described: you have a list of object names and their path. Now you need to use the --textconv or --filters option, but you do not have the commit name. What do you do? Concoct a script that goes through `git rev-list --all -- <path>` in the hopes of finding a revision soon? I hope after this little exercise you agree that introducing the --path=<path> option simplifies this use case dramatically.

Show 6 quoted lines
> We already have a precedent in "hash-object --path=<path>" for these
> two different uses cases from the primary one.  That form can be
> used when we know the contents but we do not know where the contents
> came from.  It can also be used when we want to pretend that
> contents came from a specific path, that may be different from the
> file we are hashing.
Agreed. I was unaware of hash-object's existing option.
Show 9 quoted lines
> Let's be consistent and (1) call it "--path", and (2) explain it as
> a feature to allow you to tell the path separately or allow you to
> pretend that the content is at a path different from reality.
> 
> After all, you would want to ignore <path2> part in this construct:
> 
> 	git cat-file --filter --path=<path1> HEAD:<path2>
> 
> for the purpose of filtering, right?
True, and I even make use of that in the test for the batch mode.
Show 40 quoted lines
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 0b74afa..5ff58b3 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -20,6 +20,8 @@ struct batch_options {
> >  	const char *format;
> >  };
> >  
> > +static const char *force_path;
> > +
> >  static int filter_object(const char *path, unsigned mode,
> >  			 const unsigned char *sha1,
> >  			 char **buf, unsigned long *size)
> > @@ -89,21 +91,24 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
> >  		return !has_sha1_file(sha1);
> >  
> >  	case 'w':
> > -		if (!obj_context.path[0])
> > +		if (!force_path && !obj_context.path[0])
> >  			die("git cat-file --filters %s: <object> must be "
> >  			    "<sha1:path>", obj_name);
> >  
> > -		if (filter_object(obj_context.path, obj_context.mode,
> > +		if (filter_object(force_path ? force_path : obj_context.path,
> > +				  force_path ? 0100644 : obj_context.mode,
> >  				  sha1, &buf, &size))
> 
> The mode override is somewhat questionable.  Wouldn't you want to
> reject
> 
> 	git cat-file --filter --path=Makefile HEAD:RelNotes
> 
> when HEAD:RelNotes blob is known to be a symbolic link?  After all,
> you would reject this
> 
> 	git cat-file --filter --path=Makefile HEAD:t/
> 
> and it is quite similar, no?  I think this can be argued both ways,
> but I have a feeling that obj_context.mode, if available, should be
> honored with or without force_path.
I see your point. All I wanted to do was to enable
	git cat-file --filters --path=Makefile cafebabe

in which case obj_context.mode == S_IFINVALID. I fixed it (see interdiff of the next iteration).

Show 23 quoted lines
> > diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
> > index e466634..fd17159 100755
> > --- a/t/t8010-cat-file-filters.sh
> > +++ b/t/t8010-cat-file-filters.sh
> > @@ -31,4 +31,17 @@ test_expect_success 'cat-file --filters converts to worktree version' '
> >  	has_cr actual
> >  '
> >  
> > +test_expect_success 'cat-file --filters --use-path=<path> works' '
> > +	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> > +	git cat-file --filters --use-path=world.txt $sha1 >actual &&
> > +	has_cr actual
> > +'
> > +
> > +test_expect_success 'cat-file --textconv --use-path=<path> works' '
> > +	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> > +	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
> > +	git cat-file --textconv --use-path=hello.txt $sha1 >rot13 &&
> > +	test uryyb = "$(cat rot13 | remove_cr)"
> > +'
> 
> I think I saw some code to ensure "when giving this option you need
> that option in effect, too"; they should be tested here, too, no?

No, I would rather not test for that. These conditionals are purely for any user's convenience, in case they specify an option that has no effect. They are absolutely not essential for the function introduced in this patch series.

Ciao, Dscho

Junio C Hamano· Aug 19, 2016, 16:11 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/4] cat-file --textconv/--filters: allow specifying the path separately

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 7 quoted lines
>> I think I saw some code to ensure "when giving this option you need
>> that option in effect, too"; they should be tested here, too, no?
>
> No, I would rather not test for that. These conditionals are purely for
> any user's convenience, in case they specify an option that has no effect.
> They are absolutely not essential for the function introduced in this
> patch series.
I didn't say "you would want to test these, no?", did I?

I do not want to see bugreports that say "I wanted to use this new feature and by mistake gave only --path without giving --filter; Git should have complained. I found a bug, hooray!" when somebody in the future refactors the command line option parsing and breaks the check you already have.

Johannes Schindelin· Aug 24, 2016, 07:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/4] cat-file --textconv/--filters: allow specifying the path separately

Hi Junio,
On Fri, 19 Aug 2016, Junio C Hamano wrote:
Show 17 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I think I saw some code to ensure "when giving this option you need
> >> that option in effect, too"; they should be tested here, too, no?
> >
> > No, I would rather not test for that. These conditionals are purely for
> > any user's convenience, in case they specify an option that has no effect.
> > They are absolutely not essential for the function introduced in this
> > patch series.
> 
> I didn't say "you would want to test these, no?", did I?
> 
> I do not want to see bugreports that say "I wanted to use this new
> feature and by mistake gave only --path without giving --filter; Git
> should have complained.  I found a bug, hooray!" when somebody in
> the future refactors the command line option parsing and breaks the
> check you already have.

I added a test to verify that --path without --filters nor --textconv complains.

Ciao, Dscho

Johannes Schindelin· Aug 18, 2016, 12:46 UTC · re: Johannes Schindelin · lore

[PATCH 2/4] cat-file: introduce the --filters option

As suggested by its name, the --filters option applies the filters that are currently configured for the specified path.

This feature comes in handy when a 3rd-party tool wants to work with the contents of files from past revisions as if they had been checked out, but without detouring via temporary files.

Note that we ensure that symbolic links are unaffected (we know from looking at the mode whether it refers to a symbolic link or a regular file).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt | 12 +++++++++---
 builtin/cat-file.c             | 41 ++++++++++++++++++++++++++++++++++++++++-
 t/t8010-cat-file-filters.sh    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 4 deletions(-)
 create mode 100755 t/t8010-cat-file-filters.sh
Show changes to 3 files +83 −4

Documentation/git-cat-file.txt, builtin/cat-file.c, t/t8010-cat-file-filters.sh

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 071029b..7d48735 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,15 +9,15 @@ git-cat-file - Provide content or type and size information for repository objec
 SYNOPSIS
 --------
 [verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv ) <object>
+'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) <object>
 'git cat-file' (--batch | --batch-check) [--follow-symlinks]
 
 DESCRIPTION
 -----------
 In its first form, the command provides the content or the type of an object in
 the repository. The type is required unless `-t` or `-p` is used to find the
-object type, or `-s` is used to find the object size, or `--textconv` is used
-(which implies type "blob").
+object type, or `-s` is used to find the object size, or `--textconv` or
+`--filters` is used (which imply type "blob").
 
 In the second form, a list of objects (separated by linefeeds) is provided on
 stdin, and the SHA-1, type, and size of each object is printed on stdout.
@@ -58,6 +58,12 @@ OPTIONS
 	order to apply the filter to the content recorded in the index at
 	<path>.
 
+--filters::
+	Show the content as transformed by the filters configured in
+	the current working tree for the given <path> (i.e. smudge filters,
+	end-of-line conversion, etc). In this case, <object> has to be of
+	the form <tree-ish>:<path>, or :<path>.
+
 --batch::
 --batch=<format>::
 	Print object information and contents for each object provided
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2dfe626..0b74afa 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,6 +20,33 @@ struct batch_options {
 	const char *format;
 };
 
+static int filter_object(const char *path, unsigned mode,
+			 const unsigned char *sha1,
+			 char **buf, unsigned long *size)
+{
+	enum object_type type;
+
+	*buf = read_sha1_file(sha1, &type, size);
+	if (!*buf)
+		return error(_("cannot read object %s '%s'"),
+			sha1_to_hex(sha1), path);
+	if (type != OBJ_BLOB) {
+		free(*buf);
+		return error(_("blob expected for %s '%s'"),
+			sha1_to_hex(sha1), path);
+	}
+	if (S_ISREG(mode)) {
+		struct strbuf strbuf = STRBUF_INIT;
+		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
+			free(*buf);
+			*size = strbuf.len;
+			*buf = strbuf_detach(&strbuf, NULL);
+		}
+	}
+
+	return 0;
+}
+
 static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			int unknown_type)
 {
@@ -61,6 +88,16 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	case 'e':
 		return !has_sha1_file(sha1);
 
+	case 'w':
+		if (!obj_context.path[0])
+			die("git cat-file --filters %s: <object> must be "
+			    "<sha1:path>", obj_name);
+
+		if (filter_object(obj_context.path, obj_context.mode,
+				  sha1, &buf, &size))
+			return -1;
+		break;
+
 	case 'c':
 		if (!obj_context.path[0])
 			die("git cat-file --textconv %s: <object> must be <sha1:path>",
@@ -440,7 +477,7 @@ static int batch_objects(struct batch_options *opt)
 }
 
 static const char * const cat_file_usage[] = {
-	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv) <object>"),
+	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) <object>"),
 	N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
 	NULL
 };
@@ -486,6 +523,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
 		OPT_CMDMODE(0, "textconv", &opt,
 			    N_("for blob objects, run textconv on object's content"), 'c'),
+		OPT_CMDMODE(0, "filters", &opt,
+			    N_("for blob objects, run filters on object's content"), 'w'),
 		OPT_BOOL(0, "allow-unknown-type", &unknown_type,
 			  N_("allow -s and -t to work with broken/corrupt objects")),
 		OPT_BOOL(0, "buffer", &batch.buffer_output, N_("buffer --batch output")),
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
new file mode 100755
index 0000000..e466634
--- /dev/null
+++ b/t/t8010-cat-file-filters.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='git cat-file filters support'
+. ./test-lib.sh
+
+test_expect_success 'setup ' '
+	echo "*.txt eol=crlf diff=txt" >.gitattributes &&
+	echo "hello" | append_cr >world.txt &&
+	git add .gitattributes world.txt &&
+	test_tick &&
+	git commit -m "Initial commit"
+'
+
+has_cr () {
+	tr '\015' Q <"$1" | grep Q >/dev/null
+}
+
+test_expect_success 'no filters with `git show`' '
+	git show HEAD:world.txt >actual &&
+	! has_cr actual
+
+'
+
+test_expect_success 'no filters with cat-file' '
+	git cat-file blob HEAD:world.txt >actual &&
+	! has_cr actual
+'
+
+test_expect_success 'cat-file --filters converts to worktree version' '
+	git cat-file --filters HEAD:world.txt >actual &&
+	has_cr actual
+'
+
+test_done
-- 
2.9.2.691.g78954f3
Junio C Hamano· Aug 18, 2016, 16:37 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/4] cat-file: introduce the --filters option

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 10 quoted lines
> As suggested by its name, the --filters option applies the filters
> that are currently configured for the specified path.
>
> This feature comes in handy when a 3rd-party tool wants to work with
> the contents of files from past revisions as if they had been checked
> out, but without detouring via temporary files.
>
> Note that we ensure that symbolic links are unaffected (we know from
> looking at the mode whether it refers to a symbolic link or a regular
> file).

I think you can lose the last paragraph and enrich the first one instead. The text as-given does not even say anything about the new option affecting only blobs.

	The --filters option applies the convert_to_working_tree()
	filter for the path when showing the contents of a regular
	file blob object.
or something like that.
Show 5 quoted lines
> +--filters::
> +	Show the content as transformed by the filters configured in
> +	the current working tree for the given <path> (i.e. smudge filters,
> +	end-of-line conversion, etc). In this case, <object> has to be of
> +	the form <tree-ish>:<path>, or :<path>.

Makes sense. We show the contents as if we are checking the blob out to the given <path>, in other words.

Show 31 quoted lines
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 2dfe626..0b74afa 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -20,6 +20,33 @@ struct batch_options {
>  	const char *format;
>  };
>  
> +static int filter_object(const char *path, unsigned mode,
> +			 const unsigned char *sha1,
> +			 char **buf, unsigned long *size)
> +{
> +	enum object_type type;
> +
> +	*buf = read_sha1_file(sha1, &type, size);
> +	if (!*buf)
> +		return error(_("cannot read object %s '%s'"),
> +			sha1_to_hex(sha1), path);
> +	if (type != OBJ_BLOB) {
> +		free(*buf);
> +		return error(_("blob expected for %s '%s'"),
> +			sha1_to_hex(sha1), path);
> +	}
> +	if (S_ISREG(mode)) {
> +		struct strbuf strbuf = STRBUF_INIT;
> +		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
> +			free(*buf);
> +			*size = strbuf.len;
> +			*buf = strbuf_detach(&strbuf, NULL);
> +		}
> +	}

This is just a tangent for future clean-up of the coding convention, but if we used size_t internally throughout the system, we would be able to use strbuf_detach(&strbuf, size) instead. With the current coding convention, all internal sizes are ulong, and this is the best we could do.

Show 14 quoted lines
> @@ -61,6 +88,16 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>  	case 'e':
>  		return !has_sha1_file(sha1);
>  
> +	case 'w':
> +		if (!obj_context.path[0])
> +			die("git cat-file --filters %s: <object> must be "
> +			    "<sha1:path>", obj_name);
> +
> +		if (filter_object(obj_context.path, obj_context.mode,
> +				  sha1, &buf, &size))
> +			return -1;
> +		break;
> +
Makes sense.
Show 18 quoted lines
> diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
> new file mode 100755
> index 0000000..e466634
> --- /dev/null
> +++ b/t/t8010-cat-file-filters.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='git cat-file filters support'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup ' '
> +	echo "*.txt eol=crlf diff=txt" >.gitattributes &&
> +	echo "hello" | append_cr >world.txt &&
> +	git add .gitattributes world.txt &&
> +	test_tick &&
> +	git commit -m "Initial commit"
> +'

OK, so we ask *.txt in the working tree to be CRLF (and presumably that means the in-repository objects are LF).

Show 14 quoted lines
> +has_cr () {
> +	tr '\015' Q <"$1" | grep Q >/dev/null
> +}
> +
> +test_expect_success 'no filters with `git show`' '
> +	git show HEAD:world.txt >actual &&
> +	! has_cr actual
> +
> +'
> +
> +test_expect_success 'no filters with cat-file' '
> +	git cat-file blob HEAD:world.txt >actual &&
> +	! has_cr actual
> +'
OK.
> +test_expect_success 'cat-file --filters converts to worktree version' '
> +	git cat-file --filters HEAD:world.txt >actual &&
> +	has_cr actual
> +'
OK.
Johannes Schindelin· Aug 19, 2016, 12:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/4] cat-file: introduce the --filters option

Hi Junio,
On Thu, 18 Aug 2016, Junio C Hamano wrote:
Show 16 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > As suggested by its name, the --filters option applies the filters
> > that are currently configured for the specified path.
> >
> > [...]
> 
> I think you can lose the last paragraph and enrich the first one
> instead.  The text as-given does not even say anything about the new
> option affecting only blobs.
> 
> 	The --filters option applies the convert_to_working_tree()
> 	filter for the path when showing the contents of a regular
> 	file blob object.
> 
> or something like that.
Done.

Thanks, Dscho

Torsten Bögershausen· Aug 18, 2016, 15:49 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/4] cat-file: introduce the --filters option

On Thu, Aug 18, 2016 at 02:46:17PM +0200, Johannes Schindelin wrote:
> As suggested by its name, the --filters option applies the filters
[]
> diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
Does it make sense to integrate tests into t1006-cat-file ?
Show 24 quoted lines
> new file mode 100755
> index 0000000..e466634
> --- /dev/null
> +++ b/t/t8010-cat-file-filters.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='git cat-file filters support'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup ' '
> +	echo "*.txt eol=crlf diff=txt" >.gitattributes &&
> +	echo "hello" | append_cr >world.txt &&
> +	git add .gitattributes world.txt &&
> +	test_tick &&
> +	git commit -m "Initial commit"
> +'
> +
> +has_cr () {
> +	tr '\015' Q <"$1" | grep Q >/dev/null
> +}
> +
> +test_expect_success 'no filters with `git show`' '
> +	git show HEAD:world.txt >actual &&
I would prefer to have something using
  cat >expect <<-\EOF &&
  xxx
  test_cmp expect actual
to make it easier to debug in case of a failure ?
Johannes Schindelin· Aug 19, 2016, 12:37 UTC · re: Torsten Bögershausen · lore

Re: [PATCH 2/4] cat-file: introduce the --filters option

Hi Torsten,
On Thu, 18 Aug 2016, Torsten Bögershausen wrote:
Show 7 quoted lines
> On Thu, Aug 18, 2016 at 02:46:17PM +0200, Johannes Schindelin wrote:
> > As suggested by its name, the --filters option applies the filters
> 
> []
> > diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
>
> Does it make sense to integrate tests into t1006-cat-file ?

t1006 is already a 500+ line script, running 110 cases in 13 seconds over here. I really rather have a new, small, concise and parallelizable script.

Show 8 quoted lines
> > +test_expect_success 'no filters with `git show`' '
> > +	git show HEAD:world.txt >actual &&
>
> I would prefer to have something using
>   cat >expect <<-\EOF &&
>   xxx
>   test_cmp expect actual
> to make it easier to debug in case of a failure ?

Actually, I find it much harder to debug these "the output must match these precise bytes, else we fail" type of test cases. Instead, I describe in a natural way what is expected:

	! has_cr actual

Now, when the test fails, whoever is that poor soul tasked with debugging and fixing the breakage knows *what* goes wrong, conceptually.

So I do not think it would be a good idea to change the test in the way you requested.

And now I have a request of my own. When you quote parts of my mail, you insert your answers without surrounding them by empty lines. That makes it extraordinarily hard for me to read your mails (in fact, I almost missed your second comment).

Could you please start surrounding your replies by empty lines?

Thank you, Dscho

Junio C Hamano· Aug 19, 2016, 16:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/4] cat-file: introduce the --filters option

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
> Actually, I find it much harder to debug these "the output must match
> these precise bytes, else we fail" type of test cases. Instead, I describe
> in a natural way what is expected:
>
> 	! has_cr actual
>
> Now, when the test fails, whoever is that poor soul tasked with debugging
> and fixing the breakage knows *what* goes wrong, conceptually.

I am of two minds but 60% in favor of the "We only care about presense of CR" approach. Of course, the remaining 40% is "other people may break the code in such a way that still has CR at the end of the line but change ealier bytes on the line in the future (an obvious way to do so is to turn an input "ABC\n" into "AB\r\n"), and we would want to catch it". But the conversion machinery is tested elsewhere in the test suite, and this test is more about the cat-file command making a call into the conversion machinery when and only when it is necessary, so that sways me towards "has_cr is what we want here".

> Could you please start surrounding your replies by empty lines?

Good suggestion. I have exactly the same problem whenever I read messages without blanks at paragraph boundaries.

Thanks.
Torsten Bögershausen· Aug 19, 2016, 08:57 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/4] cat-file: introduce the --filters option

[] On Thu, Aug 18, 2016 at 02:46:17PM +0200, Johannes Schindelin wrote:

Show 29 quoted lines
> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
> index 071029b..7d48735 100644
> --- a/Documentation/git-cat-file.txt
> +++ b/Documentation/git-cat-file.txt
> @@ -9,15 +9,15 @@ git-cat-file - Provide content or type and size information for repository objec
>  SYNOPSIS
>  --------
>  [verse]
> -'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv ) <object>
> +'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) <object>
>  'git cat-file' (--batch | --batch-check) [--follow-symlinks]
>  
>  DESCRIPTION
>  -----------
>  In its first form, the command provides the content or the type of an object in
>  the repository. The type is required unless `-t` or `-p` is used to find the
> -object type, or `-s` is used to find the object size, or `--textconv` is used
> -(which implies type "blob").
> +object type, or `-s` is used to find the object size, or `--textconv` or
> +`--filters` is used (which imply type "blob").
>  
>  In the second form, a list of objects (separated by linefeeds) is provided on
>  stdin, and the SHA-1, type, and size of each object is printed on stdout.
> @@ -58,6 +58,12 @@ OPTIONS
>  	order to apply the filter to the content recorded in the index at
>  	<path>.
>  
> +--filters::
> +	Show the content as transformed by the filters configured in

Minor comment: s/transformed/converted/ ?

Does it make sense to be more specific here:
The order of conversion is
- ident
- CRLF
- smudge
Johannes Schindelin· Aug 19, 2016, 15:00 UTC · re: Torsten Bögershausen · lore

Re: [PATCH 2/4] cat-file: introduce the --filters option

Hi Torsten,
On Fri, 19 Aug 2016, Torsten Bögershausen wrote:
Show 7 quoted lines
> On Thu, Aug 18, 2016 at 02:46:17PM +0200, Johannes Schindelin wrote:
> 
> > +--filters::
> > +	Show the content as transformed by the filters configured in
>
> Minor comment:
> s/transformed/converted/ ?
Sure.
Show 5 quoted lines
> Does it make sense to be more specific here:
> The order of conversion is
> - ident
> - CRLF
> - smudge

I do not think it makes sense to complexify the documentation in that manner. The filters should always be applied in the same order, methinks, and it would only clutter the man page to repeat that order here.

Ciao, Dscho

Junio C Hamano· Aug 19, 2016, 16:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/4] cat-file: introduce the --filters option

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
>> Does it make sense to be more specific here:
>> The order of conversion is
>> - ident
>> - CRLF
>> - smudge
>
> I do not think it makes sense to complexify the documentation in that
> manner.
Right.
Torsten Bögershausen· Aug 22, 2016, 16:51 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/4] cat-file: introduce the --filters option

On 19.08.16 17:00, Johannes Schindelin wrote:
Show 20 quoted lines
> Hi Torsten,
>
> On Fri, 19 Aug 2016, Torsten Bögershausen wrote:
>
>> On Thu, Aug 18, 2016 at 02:46:17PM +0200, Johannes Schindelin wrote:
>>
>>> +--filters::
>>> +	Show the content as transformed by the filters configured in
>> Minor comment:
>> s/transformed/converted/ ?
> Sure.
>
>> Does it make sense to be more specific here:
>> The order of conversion is
>> - ident
>> - CRLF
>> - smudge
> I do not think it makes sense to complexify the documentation in that
> manner. The filters should always be applied in the same order, methinks,
> and it would only clutter the man page to repeat that order here.
Can we can shorten the description and have something like this:
--filters::
+	Show the content converted by the filters configured in
+	the current working tree for the given <path>. 
+ 	<object> has to be of the form <tree-ish>:<path> or :<path>.
+
Johannes Schindelin· Aug 24, 2016, 08:00 UTC · re: Torsten Bögershausen · lore

Re: [PATCH 2/4] cat-file: introduce the --filters option

Hi Torsten,
On Mon, 22 Aug 2016, Torsten Bögershausen wrote:
Show 26 quoted lines
> On 19.08.16 17:00, Johannes Schindelin wrote:
> >
> > On Fri, 19 Aug 2016, Torsten Bögershausen wrote:
> >
> >> On Thu, Aug 18, 2016 at 02:46:17PM +0200, Johannes Schindelin wrote:
> >>
> >>> +--filters::
> >>> +	Show the content as transformed by the filters configured in
> >> Minor comment:
> >> s/transformed/converted/ ?
> > Sure.
> >
> >> Does it make sense to be more specific here:
> >> The order of conversion is
> >> - ident
> >> - CRLF
> >> - smudge
> > I do not think it makes sense to complexify the documentation in that
> > manner. The filters should always be applied in the same order, methinks,
> > and it would only clutter the man page to repeat that order here.
> Can we can shorten the description and have something like this:
> 
> --filters::
> +	Show the content converted by the filters configured in
> +	the current working tree for the given <path>. 
> + 	<object> has to be of the form <tree-ish>:<path> or :<path>.

I do not want it shortened *that* much. I, for one, would be confused reading about filters, as there are so many of them. So while I do not want cat-file's man page to include an extensive description how filters work, I do want it to have an indication which filters this option is talking about.

Ciao, Dscho

Johannes Schindelin· Aug 18, 2016, 12:46 UTC · re: Johannes Schindelin · lore

[PATCH 4/4] cat-file: support --textconv/--filters in batch mode

With this patch, --batch can be combined with --textconv or --filters. For this to work, the input needs to have the form

	<object name><single white space><path>
so that the filters can be chosen appropriately.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt | 18 +++++++++++-----
 builtin/cat-file.c             | 49 +++++++++++++++++++++++++++++++++++++-----
 t/t8010-cat-file-filters.sh    | 10 +++++++++
 3 files changed, 67 insertions(+), 10 deletions(-)
Show changes to 3 files +67 −10

Documentation/git-cat-file.txt, builtin/cat-file.c, t/t8010-cat-file-filters.sh

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 59a3c37..1f4d954 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--use-path=<path>] <object>
-'git cat-file' (--batch | --batch-check) [--follow-symlinks]
+'git cat-file' (--batch | --batch-check) [ --textconv | --filters ] [--follow-symlinks]
 
 DESCRIPTION
 -----------
@@ -20,7 +20,11 @@ object type, or `-s` is used to find the object size, or `--textconv` or
 `--filters` is used (which imply type "blob").
 
 In the second form, a list of objects (separated by linefeeds) is provided on
-stdin, and the SHA-1, type, and size of each object is printed on stdout.
+stdin, and the SHA-1, type, and size of each object is printed on stdout. The
+output format can be overridden using the optional `<format>` argument. If
+either `--textconv` or `--filters` was specified, the input is expected to
+list the object names followed by the path name, separated by a single white
+space, so that the appropriate drivers can be determined.
 
 OPTIONS
 -------
@@ -72,13 +76,17 @@ OPTIONS
 --batch::
 --batch=<format>::
 	Print object information and contents for each object provided
-	on stdin.  May not be combined with any other options or arguments.
-	See the section `BATCH OUTPUT` below for details.
+	on stdin.  May not be combined with any other options or arguments
+	except `--textconv` or `--filters`, in which case the input lines
+	also need to specify the path, separated by white space.  See the
+	section `BATCH OUTPUT` below for details.
 
 --batch-check::
 --batch-check=<format>::
 	Print object information for each object provided on stdin.  May
-	not be combined with any other options or arguments.  See the
+	not be combined with any other options or arguments except
+	`--textconv` or `--filters`, in which case the input lines also
+	need to specify the path, separated by white space.  See the
 	section `BATCH OUTPUT` below for details.
 
 --batch-all-objects::
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 5ff58b3..5f91cf4 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -17,6 +17,7 @@ struct batch_options {
 	int print_contents;
 	int buffer_output;
 	int all_objects;
+	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
 	const char *format;
 };
 
@@ -281,7 +282,32 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 	if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
-		if (stream_blob_to_fd(1, sha1, NULL, 0) < 0)
+		if (opt->cmdmode) {
+			char *contents;
+			unsigned long size;
+
+			if (!data->rest)
+				die("missing path for '%s'", sha1_to_hex(sha1));
+
+			if (opt->cmdmode == 'w') {
+				if (filter_object(data->rest, 0100644, sha1,
+						  &contents, &size))
+					die("could not convert '%s' %s",
+					    sha1_to_hex(sha1), data->rest);
+			} else if (opt->cmdmode == 'c') {
+				enum object_type type;
+				if (!textconv_object(data->rest, 0100644, sha1,
+						     1, &contents, &size))
+					contents = read_sha1_file(sha1, &type,
+								  &size);
+				if (!contents)
+					die("could not convert '%s' %s",
+					    sha1_to_hex(sha1), data->rest);
+			} else
+				die("BUG: invalid cmdmode: %c", opt->cmdmode);
+			batch_write(opt, contents, size);
+			free(contents);
+		} else if (stream_blob_to_fd(1, sha1, NULL, 0) < 0)
 			die("unable to stream %s to stdout", sha1_to_hex(sha1));
 	}
 	else {
@@ -418,6 +444,8 @@ static int batch_objects(struct batch_options *opt)
 	data.mark_query = 1;
 	strbuf_expand(&buf, opt->format, expand_format, &data);
 	data.mark_query = 0;
+	if (opt->cmdmode)
+		data.split_on_whitespace = 1;
 
 	if (opt->all_objects) {
 		struct object_info empty;
@@ -483,7 +511,7 @@ static int batch_objects(struct batch_options *opt)
 
 static const char * const cat_file_usage[] = {
 	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) [--use-path=<path>] <object>"),
-	N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
+	N_("git cat-file (--batch | --batch-check) [--follow-symlinks] [--textconv|--filters]"),
 	NULL
 };
 
@@ -554,7 +582,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
 
 	if (opt) {
-		if (argc == 1)
+		if (batch.enabled && (opt == 'c' || opt == 'w'))
+			batch.cmdmode = opt;
+		else if (argc == 1)
 			obj_name = argv[0];
 		else
 			usage_with_options(cat_file_usage, options);
@@ -566,8 +596,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		} else
 			usage_with_options(cat_file_usage, options);
 	}
-	if (batch.enabled && (opt || argc)) {
-		usage_with_options(cat_file_usage, options);
+	if (batch.enabled) {
+		if (batch.cmdmode != opt || argc)
+			usage_with_options(cat_file_usage, options);
+		if (batch.cmdmode && batch.all_objects)
+			die("--batch-all-objects cannot be combined with "
+			    "--textconv nor with --filters");
 	}
 
 	if ((batch.follow_symlinks || batch.all_objects) && !batch.enabled) {
@@ -579,6 +613,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		usage_with_options(cat_file_usage, options);
 	}
 
+	if (force_path && batch.enabled) {
+		error("--use-path=<path> incompatible with --batch");
+		usage_with_options(cat_file_usage, options);
+	}
+
 	if (batch.buffer_output < 0)
 		batch.buffer_output = batch.all_objects;
 
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index fd17159..89ae868 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -44,4 +44,14 @@ test_expect_success 'cat-file --textconv --use-path=<path> works' '
 	test uryyb = "$(cat rot13 | remove_cr)"
 '
 
+test_expect_success 'cat-file --textconv --batch works' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
+	printf "%s hello.txt\n%s hello\n" $sha1 $sha1 |
+	git cat-file --textconv --batch >actual &&
+	printf "%s blob 6\nuryyb\r\n\n%s blob 6\nhello\n\n" \
+		$sha1 $sha1 >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.9.2.691.g78954f3
Junio C Hamano· Aug 18, 2016, 17:08 UTC · re: Johannes Schindelin · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 15 quoted lines
> With this patch, --batch can be combined with --textconv or --filters.
> For this to work, the input needs to have the form
>
> 	<object name><single white space><path>
>
> so that the filters can be chosen appropriately.
>  --batch::
>  --batch=<format>::
>  	Print object information and contents for each object provided
> -	on stdin.  May not be combined with any other options or arguments.
> -	See the section `BATCH OUTPUT` below for details.
> +	on stdin.  May not be combined with any other options or arguments
> +	except `--textconv` or `--filters`, in which case the input lines
> +	also need to specify the path, separated by white space.  See the
> +	section `BATCH OUTPUT` below for details.
Makes sense.  This format still allows
	HEAD:<path2> <path1>

i.e. the object name is taken from path2 but we forget it and pretend that the blob sits at path2, which is a good feature.

If I am not mistaken, your cover letter alluded that it might be ideal to take "HEAD:<path>" (nothing else) as input, grab "<path>" and feed that to the filtering machinery, but you decided to stop short of doing that. I actually think that it is the right thing to do for this feature to ignore the trailing :<path> in the object name, iow, I agree with the result from the feature design POV.

The only thing that somewhat is worrysome is what would happen to %(rest). I guess [*1*] it is OK to declare that you cannot use %(rest) in your output format when --filter/--textconv is in use, but if that is the direction we are going, that limitation needs to be documented.

[Footnote]

*1* This is just a "guess", because I do not know what people are using %(rest) for. It is plausible that their use case do not need --filter/--textconv at all, and if that is the case, having this limitation is perfectly fine.

Johannes Schindelin· Aug 19, 2016, 12:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

Hi Junio,
On Thu, 18 Aug 2016, Junio C Hamano wrote:
Show 24 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > With this patch, --batch can be combined with --textconv or --filters.
> > For this to work, the input needs to have the form
> >
> > 	<object name><single white space><path>
> >
> > so that the filters can be chosen appropriately.
> >  --batch::
> >  --batch=<format>::
> >  	Print object information and contents for each object provided
> > -	on stdin.  May not be combined with any other options or arguments.
> > -	See the section `BATCH OUTPUT` below for details.
> > +	on stdin.  May not be combined with any other options or arguments
> > +	except `--textconv` or `--filters`, in which case the input lines
> > +	also need to specify the path, separated by white space.  See the
> > +	section `BATCH OUTPUT` below for details.
> 
> Makes sense.  This format still allows
> 
> 	HEAD:<path2> <path1>
> 
> i.e. the object name is taken from path2 but we forget it and
> pretend that the blob sits at path2, which is a good feature.
Correct.
Show 6 quoted lines
> If I am not mistaken, your cover letter alluded that it might be
> ideal to take "HEAD:<path>" (nothing else) as input, grab "<path>"
> and feed that to the filtering machinery, but you decided to stop
> short of doing that.  I actually think that it is the right thing to
> do for this feature to ignore the trailing :<path> in the object
> name, iow, I agree with the result from the feature design POV.

I actually decided against it only because it would uglify the code. From a user point of view, I am a bit annoyed having to say

	:README README
	:Makefile Makefile
	:main.c main.c
	[...]
> The only thing that somewhat is worrysome is what would happen to
> %(rest).

What would happen is that it would print out the path, as it is exactly that "rest" field that is used in the implementation, piggybacking on that very nice feature.

Of course, this means that you simply cannot send "SHA-1 path rest" to cat-file --batch and expect that to work. Should anybody need that in the future, they can give it a try themselves to patch cat-file.

Ciao, Dscho

Jeff King· Aug 19, 2016, 14:44 UTC · re: Johannes Schindelin · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

On Fri, Aug 19, 2016 at 02:59:32PM +0200, Johannes Schindelin wrote:
Show 6 quoted lines
> > The only thing that somewhat is worrysome is what would happen to
> > %(rest).
> 
> What would happen is that it would print out the path, as it is exactly
> that "rest" field that is used in the implementation, piggybacking on that
> very nice feature.
Good, that is what I would expect to happen.
> Of course, this means that you simply cannot send "SHA-1 path rest" to
> cat-file --batch and expect that to work. Should anybody need that in the
> future, they can give it a try themselves to patch cat-file.

Right. The real motivation for %(rest) was to accept "git rev-list --objects" format, so you can do stuff like:

  git rev-list --objects --all |
  git cat-file --batch-check='%(objectsize) %(rest)' |
  sort -rn |
  head

to get the paths of the largest objects. So the fact that "%(rest)" will be the filename in this case is almost certainly the most useful default (and if somebody really wants something else, they can try to figure out other semantics later).

-Peff
Jeff King· Aug 18, 2016, 22:05 UTC · re: Johannes Schindelin · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
Show 6 quoted lines
> With this patch, --batch can be combined with --textconv or --filters.
> For this to work, the input needs to have the form
> 
> 	<object name><single white space><path>
> 
> so that the filters can be chosen appropriately.
The object name can have spaces in it, too. E.g.:
  HEAD:path with spaces
or even:
  :/grep for this

(as was pointed out to me when I tried to turn on %(rest) handling by default, long ago). How do those work with your patch?

It looks like the extra split isn't enabled unless one of those options is selected. Since --filters is new, that's OK for backwards compatibility. But --textconv isn't. So I think:

  echo "HEAD:path with spaces" |
  git cat-file --textconv --batch
is regressed by this patch.

I wonder if we need an option specifically to say "the object name can be split". Right now it kicks in automatically if you use "%(rest)" in your format, but you might not care about passing along that output (e.g., a lot of times I am piping "rev-list" straight to cat-file, and I have to use a separate "cut" to throw away the pathnames).

-Peff
Junio C Hamano· Aug 18, 2016, 22:47 UTC · re: Jeff King · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
>
>> With this patch, --batch can be combined with --textconv or --filters.
>> For this to work, the input needs to have the form
>> 
>> 	<object name><single white space><path>
>> 
>> so that the filters can be chosen appropriately.
>
> The object name can have spaces in it, too. E.g.:
>
>   HEAD:path with spaces
>
> or even:
>
>   :/grep for this
When I wrote my review, I didn't consider this use case.

There is no -z format in --batch, which is unfortunate. If we had one, it would trivially make it possible to do so, and we can even have paths with LF in them ;-). On the other hand, producing a NUL separated input is a chore.

Perhaps a new and separate option that is similar to "--batch" but lacks support for %(rest) and accepts _ONLY_ 40-hex as object name is the best we can do, then?

Show 19 quoted lines
> (as was pointed out to me when I tried to turn on %(rest) handling by
> default, long ago). How do those work with your patch?
>
> It looks like the extra split isn't enabled unless one of those options
> is selected. Since --filters is new, that's OK for backwards
> compatibility. But --textconv isn't. So I think:
>
>   echo "HEAD:path with spaces" |
>   git cat-file --textconv --batch
>
> is regressed by this patch.
>
> I wonder if we need an option specifically to say "the object name can
> be split". Right now it kicks in automatically if you use "%(rest)" in
> your format, but you might not care about passing along that output
> (e.g., a lot of times I am piping "rev-list" straight to cat-file, and I
> have to use a separate "cut" to throw away the pathnames).
>
> -Peff
Johannes Schindelin· Aug 19, 2016, 13:11 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

Hi Junio,
On Thu, 18 Aug 2016, Junio C Hamano wrote:
Show 25 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
> >
> >> With this patch, --batch can be combined with --textconv or --filters.
> >> For this to work, the input needs to have the form
> >> 
> >> 	<object name><single white space><path>
> >> 
> >> so that the filters can be chosen appropriately.
> >
> > The object name can have spaces in it, too. E.g.:
> >
> >   HEAD:path with spaces
> >
> > or even:
> >
> >   :/grep for this
> 
> When I wrote my review, I didn't consider this use case.
> 
> There is no -z format in --batch, which is unfortunate.  If we had
> one, it would trivially make it possible to do so, and we can even
> have paths with LF in them ;-).  On the other hand, producing a NUL
> separated input is a chore.

I think it would make for a fine little project to add support for --batch -z.

Just not in this here patch series.

Ciao, Dscho

Johannes Schindelin· Aug 19, 2016, 13:09 UTC · re: Jeff King · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

Hi Peff,
On Thu, 18 Aug 2016, Jeff King wrote:
Show 19 quoted lines
> On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
> 
> > With this patch, --batch can be combined with --textconv or --filters.
> > For this to work, the input needs to have the form
> > 
> > 	<object name><single white space><path>
> > 
> > so that the filters can be chosen appropriately.
> 
> The object name can have spaces in it, too. E.g.:
> 
>   HEAD:path with spaces
> 
> or even:
> 
>   :/grep for this
> 
> (as was pointed out to me when I tried to turn on %(rest) handling by
> default, long ago). How do those work with your patch?
They don't ;-)

And quite frankly, the documentation should make it clear to users that batch mode with --filters or --textconv won't work when the object name contains white space: it says that the object name is split from the path at the first white space character.

> It looks like the extra split isn't enabled unless one of those options
> is selected. Since --filters is new, that's OK for backwards
> compatibility. But --textconv isn't.

Except that it is okay, because --textconv *was not even supported in batch mode*. So there is no backwards compatibility that could be broken.

Show 5 quoted lines
> I wonder if we need an option specifically to say "the object name can
> be split". Right now it kicks in automatically if you use "%(rest)" in
> your format, but you might not care about passing along that output
> (e.g., a lot of times I am piping "rev-list" straight to cat-file, and I
> have to use a separate "cut" to throw away the pathnames).

As I said elsewhere in this thread: if anybody encounters that problem, they are welcome to fix it themselves.

My patch series purpose is to introduce --filters and make it compatible with the batch mode. As I imitated --textconv, it was both easy and correct to make it compatible with the batch mode, too. But that is the extent of this here patch series.

Fixing %(rest) for object names containing spaces is distinctly outside the original intent, and certainly outside of my use case.

Ciao, Dscho

Jeff King· Aug 19, 2016, 13:22 UTC · re: Johannes Schindelin · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

On Fri, Aug 19, 2016 at 03:09:19PM +0200, Johannes Schindelin wrote:
Show 17 quoted lines
> > The object name can have spaces in it, too. E.g.:
> > 
> >   HEAD:path with spaces
> > 
> > or even:
> > 
> >   :/grep for this
> > 
> > (as was pointed out to me when I tried to turn on %(rest) handling by
> > default, long ago). How do those work with your patch?
> 
> They don't ;-)
> 
> And quite frankly, the documentation should make it clear to users that
> batch mode with --filters or --textconv won't work when the object name
> contains white space: it says that the object name is split from the path
> at the first white space character.

I think that is an obvious implication of the new documentation. I was just concerned with a regression from the previous behavior.

Show 6 quoted lines
> > It looks like the extra split isn't enabled unless one of those options
> > is selected. Since --filters is new, that's OK for backwards
> > compatibility. But --textconv isn't.
> 
> Except that it is okay, because --textconv *was not even supported in
> batch mode*. So there is no backwards compatibility that could be broken.

Ah, OK. I thought we handled "HEAD:path with spaces" there, but I see that you cannot even specify "--textconv" with "--batch", because it complains of the cmdmode.

So the new rule becomes "we split if we see %(rest), or --textconv, or --filter", which is reasonable.

> Fixing %(rest) for object names containing spaces is distinctly outside
> the original intent, and certainly outside of my use case.

Yeah, I agree it is not necessary for this series (I was only considering it as an option to fix the regression I now see doesn't exist).

-Peff
Torsten Bögershausen· Aug 18, 2016, 15:42 UTC · re: Johannes Schindelin · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
Show 70 quoted lines
> With this patch, --batch can be combined with --textconv or --filters.
> For this to work, the input needs to have the form
> 
> 	<object name><single white space><path>
> 
> so that the filters can be chosen appropriately.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Documentation/git-cat-file.txt | 18 +++++++++++-----
>  builtin/cat-file.c             | 49 +++++++++++++++++++++++++++++++++++++-----
>  t/t8010-cat-file-filters.sh    | 10 +++++++++
>  3 files changed, 67 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
> index 59a3c37..1f4d954 100644
> --- a/Documentation/git-cat-file.txt
> +++ b/Documentation/git-cat-file.txt
> @@ -10,7 +10,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--use-path=<path>] <object>
> -'git cat-file' (--batch | --batch-check) [--follow-symlinks]
> +'git cat-file' (--batch | --batch-check) [ --textconv | --filters ] [--follow-symlinks]
>  
>  DESCRIPTION
>  -----------
> @@ -20,7 +20,11 @@ object type, or `-s` is used to find the object size, or `--textconv` or
>  `--filters` is used (which imply type "blob").
>  
>  In the second form, a list of objects (separated by linefeeds) is provided on
> -stdin, and the SHA-1, type, and size of each object is printed on stdout.
> +stdin, and the SHA-1, type, and size of each object is printed on stdout. The
> +output format can be overridden using the optional `<format>` argument. If
> +either `--textconv` or `--filters` was specified, the input is expected to
> +list the object names followed by the path name, separated by a single white
> +space, so that the appropriate drivers can be determined.
>  
>  OPTIONS
>  -------
> @@ -72,13 +76,17 @@ OPTIONS
>  --batch::
>  --batch=<format>::
>  	Print object information and contents for each object provided
> -	on stdin.  May not be combined with any other options or arguments.
> -	See the section `BATCH OUTPUT` below for details.
> +	on stdin.  May not be combined with any other options or arguments
> +	except `--textconv` or `--filters`, in which case the input lines
> +	also need to specify the path, separated by white space.  See the
> +	section `BATCH OUTPUT` below for details.
>  
>  --batch-check::
>  --batch-check=<format>::
>  	Print object information for each object provided on stdin.  May
> -	not be combined with any other options or arguments.  See the
> +	not be combined with any other options or arguments except
> +	`--textconv` or `--filters`, in which case the input lines also
> +	need to specify the path, separated by white space.  See the
>  	section `BATCH OUTPUT` below for details.
>  
>  --batch-all-objects::
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 5ff58b3..5f91cf4 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -17,6 +17,7 @@ struct batch_options {
>  	int print_contents;
>  	int buffer_output;
>  	int all_objects;
> +	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */

How do I read 'w' and 'c' ? wilter and cextconv ? Does it make sense to use an enum here ? Or a #define ?

Johannes Schindelin· Aug 19, 2016, 12:25 UTC · re: Torsten Bögershausen · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

Hi Torsten,
On Thu, 18 Aug 2016, Torsten Bögershausen wrote:
Show 13 quoted lines
> On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 5ff58b3..5f91cf4 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -17,6 +17,7 @@ struct batch_options {
> >  	int print_contents;
> >  	int buffer_output;
> >  	int all_objects;
> > +	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
> How do I read 'w' and 'c' ?
> wilter and cextconv ? Does it make sense to use an enum here ?
> Or a #define ?

Sorry, Torsten, this is not my doing. So I cannot explain why it is not an enum.

I *guess* the rationale for 'c' being the cmdmode of --textconv is "c for convert". That is why I chose "w as in worktree" as new cmdmode.

Ciao, Dscho

Jeff King· Aug 19, 2016, 15:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

On Fri, Aug 19, 2016 at 02:25:51PM +0200, Johannes Schindelin wrote:
Show 19 quoted lines
> > On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
> > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > > index 5ff58b3..5f91cf4 100644
> > > --- a/builtin/cat-file.c
> > > +++ b/builtin/cat-file.c
> > > @@ -17,6 +17,7 @@ struct batch_options {
> > >  	int print_contents;
> > >  	int buffer_output;
> > >  	int all_objects;
> > > +	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
> > How do I read 'w' and 'c' ?
> > wilter and cextconv ? Does it make sense to use an enum here ?
> > Or a #define ?
> 
> Sorry, Torsten, this is not my doing. So I cannot explain why it is not an
> enum.
> 
> I *guess* the rationale for 'c' being the cmdmode of --textconv is "c for
> convert". That is why I chose "w as in worktree" as new cmdmode.

I think it started as a 'char' because it was representing the single-letter options of "cat-file -e", "cat-file -s", etc. And then the textconv patches started the monstrosity of "c".

I don't think cleaning it up needs to be part of your series, but it might be nice low-hanging fruit for somebody to clean up on top.

-Peff
Junio C Hamano· Aug 19, 2016, 16:19 UTC · re: Jeff King · lore

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
>> Sorry, Torsten, this is not my doing. So I cannot explain why it is not an
>> enum.
>> 
>> I *guess* the rationale for 'c' being the cmdmode of --textconv is "c for
>> convert". That is why I chose "w as in worktree" as new cmdmode.
>
> I think it started as a 'char' because it was representing the
> single-letter options of "cat-file -e", "cat-file -s", etc. And then the
> textconv patches started the monstrosity of "c".

I think it was merely 't' was taken for "-t" (one beauty of using the OPT_CMDMODE for options with shorthand is that we do not need any enum, as the short-form options already form an enum), so "textConv" was the compromise. It could have been 'T' or even \C-t ;-)

> I don't think cleaning it up needs to be part of your series, but it
> might be nice low-hanging fruit for somebody to clean up on top.
I agree.

I briefly wondered if we want to add a check to ensure uniqueness of these cmdmode letters in parse_options_check(), but that would forbid us from having synonymous options (e.g. "am --continue" and "am --resolved" both map to the same), so it would not work.

Johannes Schindelin· Aug 18, 2016, 12:46 UTC · re: Johannes Schindelin · lore

[PATCH 1/4] cat-file: fix a grammo in the man page

"... has be ..." -> "... has to be ..."
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to Documentation/git-cat-file.txt +3 −2
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 18d03d8..071029b 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -54,8 +54,9 @@ OPTIONS
 
 --textconv::
 	Show the content as transformed by a textconv filter. In this case,
-	<object> has be of the form <tree-ish>:<path>, or :<path> in order
-	to apply the filter to the content recorded in the index at <path>.
+	<object> has to be of the form <tree-ish>:<path>, or :<path> in
+	order to apply the filter to the content recorded in the index at
+	<path>.
 
 --batch::
 --batch=<format>::
-- 
2.9.2.691.g78954f3
Junio C Hamano· Aug 18, 2016, 16:21 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/4] cat-file: fix a grammo in the man page

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 20 quoted lines
> "... has be ..." -> "... has to be ..."
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Documentation/git-cat-file.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
> index 18d03d8..071029b 100644
> --- a/Documentation/git-cat-file.txt
> +++ b/Documentation/git-cat-file.txt
> @@ -54,8 +54,9 @@ OPTIONS
>  
>  --textconv::
>  	Show the content as transformed by a textconv filter. In this case,
> -	<object> has be of the form <tree-ish>:<path>, or :<path> in order
> -	to apply the filter to the content recorded in the index at <path>.
> +	<object> has to be of the form <tree-ish>:<path>, or :<path> in
> +	order to apply the filter to the content recorded in the index at
> +	<path>.
Thanks.  That's an ancient typo dating back to 2010 ;-)
Johannes Schindelin· Aug 24, 2016, 12:23 UTC · re: Johannes Schindelin · lore

[PATCH v2 0/4] cat-file: optionally convert to worktree version

When third-party tools need to access to contents of blobs in the database, they might be more interested in the worktree version than in the "clean" version of said contents.

This branch introduces the --filters option to make that happen, the --use-path option to provide the path separately if the blob name rather than the tree name is availale, and offers batch support in which case it expects the object names and the path on its input lines, separated by white space.

The new --filters option is an obvious sibling of --textconv, and shares the peculiar feature that the drivers (and end-of-line convention) are determined from the current worktree, not from the attributes stored in the revision that may have been part of the object name.

As --textconv is so similar to --filters, it was taught to understand the --use-path option and it was made compatible with batch mode, too.

I briefly considered teaching the batch mode to extract the path from object names if they are specified as <tree-ish>:<path>. The changes would be quite intrusive, though, and uglify the code substanitially. So I decided against that.

Changes vs v2:
- two commit messages were touched up
- --use-path=<path> was renamed to --path=<path>
- a test was added to verify that --path without --textconv/--filters
  errors out with helpful advice
Johannes Schindelin (4):
  cat-file: fix a grammo in the man page
  cat-file: introduce the --filters option
  cat-file --textconv/--filters: allow specifying the path separately
  cat-file: support --textconv/--filters in batch mode
 Documentation/git-cat-file.txt |  40 +++++++++++----
 builtin/cat-file.c             | 110 ++++++++++++++++++++++++++++++++++++++---
 t/t8010-cat-file-filters.sh    |  64 ++++++++++++++++++++++++
 3 files changed, 196 insertions(+), 18 deletions(-)
 create mode 100755 t/t8010-cat-file-filters.sh
Published-As: https://github.com/dscho/git/releases/tag/cat-file-filters-v2
Fetch-It-Via: git fetch https://github.com/dscho/git cat-file-filters-v2
Interdiff vs v1:
 diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
 index 1f4d954..204541c 100644
 --- a/Documentation/git-cat-file.txt
 +++ b/Documentation/git-cat-file.txt
 @@ -9,7 +9,7 @@ git-cat-file - Provide content or type and size information for repository objec
  SYNOPSIS
  --------
  [verse]
 -'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--use-path=<path>] <object>
 +'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
  'git cat-file' (--batch | --batch-check) [ --textconv | --filters ] [--follow-symlinks]
  
  DESCRIPTION
 @@ -63,12 +63,12 @@ OPTIONS
  	<path>.
  
  --filters::
 -	Show the content as transformed by the filters configured in
 +	Show the content as converted by the filters configured in
  	the current working tree for the given <path> (i.e. smudge filters,
  	end-of-line conversion, etc). In this case, <object> has to be of
  	the form <tree-ish>:<path>, or :<path>.
  
 ---use-path=<path>::
 +--path=<path>::
  	For use with --textconv or --filters, to allow specifying an object
  	name and a path separately, e.g. when it is difficult to figure out
  	the revision from which the blob came.
 diff --git a/builtin/cat-file.c b/builtin/cat-file.c
 index 5f91cf4..f8a3a08 100644
 --- a/builtin/cat-file.c
 +++ b/builtin/cat-file.c
 @@ -61,6 +61,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
  	struct object_info oi = {NULL};
  	struct strbuf sb = STRBUF_INIT;
  	unsigned flags = LOOKUP_REPLACE_OBJECT;
 +	const char *path = force_path;
  
  	if (unknown_type)
  		flags |= LOOKUP_UNKNOWN_OBJECT;
 @@ -68,6 +69,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
  	if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
  		die("Not a valid object name %s", obj_name);
  
 +	if (!path)
 +		path = obj_context.path;
 +	else if (obj_context.mode == S_IFINVALID)
 +		obj_context.mode = 0100644;
 +
  	buf = NULL;
  	switch (opt) {
  	case 't':
 @@ -92,23 +98,21 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
  		return !has_sha1_file(sha1);
  
  	case 'w':
 -		if (!force_path && !obj_context.path[0])
 +		if (!path[0])
  			die("git cat-file --filters %s: <object> must be "
  			    "<sha1:path>", obj_name);
  
 -		if (filter_object(force_path ? force_path : obj_context.path,
 -				  force_path ? 0100644 : obj_context.mode,
 +		if (filter_object(path, obj_context.mode,
  				  sha1, &buf, &size))
  			return -1;
  		break;
  
  	case 'c':
 -		if (!force_path && !obj_context.path[0])
 +		if (!path[0])
  			die("git cat-file --textconv %s: <object> must be <sha1:path>",
  			    obj_name);
  
 -		if (textconv_object(force_path ? force_path : obj_context.path,
 -				    force_path ? 0100644 : obj_context.mode,
 +		if (textconv_object(path, obj_context.mode,
  				    sha1, 1, &buf, &size))
  			break;
  
 @@ -510,7 +514,7 @@ static int batch_objects(struct batch_options *opt)
  }
  
  static const char * const cat_file_usage[] = {
 -	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) [--use-path=<path>] <object>"),
 +	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) [--path=<path>] <object>"),
  	N_("git cat-file (--batch | --batch-check) [--follow-symlinks] [--textconv|--filters]"),
  	NULL
  };
 @@ -558,7 +562,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
  			    N_("for blob objects, run textconv on object's content"), 'c'),
  		OPT_CMDMODE(0, "filters", &opt,
  			    N_("for blob objects, run filters on object's content"), 'w'),
 -		OPT_STRING(0, "use-path", &force_path, N_("blob"),
 +		OPT_STRING(0, "path", &force_path, N_("blob"),
  			   N_("use a specific path for --textconv/--filters")),
  		OPT_BOOL(0, "allow-unknown-type", &unknown_type,
  			  N_("allow -s and -t to work with broken/corrupt objects")),
 @@ -609,12 +613,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
  	}
  
  	if (force_path && opt != 'c' && opt != 'w') {
 -		error("--use-path=<path> needs --textconv or --filters");
 +		error("--path=<path> needs --textconv or --filters");
  		usage_with_options(cat_file_usage, options);
  	}
  
  	if (force_path && batch.enabled) {
 -		error("--use-path=<path> incompatible with --batch");
 +		error("--path=<path> incompatible with --batch");
  		usage_with_options(cat_file_usage, options);
  	}
  
 diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
 index 89ae868..acdfa09 100755
 --- a/t/t8010-cat-file-filters.sh
 +++ b/t/t8010-cat-file-filters.sh
 @@ -31,19 +31,26 @@ test_expect_success 'cat-file --filters converts to worktree version' '
  	has_cr actual
  '
  
 -test_expect_success 'cat-file --filters --use-path=<path> works' '
 +test_expect_success 'cat-file --filters --path=<path> works' '
  	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
 -	git cat-file --filters --use-path=world.txt $sha1 >actual &&
 +	git cat-file --filters --path=world.txt $sha1 >actual &&
  	has_cr actual
  '
  
 -test_expect_success 'cat-file --textconv --use-path=<path> works' '
 +test_expect_success 'cat-file --textconv --path=<path> works' '
  	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
  	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
 -	git cat-file --textconv --use-path=hello.txt $sha1 >rot13 &&
 +	git cat-file --textconv --path=hello.txt $sha1 >rot13 &&
  	test uryyb = "$(cat rot13 | remove_cr)"
  '
  
 +test_expect_success '----path=<path> complains without --textconv/--filters' '
 +	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
 +	test_must_fail git cat-file --path=hello.txt blob $sha1 >actual 2>err &&
 +	test ! -s actual &&
 +	grep "path.*needs.*filters" err
 +'
 +
  test_expect_success 'cat-file --textconv --batch works' '
  	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
  	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
-- 
2.10.0.rc1.99.gcd66998

base-commit: 2632c897f74b1cc9b5533f467da459b9ec725538
Johannes Schindelin· Aug 24, 2016, 12:23 UTC · re: Johannes Schindelin · lore

[PATCH v2 4/4] cat-file: support --textconv/--filters in batch mode

With this patch, --batch can be combined with --textconv or --filters. For this to work, the input needs to have the form

	<object name><single white space><path>
so that the filters can be chosen appropriately.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt | 18 +++++++++++-----
 builtin/cat-file.c             | 49 +++++++++++++++++++++++++++++++++++++-----
 t/t8010-cat-file-filters.sh    | 10 +++++++++
 3 files changed, 67 insertions(+), 10 deletions(-)
Show changes to 3 files +67 −10

Documentation/git-cat-file.txt, builtin/cat-file.c, t/t8010-cat-file-filters.sh

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 4fa9041..204541c 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
-'git cat-file' (--batch | --batch-check) [--follow-symlinks]
+'git cat-file' (--batch | --batch-check) [ --textconv | --filters ] [--follow-symlinks]
 
 DESCRIPTION
 -----------
@@ -20,7 +20,11 @@ object type, or `-s` is used to find the object size, or `--textconv` or
 `--filters` is used (which imply type "blob").
 
 In the second form, a list of objects (separated by linefeeds) is provided on
-stdin, and the SHA-1, type, and size of each object is printed on stdout.
+stdin, and the SHA-1, type, and size of each object is printed on stdout. The
+output format can be overridden using the optional `<format>` argument. If
+either `--textconv` or `--filters` was specified, the input is expected to
+list the object names followed by the path name, separated by a single white
+space, so that the appropriate drivers can be determined.
 
 OPTIONS
 -------
@@ -72,13 +76,17 @@ OPTIONS
 --batch::
 --batch=<format>::
 	Print object information and contents for each object provided
-	on stdin.  May not be combined with any other options or arguments.
-	See the section `BATCH OUTPUT` below for details.
+	on stdin.  May not be combined with any other options or arguments
+	except `--textconv` or `--filters`, in which case the input lines
+	also need to specify the path, separated by white space.  See the
+	section `BATCH OUTPUT` below for details.
 
 --batch-check::
 --batch-check=<format>::
 	Print object information for each object provided on stdin.  May
-	not be combined with any other options or arguments.  See the
+	not be combined with any other options or arguments except
+	`--textconv` or `--filters`, in which case the input lines also
+	need to specify the path, separated by white space.  See the
 	section `BATCH OUTPUT` below for details.
 
 --batch-all-objects::
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2c799ac..f8a3a08 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -17,6 +17,7 @@ struct batch_options {
 	int print_contents;
 	int buffer_output;
 	int all_objects;
+	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
 	const char *format;
 };
 
@@ -285,7 +286,32 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 	if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
-		if (stream_blob_to_fd(1, sha1, NULL, 0) < 0)
+		if (opt->cmdmode) {
+			char *contents;
+			unsigned long size;
+
+			if (!data->rest)
+				die("missing path for '%s'", sha1_to_hex(sha1));
+
+			if (opt->cmdmode == 'w') {
+				if (filter_object(data->rest, 0100644, sha1,
+						  &contents, &size))
+					die("could not convert '%s' %s",
+					    sha1_to_hex(sha1), data->rest);
+			} else if (opt->cmdmode == 'c') {
+				enum object_type type;
+				if (!textconv_object(data->rest, 0100644, sha1,
+						     1, &contents, &size))
+					contents = read_sha1_file(sha1, &type,
+								  &size);
+				if (!contents)
+					die("could not convert '%s' %s",
+					    sha1_to_hex(sha1), data->rest);
+			} else
+				die("BUG: invalid cmdmode: %c", opt->cmdmode);
+			batch_write(opt, contents, size);
+			free(contents);
+		} else if (stream_blob_to_fd(1, sha1, NULL, 0) < 0)
 			die("unable to stream %s to stdout", sha1_to_hex(sha1));
 	}
 	else {
@@ -422,6 +448,8 @@ static int batch_objects(struct batch_options *opt)
 	data.mark_query = 1;
 	strbuf_expand(&buf, opt->format, expand_format, &data);
 	data.mark_query = 0;
+	if (opt->cmdmode)
+		data.split_on_whitespace = 1;
 
 	if (opt->all_objects) {
 		struct object_info empty;
@@ -487,7 +515,7 @@ static int batch_objects(struct batch_options *opt)
 
 static const char * const cat_file_usage[] = {
 	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) [--path=<path>] <object>"),
-	N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
+	N_("git cat-file (--batch | --batch-check) [--follow-symlinks] [--textconv|--filters]"),
 	NULL
 };
 
@@ -558,7 +586,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
 
 	if (opt) {
-		if (argc == 1)
+		if (batch.enabled && (opt == 'c' || opt == 'w'))
+			batch.cmdmode = opt;
+		else if (argc == 1)
 			obj_name = argv[0];
 		else
 			usage_with_options(cat_file_usage, options);
@@ -570,8 +600,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		} else
 			usage_with_options(cat_file_usage, options);
 	}
-	if (batch.enabled && (opt || argc)) {
-		usage_with_options(cat_file_usage, options);
+	if (batch.enabled) {
+		if (batch.cmdmode != opt || argc)
+			usage_with_options(cat_file_usage, options);
+		if (batch.cmdmode && batch.all_objects)
+			die("--batch-all-objects cannot be combined with "
+			    "--textconv nor with --filters");
 	}
 
 	if ((batch.follow_symlinks || batch.all_objects) && !batch.enabled) {
@@ -583,6 +617,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		usage_with_options(cat_file_usage, options);
 	}
 
+	if (force_path && batch.enabled) {
+		error("--path=<path> incompatible with --batch");
+		usage_with_options(cat_file_usage, options);
+	}
+
 	if (batch.buffer_output < 0)
 		batch.buffer_output = batch.all_objects;
 
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index 0d5c33e..acdfa09 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -51,4 +51,14 @@ test_expect_success '----path=<path> complains without --textconv/--filters' '
 	grep "path.*needs.*filters" err
 '
 
+test_expect_success 'cat-file --textconv --batch works' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
+	printf "%s hello.txt\n%s hello\n" $sha1 $sha1 |
+	git cat-file --textconv --batch >actual &&
+	printf "%s blob 6\nuryyb\r\n\n%s blob 6\nhello\n\n" \
+		$sha1 $sha1 >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.10.0.rc1.99.gcd66998
Johannes Schindelin· Aug 24, 2016, 12:23 UTC · re: Johannes Schindelin · lore

[PATCH v2 2/4] cat-file: introduce the --filters option

The --filters option applies the convert_to_working_tree() filter for the path when showing the contents of a regular file blob object.

This feature comes in handy when a 3rd-party tool wants to work with the contents of files from past revisions as if they had been checked out, but without detouring via temporary files.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt | 12 +++++++++---
 builtin/cat-file.c             | 41 ++++++++++++++++++++++++++++++++++++++++-
 t/t8010-cat-file-filters.sh    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 4 deletions(-)
 create mode 100755 t/t8010-cat-file-filters.sh
Show changes to 3 files +83 −4

Documentation/git-cat-file.txt, builtin/cat-file.c, t/t8010-cat-file-filters.sh

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 071029b..537d02c 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,15 +9,15 @@ git-cat-file - Provide content or type and size information for repository objec
 SYNOPSIS
 --------
 [verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv ) <object>
+'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) <object>
 'git cat-file' (--batch | --batch-check) [--follow-symlinks]
 
 DESCRIPTION
 -----------
 In its first form, the command provides the content or the type of an object in
 the repository. The type is required unless `-t` or `-p` is used to find the
-object type, or `-s` is used to find the object size, or `--textconv` is used
-(which implies type "blob").
+object type, or `-s` is used to find the object size, or `--textconv` or
+`--filters` is used (which imply type "blob").
 
 In the second form, a list of objects (separated by linefeeds) is provided on
 stdin, and the SHA-1, type, and size of each object is printed on stdout.
@@ -58,6 +58,12 @@ OPTIONS
 	order to apply the filter to the content recorded in the index at
 	<path>.
 
+--filters::
+	Show the content as converted by the filters configured in
+	the current working tree for the given <path> (i.e. smudge filters,
+	end-of-line conversion, etc). In this case, <object> has to be of
+	the form <tree-ish>:<path>, or :<path>.
+
 --batch::
 --batch=<format>::
 	Print object information and contents for each object provided
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2dfe626..0b74afa 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,6 +20,33 @@ struct batch_options {
 	const char *format;
 };
 
+static int filter_object(const char *path, unsigned mode,
+			 const unsigned char *sha1,
+			 char **buf, unsigned long *size)
+{
+	enum object_type type;
+
+	*buf = read_sha1_file(sha1, &type, size);
+	if (!*buf)
+		return error(_("cannot read object %s '%s'"),
+			sha1_to_hex(sha1), path);
+	if (type != OBJ_BLOB) {
+		free(*buf);
+		return error(_("blob expected for %s '%s'"),
+			sha1_to_hex(sha1), path);
+	}
+	if (S_ISREG(mode)) {
+		struct strbuf strbuf = STRBUF_INIT;
+		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
+			free(*buf);
+			*size = strbuf.len;
+			*buf = strbuf_detach(&strbuf, NULL);
+		}
+	}
+
+	return 0;
+}
+
 static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			int unknown_type)
 {
@@ -61,6 +88,16 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	case 'e':
 		return !has_sha1_file(sha1);
 
+	case 'w':
+		if (!obj_context.path[0])
+			die("git cat-file --filters %s: <object> must be "
+			    "<sha1:path>", obj_name);
+
+		if (filter_object(obj_context.path, obj_context.mode,
+				  sha1, &buf, &size))
+			return -1;
+		break;
+
 	case 'c':
 		if (!obj_context.path[0])
 			die("git cat-file --textconv %s: <object> must be <sha1:path>",
@@ -440,7 +477,7 @@ static int batch_objects(struct batch_options *opt)
 }
 
 static const char * const cat_file_usage[] = {
-	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv) <object>"),
+	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) <object>"),
 	N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
 	NULL
 };
@@ -486,6 +523,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
 		OPT_CMDMODE(0, "textconv", &opt,
 			    N_("for blob objects, run textconv on object's content"), 'c'),
+		OPT_CMDMODE(0, "filters", &opt,
+			    N_("for blob objects, run filters on object's content"), 'w'),
 		OPT_BOOL(0, "allow-unknown-type", &unknown_type,
 			  N_("allow -s and -t to work with broken/corrupt objects")),
 		OPT_BOOL(0, "buffer", &batch.buffer_output, N_("buffer --batch output")),
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
new file mode 100755
index 0000000..e466634
--- /dev/null
+++ b/t/t8010-cat-file-filters.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='git cat-file filters support'
+. ./test-lib.sh
+
+test_expect_success 'setup ' '
+	echo "*.txt eol=crlf diff=txt" >.gitattributes &&
+	echo "hello" | append_cr >world.txt &&
+	git add .gitattributes world.txt &&
+	test_tick &&
+	git commit -m "Initial commit"
+'
+
+has_cr () {
+	tr '\015' Q <"$1" | grep Q >/dev/null
+}
+
+test_expect_success 'no filters with `git show`' '
+	git show HEAD:world.txt >actual &&
+	! has_cr actual
+
+'
+
+test_expect_success 'no filters with cat-file' '
+	git cat-file blob HEAD:world.txt >actual &&
+	! has_cr actual
+'
+
+test_expect_success 'cat-file --filters converts to worktree version' '
+	git cat-file --filters HEAD:world.txt >actual &&
+	has_cr actual
+'
+
+test_done
-- 
2.10.0.rc1.99.gcd66998
Junio C Hamano· Aug 24, 2016, 17:46 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 2/4] cat-file: introduce the --filters option

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 23 quoted lines
> +static int filter_object(const char *path, unsigned mode,
> +			 const unsigned char *sha1,
> +			 char **buf, unsigned long *size)
> +{
> +	enum object_type type;
> +
> +	*buf = read_sha1_file(sha1, &type, size);
> +	if (!*buf)
> +		return error(_("cannot read object %s '%s'"),
> +			sha1_to_hex(sha1), path);
> +	if (type != OBJ_BLOB) {
> +		free(*buf);
> +		return error(_("blob expected for %s '%s'"),
> +			sha1_to_hex(sha1), path);
> +	}
> +	if (S_ISREG(mode)) {
> +		struct strbuf strbuf = STRBUF_INIT;
> +		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
> +			free(*buf);
> +			*size = strbuf.len;
> +			*buf = strbuf_detach(&strbuf, NULL);
> +		}
> +	}

When we see a blob that is not ISREG, what is expected to happen? Is it an error?

We can argue both ways.

Currently the ONLY kind of blob that is not ISREG is a symbolic link, and it might be OK to output it as-is without any conversion [*1*], in which case we can simply lose the S_ISREG(mode) check altogether (and "mode" parameter to this function).

On the other hand, because "cat-file --filters" is meant to be a smaller-granularity operation that could be used as a building block to emulate what "git checkout" does, i.e. "imagine that we had the blob in the index and checking it out from the path, and hand the caller what would have been written out to the filesystem, so that the convert_to_working_tree() logic does not have to be emulated in the userspace", erroring out when we see a symbolic link may be also a valid way to handle it. We know the usual CRLF and other conversions do not apply to them.

In any case, silently succeeding without any output is probably what the end-user least expects.

If we choose to fail the request, the necessary update to the test would look like this, I think.

Show 15 quoted lines
> diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
> new file mode 100755
> index 0000000..e466634
> --- /dev/null
> +++ b/t/t8010-cat-file-filters.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='git cat-file filters support'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup ' '
> +	echo "*.txt eol=crlf diff=txt" >.gitattributes &&
> +	echo "hello" | append_cr >world.txt &&
> +	git add .gitattributes world.txt &&
	git update-index --cacheinfo :world.txt,$EMPTY_BLOB,symlink &&
Show 23 quoted lines
> +	test_tick &&
> +	git commit -m "Initial commit"
> +'
> +
> +has_cr () {
> +	tr '\015' Q <"$1" | grep Q >/dev/null
> +}
> +
> +test_expect_success 'no filters with `git show`' '
> +	git show HEAD:world.txt >actual &&
> +	! has_cr actual
> +
> +'
> +
> +test_expect_success 'no filters with cat-file' '
> +	git cat-file blob HEAD:world.txt >actual &&
> +	! has_cr actual
> +'
> +
> +test_expect_success 'cat-file --filters converts to worktree version' '
> +	git cat-file --filters HEAD:world.txt >actual &&
> +	has_cr actual
> +'
test_expect_success 'cat-file --filters rejects a non-blob' '
	test_must_fail git cat-file --filters HEAD:
'
test_expect_success 'cat-file --filters rejects a non-regular blob' '
	test_must_fail git cat-file --filters HEAD:symlink
'

And if we choose to accept as long as it is a blob, then the last step can lose test_must_fail (and perhaps the result needs to be checked against "hello" without CR).

[Footnote]
*1* But of course other kinds of non-ISREG blob we would add later
    may not be something you would want to write out as-is.
Junio C Hamano· Aug 24, 2016, 17:52 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/4] cat-file: introduce the --filters option

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
>> +test_expect_success 'setup ' '
>> +	echo "*.txt eol=crlf diff=txt" >.gitattributes &&
>> +	echo "hello" | append_cr >world.txt &&
>> +	git add .gitattributes world.txt &&
>
> 	git update-index --cacheinfo :world.txt,$EMPTY_BLOB,symlink &&

Sorry, last-minute-edit-gone-bad-without-proofreading. It should have been something like:

	git update-index --add --cacheinfo 160000,$EMPTY_BLOB,symlink &&
or
       	sym=$(echo "hello" | git hash-objects --stdin -w) &&
	git update-index --add --cacheinfo 160000,$sym,symlink &&
Johannes Schindelin· Aug 31, 2016, 20:31 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/4] cat-file: introduce the --filters option

Hi Junio,
On Wed, 24 Aug 2016, Junio C Hamano wrote:
Show 28 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +static int filter_object(const char *path, unsigned mode,
> > +			 const unsigned char *sha1,
> > +			 char **buf, unsigned long *size)
> > +{
> > +	enum object_type type;
> > +
> > +	*buf = read_sha1_file(sha1, &type, size);
> > +	if (!*buf)
> > +		return error(_("cannot read object %s '%s'"),
> > +			sha1_to_hex(sha1), path);
> > +	if (type != OBJ_BLOB) {
> > +		free(*buf);
> > +		return error(_("blob expected for %s '%s'"),
> > +			sha1_to_hex(sha1), path);
> > +	}
> > +	if (S_ISREG(mode)) {
> > +		struct strbuf strbuf = STRBUF_INIT;
> > +		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
> > +			free(*buf);
> > +			*size = strbuf.len;
> > +			*buf = strbuf_detach(&strbuf, NULL);
> > +		}
> > +	}
> 
> When we see a blob that is not ISREG, what is expected to happen?
> Is it an error?

This is not a user-facing command, therefore we have to trust the caller that they know what they are doing.

And it is quite conceivable that a caller wants to simply apply filters whenever a blob is specified, and simply not apply any filters when something else was specified.

That would be in line with specifying the --filters options on a path for which no filters are configured: the --filters option is basically a no-op, then.

> In any case, silently succeeding without any output is probably what
> the end-user least expects.

Except that 1) the command is not for an end-user, and 2) when calling `git cat-file --filters HEAD:directory/` the command does not silently succeed:

	error: blob expected for <sha-1> 'directory/'
> If we choose to fail the request, the necessary update to the test
> would look like this, I think.

Quite frankly, as cat-file is not an end-user-facing command, I think it is serious overkill to add more testing here.

Ciao, Dscho

Junio C Hamano· Aug 31, 2016, 20:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 2/4] cat-file: introduce the --filters option

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 14 quoted lines
>> > +	if (S_ISREG(mode)) {
>> > +		struct strbuf strbuf = STRBUF_INIT;
>> > +		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
>> > +			free(*buf);
>> > +			*size = strbuf.len;
>> > +			*buf = strbuf_detach(&strbuf, NULL);
>> > +		}
>> > +	}
>> 
>> When we see a blob that is not ISREG, what is expected to happen?
>> Is it an error?
>
> This is not a user-facing command, therefore we have to trust the caller
> that they know what they are doing.

The caller that knows what s/he is doing would rely on a documented behaviour out of the command. That behaviour hopefully is an intuitive and useful one for script writers.

You say
> Quite frankly, as cat-file is not an end-user-facing command, I think it
> is serious overkill to add more testing here.

and I think you would need a serious attitude adjustment here. Scriptors are also an important class of end-users and cat-file directly faces them.

Thinking about this one a bit more, as 'cat-file' especially with the "--filters" option is the lowest-level way for scriptors to externalize the data stored in Git object database to the representation used in the outside world (in other words, it would be a good ingredient if they want to implement what "checkout" does), I would expect that an intuitive behaviour for

	git cat-file --filters HEAD:Makefile >Makefile
	git cat-file --filters HEAD:RelNotes >Relnotes
	git cat-file --filters HEAD:t >t

to send the requested contents to the standard output stream, but error out when the result of the command shown above would not mimick what "checkout" would leave in the filesystem. IOW, the first one should succeed, the second and the third ones should fail the same way to signal what is requested cannot be performed to the script that is calling these commands.

Johannes Schindelin· Aug 24, 2016, 12:23 UTC · re: Johannes Schindelin · lore

[PATCH v2 1/4] cat-file: fix a grammo in the man page

"... has be ..." -> "... has to be ..."
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to Documentation/git-cat-file.txt +3 −2
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 18d03d8..071029b 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -54,8 +54,9 @@ OPTIONS
 
 --textconv::
 	Show the content as transformed by a textconv filter. In this case,
-	<object> has be of the form <tree-ish>:<path>, or :<path> in order
-	to apply the filter to the content recorded in the index at <path>.
+	<object> has to be of the form <tree-ish>:<path>, or :<path> in
+	order to apply the filter to the content recorded in the index at
+	<path>.
 
 --batch::
 --batch=<format>::
-- 
2.10.0.rc1.99.gcd66998
Johannes Schindelin· Aug 24, 2016, 12:23 UTC · re: Johannes Schindelin · lore

[PATCH v2 3/4] cat-file --textconv/--filters: allow specifying the path separately

There are circumstances when it is relatively easy to figure out the object name for a given path, but not the name of the containing tree. For example, when looking at a diff generated by Git, the object names are recorded, but not the revision. As a matter of fact, the revisions from which the diff was generated may not even exist locally.

In such a case, the user would have to generate a fake revision just to be able to use --textconv or --filters.

Let's simplify this dramatically, because we do not really need that revision at all: all we care about is that we know the path. In the scenario described above, we do know the path, and we just want to specify it separately from the object name.

Example usage:
	git cat-file --textconv --path=main.c 0f1937fd
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt |  7 ++++++-
 builtin/cat-file.c             | 26 +++++++++++++++++++++-----
 t/t8010-cat-file-filters.sh    | 20 ++++++++++++++++++++
 3 files changed, 47 insertions(+), 6 deletions(-)
Show changes to 3 files +47 −6

Documentation/git-cat-file.txt, builtin/cat-file.c, t/t8010-cat-file-filters.sh

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 537d02c..4fa9041 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,7 +9,7 @@ git-cat-file - Provide content or type and size information for repository objec
 SYNOPSIS
 --------
 [verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) <object>
+'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
 'git cat-file' (--batch | --batch-check) [--follow-symlinks]
 
 DESCRIPTION
@@ -64,6 +64,11 @@ OPTIONS
 	end-of-line conversion, etc). In this case, <object> has to be of
 	the form <tree-ish>:<path>, or :<path>.
 
+--path=<path>::
+	For use with --textconv or --filters, to allow specifying an object
+	name and a path separately, e.g. when it is difficult to figure out
+	the revision from which the blob came.
+
 --batch::
 --batch=<format>::
 	Print object information and contents for each object provided
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0b74afa..2c799ac 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,6 +20,8 @@ struct batch_options {
 	const char *format;
 };
 
+static const char *force_path;
+
 static int filter_object(const char *path, unsigned mode,
 			 const unsigned char *sha1,
 			 char **buf, unsigned long *size)
@@ -58,6 +60,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	struct object_info oi = {NULL};
 	struct strbuf sb = STRBUF_INIT;
 	unsigned flags = LOOKUP_REPLACE_OBJECT;
+	const char *path = force_path;
 
 	if (unknown_type)
 		flags |= LOOKUP_UNKNOWN_OBJECT;
@@ -65,6 +68,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
 		die("Not a valid object name %s", obj_name);
 
+	if (!path)
+		path = obj_context.path;
+	else if (obj_context.mode == S_IFINVALID)
+		obj_context.mode = 0100644;
+
 	buf = NULL;
 	switch (opt) {
 	case 't':
@@ -89,21 +97,22 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		return !has_sha1_file(sha1);
 
 	case 'w':
-		if (!obj_context.path[0])
+		if (!path[0])
 			die("git cat-file --filters %s: <object> must be "
 			    "<sha1:path>", obj_name);
 
-		if (filter_object(obj_context.path, obj_context.mode,
+		if (filter_object(path, obj_context.mode,
 				  sha1, &buf, &size))
 			return -1;
 		break;
 
 	case 'c':
-		if (!obj_context.path[0])
+		if (!path[0])
 			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))
+		if (textconv_object(path, obj_context.mode,
+				    sha1, 1, &buf, &size))
 			break;
 
 	case 'p':
@@ -477,7 +486,7 @@ static int batch_objects(struct batch_options *opt)
 }
 
 static const char * const cat_file_usage[] = {
-	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) <object>"),
+	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) [--path=<path>] <object>"),
 	N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
 	NULL
 };
@@ -525,6 +534,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 			    N_("for blob objects, run textconv on object's content"), 'c'),
 		OPT_CMDMODE(0, "filters", &opt,
 			    N_("for blob objects, run filters on object's content"), 'w'),
+		OPT_STRING(0, "path", &force_path, N_("blob"),
+			   N_("use a specific path for --textconv/--filters")),
 		OPT_BOOL(0, "allow-unknown-type", &unknown_type,
 			  N_("allow -s and -t to work with broken/corrupt objects")),
 		OPT_BOOL(0, "buffer", &batch.buffer_output, N_("buffer --batch output")),
@@ -567,6 +578,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		usage_with_options(cat_file_usage, options);
 	}
 
+	if (force_path && opt != 'c' && opt != 'w') {
+		error("--path=<path> needs --textconv or --filters");
+		usage_with_options(cat_file_usage, options);
+	}
+
 	if (batch.buffer_output < 0)
 		batch.buffer_output = batch.all_objects;
 
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index e466634..0d5c33e 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -31,4 +31,24 @@ test_expect_success 'cat-file --filters converts to worktree version' '
 	has_cr actual
 '
 
+test_expect_success 'cat-file --filters --path=<path> works' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	git cat-file --filters --path=world.txt $sha1 >actual &&
+	has_cr actual
+'
+
+test_expect_success 'cat-file --textconv --path=<path> works' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
+	git cat-file --textconv --path=hello.txt $sha1 >rot13 &&
+	test uryyb = "$(cat rot13 | remove_cr)"
+'
+
+test_expect_success '----path=<path> complains without --textconv/--filters' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	test_must_fail git cat-file --path=hello.txt blob $sha1 >actual 2>err &&
+	test ! -s actual &&
+	grep "path.*needs.*filters" err
+'
+
 test_done
-- 
2.10.0.rc1.99.gcd66998
Junio C Hamano· Aug 24, 2016, 17:49 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 3/4] cat-file --textconv/--filters: allow specifying the path separately

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 12 quoted lines
> @@ -65,6 +68,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>  	if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
>  		die("Not a valid object name %s", obj_name);
>  
> +	if (!path)
> +		path = obj_context.path;
> +	else if (obj_context.mode == S_IFINVALID)
> +		obj_context.mode = 0100644;
> +
>  	buf = NULL;
>  	switch (opt) {
>  	case 't':
Mentioned elsewhere, but I think the above should be
	if (!path)
        	path = obj_context.path;
	if (obj_context.mode == S_IFINVALID)
        	obj_context.mode = 0100644;

IOW, even when there is an explicit path supplied, we should fall back to assumed "regular blob" mode, so that

	git cat-file --filters --path=README $(git rev-parse :README)
would work as expected.
Junio C Hamano· Aug 24, 2016, 18:25 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/4] cat-file --textconv/--filters: allow specifying the path separately

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Mentioned elsewhere, but I think the above should be
>
> 	if (!path)
>         	path = obj_context.path;
>
> 	if (obj_context.mode == S_IFINVALID)
>         	obj_context.mode = 0100644;
>
> IOW, even when there is an explicit path supplied, we should fall
> back to assumed "regular blob" mode, so that
>
> 	git cat-file --filters --path=README $(git rev-parse :README)
>
> would work as expected.

Actually, I am reading the conditional the other way, but the conclusion "defaulting from unknown mode to regular blob is necessary whether the user gave us a path or not" is the same.

The current code may fail if --path is not available and 40-hex that does not give us any context of look up is given because it won't be able to decide how to filter, so using "else if" would not have practical difference there, but conceptually it still is wrong.

Johannes Schindelin· Aug 31, 2016, 19:49 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/4] cat-file --textconv/--filters: allow specifying the path separately

Hi Junio,
On Wed, 24 Aug 2016, Junio C Hamano wrote:
Show 25 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Mentioned elsewhere, but I think the above should be
> >
> > 	if (!path)
> >         	path = obj_context.path;
> >
> > 	if (obj_context.mode == S_IFINVALID)
> >         	obj_context.mode = 0100644;
> >
> > IOW, even when there is an explicit path supplied, we should fall
> > back to assumed "regular blob" mode, so that
> >
> > 	git cat-file --filters --path=README $(git rev-parse :README)
> >
> > would work as expected.
> 
> Actually, I am reading the conditional the other way, but the
> conclusion "defaulting from unknown mode to regular blob is
> necessary whether the user gave us a path or not" is the same.
> 
> The current code may fail if --path is not available and 40-hex that
> does not give us any context of look up is given because it won't be
> able to decide how to filter, so using "else if" would not have
> practical difference there, but conceptually it still is wrong.
Let's translate the logic
	if (!path)
		path = obj_context.path;
	else if (obj_context.mode == S_IFINVALID)
		obj_context.mode = 0100644;
into plain English.
Basically, we have two cases:
1) the user provided us with a --path option, in which case we only
   override the mode if get_sha1_with_context() could not determine the
   mode.
2) the user did *not* provide us with a --path, in which case we keep the
   mode exactly as determined by get_sha1_with_context().

Now, the change you propose would change 2) such that we would *still* override an undecided mode with 100644, even if the user did not bother to specify a --path option.

It is true that it is currently impossible to infer a path from a blob ID without being able to infer also a mode. So the question is whether it makes sense to allow cat-file to proceed on the assumption that it is a regular file if it does not know?

I would say: no, it does not make sense.

However, I do not want to hold this patch series up just by being stubborn.

Will change it, Dscho

Junio C Hamano· Aug 31, 2016, 20:17 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 3/4] cat-file --textconv/--filters: allow specifying the path separately

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> However, I do not want to hold this patch series up just by being
> stubborn.

Peff and I disussed this further in the thread in which the message <xmqqa8g2qcf3.fsf@gitster.mtv.corp.google.com> appears, and agreed that it does not matter that much either way.

The comment to [v2 2/4] would be more important than this one, I would think. What should happen when a blob that is not ISREG is given to filter_object()? Giving a symlink contents as-is without filtering would be OK. Erroring out saying "regular file blob expected" just like the function reacts to non-blob is also OK. Just being silent and not doing anything is probably not.

Thanks.
Junio C Hamano· Aug 24, 2016, 16:09 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 24 quoted lines
>  diff --git a/builtin/cat-file.c b/builtin/cat-file.c
>  index 5f91cf4..f8a3a08 100644
>  --- a/builtin/cat-file.c
>  +++ b/builtin/cat-file.c
>  @@ -61,6 +61,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>   	struct object_info oi = {NULL};
>   	struct strbuf sb = STRBUF_INIT;
>   	unsigned flags = LOOKUP_REPLACE_OBJECT;
>  +	const char *path = force_path;
>   
>   	if (unknown_type)
>   		flags |= LOOKUP_UNKNOWN_OBJECT;
>  @@ -68,6 +69,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>   	if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
>   		die("Not a valid object name %s", obj_name);
>   
>  +	if (!path)
>  +		path = obj_context.path;
>  +	else if (obj_context.mode == S_IFINVALID)
>  +		obj_context.mode = 0100644;
>  +
>   	buf = NULL;
>   	switch (opt) {
>   	case 't':

The above two hunks make all the difference in the ease of reading the remainder of the function. Very good.

Show 6 quoted lines
>  +test_expect_success '----path=<path> complains without --textconv/--filters' '
>  +	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
>  +	test_must_fail git cat-file --path=hello.txt blob $sha1 >actual 2>err &&
>  +	test ! -s actual &&
>  +	grep "path.*needs.*filters" err
>  +'

This will need to become test_i18ngrep once the error message is made translatable, but it is correct for now. I personally think there is no need to check "actual" or "err", though---just running cat-file under test_must_fail should be sufficient.

Thanks, will queue.
Jeff King· Aug 24, 2016, 16:19 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version

On Wed, Aug 24, 2016 at 09:09:06AM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> >  +	if (!path)
> >  +		path = obj_context.path;
> >  +	else if (obj_context.mode == S_IFINVALID)
> >  +		obj_context.mode = 0100644;
> >  +
> >   	buf = NULL;
> >   	switch (opt) {
> >   	case 't':
> 
> The above two hunks make all the difference in the ease of reading
> the remainder of the function.  Very good.

Yeah, I agree. Though it took me a moment to figure out why we were setting obj_context.mode but not obj_context.path; the reason is that "mode" is convenient to use as local storage, but "path" is not, because it is not a pointer but an array.

So it would have been a little clearer to me as:
  const char *path;
  unsigned mode;
  ...
  if (!force_path) {
	/* use file info from sha1 lookup */
	path = obj_context.path;
	mode = obj_context.mode;
  } else {
	/* use path requested by user, and assume it is a regular file */
	path = force_path;
	mode = 0100644;
  }
but I don't know if that is even worth a re-roll.
Show 13 quoted lines
> >  +test_expect_success '----path=<path> complains without --textconv/--filters' '
> >  +	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> >  +	test_must_fail git cat-file --path=hello.txt blob $sha1 >actual 2>err &&
> >  +	test ! -s actual &&
> >  +	grep "path.*needs.*filters" err
> >  +'
> 
> This will need to become test_i18ngrep once the error message is
> made translatable, but it is correct for now.  I personally think
> there is no need to check "actual" or "err", though---just running
> cat-file under test_must_fail should be sufficient.
> 
> Thanks, will queue.

The whole thing looks good to me, except for the weird doubled "--" in the test description you quoted above. :)

-Peff
Junio C Hamano· Aug 24, 2016, 17:02 UTC · re: Jeff King · lore

Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> On Wed, Aug 24, 2016 at 09:09:06AM -0700, Junio C Hamano wrote:
>
>> >  +	if (!path)
>> >  +		path = obj_context.path;
>> >  +	else if (obj_context.mode == S_IFINVALID)
>> >  +		obj_context.mode = 0100644;
>> >  +
>> >   	buf = NULL;
>> >   	switch (opt) {
>> >   	case 't':
>> 
>> The above two hunks make all the difference in the ease of reading
>> the remainder of the function.  Very good.
>
> Yeah, I agree. Though it took me a moment to figure out why we were
> setting obj_context.mode but not obj_context.path; the reason is that
> "mode" is convenient to use as local storage, but "path" is not, because
> it is not a pointer but an array.

Wait a minute. Why is it a cascaded if/elseif, not two independent if statements that gives a default value? In other words, wouldn't these two independent and orthogonal decisions?

 * When forced to use some path, we ignore obj_context.path
 * Whether we are forced to use a path or not, if we do not know the
   mode from the lookup context, we want to use the regular blob
   mode.
So that part of the patch is wrong after all, I would have to say.
	if (!path)
        	path = obj_context.path;
	if (obj_context.mode == S_IFINVALID)
        	obj_context.mode = 0100644;
or something like that, perhaps.
Show 14 quoted lines
> So it would have been a little clearer to me as:
>
>   const char *path;
>   unsigned mode;
>   ...
>   if (!force_path) {
> 	/* use file info from sha1 lookup */
> 	path = obj_context.path;
> 	mode = obj_context.mode;
>   } else {
> 	/* use path requested by user, and assume it is a regular file */
> 	path = force_path;
> 	mode = 0100644;
>   }

Hmph, if you read it that way, then if/elseif makes some sense, but we need to assume that the obj_context.mode can be garbage and have a fallback for it.

Just like
	git cat-file --filters --path=git.c HEAD:t
would error out because HEAD:t is not even a blob, I would expect
	git cat-file --filters --path=git.c :RelNotes

to error out, because the object itself _is_ known to be a blob that is not a regular file.

And that kind of type checking will not be possible with "if the user gave us a path, assume it is a regular file".

Jeff King· Aug 24, 2016, 17:32 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version

On Wed, Aug 24, 2016 at 10:02:39AM -0700, Junio C Hamano wrote:
Show 39 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Aug 24, 2016 at 09:09:06AM -0700, Junio C Hamano wrote:
> >
> >> >  +	if (!path)
> >> >  +		path = obj_context.path;
> >> >  +	else if (obj_context.mode == S_IFINVALID)
> >> >  +		obj_context.mode = 0100644;
> >> >  +
> >> >   	buf = NULL;
> >> >   	switch (opt) {
> >> >   	case 't':
> >> 
> >> The above two hunks make all the difference in the ease of reading
> >> the remainder of the function.  Very good.
> >
> > Yeah, I agree. Though it took me a moment to figure out why we were
> > setting obj_context.mode but not obj_context.path; the reason is that
> > "mode" is convenient to use as local storage, but "path" is not, because
> > it is not a pointer but an array.
> 
> Wait a minute.  Why is it a cascaded if/elseif, not two independent
> if statements that gives a default value?  In other words, wouldn't
> these two independent and orthogonal decisions?
> 
>  * When forced to use some path, we ignore obj_context.path
> 
>  * Whether we are forced to use a path or not, if we do not know the
>    mode from the lookup context, we want to use the regular blob
>    mode.
> 
> So that part of the patch is wrong after all, I would have to say.
> 
> 	if (!path)
>         	path = obj_context.path;
> 	if (obj_context.mode == S_IFINVALID)
>         	obj_context.mode = 0100644;
> 
> or something like that, perhaps.

Oh, hrm, you are right. I assumed we wanted to force the mode when --path was in effect, but that is not what the original does. If you say:

  --path=foo HEAD:bar

then we will take the mode for "bar", whatever it is (maybe a tree or symlink). But if you say:

  --path=foo $(git rev-parse HEAD:bar)
then we will use 100644, regardless of what "bar" is in HEAD.

I have not thought about it enough to know if that is a good thing or a bad thing. But I'll bet Dscho has, so I will wait for him to comment. :)

Show 27 quoted lines
> >   if (!force_path) {
> > 	/* use file info from sha1 lookup */
> > 	path = obj_context.path;
> > 	mode = obj_context.mode;
> >   } else {
> > 	/* use path requested by user, and assume it is a regular file */
> > 	path = force_path;
> > 	mode = 0100644;
> >   }
> 
> Hmph, if you read it that way, then if/elseif makes some sense, but
> we need to assume that the obj_context.mode can be garbage and have
> a fallback for it.
> 
> Just like
> 
> 	git cat-file --filters --path=git.c HEAD:t
> 
> would error out because HEAD:t is not even a blob, I would expect
> 
> 	git cat-file --filters --path=git.c :RelNotes
> 
> to error out, because the object itself _is_ known to be a
> blob that is not a regular file.
> 
> And that kind of type checking will not be possible with "if the
> user gave us a path, assume it is a regular file".

Right, I agree that is the outcome, but I just wasn't sure that the second case _should_ error out. IOW, does "--filters --path" mean "treat this as a regular file at path X", or is the "regular file" part not implied?

I don't suppose anybody cares that much either way, but it feels weird to behave differently depending on how we looked up the blob (whereas for the HEAD:t case, a tree is always a tree).

-Peff
Junio C Hamano· Aug 24, 2016, 17:55 UTC · re: Jeff King · lore

Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version

Jeff King <peff@peff.net> writes:
> I don't suppose anybody cares that much either way, but it feels weird
> to behave differently depending on how we looked up the blob (whereas
> for the HEAD:t case, a tree is always a tree).

I do not care strongly either way, but HEAD:RelNotes case we _know_ it is not a regular blob, and discarding that info for the sake of being consistent with the common denominator case, instead of using it to point out a possible mistake, feels wrong, too.

In any case, not outputting anything and silently succeeding is definitely wrong ;-).

Junio C Hamano· Aug 29, 2016, 21:02 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> When third-party tools need to access to contents of blobs in the
> database, they might be more interested in the worktree version than in
> the "clean" version of said contents.

Just a friendly reminder before you completely shift your attention to unrelated topics. I think this topic is almost there; let's not stretch ourselves too thin by nibbling-here-nibbling-there and leaving loose ends untied.

Thanks.
Johannes Schindelin· Aug 31, 2016, 20:34 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version

Hi Junio,
On Mon, 29 Aug 2016, Junio C Hamano wrote:
Show 10 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > When third-party tools need to access to contents of blobs in the
> > database, they might be more interested in the worktree version than in
> > the "clean" version of said contents.
> 
> Just a friendly reminder before you completely shift your attention
> to unrelated topics.  I think this topic is almost there; let's not
> stretch ourselves too thin by nibbling-here-nibbling-there and leaving
> loose ends untied.

I answered the comments and changed one thing locally. Will send out v3 tomorrow.

Ciao, Dscho

Johannes Schindelin· Sep 9, 2016, 10:10 UTC · re: Johannes Schindelin · lore

[PATCH v3 0/4] cat-file: optionally convert to worktree version

When third-party tools need to access to contents of blobs in the database, they might be more interested in the worktree version than in the "clean" version of said contents.

This branch introduces the --filters option to make that happen, the --use-path option to provide the path separately if the blob name rather than the tree name is availale, and offers batch support in which case it expects the object names and the path on its input lines, separated by white space.

The new --filters option is an obvious sibling of --textconv, and shares the peculiar feature that the drivers (and end-of-line convention) are determined from the current worktree, not from the attributes stored in the revision that may have been part of the object name.

As --textconv is so similar to --filters, it was taught to understand the --use-path option and it was made compatible with batch mode, too.

I briefly considered teaching the batch mode to extract the path from object names if they are specified as <tree-ish>:<path>. The changes would be quite intrusive, though, and uglify the code substanitially. So I decided against that.

Changes vs v2:
- always override unknown mode to imply 0100644, even if the user did
  not specify the --path option.
Johannes Schindelin (4):
  cat-file: fix a grammo in the man page
  cat-file: introduce the --filters option
  cat-file --textconv/--filters: allow specifying the path separately
  cat-file: support --textconv/--filters in batch mode
 Documentation/git-cat-file.txt |  40 +++++++++++----
 builtin/cat-file.c             | 110 ++++++++++++++++++++++++++++++++++++++---
 t/t8010-cat-file-filters.sh    |  64 ++++++++++++++++++++++++
 3 files changed, 196 insertions(+), 18 deletions(-)
 create mode 100755 t/t8010-cat-file-filters.sh
Published-As: https://github.com/dscho/git/releases/tag/cat-file-filters-v3
Fetch-It-Via: git fetch https://github.com/dscho/git cat-file-filters-v3
Interdiff vs v2:
 diff --git a/builtin/cat-file.c b/builtin/cat-file.c
 index f8a3a08..4461153 100644
 --- a/builtin/cat-file.c
 +++ b/builtin/cat-file.c
 @@ -71,7 +71,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
  
  	if (!path)
  		path = obj_context.path;
 -	else if (obj_context.mode == S_IFINVALID)
 +	if (obj_context.mode == S_IFINVALID)
  		obj_context.mode = 0100644;
  
  	buf = NULL;
-- 
2.10.0.windows.1.10.g803177d

base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
Johannes Schindelin· Sep 9, 2016, 10:10 UTC · re: Johannes Schindelin · lore

[PATCH v3 1/4] cat-file: fix a grammo in the man page

"... has be ..." -> "... has to be ..."
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to Documentation/git-cat-file.txt +3 −2
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 18d03d8..071029b 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -54,8 +54,9 @@ OPTIONS
 
 --textconv::
 	Show the content as transformed by a textconv filter. In this case,
-	<object> has be of the form <tree-ish>:<path>, or :<path> in order
-	to apply the filter to the content recorded in the index at <path>.
+	<object> has to be of the form <tree-ish>:<path>, or :<path> in
+	order to apply the filter to the content recorded in the index at
+	<path>.
 
 --batch::
 --batch=<format>::
-- 
2.10.0.windows.1.10.g803177d
Johannes Schindelin· Sep 9, 2016, 10:10 UTC · re: Johannes Schindelin · lore

[PATCH v3 3/4] cat-file --textconv/--filters: allow specifying the path separately

There are circumstances when it is relatively easy to figure out the object name for a given path, but not the name of the containing tree. For example, when looking at a diff generated by Git, the object names are recorded, but not the revision. As a matter of fact, the revisions from which the diff was generated may not even exist locally.

In such a case, the user would have to generate a fake revision just to be able to use --textconv or --filters.

Let's simplify this dramatically, because we do not really need that revision at all: all we care about is that we know the path. In the scenario described above, we do know the path, and we just want to specify it separately from the object name.

Example usage:
	git cat-file --textconv --path=main.c 0f1937fd
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt |  7 ++++++-
 builtin/cat-file.c             | 26 +++++++++++++++++++++-----
 t/t8010-cat-file-filters.sh    | 20 ++++++++++++++++++++
 3 files changed, 47 insertions(+), 6 deletions(-)
Show changes to 3 files +47 −6

Documentation/git-cat-file.txt, builtin/cat-file.c, t/t8010-cat-file-filters.sh

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 537d02c..4fa9041 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,7 +9,7 @@ git-cat-file - Provide content or type and size information for repository objec
 SYNOPSIS
 --------
 [verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) <object>
+'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
 'git cat-file' (--batch | --batch-check) [--follow-symlinks]
 
 DESCRIPTION
@@ -64,6 +64,11 @@ OPTIONS
 	end-of-line conversion, etc). In this case, <object> has to be of
 	the form <tree-ish>:<path>, or :<path>.
 
+--path=<path>::
+	For use with --textconv or --filters, to allow specifying an object
+	name and a path separately, e.g. when it is difficult to figure out
+	the revision from which the blob came.
+
 --batch::
 --batch=<format>::
 	Print object information and contents for each object provided
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0b74afa..b648056 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,6 +20,8 @@ struct batch_options {
 	const char *format;
 };
 
+static const char *force_path;
+
 static int filter_object(const char *path, unsigned mode,
 			 const unsigned char *sha1,
 			 char **buf, unsigned long *size)
@@ -58,6 +60,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	struct object_info oi = {NULL};
 	struct strbuf sb = STRBUF_INIT;
 	unsigned flags = LOOKUP_REPLACE_OBJECT;
+	const char *path = force_path;
 
 	if (unknown_type)
 		flags |= LOOKUP_UNKNOWN_OBJECT;
@@ -65,6 +68,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
 		die("Not a valid object name %s", obj_name);
 
+	if (!path)
+		path = obj_context.path;
+	if (obj_context.mode == S_IFINVALID)
+		obj_context.mode = 0100644;
+
 	buf = NULL;
 	switch (opt) {
 	case 't':
@@ -89,21 +97,22 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		return !has_sha1_file(sha1);
 
 	case 'w':
-		if (!obj_context.path[0])
+		if (!path[0])
 			die("git cat-file --filters %s: <object> must be "
 			    "<sha1:path>", obj_name);
 
-		if (filter_object(obj_context.path, obj_context.mode,
+		if (filter_object(path, obj_context.mode,
 				  sha1, &buf, &size))
 			return -1;
 		break;
 
 	case 'c':
-		if (!obj_context.path[0])
+		if (!path[0])
 			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))
+		if (textconv_object(path, obj_context.mode,
+				    sha1, 1, &buf, &size))
 			break;
 
 	case 'p':
@@ -477,7 +486,7 @@ static int batch_objects(struct batch_options *opt)
 }
 
 static const char * const cat_file_usage[] = {
-	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) <object>"),
+	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) [--path=<path>] <object>"),
 	N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
 	NULL
 };
@@ -525,6 +534,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 			    N_("for blob objects, run textconv on object's content"), 'c'),
 		OPT_CMDMODE(0, "filters", &opt,
 			    N_("for blob objects, run filters on object's content"), 'w'),
+		OPT_STRING(0, "path", &force_path, N_("blob"),
+			   N_("use a specific path for --textconv/--filters")),
 		OPT_BOOL(0, "allow-unknown-type", &unknown_type,
 			  N_("allow -s and -t to work with broken/corrupt objects")),
 		OPT_BOOL(0, "buffer", &batch.buffer_output, N_("buffer --batch output")),
@@ -567,6 +578,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		usage_with_options(cat_file_usage, options);
 	}
 
+	if (force_path && opt != 'c' && opt != 'w') {
+		error("--path=<path> needs --textconv or --filters");
+		usage_with_options(cat_file_usage, options);
+	}
+
 	if (batch.buffer_output < 0)
 		batch.buffer_output = batch.all_objects;
 
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index e466634..0d5c33e 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -31,4 +31,24 @@ test_expect_success 'cat-file --filters converts to worktree version' '
 	has_cr actual
 '
 
+test_expect_success 'cat-file --filters --path=<path> works' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	git cat-file --filters --path=world.txt $sha1 >actual &&
+	has_cr actual
+'
+
+test_expect_success 'cat-file --textconv --path=<path> works' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
+	git cat-file --textconv --path=hello.txt $sha1 >rot13 &&
+	test uryyb = "$(cat rot13 | remove_cr)"
+'
+
+test_expect_success '----path=<path> complains without --textconv/--filters' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	test_must_fail git cat-file --path=hello.txt blob $sha1 >actual 2>err &&
+	test ! -s actual &&
+	grep "path.*needs.*filters" err
+'
+
 test_done
-- 
2.10.0.windows.1.10.g803177d
Junio C Hamano· Sep 9, 2016, 18:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH v3 3/4] cat-file --textconv/--filters: allow specifying the path separately

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> +test_expect_success '----path=<path> complains without --textconv/--filters' '

I wonder where this "----path" came from; it wasn't in v2 I queued, but somehow came back mysteriously.

Will locally amend.
Show 7 quoted lines
> +	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> +	test_must_fail git cat-file --path=hello.txt blob $sha1 >actual 2>err &&
> +	test ! -s actual &&
> +	grep "path.*needs.*filters" err
> +'
> +
>  test_done
Johannes Schindelin· Sep 10, 2016, 07:59 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 3/4] cat-file --textconv/--filters: allow specifying the path separately

Hi Junio,
On Fri, 9 Sep 2016, Junio C Hamano wrote:
Show 6 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +test_expect_success '----path=<path> complains without --textconv/--filters' '
> 
> I wonder where this "----path" came from; it wasn't in v2 I queued,
> but somehow came back mysteriously.
No idea how it crept back in. Sorry.
> Will locally amend.

Thanks! Dscho

Johannes Schindelin· Sep 9, 2016, 10:10 UTC · re: Johannes Schindelin · lore

[PATCH v3 4/4] cat-file: support --textconv/--filters in batch mode

With this patch, --batch can be combined with --textconv or --filters. For this to work, the input needs to have the form

	<object name><single white space><path>
so that the filters can be chosen appropriately.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt | 18 +++++++++++-----
 builtin/cat-file.c             | 49 +++++++++++++++++++++++++++++++++++++-----
 t/t8010-cat-file-filters.sh    | 10 +++++++++
 3 files changed, 67 insertions(+), 10 deletions(-)
Show changes to 3 files +67 −10

Documentation/git-cat-file.txt, builtin/cat-file.c, t/t8010-cat-file-filters.sh

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 4fa9041..204541c 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
-'git cat-file' (--batch | --batch-check) [--follow-symlinks]
+'git cat-file' (--batch | --batch-check) [ --textconv | --filters ] [--follow-symlinks]
 
 DESCRIPTION
 -----------
@@ -20,7 +20,11 @@ object type, or `-s` is used to find the object size, or `--textconv` or
 `--filters` is used (which imply type "blob").
 
 In the second form, a list of objects (separated by linefeeds) is provided on
-stdin, and the SHA-1, type, and size of each object is printed on stdout.
+stdin, and the SHA-1, type, and size of each object is printed on stdout. The
+output format can be overridden using the optional `<format>` argument. If
+either `--textconv` or `--filters` was specified, the input is expected to
+list the object names followed by the path name, separated by a single white
+space, so that the appropriate drivers can be determined.
 
 OPTIONS
 -------
@@ -72,13 +76,17 @@ OPTIONS
 --batch::
 --batch=<format>::
 	Print object information and contents for each object provided
-	on stdin.  May not be combined with any other options or arguments.
-	See the section `BATCH OUTPUT` below for details.
+	on stdin.  May not be combined with any other options or arguments
+	except `--textconv` or `--filters`, in which case the input lines
+	also need to specify the path, separated by white space.  See the
+	section `BATCH OUTPUT` below for details.
 
 --batch-check::
 --batch-check=<format>::
 	Print object information for each object provided on stdin.  May
-	not be combined with any other options or arguments.  See the
+	not be combined with any other options or arguments except
+	`--textconv` or `--filters`, in which case the input lines also
+	need to specify the path, separated by white space.  See the
 	section `BATCH OUTPUT` below for details.
 
 --batch-all-objects::
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b648056..4461153 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -17,6 +17,7 @@ struct batch_options {
 	int print_contents;
 	int buffer_output;
 	int all_objects;
+	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
 	const char *format;
 };
 
@@ -285,7 +286,32 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 	if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
-		if (stream_blob_to_fd(1, sha1, NULL, 0) < 0)
+		if (opt->cmdmode) {
+			char *contents;
+			unsigned long size;
+
+			if (!data->rest)
+				die("missing path for '%s'", sha1_to_hex(sha1));
+
+			if (opt->cmdmode == 'w') {
+				if (filter_object(data->rest, 0100644, sha1,
+						  &contents, &size))
+					die("could not convert '%s' %s",
+					    sha1_to_hex(sha1), data->rest);
+			} else if (opt->cmdmode == 'c') {
+				enum object_type type;
+				if (!textconv_object(data->rest, 0100644, sha1,
+						     1, &contents, &size))
+					contents = read_sha1_file(sha1, &type,
+								  &size);
+				if (!contents)
+					die("could not convert '%s' %s",
+					    sha1_to_hex(sha1), data->rest);
+			} else
+				die("BUG: invalid cmdmode: %c", opt->cmdmode);
+			batch_write(opt, contents, size);
+			free(contents);
+		} else if (stream_blob_to_fd(1, sha1, NULL, 0) < 0)
 			die("unable to stream %s to stdout", sha1_to_hex(sha1));
 	}
 	else {
@@ -422,6 +448,8 @@ static int batch_objects(struct batch_options *opt)
 	data.mark_query = 1;
 	strbuf_expand(&buf, opt->format, expand_format, &data);
 	data.mark_query = 0;
+	if (opt->cmdmode)
+		data.split_on_whitespace = 1;
 
 	if (opt->all_objects) {
 		struct object_info empty;
@@ -487,7 +515,7 @@ static int batch_objects(struct batch_options *opt)
 
 static const char * const cat_file_usage[] = {
 	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) [--path=<path>] <object>"),
-	N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
+	N_("git cat-file (--batch | --batch-check) [--follow-symlinks] [--textconv|--filters]"),
 	NULL
 };
 
@@ -558,7 +586,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
 
 	if (opt) {
-		if (argc == 1)
+		if (batch.enabled && (opt == 'c' || opt == 'w'))
+			batch.cmdmode = opt;
+		else if (argc == 1)
 			obj_name = argv[0];
 		else
 			usage_with_options(cat_file_usage, options);
@@ -570,8 +600,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		} else
 			usage_with_options(cat_file_usage, options);
 	}
-	if (batch.enabled && (opt || argc)) {
-		usage_with_options(cat_file_usage, options);
+	if (batch.enabled) {
+		if (batch.cmdmode != opt || argc)
+			usage_with_options(cat_file_usage, options);
+		if (batch.cmdmode && batch.all_objects)
+			die("--batch-all-objects cannot be combined with "
+			    "--textconv nor with --filters");
 	}
 
 	if ((batch.follow_symlinks || batch.all_objects) && !batch.enabled) {
@@ -583,6 +617,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		usage_with_options(cat_file_usage, options);
 	}
 
+	if (force_path && batch.enabled) {
+		error("--path=<path> incompatible with --batch");
+		usage_with_options(cat_file_usage, options);
+	}
+
 	if (batch.buffer_output < 0)
 		batch.buffer_output = batch.all_objects;
 
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index 0d5c33e..acdfa09 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -51,4 +51,14 @@ test_expect_success '----path=<path> complains without --textconv/--filters' '
 	grep "path.*needs.*filters" err
 '
 
+test_expect_success 'cat-file --textconv --batch works' '
+	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
+	printf "%s hello.txt\n%s hello\n" $sha1 $sha1 |
+	git cat-file --textconv --batch >actual &&
+	printf "%s blob 6\nuryyb\r\n\n%s blob 6\nhello\n\n" \
+		$sha1 $sha1 >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.10.0.windows.1.10.g803177d
Johannes Schindelin· Sep 9, 2016, 10:10 UTC · re: Johannes Schindelin · lore

[PATCH v3 2/4] cat-file: introduce the --filters option

The --filters option applies the convert_to_working_tree() filter for the path when showing the contents of a regular file blob object.

This feature comes in handy when a 3rd-party tool wants to work with the contents of files from past revisions as if they had been checked out, but without detouring via temporary files.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-cat-file.txt | 12 +++++++++---
 builtin/cat-file.c             | 41 ++++++++++++++++++++++++++++++++++++++++-
 t/t8010-cat-file-filters.sh    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 4 deletions(-)
 create mode 100755 t/t8010-cat-file-filters.sh
Show changes to 3 files +83 −4

Documentation/git-cat-file.txt, builtin/cat-file.c, t/t8010-cat-file-filters.sh

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 071029b..537d02c 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,15 +9,15 @@ git-cat-file - Provide content or type and size information for repository objec
 SYNOPSIS
 --------
 [verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv ) <object>
+'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) <object>
 'git cat-file' (--batch | --batch-check) [--follow-symlinks]
 
 DESCRIPTION
 -----------
 In its first form, the command provides the content or the type of an object in
 the repository. The type is required unless `-t` or `-p` is used to find the
-object type, or `-s` is used to find the object size, or `--textconv` is used
-(which implies type "blob").
+object type, or `-s` is used to find the object size, or `--textconv` or
+`--filters` is used (which imply type "blob").
 
 In the second form, a list of objects (separated by linefeeds) is provided on
 stdin, and the SHA-1, type, and size of each object is printed on stdout.
@@ -58,6 +58,12 @@ OPTIONS
 	order to apply the filter to the content recorded in the index at
 	<path>.
 
+--filters::
+	Show the content as converted by the filters configured in
+	the current working tree for the given <path> (i.e. smudge filters,
+	end-of-line conversion, etc). In this case, <object> has to be of
+	the form <tree-ish>:<path>, or :<path>.
+
 --batch::
 --batch=<format>::
 	Print object information and contents for each object provided
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2dfe626..0b74afa 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,6 +20,33 @@ struct batch_options {
 	const char *format;
 };
 
+static int filter_object(const char *path, unsigned mode,
+			 const unsigned char *sha1,
+			 char **buf, unsigned long *size)
+{
+	enum object_type type;
+
+	*buf = read_sha1_file(sha1, &type, size);
+	if (!*buf)
+		return error(_("cannot read object %s '%s'"),
+			sha1_to_hex(sha1), path);
+	if (type != OBJ_BLOB) {
+		free(*buf);
+		return error(_("blob expected for %s '%s'"),
+			sha1_to_hex(sha1), path);
+	}
+	if (S_ISREG(mode)) {
+		struct strbuf strbuf = STRBUF_INIT;
+		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
+			free(*buf);
+			*size = strbuf.len;
+			*buf = strbuf_detach(&strbuf, NULL);
+		}
+	}
+
+	return 0;
+}
+
 static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			int unknown_type)
 {
@@ -61,6 +88,16 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	case 'e':
 		return !has_sha1_file(sha1);
 
+	case 'w':
+		if (!obj_context.path[0])
+			die("git cat-file --filters %s: <object> must be "
+			    "<sha1:path>", obj_name);
+
+		if (filter_object(obj_context.path, obj_context.mode,
+				  sha1, &buf, &size))
+			return -1;
+		break;
+
 	case 'c':
 		if (!obj_context.path[0])
 			die("git cat-file --textconv %s: <object> must be <sha1:path>",
@@ -440,7 +477,7 @@ static int batch_objects(struct batch_options *opt)
 }
 
 static const char * const cat_file_usage[] = {
-	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv) <object>"),
+	N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) <object>"),
 	N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
 	NULL
 };
@@ -486,6 +523,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
 		OPT_CMDMODE(0, "textconv", &opt,
 			    N_("for blob objects, run textconv on object's content"), 'c'),
+		OPT_CMDMODE(0, "filters", &opt,
+			    N_("for blob objects, run filters on object's content"), 'w'),
 		OPT_BOOL(0, "allow-unknown-type", &unknown_type,
 			  N_("allow -s and -t to work with broken/corrupt objects")),
 		OPT_BOOL(0, "buffer", &batch.buffer_output, N_("buffer --batch output")),
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
new file mode 100755
index 0000000..e466634
--- /dev/null
+++ b/t/t8010-cat-file-filters.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='git cat-file filters support'
+. ./test-lib.sh
+
+test_expect_success 'setup ' '
+	echo "*.txt eol=crlf diff=txt" >.gitattributes &&
+	echo "hello" | append_cr >world.txt &&
+	git add .gitattributes world.txt &&
+	test_tick &&
+	git commit -m "Initial commit"
+'
+
+has_cr () {
+	tr '\015' Q <"$1" | grep Q >/dev/null
+}
+
+test_expect_success 'no filters with `git show`' '
+	git show HEAD:world.txt >actual &&
+	! has_cr actual
+
+'
+
+test_expect_success 'no filters with cat-file' '
+	git cat-file blob HEAD:world.txt >actual &&
+	! has_cr actual
+'
+
+test_expect_success 'cat-file --filters converts to worktree version' '
+	git cat-file --filters HEAD:world.txt >actual &&
+	has_cr actual
+'
+
+test_done
-- 
2.10.0.windows.1.10.g803177d
Junio C Hamano· Sep 9, 2016, 15:09 UTC · re: Johannes Schindelin · lore

Re: [PATCH v3 2/4] cat-file: introduce the --filters option

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 23 quoted lines
> +static int filter_object(const char *path, unsigned mode,
> +			 const unsigned char *sha1,
> +			 char **buf, unsigned long *size)
> +{
> +	enum object_type type;
> +
> +	*buf = read_sha1_file(sha1, &type, size);
> +	if (!*buf)
> +		return error(_("cannot read object %s '%s'"),
> +			sha1_to_hex(sha1), path);
> +	if (type != OBJ_BLOB) {
> +		free(*buf);
> +		return error(_("blob expected for %s '%s'"),
> +			sha1_to_hex(sha1), path);
> +	}
> +	if (S_ISREG(mode)) {
> +		struct strbuf strbuf = STRBUF_INIT;
> +		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
> +			free(*buf);
> +			*size = strbuf.len;
> +			*buf = strbuf_detach(&strbuf, NULL);
> +		}
> +	}

This needs to error out when mode is not ISREG just like it errors out when type is not BLOB.

Other than that, I think these four patches are good to go.
Thanks.
Johannes Schindelin· Sep 9, 2016, 16:01 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 2/4] cat-file: introduce the --filters option

Hi Junio,
On Fri, 9 Sep 2016, Junio C Hamano wrote:
Show 28 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +static int filter_object(const char *path, unsigned mode,
> > +			 const unsigned char *sha1,
> > +			 char **buf, unsigned long *size)
> > +{
> > +	enum object_type type;
> > +
> > +	*buf = read_sha1_file(sha1, &type, size);
> > +	if (!*buf)
> > +		return error(_("cannot read object %s '%s'"),
> > +			sha1_to_hex(sha1), path);
> > +	if (type != OBJ_BLOB) {
> > +		free(*buf);
> > +		return error(_("blob expected for %s '%s'"),
> > +			sha1_to_hex(sha1), path);
> > +	}
> > +	if (S_ISREG(mode)) {
> > +		struct strbuf strbuf = STRBUF_INIT;
> > +		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
> > +			free(*buf);
> > +			*size = strbuf.len;
> > +			*buf = strbuf_detach(&strbuf, NULL);
> > +		}
> > +	}
> 
> This needs to error out when mode is not ISREG just like it errors
> out when type is not BLOB.
Are you sure that this is desirable in batch mode?

Ciao, Dscho

Junio C Hamano· Sep 9, 2016, 16:08 UTC · re: Johannes Schindelin · lore

Re: [PATCH v3 2/4] cat-file: introduce the --filters option

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 18 quoted lines
>> > +	if (type != OBJ_BLOB) {
>> > +		free(*buf);
>> > +		return error(_("blob expected for %s '%s'"),
>> > +			sha1_to_hex(sha1), path);
>> > +	}
>> > +	if (S_ISREG(mode)) {
>> > +		struct strbuf strbuf = STRBUF_INIT;
>> > +		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
>> > +			free(*buf);
>> > +			*size = strbuf.len;
>> > +			*buf = strbuf_detach(&strbuf, NULL);
>> > +		}
>> > +	}
>> 
>> This needs to error out when mode is not ISREG just like it errors
>> out when type is not BLOB.
>
> Are you sure that this is desirable in batch mode?

I do not quite see a reason why we should not diagnose a bad input that does not produce a filtered result. In batch mode or not, it diagnoses when the user feeds it a non-blob, and I think it should do so for non-regular, too. Both are "you asked me to filter, but you shouldn't have".

Junio C Hamano· Sep 9, 2016, 17:16 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 2/4] cat-file: introduce the --filters option

Junio C Hamano <gitster@pobox.com> writes:
Show 26 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> > +	if (type != OBJ_BLOB) {
>>> > +		free(*buf);
>>> > +		return error(_("blob expected for %s '%s'"),
>>> > +			sha1_to_hex(sha1), path);
>>> > +	}
>>> > +	if (S_ISREG(mode)) {
>>> > +		struct strbuf strbuf = STRBUF_INIT;
>>> > +		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
>>> > +			free(*buf);
>>> > +			*size = strbuf.len;
>>> > +			*buf = strbuf_detach(&strbuf, NULL);
>>> > +		}
>>> > +	}
>>> 
>>> This needs to error out when mode is not ISREG just like it errors
>>> out when type is not BLOB.
>>
>> Are you sure that this is desirable in batch mode?
>
> I do not quite see a reason why we should not diagnose a bad input
> that does not produce a filtered result.  In batch mode or not, it
> diagnoses when the user feeds it a non-blob, and I think it should
> do so for non-regular, too. Both are "you asked me to filter, but
> you shouldn't have".

Stepping back a bit, I can also see a use case that would be helped if this filter_object() function by default gives the contents of the requested object as-is, unless the object is a regular blob with a path for which filtering is defined. Driving such a mechanism via the batch interface will allow you to first ask about the top-level tree object (given back to you as-is), and you can iterate over its entries recursively and get the blobs to be placed in a new working directory (i.e. "git archive" piped to "tar xf" but regular files are passed thru convert_to_working_tree()). In such an application, after you learn the mode from the containing tree object and know that RelNotes is a symbolic link blob, you still would want the contents out of the pipe going to the same batch interface process that is not filtered.

So I would not mind if we define the semantics of "--filters" as such (as long as it is clearly documented, of course). AFAICS, the batch interface does not call filter_object() for non-blobs, and by returning successfully without doing anything special for a symbolic link from filter_object() automatically gives us the "by default return as-is, but give processed output only for regular file blobs" semantics to the batch mode.

But for a non-batch mode, it feels somewhat funny to be giving the as-is output without saying anything to symbolic links; we can argue that it is being consistent with what we do in the batch mode, though.

Thanks.
Junio C Hamano· Sep 9, 2016, 17:26 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 2/4] cat-file: introduce the --filters option

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> So I would not mind if we define the semantics of "--filters" as
> such (as long as it is clearly documented, of course).  AFAICS, the
> batch interface does not call filter_object() for non-blobs, and by
> returning successfully without doing anything special for a symbolic
> link from filter_object() automatically gives us the "by default
> return as-is, but give processed output only for regular file blobs"
> semantics to the batch mode.
>
> But for a non-batch mode, it feels somewhat funny to be giving the
> as-is output without saying anything to symbolic links; we can argue
> that it is being consistent with what we do in the batch mode,
> though.

In other words, instead of trying to be consistent by erroring out in non-regular blob case, I think the attached change on top would make more sense, by consistently passing the object contents as-is for all "not filtered" cases, whether it is run from the batch mode or from the command line.

 builtin/cat-file.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)
Show changes to builtin/cat-file.c +1 −6
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f8a3a08..99cb525 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -33,12 +33,7 @@ static int filter_object(const char *path, unsigned mode,
 	if (!*buf)
 		return error(_("cannot read object %s '%s'"),
 			sha1_to_hex(sha1), path);
-	if (type != OBJ_BLOB) {
-		free(*buf);
-		return error(_("blob expected for %s '%s'"),
-			sha1_to_hex(sha1), path);
-	}
-	if (S_ISREG(mode)) {
+	if ((type == OBJ_BLOB) && S_ISREG(mode)) {
 		struct strbuf strbuf = STRBUF_INIT;
 		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
 			free(*buf);
Johannes Schindelin· Sep 10, 2016, 07:57 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 2/4] cat-file: introduce the --filters option

Hi Junio,
On Fri, 9 Sep 2016, Junio C Hamano wrote:
Show 42 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > So I would not mind if we define the semantics of "--filters" as
> > such (as long as it is clearly documented, of course).  AFAICS, the
> > batch interface does not call filter_object() for non-blobs, and by
> > returning successfully without doing anything special for a symbolic
> > link from filter_object() automatically gives us the "by default
> > return as-is, but give processed output only for regular file blobs"
> > semantics to the batch mode.
> >
> > But for a non-batch mode, it feels somewhat funny to be giving the
> > as-is output without saying anything to symbolic links; we can argue
> > that it is being consistent with what we do in the batch mode,
> > though.
> 
> In other words, instead of trying to be consistent by erroring out
> in non-regular blob case, I think the attached change on top would
> make more sense, by consistently passing the object contents as-is
> for all "not filtered" cases, whether it is run from the batch mode
> or from the command line.
> 
>  builtin/cat-file.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index f8a3a08..99cb525 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -33,12 +33,7 @@ static int filter_object(const char *path, unsigned mode,
>  	if (!*buf)
>  		return error(_("cannot read object %s '%s'"),
>  			sha1_to_hex(sha1), path);
> -	if (type != OBJ_BLOB) {
> -		free(*buf);
> -		return error(_("blob expected for %s '%s'"),
> -			sha1_to_hex(sha1), path);
> -	}
> -	if (S_ISREG(mode)) {
> +	if ((type == OBJ_BLOB) && S_ISREG(mode)) {
>  		struct strbuf strbuf = STRBUF_INIT;
>  		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
>  			free(*buf);
Yes, that makes most sense to me, too.

Ciao, Dscho

Junio C Hamano· Sep 11, 2016, 21:44 UTC · re: Johannes Schindelin · lore

Re: [PATCH v3 2/4] cat-file: introduce the --filters option

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
>> In other words, instead of trying to be consistent by erroring out
>> in non-regular blob case, I think the attached change on top would
>> make more sense, by consistently passing the object contents as-is
>> for all "not filtered" cases, whether it is run from the batch mode
>> or from the command line.
>>  ...
>> +	if ((type == OBJ_BLOB) && S_ISREG(mode)) {
>>  		struct strbuf strbuf = STRBUF_INIT;
>>  		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
>>  			free(*buf);
>
> Yes, that makes most sense to me, too.
Alright; will squash it in then before merging it down to 'next'.

← back to recent threads