diff mbox series

[v2,3/4] pack-objects.c: do stdin parsing via revision.c's API

Message ID patch-3.4-d433d7b24a-20210617T105537Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series pack-objects: use revision.c's --stdin parsing | expand

Commit Message

Ævar Arnfjörð Bjarmason June 17, 2021, 10:57 a.m. UTC
Change the fgets(..., stdin) parsing in pack-objects.c to use a
now-extended version of the rev_info stdin parsing API.

The fgets() loop being refactored away here was first added in Linus's
c323ac7d9c5 (git-pack-objects: create a packed object representation.,
2005-06-25).

Later on rev-list started doing similar parsing in 42cabc341c4 (Teach
rev-list an option to read revs from the standard input., 2006-09-05),
and that code was promoted to a more general API in 1fc561d169a (Move
read_revisions_from_stdin from builtin-rev-list.c to revision.c,
2008-07-05).

Since then the API in revision.c has received improvements that have
been missed here. E.g. the arbitrary limit of 1000 bytes was removed
in 63d564b3002 (read_revision_from_stdin(): use strbuf, 2009-11-20),
and it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:
read --stdin with strbuf_getline(), 2015-10-28).

Let's do the same here, as in 6e8d46f9d4b we can remove the "if (len
&& line[len - 1] == '\n')" check, it's now redundant to using
strbuf_getline(), and we get to skip the whole
"warn_on_object_refname_ambiguity" dance. The
read_revisions_from_stdin() function in revision.c we're now using
does it for us.

For now there isn't all that much point in this whole exercises. We
just end up calling setup_revisions() to loop over stdin for us, but
the callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS, we
still need to call handle_revision_arg() ourselves because we'd like
to call it with different flags.

This very light use of the API will be further refined in a subsequent
commit, for now we're just doing the bare minimum to move this
existing code over to the new callback pattern without any functional
changes, and making it as friendly to "git show -w" and "the
--color-moved-ws=allow-indentation-change" mode as possible.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 66 +++++++++++++++++++++---------------------
 revision.c             | 27 +++++++++++++++++
 revision.h             | 35 ++++++++++++++++++++++
 3 files changed, 95 insertions(+), 33 deletions(-)

Comments

Jonathan Tan June 18, 2021, 6:57 p.m. UTC | #1
> Change the fgets(..., stdin) parsing in pack-objects.c to use a
> now-extended version of the rev_info stdin parsing API.

This leads me to think that the stdin parsing API is already present,
but it turns out that it still needs to be extended. Maybe rewrite this
as "extend the rev_info stdin parsing API to support <whatever we need
to support>, and change the <...> in pack-objects.c to use it".

> @@ -136,6 +149,28 @@ struct rev_info {
>  	 */
>  	int consumed_stdin_per_option;
>  
> +	/*
> +	 * When reading from stdin (see "stdin_handling" above) define
> +	 * a handle_stdin_line function to consume the lines.
> +	 *
> +	 * - Return REV_INFO_STDIN_LINE_PROCESS to continue
> +	 *   revision.c's normal processing of the line (after
> +	 *   possibly munging the provided strbuf).
> +	 *
> +	 * - Return REV_INFO_STDIN_LINE_BREAK to process no further
> +	 *   lines, or anything further from the current line (just
> +	 *   like REV_INFO_STDIN_LINE_CONTINUE).
> +	 *
> +	 * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
> +	 *   line is fully processed, moving onto the next line (if
> +	 *   any)
> +	 *
> +	 * Use the "stdin_line_priv" to optionally pass your own data
> +	 * around.
> +	 */
> +	rev_info_stdin_line_func handle_stdin_line;
> +	void *stdin_line_priv;

I assume that all 3 of these are needed now to minimize the diff (which
is fine), but maybe comment on which of these are intended to stay and
which are only temporary. In particular, (once things are stabilized)
when would a caller need BREAK?

Also mention what happens if handle_stdin_line is not given (I presume
that it would just be like a no-op function that returns PROCESS every
time).

Looking at the bigger picture, though, and looking at the next patch,
this looks like a confusing API especially with the presence of "--not".
(In the next patch, you had to introduce a flags field, and the callback
has to remember to always handle "--not" and to toggle the flag.)
Perhaps the "--not" problem can be solved by making it revision.c's
concern and having the callback take a boolean parameter indicating
whether "--not" is active or not.
diff mbox series

Patch

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1fbaa34f91..cbb01f2b2d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3745,15 +3745,43 @@  static void mark_bitmap_preferred_tips(void)
 	}
 }
 
