[PATCH RFC 0/20] cat-file: start using formatting logic from ref-filter

[PATCH RFC 0/20] cat-file: start using formatting logic from ref-filter

[Olga Telezhnaya]

Hi everyone,
It was a long way for me, I got older (by 1 year) and smarter
(hopefully), and maybe I will finish my Outreachy Internship task for
now. (I am doing it just for one year and a half, that's OK)

If serious:
In this patch we remove cat-file formatting logic and reuse ref-filter
logic there. As a positive side effect, cat-file now has many new
formatting tokens (all from ref-filter formatting), including deref
(like %(*objectsize:disk)). I have already tried to do this task one
year ago, and it was bad attempt. I feel that today's patch is much
better.

In my opinion, it still has some issues. I mentioned all of them in
TODOs in comments. All of them considered to be separate tasks for
other patches. Some of them sound easy and could be great tasks for
newbies.

I also have a question about site https://git-scm.com/docs/
I thought it is updated automatically based on Documentation folder in
the project, but it is not true. I edited docs for for-each-ref in
December, I still see my patch in master, but for-each-ref docs in
git-csm is outdated. Is it OK?

Thank you!
Olga

[PATCH RFC 01/20] cat-file: reuse struct ref_format

[Olga Telezhnaya]

Start using ref_format struct instead of simple char*.
Need that for further reusing of formatting logic from ref-filter.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0f092382e175c..e5de596611800 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -15,8 +15,10 @@
 #include "sha1-array.h"
 #include "packfile.h"
 #include "object-store.h"
+#include "ref-filter.h"
 
 struct batch_options {
+	struct ref_format format;
 	int enabled;
 	int follow_symlinks;
 	int print_contents;
@@ -24,7 +26,6 @@ struct batch_options {
 	int all_objects;
 	int unordered;
 	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
-	const char *format;
 };
 
 static const char *force_path;
@@ -365,7 +366,7 @@ static void batch_object_write(const char *obj_name,
 	}
 
 	strbuf_reset(scratch);
-	strbuf_expand(scratch, opt->format, expand_format, data);
+	strbuf_expand(scratch, opt->format.format, expand_format, data);
 	strbuf_addch(scratch, '\n');
 	batch_write(opt, scratch->buf, scratch->len);
 
@@ -491,9 +492,6 @@ static int batch_objects(struct batch_options *opt)
 	int save_warning;
 	int retval = 0;
 
-	if (!opt->format)
-		opt->format = "%(objectname) %(objecttype) %(objectsize)";
-
 	/*
 	 * Expand once with our special mark_query flag, which will prime the
 	 * object_info to be handed to oid_object_info_extended for each
@@ -501,7 +499,7 @@ static int batch_objects(struct batch_options *opt)
 	 */
 	memset(&data, 0, sizeof(data));
 	data.mark_query = 1;
-	strbuf_expand(&output, opt->format, expand_format, &data);
+	strbuf_expand(&output, opt->format.format, expand_format, &data);
 	data.mark_query = 0;
 	strbuf_release(&output);
 	if (opt->cmdmode)
@@ -617,7 +615,7 @@ static int batch_option_callback(const struct option *opt,
 
 	bo->enabled = 1;
 	bo->print_contents = !strcmp(opt->long_name, "batch");
-	bo->format = arg;
+	bo->format.format = arg;
 
 	return 0;
 }
@@ -626,7 +624,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 {
 	int opt = 0;
 	const char *exp_type = NULL, *obj_name = NULL;
-	struct batch_options batch = {0};
+	struct batch_options batch = { REF_FORMAT_INIT };
 	int unknown_type = 0;
 
 	const struct option options[] = {
@@ -707,6 +705,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	if (batch.buffer_output < 0)
 		batch.buffer_output = batch.all_objects;
 
+	if (!batch.format.format)
+		batch.format.format = "%(objectname) %(objecttype) %(objectsize)";
+
 	if (batch.enabled)
 		return batch_objects(&batch);
 

--
https://github.com/git/git/pull/568

[PATCH RFC 20/20] cat-file: update docs

[Olga Telezhnaya]

Update the docs for cat-file command. Some new formatting atoms added
because of reusing ref-filter code.

Actually, %(rest) is supported for all ref-filter commands, but it
has the meaning only for cat-file, that's why I decided to leave it here.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 Documentation/git-cat-file.txt | 38 ++--------------------------------
 1 file changed, 2 insertions(+), 36 deletions(-)

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 8eca671b8278c..b32fcde802ca1 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -195,28 +195,8 @@ the whole line is considered as an object, as if it were fed to
 linkgit:git-rev-parse[1].
 
 You can specify the information shown for each object by using a custom
-`<format>`. The `<format>` is copied literally to stdout for each
-object, with placeholders of the form `%(atom)` expanded, followed by a
-newline. The available atoms are:
-
-`objectname`::
-	The 40-hex object name of the object.
-
-`objecttype`::
-	The type of the object (the same as `cat-file -t` reports).
-
-`objectsize`::
-	The size, in bytes, of the object (the same as `cat-file -s`
-	reports).
-
-`objectsize:disk`::
-	The size, in bytes, that the object takes up on disk. See the
-	note about on-disk sizes in the `CAVEATS` section below.
-
-`deltabase`::
-	If the object is stored as a delta on-disk, this expands to the
-	40-hex sha1 of the delta base object. Otherwise, expands to the
-	null sha1 (40 zeroes). See `CAVEATS` below.
+`<format>`. The format is the same as that of linkgit:git-for-each-ref[1],
+with one additional option:
 
 `rest`::
 	If this atom is used in the output string, input lines are split
@@ -300,20 +280,6 @@ notdir SP <size> LF
 is printed when, during symlink resolution, a file is used as a
 directory name.
 
-CAVEATS
--------
-
-Note that the sizes of objects on disk are reported accurately, but care
-should be taken in drawing conclusions about which refs or objects are
-responsible for disk usage. The size of a packed non-delta object may be
-much larger than the size of objects which delta against it, but the
-choice of which object is the base and which is the delta is arbitrary
-and is subject to change during a repack.
-
-Note also that multiple copies of an object may be present in the object
-database; in this case, it is undefined which copy's size or delta base
-will be reported.
-
 GIT
 ---
 Part of the linkgit:git[1] suite

--
https://github.com/git/git/pull/568

[PATCH RFC 14/20] cat-file: move print_object_or_die to ref-filter

[Olga Telezhnaya]

Move printing function to ref-filter, it is logical because
we move all formatting/printing logic to ref-filter.
It could be much better if we embed this logic into current
flows in ref-filter, but it looks like the task for another patch.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 51 ---------------------------------------------
 ref-filter.c       | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 ref-filter.h       |  3 +++
 3 files changed, 55 insertions(+), 51 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2066ff1e697e4..6c0cbf71f0f0c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -226,57 +226,6 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)
 	return end - start + 1;
 }
 
-static void print_object_or_die(struct expand_data *data, int cmdmode,
-				int buffered, const char *rest)
-{
-	const struct object_id *oid = &data->oid;
-	unsigned long size;
-	char *contents;
-
-	assert(data->info.typep);
-
-	if (data->type != OBJ_BLOB) {
-		enum object_type type;
-		contents = read_object_file(oid, &type, &size);
-		if (!contents)
-			die("object %s disappeared", oid_to_hex(oid));
-		if (type != data->type)
-			die("object %s changed type!?", oid_to_hex(oid));
-		if (data->info.sizep && size != data->size)
-			die("object %s changed size!?", oid_to_hex(oid));
-
-		write_or_die(1, contents, size);
-		free(contents);
-		return;
-	}
-
-	if (buffered)
-		fflush(stdout);
-	if (!cmdmode) {
-		if (stream_blob_to_fd(1, oid, NULL, 0))
-			die("unable to stream %s to stdout", oid_to_hex(oid));
-		return;
-	}
-
-	if (!rest)
-		die("missing path for '%s'", oid_to_hex(oid));
-
-	if (cmdmode == 'w') {
-		if (filter_object(rest, 0100644, oid, &contents, &size))
-			die("could not convert '%s' %s", oid_to_hex(oid), rest);
-	} else if (cmdmode == 'c') {
-		enum object_type type;
-		if (!textconv_object(the_repository, rest, 0100644, oid, 1,
-				     &contents, &size))
-			contents = read_object_file(oid, &type, &size);
-		if (!contents)
-			die("could not convert '%s' %s", oid_to_hex(oid), rest);
-	} else
-		BUG("invalid cmdmode: %c", cmdmode);
-	write_or_die(1, contents, size);
-	free(contents);
-}
-
 static void batch_object_write(const char *obj_name,
 			       struct strbuf *scratch,
 			       struct batch_options *opt,
diff --git a/ref-filter.c b/ref-filter.c
index 65b94ea21e54f..68d9741a56468 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -20,6 +20,7 @@
 #include "commit-slab.h"
 #include "commit-graph.h"
 #include "commit-reach.h"
+#include "streaming.h"
 
 static struct ref_msg {
 	const char *gone;
@@ -2366,3 +2367,54 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 
 	return 0;
 }
+
+void print_object_or_die(struct expand_data *data, int cmdmode,
+			 int buffered, const char *rest)
+{
+	const struct object_id *oid = &data->oid;
+	unsigned long size;
+	char *contents;
+
+	assert(data->info.typep);
+
+	if (data->type != OBJ_BLOB) {
+		enum object_type type;
+		contents = read_object_file(oid, &type, &size);
+		if (!contents)
+			die("object %s disappeared", oid_to_hex(oid));
+		if (type != data->type)
+			die("object %s changed type!?", oid_to_hex(oid));
+		if (data->info.sizep && size != data->size)
+			die("object %s changed size!?", oid_to_hex(oid));
+
+		write_or_die(1, contents, size);
+		free(contents);
+		return;
+	}
+
+	if (buffered)
+		fflush(stdout);
+	if (!cmdmode) {
+		if (stream_blob_to_fd(1, oid, NULL, 0))
+			die("unable to stream %s to stdout", oid_to_hex(oid));
+		return;
+	}
+
+	if (!rest)
+		die("missing path for '%s'", oid_to_hex(oid));
+
+	if (cmdmode == 'w') {
+		if (filter_object(rest, 0100644, oid, &contents, &size))
+			die("could not convert '%s' %s", oid_to_hex(oid), rest);
+	} else if (cmdmode == 'c') {
+		enum object_type type;
+		if (!textconv_object(the_repository, rest, 0100644, oid, 1,
+				     &contents, &size))
+			contents = read_object_file(oid, &type, &size);
+		if (!contents)
+			die("could not convert '%s' %s", oid_to_hex(oid), rest);
+	} else
+		BUG("invalid cmdmode: %c", cmdmode);
+	write_or_die(1, contents, size);
+	free(contents);
+}
diff --git a/ref-filter.h b/ref-filter.h
index fc61457d4d660..3422f39e64b5b 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -157,4 +157,7 @@ struct ref_array_item *ref_array_push(struct ref_array *array,
 				      const char *refname,
 				      const struct object_id *oid);
 
+void print_object_or_die(struct expand_data *data, int cmdmode,
+			 int buffered, const char *rest);
+
 #endif /*  REF_FILTER_H  */

--
https://github.com/git/git/pull/568

[PATCH RFC 13/20] cat-file: rewrite print_object_or_die

[Olga Telezhnaya]

In the next commit I will move function print_object_or_die
to ref-filter, and I decided to rewrite it a little so that it
becomes much more flatten and a little bit shorter.
I also changed input parameters, it allows me to move it
to ref-filter, ref-filter knows nothing about batch_options.

The logic of the function remains the same.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 72 +++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 39 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index a4e56762f9e56..2066ff1e697e4 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -226,50 +226,17 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)
 	return end - start + 1;
 }
 
-static void print_object_or_die(struct batch_options *opt, struct expand_data *data)
+static void print_object_or_die(struct expand_data *data, int cmdmode,
+				int buffered, const char *rest)
 {
 	const struct object_id *oid = &data->oid;
+	unsigned long size;
+	char *contents;
 
 	assert(data->info.typep);
 
-	if (data->type == OBJ_BLOB) {
-		if (opt->buffer_output)
-			fflush(stdout);
-		if (opt->cmdmode) {
-			char *contents;
-			unsigned long size;
-
-			if (!rest)
-				die("missing path for '%s'", oid_to_hex(oid));
-
-			if (opt->cmdmode == 'w') {
-				if (filter_object(rest, 0100644, oid,
-						  &contents, &size))
-					die("could not convert '%s' %s",
-					    oid_to_hex(oid), rest);
-			} else if (opt->cmdmode == 'c') {
-				enum object_type type;
-				if (!textconv_object(the_repository,
-						     rest, 0100644, oid,
-						     1, &contents, &size))
-					contents = read_object_file(oid,
-								    &type,
-								    &size);
-				if (!contents)
-					die("could not convert '%s' %s",
-					    oid_to_hex(oid), rest);
-			} else
-				BUG("invalid cmdmode: %c", opt->cmdmode);
-			write_or_die(1, contents, size);
-			free(contents);
-		} else if (stream_blob_to_fd(1, oid, NULL, 0))
-			die("unable to stream %s to stdout", oid_to_hex(oid));
-	}
-	else {
+	if (data->type != OBJ_BLOB) {
 		enum object_type type;
-		unsigned long size;
-		void *contents;
-
 		contents = read_object_file(oid, &type, &size);
 		if (!contents)
 			die("object %s disappeared", oid_to_hex(oid));
@@ -280,7 +247,34 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 
 		write_or_die(1, contents, size);
 		free(contents);
+		return;
+	}
+
+	if (buffered)
+		fflush(stdout);
+	if (!cmdmode) {
+		if (stream_blob_to_fd(1, oid, NULL, 0))
+			die("unable to stream %s to stdout", oid_to_hex(oid));
+		return;
 	}
+
+	if (!rest)
+		die("missing path for '%s'", oid_to_hex(oid));
+
+	if (cmdmode == 'w') {
+		if (filter_object(rest, 0100644, oid, &contents, &size))
+			die("could not convert '%s' %s", oid_to_hex(oid), rest);
+	} else if (cmdmode == 'c') {
+		enum object_type type;
+		if (!textconv_object(the_repository, rest, 0100644, oid, 1,
+				     &contents, &size))
+			contents = read_object_file(oid, &type, &size);
+		if (!contents)
+			die("could not convert '%s' %s", oid_to_hex(oid), rest);
+	} else
+		BUG("invalid cmdmode: %c", cmdmode);
+	write_or_die(1, contents, size);
+	free(contents);
 }
 
 static void batch_object_write(const char *obj_name,
@@ -303,7 +297,7 @@ static void batch_object_write(const char *obj_name,
 	write_or_die(1, scratch->buf, scratch->len);
 
 	if (opt->print_contents) {
-		print_object_or_die(opt, data);
+		print_object_or_die(data, opt->cmdmode, opt->buffer_output, rest);
 		write_or_die(1, "\n", 1);
 	}
 }

--
https://github.com/git/git/pull/568

[PATCH RFC 10/20] cat-file: inline stream_blob

[Olga Telezhnaya]

Inline function stream_blob, it simplifies further
migrating process.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index edf45f078b919..cd9a4447c8da9 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -56,13 +56,6 @@ static int filter_object(const char *path, unsigned mode,
 	return 0;
 }
 
-static int stream_blob(const struct object_id *oid)
-{
-	if (stream_blob_to_fd(1, oid, NULL, 0))
-		die("unable to stream %s to stdout", oid_to_hex(oid));
-	return 0;
-}
-
 static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			int unknown_type)
 {
@@ -145,8 +138,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			return cmd_ls_tree(2, ls_args, NULL);
 		}
 
-		if (type == OBJ_BLOB)
-			return stream_blob(&oid);
+		if (type == OBJ_BLOB) {
+			if (stream_blob_to_fd(1, &oid, NULL, 0))
+				die("unable to stream %s to stdout", oid_to_hex(&oid));
+			return 0;
+		}
 		buf = read_object_file(&oid, &type, &size);
 		if (!buf)
 			die("Cannot read object %s", obj_name);
@@ -168,8 +164,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			} else
 				oidcpy(&blob_oid, &oid);
 
-			if (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB)
-				return stream_blob(&blob_oid);
+			if (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB) {
+				if (stream_blob_to_fd(1, &blob_oid, NULL, 0))
+					die("unable to stream %s to stdout", oid_to_hex(&blob_oid));
+				return 0;
+			}
 			/*
 			 * we attempted to dereference a tag to a blob
 			 * and failed; there may be new dereference
@@ -295,9 +294,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 				BUG("invalid cmdmode: %c", opt->cmdmode);
 			batch_write(opt, contents, size);
 			free(contents);
-		} else {
-			stream_blob(oid);
-		}
+		} else if (stream_blob_to_fd(1, oid, NULL, 0))
+			die("unable to stream %s to stdout", oid_to_hex(oid));
 	}
 	else {
 		enum object_type type;

--
https://github.com/git/git/pull/568

Re: [PATCH RFC 10/20] cat-file: inline stream_blob

[Jeff King]

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Inline function stream_blob, it simplifies further
> migrating process.

I'd have to see what exactly gets simplified later on, but I'm mildly
negative on this by itself. The reason this function was added in
98f425b453 (cat-file: handle streaming failures consistently,
2018-10-30) was to keep the outcomes consistent.

The function right now isn't _too_ long, so we're really just
duplicating the message text. But I wonder if it might eventually get
more complicated, if we ever do the "future work" discussed in
98f425b453. So this seems like a step in the wrong direction.

-Peff

[PATCH RFC 05/20] cat-file: remove split_on_whitespace

[Olga Telezhnaya]

Get rid of split_on_whitespace field in struct expand_data.
expand_data may be global further as we use it in ref-filter also,
so we need to remove cat-file specific fields from it.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index e5de596611800..60f3839b06f8c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -203,13 +203,6 @@ struct expand_data {
 	 */
 	int mark_query;
 
-	/*
-	 * Whether to split the input on whitespace before feeding it to
-	 * get_sha1; this is decided during the mark_query phase based on
-	 * whether we have a %(rest) token in our format.
-	 */
-	int split_on_whitespace;
-
 	/*
 	 * After a mark_query run, this object_info is set up to be
 	 * passed to oid_object_info_extended. It will point to the data
@@ -255,9 +248,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 		else
 			strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
 	} else if (is_atom("rest", atom, len)) {
-		if (data->mark_query)
-			data->split_on_whitespace = 1;
-		else if (data->rest)
+		if (data->rest)
 			strbuf_addstr(sb, data->rest);
 	} else if (is_atom("deltabase", atom, len)) {
 		if (data->mark_query)
@@ -491,6 +482,7 @@ static int batch_objects(struct batch_options *opt)
 	struct expand_data data;
 	int save_warning;
 	int retval = 0;
+	int is_rest = strstr(opt->format.format, "%(rest)") != NULL || opt->cmdmode;
 
 	/*
 	 * Expand once with our special mark_query flag, which will prime the
@@ -502,8 +494,6 @@ static int batch_objects(struct batch_options *opt)
 	strbuf_expand(&output, opt->format.format, expand_format, &data);
 	data.mark_query = 0;
 	strbuf_release(&output);
-	if (opt->cmdmode)
-		data.split_on_whitespace = 1;
 
 	if (opt->all_objects) {
 		struct object_info empty = OBJECT_INFO_INIT;
@@ -564,7 +554,7 @@ static int batch_objects(struct batch_options *opt)
 	warn_on_object_refname_ambiguity = 0;
 
 	while (strbuf_getline(&input, stdin) != EOF) {
-		if (data.split_on_whitespace) {
+		if (is_rest) {
 			/*
 			 * Split at first whitespace, tying off the beginning
 			 * of the string and saving the remainder (or NULL) in

--
https://github.com/git/git/pull/568

Re: [PATCH RFC 05/20] cat-file: remove split_on_whitespace

[Jeff King]

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Get rid of split_on_whitespace field in struct expand_data.
> expand_data may be global further as we use it in ref-filter also,
> so we need to remove cat-file specific fields from it.

OK, that makes some sense.

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index e5de596611800..60f3839b06f8c 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -203,13 +203,6 @@ struct expand_data {
>  	 */
>  	int mark_query;
>  
> -	/*
> -	 * Whether to split the input on whitespace before feeding it to
> -	 * get_sha1; this is decided during the mark_query phase based on
> -	 * whether we have a %(rest) token in our format.
> -	 */
> -	int split_on_whitespace;

It looks like we lose this name and comment in the movement, though
(it's now "is_rest"). If it's just a local variable inside
batch_objects(), I don't know that we need the comment. But I think it's
more than is_rest, isn't it?

It looks like we auto-enable it when --textconv or --filters is given.
Can we stick with the split_on_whitespace name (which I think is also
more descriptive about how we intend it to be used)?

> @@ -491,6 +482,7 @@ static int batch_objects(struct batch_options *opt)
>  	struct expand_data data;
>  	int save_warning;
>  	int retval = 0;
> +	int is_rest = strstr(opt->format.format, "%(rest)") != NULL || opt->cmdmode;

I'm not excited by this loose parsing. It would do the wrong thing in
some funny corner cases (e.g., "%%(rest)").

We should be able to ask the format parser whether the "rest"
placeholder was used. That's what the initial strbuf_expand() call is
doing. I see that it's hard for us to pass something to its callback
outside of expand_data (since after all, expand_data takes up the
void-pointer data slot).

But doesn't that point to this being the wrong change (or perhaps the
wrong time to make it)?  I think we'd want to keep using our own local
expand_data as long as we are not using ref-filter. And then ref-filter
would grow its own struct to hold the data that _it_ needs. Some of that
would be duplicates of what we have here, but that's OK. When we cut
over to ref-filter, that's when we'd drop the fields here.

And eventually we'd drop that strbuf_expand(), too, as ref-filter would
do the parsing. But at that point we wouldn't want this strstr() either:
we'd have ref-filter parse the format, and then check the parsed atoms
to see if one of them is "rest".

-Peff

[PATCH RFC 08/20] cat-file: remove rest from expand_data

[Olga Telezhnaya]

Get rid of rest field in struct expand_data.
expand_data may be global further as we use it in ref-filter also,
so we need to remove cat-file specific fields from it.

All globals that I add through this patch will be deleted in the end,
so treat it just as the middle step.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f6470380f55b3..e52646c0e6b5b 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -29,9 +29,10 @@ struct batch_options {
 };
 
 static const char *force_path;
-/* Next 2 vars will be deleted at the end of this patch */
+/* Next 3 vars will be deleted at the end of this patch */
 static int mark_query;
 static int skip_object_info;
+static const char *rest;
 
 static int filter_object(const char *path, unsigned mode,
 			 const struct object_id *oid,
@@ -197,7 +198,6 @@ struct expand_data {
 	enum object_type type;
 	unsigned long size;
 	off_t disk_size;
-	const char *rest;
 	struct object_id delta_base_oid;
 
 	/*
@@ -238,8 +238,8 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 		else
 			strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
 	} else if (is_atom("rest", atom, len)) {
-		if (data->rest)
-			strbuf_addstr(sb, data->rest);
+		if (rest)
+			strbuf_addstr(sb, rest);
 	} else if (is_atom("deltabase", atom, len)) {
 		if (mark_query)
 			data->info.delta_base_sha1 = data->delta_base_oid.hash;
@@ -287,25 +287,25 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 			char *contents;
 			unsigned long size;
 
-			if (!data->rest)
+			if (!rest)
 				die("missing path for '%s'", oid_to_hex(oid));
 
 			if (opt->cmdmode == 'w') {
-				if (filter_object(data->rest, 0100644, oid,
+				if (filter_object(rest, 0100644, oid,
 						  &contents, &size))
 					die("could not convert '%s' %s",
-					    oid_to_hex(oid), data->rest);
+					    oid_to_hex(oid), rest);
 			} else if (opt->cmdmode == 'c') {
 				enum object_type type;
 				if (!textconv_object(the_repository,
-						     data->rest, 0100644, oid,
+						     rest, 0100644, oid,
 						     1, &contents, &size))
 					contents = read_object_file(oid,
 								    &type,
 								    &size);
 				if (!contents)
 					die("could not convert '%s' %s",
-					    oid_to_hex(oid), data->rest);
+					    oid_to_hex(oid), rest);
 			} else
 				BUG("invalid cmdmode: %c", opt->cmdmode);
 			batch_write(opt, contents, size);
@@ -555,7 +555,7 @@ static int batch_objects(struct batch_options *opt)
 				while (*p && strchr(" \t", *p))
 					*p++ = '\0';
 			}
-			data.rest = p;
+			rest = p;
 		}
 
 		batch_one_object(input.buf, &output, opt, &data);

--
https://github.com/git/git/pull/568

Re: [PATCH RFC 08/20] cat-file: remove rest from expand_data

[Jeff King]

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Get rid of rest field in struct expand_data.
> expand_data may be global further as we use it in ref-filter also,
> so we need to remove cat-file specific fields from it.
> 
> All globals that I add through this patch will be deleted in the end,
> so treat it just as the middle step.

Same comments apply as patch 7 (and 6 and 5). :)

-Peff

[PATCH RFC 18/20] cat-file: get rid of expand_data

[Olga Telezhnaya]

Clean up cat-file after moving all formatting logic
to ref-filter.
We do not need to use struct expand_data anymore.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 43 +++++++++++++++++++++++--------------------
 ref-filter.c       | 11 ++++++++++-
 ref-filter.h       | 12 ------------
 3 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 6fa100d1bea72..ee7557e1e0975 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -28,8 +28,6 @@ struct batch_options {
 };
 
 static const char *force_path;
-/* global rest will be deleted at the end of this patch */
-static const char *rest;
 
 static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			int unknown_type)
@@ -170,16 +168,19 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 static void batch_object_write(const char *obj_name,
 			       struct strbuf *scratch,
 			       struct batch_options *opt,
-			       struct expand_data *data)
+			       struct ref_array_item *item)
 {
 	struct strbuf err = STRBUF_INIT;
-	struct ref_array_item item = { data->oid };
-	item.request_rest = rest;
-	item.check_obj = 1;
+	/*
+	 * TODO: get rid of memory leak. The best way is to reuse ref_array
+	 * in batch_objects and then call ref_array_clear.
+	 */
+	item->value = 0;
+	item->check_obj = 1;
 	strbuf_reset(scratch);
 
-	if (format_ref_array_item(&item, &opt->format, scratch, &err)) {
-		printf("%s missing\n", obj_name ? obj_name : oid_to_hex(&item.oid));
+	if (format_ref_array_item(item, &opt->format, scratch, &err)) {
+		printf("%s missing\n", obj_name ? obj_name : oid_to_hex(&item->oid));
 		fflush(stdout);
 		return;
 	}
@@ -189,7 +190,7 @@ static void batch_object_write(const char *obj_name,
 	strbuf_release(&err);
 
 	if (opt->print_contents) {
-		print_raw_object_or_die(&item, opt->cmdmode, opt->buffer_output);
+		print_raw_object_or_die(item, opt->cmdmode, opt->buffer_output);
 		write_or_die(1, "\n", 1);
 	}
 }
@@ -197,14 +198,14 @@ static void batch_object_write(const char *obj_name,
 static void batch_one_object(const char *obj_name,
 			     struct strbuf *scratch,
 			     struct batch_options *opt,
-			     struct expand_data *data)
+			     struct ref_array_item *item)
 {
 	struct object_context ctx;
 	int flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0;
 	enum get_oid_result result;
 
 	result = get_oid_with_context(the_repository, obj_name,
-				      flags, &data->oid, &ctx);
+				      flags, &item->oid, &ctx);
 	if (result != FOUND) {
 		switch (result) {
 		case MISSING_OBJECT:
@@ -242,12 +243,12 @@ static void batch_one_object(const char *obj_name,
 		return;
 	}
 
-	batch_object_write(obj_name, scratch, opt, data);
+	batch_object_write(obj_name, scratch, opt, item);
 }
 
 struct object_cb_data {
 	struct batch_options *opt;
-	struct expand_data *expand;
+	struct ref_array_item *item;
 	struct oidset *seen;
 	struct strbuf *scratch;
 };
@@ -255,8 +256,8 @@ struct object_cb_data {
 static int batch_object_cb(const struct object_id *oid, void *vdata)
 {
 	struct object_cb_data *data = vdata;
-	oidcpy(&data->expand->oid, oid);
-	batch_object_write(NULL, data->scratch, data->opt, data->expand);
+	oidcpy(&data->item->oid, oid);
+	batch_object_write(NULL, data->scratch, data->opt, data->item);
 	return 0;
 }
 
@@ -306,20 +307,20 @@ static int batch_objects(struct batch_options *opt)
 {
 	struct strbuf input = STRBUF_INIT;
 	struct strbuf output = STRBUF_INIT;
-	struct expand_data data;
 	int save_warning;
 	int retval = 0;
 	int is_rest = strstr(opt->format.format, "%(rest)") != NULL || opt->cmdmode;
-	memset(&data, 0, sizeof(data));
 
 	if (opt->all_objects) {
 		struct object_cb_data cb;
+		struct ref_array_item item;
+		memset(&item, 0, sizeof(item));
 
 		if (repository_format_partial_clone)
 			warning("This repository has extensions.partialClone set. Some objects may not be loaded.");
 
 		cb.opt = opt;
-		cb.expand = &data;
+		cb.item = &item;
 		cb.scratch = &output;
 
 		if (opt->unordered) {
@@ -358,6 +359,8 @@ static int batch_objects(struct batch_options *opt)
 	warn_on_object_refname_ambiguity = 0;
 
 	while (strbuf_getline(&input, stdin) != EOF) {
+		struct ref_array_item item;
+		memset(&item, 0, sizeof(item));
 		if (is_rest) {
 			/*
 			 * Split at first whitespace, tying off the beginning
@@ -369,10 +372,10 @@ static int batch_objects(struct batch_options *opt)
 				while (*p && strchr(" \t", *p))
 					*p++ = '\0';
 			}
-			rest = p;
+			item.request_rest = p;
 		}
 
-		batch_one_object(input.buf, &output, opt, &data);
+		batch_one_object(input.buf, &output, opt, &item);
 	}
 
 	strbuf_release(&input);
diff --git a/ref-filter.c b/ref-filter.c
index 45d163246e3f3..3f9bd2fc6a76a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -65,7 +65,16 @@ struct refname_atom {
 	int lstrip, rstrip;
 };
 
-static struct expand_data oi, oi_deref;
+static struct expand_data {
+	struct object_id oid;
+	enum object_type type;
+	unsigned long size;
+	off_t disk_size;
+	struct object_id delta_base_oid;
+	void *content;
+
+	struct object_info info;
+} oi, oi_deref;
 
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
diff --git a/ref-filter.h b/ref-filter.h
index e8cd97a49632c..237eed9818949 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -5,7 +5,6 @@
 #include "refs.h"
 #include "commit.h"
 #include "parse-options.h"
-#include "object-store.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -75,17 +74,6 @@ struct ref_filter {
 		verbose;
 };
 
-struct expand_data {
-	struct object_id oid;
-	enum object_type type;
-	unsigned long size;
-	off_t disk_size;
-	struct object_id delta_base_oid;
-	void *content;
-
-	struct object_info info;
-};
-
 struct ref_format {
 	/*
 	 * Set these to define the format; make sure you call

--
https://github.com/git/git/pull/568

[PATCH RFC 17/20] cat-file: reuse ref-filter formatting logic

[Olga Telezhnaya]

Start using general ref-filter formatting logic in cat-file.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 111 ++++++++-------------------------------------
 ref-filter.c       |  39 +++++++++++-----
 ref-filter.h       |   4 +-
 3 files changed, 49 insertions(+), 105 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 6c0cbf71f0f0c..6fa100d1bea72 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -28,9 +28,7 @@ struct batch_options {
 };
 
 static const char *force_path;
-/* Next 3 vars will be deleted at the end of this patch */
-static int mark_query;
-static int skip_object_info;
+/* global rest will be deleted at the end of this patch */
 static const char *rest;
 
 static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
@@ -169,84 +167,29 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	return 0;
 }
 
-static int is_atom(const char *atom, const char *s, int slen)
-{
-	int alen = strlen(atom);
-	return alen == slen && !memcmp(atom, s, alen);
-}
-
-static void expand_atom(struct strbuf *sb, const char *atom, int len,
-			void *vdata)
-{
-	struct expand_data *data = vdata;
-
-	if (is_atom("objectname", atom, len)) {
-		if (!mark_query)
-			strbuf_addstr(sb, oid_to_hex(&data->oid));
-	} else if (is_atom("objecttype", atom, len)) {
-		if (mark_query)
-			data->info.typep = &data->type;
-		else
-			strbuf_addstr(sb, type_name(data->type));
-	} else if (is_atom("objectsize", atom, len)) {
-		if (mark_query)
-			data->info.sizep = &data->size;
-		else
-			strbuf_addf(sb, "%"PRIuMAX , (uintmax_t)data->size);
-	} else if (is_atom("objectsize:disk", atom, len)) {
-		if (mark_query)
-			data->info.disk_sizep = &data->disk_size;
-		else
-			strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
-	} else if (is_atom("rest", atom, len)) {
-		if (rest)
-			strbuf_addstr(sb, rest);
-	} else if (is_atom("deltabase", atom, len)) {
-		if (mark_query)
-			data->info.delta_base_sha1 = data->delta_base_oid.hash;
-		else
-			strbuf_addstr(sb,
-				      oid_to_hex(&data->delta_base_oid));
-	} else
-		die("unknown format element: %.*s", len, atom);
-}
-
-static size_t expand_format(struct strbuf *sb, const char *start, void *data)
-{
-	const char *end;
-
-	if (*start != '(')
-		return 0;
-	end = strchr(start + 1, ')');
-	if (!end)
-		die("format element '%s' does not end in ')'", start);
-
-	expand_atom(sb, start + 1, end - start - 1, data);
-
-	return end - start + 1;
-}
-
 static void batch_object_write(const char *obj_name,
 			       struct strbuf *scratch,
 			       struct batch_options *opt,
 			       struct expand_data *data)
 {
-	if (!skip_object_info &&
-	    oid_object_info_extended(the_repository, &data->oid, &data->info,
-				     OBJECT_INFO_LOOKUP_REPLACE) < 0) {
-		printf("%s missing\n",
-		       obj_name ? obj_name : oid_to_hex(&data->oid));
+	struct strbuf err = STRBUF_INIT;
+	struct ref_array_item item = { data->oid };
+	item.request_rest = rest;
+	item.check_obj = 1;
+	strbuf_reset(scratch);
+
+	if (format_ref_array_item(&item, &opt->format, scratch, &err)) {
+		printf("%s missing\n", obj_name ? obj_name : oid_to_hex(&item.oid));
 		fflush(stdout);
 		return;
 	}
 
-	strbuf_reset(scratch);
-	strbuf_expand(scratch, opt->format.format, expand_format, data);
 	strbuf_addch(scratch, '\n');
 	write_or_die(1, scratch->buf, scratch->len);
+	strbuf_release(&err);
 
 	if (opt->print_contents) {
-		print_object_or_die(data, opt->cmdmode, opt->buffer_output, rest);
+		print_raw_object_or_die(&item, opt->cmdmode, opt->buffer_output);
 		write_or_die(1, "\n", 1);
 	}
 }
@@ -367,30 +310,7 @@ static int batch_objects(struct batch_options *opt)
 	int save_warning;
 	int retval = 0;
 	int is_rest = strstr(opt->format.format, "%(rest)") != NULL || opt->cmdmode;
-
-	/*
-	 * Expand once with our special mark_query flag, which will prime the
-	 * object_info to be handed to oid_object_info_extended for each
-	 * object.
-	 */
 	memset(&data, 0, sizeof(data));
-	mark_query = 1;
-	strbuf_expand(&output, opt->format.format, expand_format, &data);
-	mark_query = 0;
-	strbuf_release(&output);
-
-	if (opt->all_objects) {
-		struct object_info empty = OBJECT_INFO_INIT;
-		if (!memcmp(&data.info, &empty, sizeof(empty)))
-			skip_object_info = 1;
-	}
-
-	/*
-	 * If we are printing out the object, then always fill in the type,
-	 * since we will want to decide whether or not to stream.
-	 */
-	if (opt->print_contents)
-		data.info.typep = &data.type;
 
 	if (opt->all_objects) {
 		struct object_cb_data cb;
@@ -581,6 +501,15 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 
 	if (!batch.format.format)
 		batch.format.format = "%(objectname) %(objecttype) %(objectsize)";
+	if (batch.print_contents) {
+		const char *contents = "%(raw)";
+		char *format = (char *)calloc(strlen(batch.format.format) + strlen(contents) + 1, 1);
+		memcpy(format, batch.format.format, strlen(batch.format.format));
+		memcpy(format + strlen(format), contents, strlen(contents));
+		batch.format.format = format;
+	}
+	if (verify_ref_format(&batch.format))
+		usage_with_options(cat_file_usage, options);
 
 	if (batch.enabled)
 		return batch_objects(&batch);
diff --git a/ref-filter.c b/ref-filter.c
index bb963a4110fb2..45d163246e3f3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2231,6 +2231,7 @@ int format_ref_array_item(struct ref_array_item *info,
 {
 	const char *cp, *sp, *ep;
 	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
+	struct object_info empty = OBJECT_INFO_INIT;
 
 	state.quote_style = format->quote_style;
 	push_stack_element(&state.stack);
@@ -2253,6 +2254,11 @@ int format_ref_array_item(struct ref_array_item *info,
 		sp = cp + strlen(cp);
 		append_literal(cp, sp, &state);
 	}
+	if (info->check_obj &&
+	    oid_object_info_extended(the_repository, &info->oid, &empty,
+				     OBJECT_INFO_LOOKUP_REPLACE))
+		return strbuf_addf_ret(error_buf, -1, _("%s missing\n"),
+				       oid_to_hex(&info->oid));
 	if (format->need_color_reset_at_eol) {
 		struct atom_value resetv;
 		resetv.s = GIT_COLOR_RESET;
@@ -2381,23 +2387,32 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-void print_object_or_die(struct expand_data *data, int cmdmode,
-			 int buffered, const char *rest)
+/*
+ * TODO: add support of %(*raw). Need to switch between oi and oi_deref for that.
+ * TODO: split logic and printing (as it is done in format_ref_array_item and
+ * show_ref_array_item).
+ * TODO: rewrite print_object_or_die so that it will reuse result of general
+ * oid_object_info_extended call.
+ * TODO: embed this function into general ref_filter flow, make it static.
+ * That will allow other ref-filter users to print raw file
+ * (now only cat_file can use it).
+ */
+void print_raw_object_or_die(struct ref_array_item *item, int cmdmode, int buffered)
 {
-	const struct object_id *oid = &data->oid;
+	const struct object_id *oid = &oi.oid;
 	unsigned long size;
 	char *contents;
 
-	assert(data->info.typep);
+	assert(oi.info.typep);
 
-	if (data->type != OBJ_BLOB) {
+	if (oi.type != OBJ_BLOB) {
 		enum object_type type;
 		contents = read_object_file(oid, &type, &size);
 		if (!contents)
 			die("object %s disappeared", oid_to_hex(oid));
-		if (type != data->type)
+		if (type != oi.type)
 			die("object %s changed type!?", oid_to_hex(oid));
-		if (data->info.sizep && size != data->size)
+		if (oi.info.sizep && size != oi.size)
 			die("object %s changed size!?", oid_to_hex(oid));
 
 		write_or_die(1, contents, size);
@@ -2413,19 +2428,19 @@ void print_object_or_die(struct expand_data *data, int cmdmode,
 		return;
 	}
 
-	if (!rest)
+	if (!item->request_rest)
 		die("missing path for '%s'", oid_to_hex(oid));
 
 	if (cmdmode == 'w') {
-		if (filter_object(rest, 0100644, oid, &contents, &size))
-			die("could not convert '%s' %s", oid_to_hex(oid), rest);
+		if (filter_object(item->request_rest, 0100644, oid, &contents, &size))
+			die("could not convert '%s' %s", oid_to_hex(oid), item->request_rest);
 	} else if (cmdmode == 'c') {
 		enum object_type type;
-		if (!textconv_object(the_repository, rest, 0100644, oid, 1,
+		if (!textconv_object(the_repository, item->request_rest, 0100644, oid, 1,
 				     &contents, &size))
 			contents = read_object_file(oid, &type, &size);
 		if (!contents)
-			die("could not convert '%s' %s", oid_to_hex(oid), rest);
+			die("could not convert '%s' %s", oid_to_hex(oid), item->request_rest);
 	} else
 		BUG("invalid cmdmode: %c", cmdmode);
 	write_or_die(1, contents, size);
diff --git a/ref-filter.h b/ref-filter.h
index 3422f39e64b5b..e8cd97a49632c 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -42,6 +42,7 @@ struct ref_array_item {
 	struct commit *commit;
 	struct atom_value *value;
 	const char *request_rest;
+	int check_obj;
 	char refname[FLEX_ARRAY];
 };
 
@@ -157,7 +158,6 @@ struct ref_array_item *ref_array_push(struct ref_array *array,
 				      const char *refname,
 				      const struct object_id *oid);
 
-void print_object_or_die(struct expand_data *data, int cmdmode,
-			 int buffered, const char *rest);
+void print_raw_object_or_die(struct ref_array_item *item, int cmdmode, int buffered);
 
 #endif /*  REF_FILTER_H  */

--
https://github.com/git/git/pull/568

[PATCH RFC 19/20] cat-file: tests for new atoms added

[Olga Telezhnaya]

Add some tests for new formatting atoms from ref-filter.
Some of new atoms are supported automatically,
some of them are expanded into empty string
(because they are useless for some types of objects).

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 t/t1006-cat-file.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 43c4be1e5ef55..3c848f2773bbb 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -20,6 +20,19 @@ maybe_remove_timestamp () {
     fi
 }
 
+test_atom () {
+    name=$1
+    sha1=$2
+    atoms=$3
+    expected=$4
+
+    test_expect_success "$name" '
+	echo "$expected" >expect &&
+	echo $sha1 | git cat-file --batch-check="$atoms" >actual &&
+	test_cmp expect actual
+    '
+}
+
 run_tests () {
     type=$1
     sha1=$2
@@ -119,6 +132,13 @@ $content"
 	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
 	test_cmp expect actual
     '
+
+    for atom in refname parent body trailers upstream push symref flag
+    do
+	test_atom "Check %($atom) gives empty output" "$sha1" "%($atom)" ""
+    done
+
+    test_atom "Check %(HEAD) gives only one space as output" "$sha1" '%(HEAD)' ' '
 }
 
 hello_content="Hello World"
@@ -140,6 +160,12 @@ test_expect_success '--batch-check without %(rest) considers whole line' '
 	test_cmp expect actual
 '
 
+shortname=`echo $hello_sha1 | sed 's/^.\{0\}\(.\{7\}\).*/\1/'`
+test_atom 'Check format option %(objectname:short) works' "$hello_sha1" '%(objectname:short)' "$shortname"
+
+test_atom 'Check format option %(align) is not broken' \
+    "$hello_sha1" "%(align:8)%(objecttype)%(end)%(objectname)" "blob    $hello_sha1"
+
 test_oid_init
 
 tree_sha1=$(git write-tree)
@@ -159,6 +185,17 @@ $commit_message"
 
 run_tests 'commit' $commit_sha1 $commit_size "$commit_content" "$commit_content" 1
 
+test_atom "Check format option %(if) is not broken" "$commit_sha1" \
+    "%(if)%(author)%(then)%(objectname)%(end)" "$commit_sha1"
+test_atom "Check %(tree) works for commit" "$commit_sha1" "%(tree)" "$tree_sha1"
+test_atom "Check %(numparent) works for commit" "$commit_sha1" "%(numparent)" "0"
+test_atom "Check %(authorname) works for commit" "$commit_sha1" "%(authorname)" "$GIT_AUTHOR_NAME"
+test_atom "Check %(authoremail) works for commit" "$commit_sha1" "%(authoremail)" "<$GIT_AUTHOR_EMAIL>"
+test_atom "Check %(committername) works for commit" "$commit_sha1" "%(committername)" "$GIT_COMMITTER_NAME"
+test_atom "Check %(committeremail) works for commit" "$commit_sha1" "%(committeremail)" "<$GIT_COMMITTER_EMAIL>"
+test_atom "Check %(subject) works for commit" "$commit_sha1" "%(subject)" "$commit_message"
+test_atom "Check %(contents) works for commit" "$commit_sha1" "%(contents)" "$commit_message"
+
 tag_header_without_timestamp="object $hello_sha1
 type blob
 tag hellotag
@@ -173,6 +210,17 @@ tag_size=$(strlen "$tag_content")
 
 run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_content" 1
 
+test_atom "Check %(object) works for tag" "$tag_sha1" "%(object)" "$hello_sha1"
+test_atom "Check %(type) works for tag" "$tag_sha1" "%(type)" "blob"
+test_atom "Check %(tag) works for tag" "$tag_sha1" "%(tag)" "hellotag"
+test_atom "Check %(taggername) works for tag" "$tag_sha1" "%(taggername)" "$GIT_COMMITTER_NAME"
+test_atom "Check %(taggeremail) works for tag" "$tag_sha1" "%(taggeremail)" "<$GIT_COMMITTER_EMAIL>"
+test_atom "Check %(subject) works for tag" "$tag_sha1" "%(subject)" "$tag_description"
+test_atom "Check %(contents) works for tag" "$tag_sha1" "%(contents)" "$tag_description"
+
+test_atom "Check %(color) gives no additional output" "$sha1" \
+    "%(objectname) %(color:green) %(objecttype)" "$sha1  $type"
+
 test_expect_success \
     "Reach a blob from a tag pointing to it" \
     "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""

--
https://github.com/git/git/pull/568

[PATCH RFC 04/20] for-each-ref: tests for new atom %(rest) added

[Olga Telezhnaya]

Add tests for new formatting atom %(rest).
We need this atom for cat-file command.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 t/t6300-for-each-ref.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 0ffd63071392e..fb361369a037c 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -322,6 +322,12 @@ test_expect_success 'exercise strftime with odd fields' '
 	test_cmp expected actual
 '
 
+test_expect_success 'Check format %(rest) gives empty output ' '
+	echo >expected &&
+	git for-each-ref --format="%(rest)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
 cat >expected <<\EOF
 refs/heads/master
 refs/remotes/origin/master

--
https://github.com/git/git/pull/568

Re: [PATCH RFC 04/20] for-each-ref: tests for new atom %(rest) added

[Jeff King]

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Add tests for new formatting atom %(rest).
> We need this atom for cat-file command.

While I do normally encourage splitting up commits, in this case I think
it would make sense to squash this together with patch 3. There's
nothing to say here about what %(rest) is that isn't already said in
that commit message.

That said, I'm still not sure that for-each-ref should be supporting
%(rest) at all. We should hopefully already have coverage of cat-file
using "%(rest)" (and if not, we should add some to make sure it's not
regressed by the conversion).

-Peff

Re: [PATCH RFC 04/20] for-each-ref: tests for new atom %(rest) added

[Olga Telezhnaya]

пт, 1 мар. 2019 г. в 00:11, Jeff King <peff@peff.net>:
>
> On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:
>
> > Add tests for new formatting atom %(rest).
> > We need this atom for cat-file command.
>
> While I do normally encourage splitting up commits, in this case I think
> it would make sense to squash this together with patch 3. There's
> nothing to say here about what %(rest) is that isn't already said in
> that commit message.

Agree, will squash.

>
> That said, I'm still not sure that for-each-ref should be supporting
> %(rest) at all. We should hopefully already have coverage of cat-file
> using "%(rest)" (and if not, we should add some to make sure it's not
> regressed by the conversion).

If we want to use ref-filter formatting logic in cat-file, we have to
add this atom in ref-filter. I agree that we do not need it in
ref-filter, and that's why I left %(rest) in cat-file docs (it's in
the end of the patch). But in the code, I am not sure we want to make
one more array with specific cat-file atoms (or atoms for other
command).

>
> -Peff

[PATCH RFC 15/20] ref-filter: add raw formatting option

[Olga Telezhnaya]

Add new formatting option %(raw), it means that we want to print
all the file without any changes. It will help further
to migrate all cat-file formatting logic from cat-file
to ref-filter. For now, we just treat it as the empty string.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index 68d9741a56468..bb963a4110fb2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -208,6 +208,15 @@ static int remote_ref_atom_parser(const struct ref_format *format, struct used_a
 	return 0;
 }
 
+static int raw_atom_parser(const struct ref_format *format, struct used_atom *atom,
+			   const char *arg, struct strbuf *err)
+{
+	if (arg)
+		return strbuf_addf_ret(err, -1, _("%%(raw) does not take arguments"));
+	oi.info.typep = &oi.type;
+	return 0;
+}
+
 static int objecttype_atom_parser(const struct ref_format *format, struct used_atom *atom,
 				  const char *arg, struct strbuf *err)
 {
@@ -478,6 +487,7 @@ static struct {
 	{ "then", SOURCE_NONE },
 	{ "else", SOURCE_NONE },
 	{ "rest", SOURCE_NONE },
+	{ "raw", SOURCE_NONE, FIELD_STR, raw_atom_parser },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -1619,6 +1629,9 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 		} else if (starts_with(name, "rest")) {
 			v->s = xstrdup(ref->request_rest ? ref->request_rest : "");
 			continue;
+		} else if (!strcmp(name, "raw")) {
+			v->s = xstrdup("");
+			continue;
 		} else if (starts_with(name, "align")) {
 			v->handler = align_atom_handler;
 			v->s = xstrdup("");

--
https://github.com/git/git/pull/568

[PATCH RFC 03/20] ref-filter: add rest formatting option

[Olga Telezhnaya]

Add rest option that allows to add string into ref_array_item
and then put it into specific place of the output.
We are using it now in cat-file command: user could put anything
in the input after objectname, and it will appear in the output
in place of %(rest).

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 4 ++++
 ref-filter.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index 736e1f9cc38fc..46bf89b3330de 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -485,6 +485,7 @@ static struct {
 	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
 	{ "then", SOURCE_NONE },
 	{ "else", SOURCE_NONE },
+	{ "rest", SOURCE_NONE },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -1623,6 +1624,9 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			else
 				v->s = xstrdup(" ");
 			continue;
+		} else if (starts_with(name, "rest")) {
+			v->s = xstrdup(ref->request_rest ? ref->request_rest : "");
+			continue;
 		} else if (starts_with(name, "align")) {
 			v->handler = align_atom_handler;
 			v->s = xstrdup("");
diff --git a/ref-filter.h b/ref-filter.h
index 4d7d36e9f522d..aaeda9f324f5c 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -40,6 +40,7 @@ struct ref_array_item {
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
+	const char *request_rest;
 	char refname[FLEX_ARRAY];
 };
 

--
https://github.com/git/git/pull/568

Re: [PATCH RFC 03/20] ref-filter: add rest formatting option

[Jeff King]

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Add rest option that allows to add string into ref_array_item
> and then put it into specific place of the output.
> We are using it now in cat-file command: user could put anything
> in the input after objectname, and it will appear in the output
> in place of %(rest).

This would make:

  git for-each-ref --format='%(rest)'

do something. But what (and could it ever be useful or meaningful)?

Should we add an option to ref-filter to enable/disable this
placeholder?

-Peff

[PATCH RFC 11/20] cat-file: move filter_object to diff.c

[Olga Telezhnaya]

Move function filter_object to diff.c, like it is done with
function textconv_object.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 23 -----------------------
 diff.c             | 23 +++++++++++++++++++++++
 diff.h             |  4 ++++
 3 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index cd9a4447c8da9..41f333b73d851 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -3,7 +3,6 @@
  *
  * Copyright (C) Linus Torvalds, 2005
  */
-#define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "config.h"
 #include "builtin.h"
@@ -34,28 +33,6 @@ static int mark_query;
 static int skip_object_info;
 static const char *rest;
 
-static int filter_object(const char *path, unsigned mode,
-			 const struct object_id *oid,
-			 char **buf, unsigned long *size)
-{
-	enum object_type type;
-
-	*buf = read_object_file(oid, &type, size);
-	if (!*buf)
-		return error(_("cannot read object %s '%s'"),
-			     oid_to_hex(oid), path);
-	if ((type == OBJ_BLOB) && S_ISREG(mode)) {
-		struct strbuf strbuf = STRBUF_INIT;
-		if (convert_to_working_tree(&the_index, 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)
 {
diff --git a/diff.c b/diff.c
index 5306c48652db5..fe7160c86755d 100644
--- a/diff.c
+++ b/diff.c
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2005 Junio C Hamano
  */
+#define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "config.h"
 #include "tempfile.h"
@@ -6524,6 +6525,28 @@ int textconv_object(struct repository *r,
 	return 1;
 }
 
+int filter_object(const char *path, unsigned mode,
+		  const struct object_id *oid,
+		  char **buf, unsigned long *size)
+{
+	enum object_type type;
+
+	*buf = read_object_file(oid, &type, size);
+	if (!*buf)
+		return error(_("cannot read object %s '%s'"),
+			     oid_to_hex(oid), path);
+	if ((type == OBJ_BLOB) && S_ISREG(mode)) {
+		struct strbuf strbuf = STRBUF_INIT;
+		if (convert_to_working_tree(&the_index, path, *buf, *size, &strbuf)) {
+			free(*buf);
+			*size = strbuf.len;
+			*buf = strbuf_detach(&strbuf, NULL);
+		}
+	}
+
+	return 0;
+}
+
 void setup_diff_pager(struct diff_options *opt)
 {
 	/*
diff --git a/diff.h b/diff.h
index b512d0477ac3a..c3709e611870a 100644
--- a/diff.h
+++ b/diff.h
@@ -476,6 +476,10 @@ int textconv_object(struct repository *repo,
 		    const struct object_id *oid, int oid_valid,
 		    char **buf, unsigned long *buf_size);
 
+int filter_object(const char *path, unsigned mode,
+		  const struct object_id *oid,
+		  char **buf, unsigned long *size);
+
 int parse_rename_score(const char **cp_p);
 
 long parse_algorithm_value(const char *value);

--
https://github.com/git/git/pull/568

[PATCH RFC 07/20] cat-file: remove skip_object_info

[Olga Telezhnaya]

Get rid of skip_object_info field in struct expand_data.
expand_data may be global further as we use it in ref-filter also,
so we need to remove cat-file specific fields from it.

All globals that I add through this patch will be deleted in the end,
so treat it just as the middle step.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 9bcb02fad1f0d..f6470380f55b3 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -29,8 +29,9 @@ struct batch_options {
 };
 
 static const char *force_path;
-/* Will be deleted at the end of this patch */
+/* Next 2 vars will be deleted at the end of this patch */
 static int mark_query;
+static int skip_object_info;
 
 static int filter_object(const char *path, unsigned mode,
 			 const struct object_id *oid,
@@ -205,13 +206,6 @@ struct expand_data {
 	 * elements above, so you can retrieve the response from there.
 	 */
 	struct object_info info;
-
-	/*
-	 * This flag will be true if the requested batch format and options
-	 * don't require us to call oid_object_info, which can then be
-	 * optimized out.
-	 */
-	unsigned skip_object_info : 1;
 };
 
 static int is_atom(const char *atom, const char *s, int slen)
@@ -343,7 +337,7 @@ static void batch_object_write(const char *obj_name,
 			       struct batch_options *opt,
 			       struct expand_data *data)
 {
-	if (!data->skip_object_info &&
+	if (!skip_object_info &&
 	    oid_object_info_extended(the_repository, &data->oid, &data->info,
 				     OBJECT_INFO_LOOKUP_REPLACE) < 0) {
 		printf("%s missing\n",
@@ -494,7 +488,7 @@ static int batch_objects(struct batch_options *opt)
 	if (opt->all_objects) {
 		struct object_info empty = OBJECT_INFO_INIT;
 		if (!memcmp(&data.info, &empty, sizeof(empty)))
-			data.skip_object_info = 1;
+			skip_object_info = 1;
 	}
 
 	/*

--
https://github.com/git/git/pull/568

Re: [PATCH RFC 07/20] cat-file: remove skip_object_info

[Jeff King]

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Get rid of skip_object_info field in struct expand_data.
> expand_data may be global further as we use it in ref-filter also,
> so we need to remove cat-file specific fields from it.
> 
> All globals that I add through this patch will be deleted in the end,
> so treat it just as the middle step.

OK, makes sense in the same way that the previous patch does. I actually
wonder if it would make sense to just do them all in a single patch; the
justification is identical for all cases. But I'm also OK leaving them
separate.

-Peff

[PATCH RFC 06/20] cat-file: remove mark_query from expand_data

[Olga Telezhnaya]

Get rid of mark_query field in struct expand_data.
expand_data may be global further as we use it in ref-filter also,
so we need to remove cat-file specific fields from it.

All globals that I add through this patch will be deleted in the end,
so treat it just as the middle step.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 60f3839b06f8c..9bcb02fad1f0d 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -29,6 +29,8 @@ struct batch_options {
 };
 
 static const char *force_path;
+/* Will be deleted at the end of this patch */
+static int mark_query;
 
 static int filter_object(const char *path, unsigned mode,
 			 const struct object_id *oid,
@@ -197,12 +199,6 @@ struct expand_data {
 	const char *rest;
 	struct object_id delta_base_oid;
 
-	/*
-	 * If mark_query is true, we do not expand anything, but rather
-	 * just mark the object_info with items we wish to query.
-	 */
-	int mark_query;
-
 	/*
 	 * After a mark_query run, this object_info is set up to be
 	 * passed to oid_object_info_extended. It will point to the data
@@ -230,20 +226,20 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 	struct expand_data *data = vdata;
 
 	if (is_atom("objectname", atom, len)) {
-		if (!data->mark_query)
+		if (!mark_query)
 			strbuf_addstr(sb, oid_to_hex(&data->oid));
 	} else if (is_atom("objecttype", atom, len)) {
-		if (data->mark_query)
+		if (mark_query)
 			data->info.typep = &data->type;
 		else
 			strbuf_addstr(sb, type_name(data->type));
 	} else if (is_atom("objectsize", atom, len)) {
-		if (data->mark_query)
+		if (mark_query)
 			data->info.sizep = &data->size;
 		else
 			strbuf_addf(sb, "%"PRIuMAX , (uintmax_t)data->size);
 	} else if (is_atom("objectsize:disk", atom, len)) {
-		if (data->mark_query)
+		if (mark_query)
 			data->info.disk_sizep = &data->disk_size;
 		else
 			strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
@@ -251,7 +247,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 		if (data->rest)
 			strbuf_addstr(sb, data->rest);
 	} else if (is_atom("deltabase", atom, len)) {
-		if (data->mark_query)
+		if (mark_query)
 			data->info.delta_base_sha1 = data->delta_base_oid.hash;
 		else
 			strbuf_addstr(sb,
@@ -490,9 +486,9 @@ static int batch_objects(struct batch_options *opt)
 	 * object.
 	 */
 	memset(&data, 0, sizeof(data));
-	data.mark_query = 1;
+	mark_query = 1;
 	strbuf_expand(&output, opt->format.format, expand_format, &data);
-	data.mark_query = 0;
+	mark_query = 0;
 	strbuf_release(&output);
 
 	if (opt->all_objects) {

--
https://github.com/git/git/pull/568

Re: [PATCH RFC 06/20] cat-file: remove mark_query from expand_data

[Jeff King]

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Get rid of mark_query field in struct expand_data.
> expand_data may be global further as we use it in ref-filter also,
> so we need to remove cat-file specific fields from it.
> 
> All globals that I add through this patch will be deleted in the end,
> so treat it just as the middle step.

So this is a similar situation to the split_on_whitespace thing we have
in the previous patch.

I think many of my comments there could apply here. I.e., do we need to
be removing them from expand_data now, instead of just moving the bits
from expand_data over to ref-filter?

But if we assume for a moment that doing it that way isn't feasible (or
at least isn't as easy as this way), then I think what this patch does
is preferable to the previous one. By making it a global variable, we
can still interact with it from the expand callback, even if it's not
part of expand_data().

So the previous patch could make "split_on_whitespace" a global, and
then continue to set it from expand_atom() as the current code does.

-Peff

Re: [PATCH RFC 06/20] cat-file: remove mark_query from expand_data

[Christian Couder]

On Fri, Feb 22, 2019 at 5:07 PM Olga Telezhnaya
<olyatelezhnaya@gmail.com> wrote:
>
> Get rid of mark_query field in struct expand_data.
> expand_data may be global further as we use it in ref-filter also,
> so we need to remove cat-file specific fields from it.
>
> All globals that I add through this patch will be deleted in the end,
> so treat it just as the middle step.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
>  builtin/cat-file.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 60f3839b06f8c..9bcb02fad1f0d 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -29,6 +29,8 @@ struct batch_options {
>  };
>
>  static const char *force_path;
> +/* Will be deleted at the end of this patch */

When this patch is committed, there is no patch anymore, only a
commit. And of course the variable will be deleted in a following
commit. So instead I'd rather see something like:

/* Will be deleted in a following commit */

or maybe:

/* TODO: delete this in a following commit */

[PATCH RFC 09/20] ref-filter: make expand_data global

[Olga Telezhnaya]

Put struct expand_data into global scope to reuse it
in cat-file.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 15 ---------------
 ref-filter.c       | 11 +----------
 ref-filter.h       | 12 ++++++++++++
 3 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index e52646c0e6b5b..edf45f078b919 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -193,21 +193,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	return 0;
 }
 
-struct expand_data {
-	struct object_id oid;
-	enum object_type type;
-	unsigned long size;
-	off_t disk_size;
-	struct object_id delta_base_oid;
-
-	/*
-	 * After a mark_query run, this object_info is set up to be
-	 * passed to oid_object_info_extended. It will point to the data
-	 * elements above, so you can retrieve the response from there.
-	 */
-	struct object_info info;
-};
-
 static int is_atom(const char *atom, const char *s, int slen)
 {
 	int alen = strlen(atom);
diff --git a/ref-filter.c b/ref-filter.c
index 46bf89b3330de..65b94ea21e54f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -64,16 +64,7 @@ struct refname_atom {
 	int lstrip, rstrip;
 };
 
-static struct expand_data {
-	struct object_id oid;
-	enum object_type type;
-	unsigned long size;
-	off_t disk_size;
-	struct object_id delta_base_oid;
-	void *content;
-
-	struct object_info info;
-} oi, oi_deref;
+static struct expand_data oi, oi_deref;
 
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
diff --git a/ref-filter.h b/ref-filter.h
index aaeda9f324f5c..fc61457d4d660 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -5,6 +5,7 @@
 #include "refs.h"
 #include "commit.h"
 #include "parse-options.h"
+#include "object-store.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -73,6 +74,17 @@ struct ref_filter {
 		verbose;
 };
 
+struct expand_data {
+	struct object_id oid;
+	enum object_type type;
+	unsigned long size;
+	off_t disk_size;
+	struct object_id delta_base_oid;
+	void *content;
+
+	struct object_info info;
+};
+
 struct ref_format {
 	/*
 	 * Set these to define the format; make sure you call

--
https://github.com/git/git/pull/568

Re: [PATCH RFC 09/20] ref-filter: make expand_data global

[Jeff King]

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Put struct expand_data into global scope to reuse it
> in cat-file.

So this is the payoff for moving all those things out of expand_data.
Instead of just replicating the bits it needs in ref-filter, we're
making it globally available.

At this point in the series, I'm still unconvinced that this is the
right direction, but I haven't read all the way to the end yet.

This probably needs a better name. In the context of cat-file,
expand_data is the data struct we feed to strbuf_expand(). But in the
global namespace of all of Git, it needs a more descriptive name.

This likely goes away (or becomes private to ref-filter.c) in the end,
but it probably needs a different name there, too. We're not calling
strbuf_expand() from there.

-Peff

[PATCH RFC 12/20] cat-file: remove batch_write function

[Olga Telezhnaya]

Correct me if I am wrong, but it was not really good idea
to implement batch_write in cmd_cat_file. Maybe it's
a good task for newbies to add flag
(whether we accept batch write or not) to write_or_die?

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/cat-file.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 41f333b73d851..a4e56762f9e56 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -226,15 +226,6 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)
 	return end - start + 1;
 }
 
-static void batch_write(struct batch_options *opt, const void *data, int len)
-{
-	if (opt->buffer_output) {
-		if (fwrite(data, 1, len, stdout) != len)
-			die_errno("unable to write to stdout");
-	} else
-		write_or_die(1, data, len);
-}
-
 static void print_object_or_die(struct batch_options *opt, struct expand_data *data)
 {
 	const struct object_id *oid = &data->oid;
@@ -269,7 +260,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 					    oid_to_hex(oid), rest);
 			} else
 				BUG("invalid cmdmode: %c", opt->cmdmode);
-			batch_write(opt, contents, size);
+			write_or_die(1, contents, size);
 			free(contents);
 		} else if (stream_blob_to_fd(1, oid, NULL, 0))
 			die("unable to stream %s to stdout", oid_to_hex(oid));
@@ -287,7 +278,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 		if (data->info.sizep && size != data->size)
 			die("object %s changed size!?", oid_to_hex(oid));
 
-		batch_write(opt, contents, size);
+		write_or_die(1, contents, size);
 		free(contents);
 	}
 }
@@ -309,11 +300,11 @@ static void batch_object_write(const char *obj_name,
 	strbuf_reset(scratch);
 	strbuf_expand(scratch, opt->format.format, expand_format, data);
 	strbuf_addch(scratch, '\n');
-	batch_write(opt, scratch->buf, scratch->len);
+	write_or_die(1, scratch->buf, scratch->len);
 
 	if (opt->print_contents) {
 		print_object_or_die(opt, data);
-		batch_write(opt, "\n", 1);
+		write_or_die(1, "\n", 1);
 	}
 }
 

--
https://github.com/git/git/pull/568

[PATCH RFC 16/20] for-each-ref: tests for new atom %(raw) added

[Olga Telezhnaya]

Add tests for new formatting atom %(raw).
We need this atom for cat-file command.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 t/t6300-for-each-ref.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index fb361369a037c..6a5626d537f35 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -328,6 +328,12 @@ test_expect_success 'Check format %(rest) gives empty output ' '
 	test_cmp expected actual
 '
 
+test_expect_success 'Check format %(raw) gives empty output ' '
+	echo >expected &&
+	git for-each-ref --format="%(raw)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
 cat >expected <<\EOF
 refs/heads/master
 refs/remotes/origin/master

--
https://github.com/git/git/pull/568

[PATCH RFC 02/20] ref-filter: rename field in ref_array_item stuct

[Olga Telezhnaya]

Rename objectname field to oid in struct ref_array_item.
We usually use objectname word for string representation
of object id, so oid explains the content better.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/ls-remote.c | 2 +-
 ref-filter.c        | 8 ++++----
 ref-filter.h        | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 1d7f1f5ce2783..ce79aede726c7 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -143,7 +143,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		const struct ref_array_item *ref = ref_array.items[i];
 		if (show_symref_target && ref->symref)
 			printf("ref: %s\t%s\n", ref->symref, ref->refname);
-		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->oid), ref->refname);
 		status = 0; /* we found something */
 	}
 
diff --git a/ref-filter.c b/ref-filter.c
index 422a9c9ae3fd2..736e1f9cc38fc 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1615,7 +1615,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		} else if (!deref && grab_objectname(name, &ref->objectname, v, atom)) {
+		} else if (!deref && grab_objectname(name, &ref->oid, v, atom)) {
 			continue;
 		} else if (!strcmp(name, "HEAD")) {
 			if (atom->u.head && !strcmp(ref->refname, atom->u.head))
@@ -1661,7 +1661,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 		struct atom_value *v = &ref->value[i];
 		if (v->s == NULL && used_atom[i].source == SOURCE_NONE)
 			return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
-					       oid_to_hex(&ref->objectname), ref->refname);
+					       oid_to_hex(&ref->oid), ref->refname);
 	}
 
 	if (need_tagged)
@@ -1671,7 +1671,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 		return 0;
 
 
-	oi.oid = ref->objectname;
+	oi.oid = ref->oid;
 	if (get_object(ref, 0, &obj, &oi, err))
 		return -1;
 
@@ -1898,7 +1898,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	struct ref_array_item *ref;
 
 	FLEX_ALLOC_STR(ref, refname, refname);
-	oidcpy(&ref->objectname, oid);
+	oidcpy(&ref->oid, oid);
 
 	return ref;
 }
diff --git a/ref-filter.h b/ref-filter.h
index 85c8ebc3b904e..4d7d36e9f522d 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -34,7 +34,7 @@ struct ref_sorting {
 };
 
 struct ref_array_item {
-	struct object_id objectname;
+	struct object_id oid;
 	int flag;
 	unsigned int kind;
 	const char *symref;

--
https://github.com/git/git/pull/568

Re: [PATCH RFC 02/20] ref-filter: rename field in ref_array_item stuct

[Jeff King]

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Rename objectname field to oid in struct ref_array_item.
> We usually use objectname word for string representation
> of object id, so oid explains the content better.

OK. I suspect the original was selected to match the %(objectname)
placeholder. But I agree that "oid" is the more common variable name,
and I think the connection between the placeholder and the variable
should be pretty clear.

-Peff

Re: [PATCH RFC 01/20] cat-file: reuse struct ref_format

[Jeff King]

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Start using ref_format struct instead of simple char*.
> Need that for further reusing of formatting logic from ref-filter.

Makes sense.

>  struct batch_options {
> +	struct ref_format format;
>  	int enabled;
>  	int follow_symlinks;
>  	int print_contents;
> @@ -24,7 +26,6 @@ struct batch_options {
>  	int all_objects;
>  	int unordered;
>  	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
> -	const char *format;
>  };

Not a huge deal, but unless there's a compelling reason to move the
field around in the struct, the diff is easier to read if the deleted
and added lines stay in the same place.

> @@ -491,9 +492,6 @@ static int batch_objects(struct batch_options *opt)
>  	int save_warning;
>  	int retval = 0;
>  
> -	if (!opt->format)
> -		opt->format = "%(objectname) %(objecttype) %(objectsize)";
> -

This assignment moves down to cmd_cat_file(). I don't see any reason
that shouldn't work, but it makes reviewing easier if there aren't
unexpected changes (so if it doesn't need moved in the grand scheme of
things, leave it as it was; if it does, it should either come in its own
patch, or get a note in the commit message as to why it needed to move).

-Peff

Re: [PATCH RFC 0/20] cat-file: start using formatting logic from ref-filter

[Eric Sunshine]

On Fri, Feb 22, 2019 at 10:58 AM Olga Telezhnaya
<olyatelezhnaya@gmail.com> wrote:
> I also have a question about site https://git-scm.com/docs/
> I thought it is updated automatically based on Documentation folder in
> the project, but it is not true. I edited docs for for-each-ref in
> December, I still see my patch in master, but for-each-ref docs in
> git-csm is outdated. Is it OK?

If you look at https://git-scm.com/docs/git-for-each-ref, you'll find
a pop-up control at the top of the page which allows you to select
documentation for a particular release of Git (say, 2.19.1). Your
change to git-for-each-ref.txt may be in "master" but is not yet in
any official final release. It will be in 2.21.0, but that release is
still in the RC stage, thus doesn't appear at
https://git-scm.com/docs/git-for-each-ref.

Re: [PATCH RFC 0/20] cat-file: start using formatting logic from ref-filter

[Olga Telezhnaya]

пт, 22 февр. 2019 г. в 19:09, Eric Sunshine <sunshine@sunshineco.com>:
>
> On Fri, Feb 22, 2019 at 10:58 AM Olga Telezhnaya
> <olyatelezhnaya@gmail.com> wrote:
> > I also have a question about site https://git-scm.com/docs/
> > I thought it is updated automatically based on Documentation folder in
> > the project, but it is not true. I edited docs for for-each-ref in
> > December, I still see my patch in master, but for-each-ref docs in
> > git-csm is outdated. Is it OK?
>
> If you look at https://git-scm.com/docs/git-for-each-ref, you'll find
> a pop-up control at the top of the page which allows you to select
> documentation for a particular release of Git (say, 2.19.1). Your
> change to git-for-each-ref.txt may be in "master" but is not yet in
> any official final release. It will be in 2.21.0, but that release is
> still in the RC stage, thus doesn't appear at
> https://git-scm.com/docs/git-for-each-ref.

Oh, thank you, I missed that.

Re: [PATCH RFC 0/20] cat-file: start using formatting logic from ref-filter

[Jeff King]

On Fri, Feb 22, 2019 at 06:50:06PM +0300, Olga Telezhnaya wrote:

> It was a long way for me, I got older (by 1 year) and smarter
> (hopefully), and maybe I will finish my Outreachy Internship task for
> now. (I am doing it just for one year and a half, that's OK)

Welcome back!

Sorry to be a bit slow on the review. I've read through and commented on
patch 10. Some of my comments were "I'll have to see how this plays out
later in the series", so you may want to hold off on responding until I
read the rest. :)

> If serious:
> In this patch we remove cat-file formatting logic and reuse ref-filter
> logic there. As a positive side effect, cat-file now has many new
> formatting tokens (all from ref-filter formatting), including deref
> (like %(*objectsize:disk)). I have already tried to do this task one
> year ago, and it was bad attempt. I feel that today's patch is much
> better.

I'm still concerned that this is going to regress the performance of
cat-file noticeably without some big cleanups in ref-filter. Here are
timings on linux.git before and after your patches:

  [before]
  $ time git cat-file --unordered --batch-all-objects --batch-check >/dev/null
  real	0m16.602s
  user	0m15.545s
  sys	0m0.495s

  [after]
  $ time git cat-file --unordered --batch-all-objects --batch-check >/dev/null
  real	0m27.301s
  user	0m24.549s
  sys	0m2.752s

I don't think that's anything particularly wrong with your patches. It's
the existing strategy of ref-filter (in particular how it is very eager
to allocate lots of separate strings). And it may be too early to switch
cat-file over to it.

> I also have a question about site https://git-scm.com/docs/
> I thought it is updated automatically based on Documentation folder in
> the project, but it is not true. I edited docs for for-each-ref in
> December, I still see my patch in master, but for-each-ref docs in
> git-csm is outdated. Is it OK?

Yeah, as Eric noted, we only build docs for the tagged releases. In
theory it would be easy to just build the tip of master nightly, but the
data model for the site would need quite a bit of adjustment.

-Peff

Re: [PATCH RFC 0/20] cat-file: start using formatting logic from ref-filter

[Olga Telezhnaya]

пт, 1 мар. 2019 г. в 00:41, Jeff King <peff@peff.net>:
>
> On Fri, Feb 22, 2019 at 06:50:06PM +0300, Olga Telezhnaya wrote:
>
> > It was a long way for me, I got older (by 1 year) and smarter
> > (hopefully), and maybe I will finish my Outreachy Internship task for
> > now. (I am doing it just for one year and a half, that's OK)
>
> Welcome back!
>
> Sorry to be a bit slow on the review. I've read through and commented on
> patch 10. Some of my comments were "I'll have to see how this plays out
> later in the series", so you may want to hold off on responding until I
> read the rest. :)
>
> > If serious:
> > In this patch we remove cat-file formatting logic and reuse ref-filter
> > logic there. As a positive side effect, cat-file now has many new
> > formatting tokens (all from ref-filter formatting), including deref
> > (like %(*objectsize:disk)). I have already tried to do this task one
> > year ago, and it was bad attempt. I feel that today's patch is much
> > better.
>
> I'm still concerned that this is going to regress the performance of
> cat-file noticeably without some big cleanups in ref-filter. Here are
> timings on linux.git before and after your patches:
>
>   [before]
>   $ time git cat-file --unordered --batch-all-objects --batch-check >/dev/null
>   real  0m16.602s
>   user  0m15.545s
>   sys   0m0.495s
>
>   [after]
>   $ time git cat-file --unordered --batch-all-objects --batch-check >/dev/null
>   real  0m27.301s
>   user  0m24.549s
>   sys   0m2.752s
>
> I don't think that's anything particularly wrong with your patches. It's
> the existing strategy of ref-filter (in particular how it is very eager
> to allocate lots of separate strings). And it may be too early to switch
> cat-file over to it.

I have a guess that we need to add batch printing argument to our
general printing functions, that could make my version faster.

>
> > I also have a question about site https://git-scm.com/docs/
> > I thought it is updated automatically based on Documentation folder in
> > the project, but it is not true. I edited docs for for-each-ref in
> > December, I still see my patch in master, but for-each-ref docs in
> > git-csm is outdated. Is it OK?
>
> Yeah, as Eric noted, we only build docs for the tagged releases. In
> theory it would be easy to just build the tip of master nightly, but the
> data model for the site would need quite a bit of adjustment.
>
> -Peff

Re: [PATCH RFC 0/20] cat-file: start using formatting logic from ref-filter

[Jeff King]

On Fri, Feb 22, 2019 at 06:50:06PM +0300, Olga Telezhnaya wrote:

> In my opinion, it still has some issues. I mentioned all of them in
> TODOs in comments. All of them considered to be separate tasks for
> other patches. Some of them sound easy and could be great tasks for
> newbies.

One other thing I forgot to mention: your patches ended up on the list
in jumbled order. How do you send them? Usually `send-email` would add 1
second to the timestamp of each, so that threading mail readers sort
them as you'd expect (even if they arrive out of order due to the
vagaries of SMTP servers).

-Peff

Re: [PATCH RFC 0/20] cat-file: start using formatting logic from ref-filter

[Olga Telezhnaya]

пт, 1 мар. 2019 г. в 00:43, Jeff King <peff@peff.net>:
>
> On Fri, Feb 22, 2019 at 06:50:06PM +0300, Olga Telezhnaya wrote:
>
> > In my opinion, it still has some issues. I mentioned all of them in
> > TODOs in comments. All of them considered to be separate tasks for
> > other patches. Some of them sound easy and could be great tasks for
> > newbies.
>
> One other thing I forgot to mention: your patches ended up on the list
> in jumbled order. How do you send them? Usually `send-email` would add 1
> second to the timestamp of each, so that threading mail readers sort
> them as you'd expect (even if they arrive out of order due to the
> vagaries of SMTP servers).

Oh, that's one more bug in submitgit, I guess. I will not use it
anymore, OK, it's time to change the habits.

>
> -Peff

Re: [PATCH RFC 0/20] cat-file: start using formatting logic from ref-filter

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Feb 22, 2019 at 06:50:06PM +0300, Olga Telezhnaya wrote:
>
>> In my opinion, it still has some issues. I mentioned all of them in
>> TODOs in comments. All of them considered to be separate tasks for
>> other patches. Some of them sound easy and could be great tasks for
>> newbies.
>
> One other thing I forgot to mention: your patches ended up on the list
> in jumbled order. How do you send them? Usually `send-email` would add 1
> second to the timestamp of each, so that threading mail readers sort
> them as you'd expect (even if they arrive out of order due to the
> vagaries of SMTP servers).

Yes, the 1 second increment has served us so well in the entire life
of this project, and I am finding a bit irritating that we seem to
be seeing topics that are shown in jumbled order more often.  I'd love
to see why and get them fixed at the source eventually.