+static enum rev_info_stdin_line get_object_list_handle_stdin_line(
+	struct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)
+{
+	int *flags = stdin_line_priv;
+	char *line = line_sb->buf;
+	size_t len = line_sb->len;
+
+	if (!len)
+		return REV_INFO_STDIN_LINE_BREAK;
+	if (*line == '-') {
+		if (!strcmp(line, "--not")) {
+			*flags ^= UNINTERESTING;
+			write_bitmap_index = 0;
+			return REV_INFO_STDIN_LINE_CONTINUE;
+		}
+		if (starts_with(line, "--shallow ")) {
+			struct object_id oid;
+			if (get_oid_hex(line + 10, &oid))
+				die("not an object name '%s'", line + 10);
+			register_shallow(the_repository, &oid);
+			use_bitmap_index = 0;
+			return REV_INFO_STDIN_LINE_CONTINUE;
+		}
+		die(_("not a rev '%s'"), line);
+	}
+	if (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))
+			die(_("bad revision '%s'"), line);
+	return REV_INFO_STDIN_LINE_CONTINUE;
+}
+
 static void get_object_list(int ac, const char **av)
 {
 	struct rev_info revs;
 	struct setup_revision_opt s_r_opt = {
 		.allow_exclude_promisor_objects = 1,
 	};
-	char line[1000];
 	int flags = 0;
-	int save_warning;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
@@ -3761,39 +3789,11 @@  static void get_object_list(int ac, const char **av)
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
 
+	revs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;
+	revs.handle_stdin_line = get_object_list_handle_stdin_line;
+	revs.stdin_line_priv = &flags;
 	setup_revisions(ac, av, &revs, &s_r_opt);
 
-	save_warning = warn_on_object_refname_ambiguity;
-	warn_on_object_refname_ambiguity = 0;
-
-	while (fgets(line, sizeof(line), stdin) != NULL) {
-		int len = strlen(line);
-		if (len && line[len - 1] == '\n')
-			line[--len] = 0;
-		if (!len)
-			break;
-		if (*line == '-') {
-			if (!strcmp(line, "--not")) {
-				flags ^= UNINTERESTING;
-				write_bitmap_index = 0;
-				continue;
-			}
-			if (starts_with(line, "--shallow ")) {
-				struct object_id oid;
-				if (get_oid_hex(line + 10, &oid))
-					die("not an object name '%s'", line + 10);
-				register_shallow(the_repository, &oid);
-				use_bitmap_index = 0;
-				continue;
-			}
-			die(_("not a rev '%s'"), line);
-		}
-		if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
-			die(_("bad revision '%s'"), line);
-	}
-
-	warn_on_object_refname_ambiguity = save_warning;
-
 	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
 		return;
 
diff --git a/revision.c b/revision.c
index 69b3812093..2ca2c38447 100644
--- a/revision.c
+++ b/revision.c
@@ -2119,6 +2119,25 @@  static void read_revisions_from_stdin(struct rev_info *revs,
 		int len = sb.len;
 		if (!len)
 			break;
+
+		if (revs->handle_stdin_line) {
+			int do_break = 0;
+			enum rev_info_stdin_line ret = revs->handle_stdin_line(
+				revs, &sb, revs->stdin_line_priv);
+
+			switch (ret) {
+			case REV_INFO_STDIN_LINE_PROCESS:
+				break;
+			case REV_INFO_STDIN_LINE_BREAK:
+				do_break = 1;
+				break;
+			case REV_INFO_STDIN_LINE_CONTINUE:
+				continue;
+			}
+			if (do_break)
+				break;
+		}
+
 		if (sb.buf[0] == '-') {
 			if (len == 2 && sb.buf[1] == '-') {
 				seen_dashdash = 1;
@@ -2787,6 +2806,14 @@  int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		}
 	}
 
+	/*
+	 * We've got always_read_from_stdin but no --stdin (or
+	 * "consumed_stdin_per_option" would be set).
+	 */
+	if (revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ &&
+	    !revs->consumed_stdin_per_option)
+		read_revisions_from_stdin(revs, &prune_data);
+
 	if (prune_data.nr) {
 		/*
 		 * If we need to introduce the magic "a lone ':' means no
diff --git a/revision.h b/revision.h
index 475c8ed785..5df1523a6c 100644
--- a/revision.h
+++ b/revision.h
@@ -89,8 +89,18 @@  struct topo_walk_info;
 enum rev_info_stdin {
 	REV_INFO_STDIN_CONSUME_ON_OPTION = 0,
 	REV_INFO_STDIN_IGNORE,
+	REV_INFO_STDIN_ALWAYS_READ,
 };
 
+enum rev_info_stdin_line {
+	REV_INFO_STDIN_LINE_PROCESS,
+	REV_INFO_STDIN_LINE_BREAK,
+	REV_INFO_STDIN_LINE_CONTINUE,
+};
+
+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(
+	struct rev_info *revs, struct strbuf *line, void *stdin_line_priv);
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -126,6 +136,9 @@  struct rev_info {
 	 *
 	 * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided
 	 * --stdin option.
+	 *
+	 * Set it to REV_INFO_STDIN_ALWAYS_READ if there's always data
+	 * on stdin to be read, even if no --stdin option is provided.
 	 */
 	enum rev_info_stdin stdin_handling;
 
@@ -136,6 +149,28 @@  struct rev_info {
 	 */
 	int consumed_stdin_per_option;
 
+	/*
+	 * When reading from stdin (see "stdin_handling" above) define
+	 * a handle_stdin_line function to consume the lines.
+	 *
+	 * - Return REV_INFO_STDIN_LINE_PROCESS to continue
+	 *   revision.c's normal processing of the line (after
+	 *   possibly munging the provided strbuf).
+	 *
+	 * - Return REV_INFO_STDIN_LINE_BREAK to process no further
+	 *   lines, or anything further from the current line (just
+	 *   like REV_INFO_STDIN_LINE_CONTINUE).
+	 *
+	 * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
+	 *   line is fully processed, moving onto the next line (if
+	 *   any)
+	 *
+	 * Use the "stdin_line_priv" to optionally pass your own data
+	 * around.
+	 */
+	rev_info_stdin_line_func handle_stdin_line;
+	void *stdin_line_priv;
+
 	/* topo-sort */
 	enum rev_sort_order sort_order;