[PATCH 0/4] pack-objects: use revision.c's --stdin parsing

[PATCH 0/4] pack-objects: use revision.c's --stdin parsing

[Ævar Arnfjörð Bjarmason]

The revision.c handles the --stdin flag given to "git rev-list", and
various other commands use it to take rev-list-like arguments.

The pack-objects.c builtin made use of the revevisio.c API, and did
mostly the same handling of stdin, with a couple of minor differences.

This series extends the revision.c API to be flexible enough to handle
the pack-objects.c case, and moves pack-objects.c over to that new
API. The missing piece was to allow the definition of a
"handle_stdin_line" callback. This new callback allows for either
partially or entirely consuming the stdin before handing it off (or
not) to the revision.c logic.

I'm planning on submitting another series on top of this that'll make
further use of this new API, but wanted to send this part as a
stand-alone series first.

Ævar Arnfjörð Bjarmason (4):
  upload-pack: run is_repository_shallow() before setup_revisions()
  revision.h: unify "disable_stdin" and "read_from_stdin"
  pack-objects.c: do stdin parsing via revision.c's API
  pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

 builtin/am.c           |  4 +--
 builtin/blame.c        |  2 +-
 builtin/diff-tree.c    |  2 +-
 builtin/pack-objects.c | 64 ++++++++++++++++++++----------------------
 builtin/rev-list.c     |  2 +-
 revision.c             | 35 ++++++++++++++++++++---
 revision.h             | 60 +++++++++++++++++++++++++++++++++++----
 sequencer.c            |  4 +--
 8 files changed, 123 insertions(+), 50 deletions(-)

-- 
2.32.0.rc3.434.gd8aed1f08a7

[PATCH 1/4] upload-pack: run is_repository_shallow() before setup_revisions()

[Ævar Arnfjörð Bjarmason]

Move the is_repository_shallow() added in b790e0f67cd (upload-pack:
send shallow info over stdin to pack-objects, 2014-03-11) to above
setup_revisions().

Running is_repository_shallow() before setup_revisions() doesn't
matter now, but in subsequent commits we'll make the code that
followed setup_revisions() happen inside a callback in that
function. This isolated change documents that re-arranging this part
of the code is OK in isolation.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index de00adbb9e..1fbaa34f91 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3757,11 +3757,12 @@ static void get_object_list(int ac, const char **av)
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
-	setup_revisions(ac, av, &revs, &s_r_opt);
 
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
 
+	setup_revisions(ac, av, &revs, &s_r_opt);
+
 	save_warning = warn_on_object_refname_ambiguity;
 	warn_on_object_refname_ambiguity = 0;
 
-- 
2.32.0.rc3.434.gd8aed1f08a7

[PATCH 2/4] revision.h: unify "disable_stdin" and "read_from_stdin"

[Ævar Arnfjörð Bjarmason]

In 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03) we
added the "disable_stdin" flag, and then much later in
a12cbe23ef7 (rev-list: make empty --stdin not an error, 2018-08-22) we
gained a "read_from_stdin" flag.

The interaction between these is more subtle than they might appear at
first sight, as noted in a12cbe23ef7. "read_stdin" is not the inverse
of "disable_stdin", rather we read stdin if we see the "--stdin"
option.

The "read" is intended to understood as "I read it", not "you should
read it". Let's avoid this confusion by using "consume" and "consumed"
instead, i.e. a word whose present and past tense isn't the same.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/am.c        |  4 ++--
 builtin/blame.c     |  2 +-
 builtin/diff-tree.c |  2 +-
 builtin/rev-list.c  |  2 +-
 revision.c          |  4 ++--
 revision.h          | 23 ++++++++++++++++++++---
 sequencer.c         |  4 ++--
 7 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81..3a6c8455b4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
 	repo_init_revisions(the_repository, &rev_info, NULL);
 	rev_info.diff = 1;
 	rev_info.abbrev = 0;
-	rev_info.disable_stdin = 1;
+	rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
 	rev_info.show_root_diff = 1;
 	rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev_info.no_commit_id = 1;
@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)
 	fp = xfopen(am_path(state, "patch"), "w");
 	repo_init_revisions(the_repository, &rev_info, NULL);
 	rev_info.diff = 1;
-	rev_info.disable_stdin = 1;
+	rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
 	rev_info.no_commit_id = 1;
 	rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev_info.diffopt.use_color = 0;
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9a..c9f66c58c4 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		argv[argc - 1] = "--";
 	}
 
-	revs.disable_stdin = 1;
+	revs.stdin_handling = REV_INFO_STDIN_IGNORE;
 	setup_revisions(argc, argv, &revs, NULL);
 	if (!revs.pending.nr && is_bare_repository()) {
 		struct commit *head_commit;
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index f33d30d57b..fc548ebe16 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 		die(_("index file corrupt"));
 	opt->abbrev = 0;
 	opt->diff = 1;
-	opt->disable_stdin = 1;
+	opt->stdin_handling = REV_INFO_STDIN_IGNORE;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.tweak = diff_tree_tweak_rev;
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 7677b1af5a..88bd9ef954 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -651,7 +651,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
 	     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
 	      !revs.pending.nr) &&
-	     !revs.rev_input_given && !revs.read_from_stdin) ||
+	     !revs.rev_input_given && !revs.consumed_stdin_per_option) ||
 	    revs.diff)
 		usage(rev_list_usage);
 
diff --git a/revision.c b/revision.c
index 8140561b6c..69b3812093 100644
--- a/revision.c
+++ b/revision.c
@@ -2741,11 +2741,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 
 			if (!strcmp(arg, "--stdin")) {
-				if (revs->disable_stdin) {
+				if (revs->stdin_handling == REV_INFO_STDIN_IGNORE) {
 					argv[left++] = arg;
 					continue;
 				}
-				if (revs->read_from_stdin++)
+				if (revs->consumed_stdin_per_option++)
 					die("--stdin given twice?");
 				read_revisions_from_stdin(revs, &prune_data);
 				continue;
diff --git a/revision.h b/revision.h
index 93aa012f51..03231f089f 100644
--- a/revision.h
+++ b/revision.h
@@ -86,6 +86,11 @@ struct rev_cmdline_info {
 struct oidset;
 struct topo_walk_info;
 
+enum rev_info_stdin {
+	REV_INFO_STDIN_CONSUME_ON_OPTION = 0,
+	REV_INFO_STDIN_IGNORE,
+};
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -114,9 +119,22 @@ struct rev_info {
 	int rev_input_given;
 
 	/*
-	 * Whether we read from stdin due to the --stdin option.
+	 * How should we handle seeing --stdin?
+	 *
+	 * Defaults to reading if we see it with
+	 * REV_INFO_STDIN_CONSUME_ON_OPTION.
+	 *
+	 * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided
+	 * --stdin option.
+	 */
+	enum rev_info_stdin stdin_handling;
+
+	/*
+	 * Did we read from stdin due to stdin_handling ==
+	 * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
+	 * option?
 	 */
-	int read_from_stdin;
+	int consumed_stdin_per_option;
 
 	/* topo-sort */
 	enum rev_sort_order sort_order;
@@ -216,7 +234,6 @@ struct rev_info {
 			date_mode_explicit:1,
 			preserve_subject:1,
 			encode_email_headers:1;
-	unsigned int	disable_stdin:1;
 	/* --show-linear-break */
 	unsigned int	track_linear:1,
 			track_first_time:1,
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38..4e73bd79d6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,
 	log_tree_opt.abbrev = 0;
 	log_tree_opt.diff = 1;
 	log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
-	log_tree_opt.disable_stdin = 1;
+	log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
 	log_tree_opt.no_commit_id = 1;
 	log_tree_opt.diffopt.file = fopen(buf.buf, "w");
 	log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,
 			log_tree_opt.diff = 1;
 			log_tree_opt.diffopt.output_format =
 				DIFF_FORMAT_DIFFSTAT;
-			log_tree_opt.disable_stdin = 1;
+			log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
 
 			if (read_oneliner(&buf, rebase_path_orig_head(), 0) &&
 			    !get_oid(buf.buf, &orig) &&
-- 
2.32.0.rc3.434.gd8aed1f08a7

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

[Ævar Arnfjörð Bjarmason]

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             | 31 ++++++++++++++++++++
 3 files changed, 91 insertions(+), 33 deletions(-)

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..878eb51032 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 03231f089f..9d977cd3cc 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,24 @@ 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 0 to continue revision.c's normal processing of the
+	 *   line (after possibly munging the provided strbuf).
+	 *
+	 * - Return 1 to indicate that the line is fully processed,
+         *   moving onto the next line (if any)
+	 *
+	 * - Return 2 to process no further lines.
+	 *
+	 * 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;
 
-- 
2.32.0.rc3.434.gd8aed1f08a7

[PATCH 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

[Ævar Arnfjörð Bjarmason]

Continue the work started in the preceding commit of porting
pack-objects.c over to the new handle_stdin_line callback pattern. The
common case for the users of this API is to do some of their own
parsing or munging, and then have handle_revision_arg() handle the
rest.

The existing users of the --stdin parsing always wanted a flag of "0"
to be passed to handle_revision_arg(), but pack-objects.c wants to set
custom flags. Let's support this common case by having a
"revarg_flags" member in the "rev_info" struct.

This allows us to return REV_INFO_STDIN_LINE_PROCESS in the new
get_object_list_handle_stdin_line() instead of
REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now
pass down the right flag for us.

This also means that we can make the handle_revision_arg() function
static. Now that the only external user of the API has been migrated
over to the callback mechanism nothing external to revision.c needs to
call handle_revision_arg() anymore.

That handle_revision_arg() function was made public in a combination
of 5d6f0935e6d (revision.c: allow injecting revision parameters after
setup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run
rev-list equivalent internally., 2006-09-04).

This change leaves the briefly-used in preceding commits
"void *stdin_line_priv" without any in-tree user, as
builtin/pack-objects.c could be ported over to our new "revarg_flags"
common case.

I'm leaving that "void *stdin_line_priv" in place anyway for two
reasons:

 1. It's a common pattern to allow such a "void *" to be used for
    callback data, so we might as well follow that pattern here in
    anticipation of a future in-tree user.

 2. I have patches for such an in-tree user already in a series
    that'll be submitted after this one.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 9 ++-------
 revision.c             | 4 ++--
 revision.h             | 6 ++++--
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index cbb01f2b2d..0ab2d10853 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3748,7 +3748,6 @@ 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;
 
@@ -3756,7 +3755,7 @@ static enum rev_info_stdin_line get_object_list_handle_stdin_line(
 		return REV_INFO_STDIN_LINE_BREAK;
 	if (*line == '-') {
 		if (!strcmp(line, "--not")) {
-			*flags ^= UNINTERESTING;
+			revs->revarg_flags ^= UNINTERESTING;
 			write_bitmap_index = 0;
 			return REV_INFO_STDIN_LINE_CONTINUE;
 		}
@@ -3770,9 +3769,7 @@ static enum rev_info_stdin_line get_object_list_handle_stdin_line(
 		}
 		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;
+	return REV_INFO_STDIN_LINE_PROCESS;
 }
 
 static void get_object_list(int ac, const char **av)
@@ -3781,7 +3778,6 @@ static void get_object_list(int ac, const char **av)
 	struct setup_revision_opt s_r_opt = {
 		.allow_exclude_promisor_objects = 1,
 	};
-	int flags = 0;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
@@ -3791,7 +3787,6 @@ static void get_object_list(int ac, const char **av)
 
 	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);
 
 	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
diff --git a/revision.c b/revision.c
index 878eb51032..8054a9d037 100644
--- a/revision.c
+++ b/revision.c
@@ -2089,7 +2089,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 	return 0;
 }
 
-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
 {
 	int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);
 	if (!ret)
@@ -2145,7 +2145,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 			}
 			die("options not supported in --stdin mode");
 		}
-		if (handle_revision_arg(sb.buf, revs, 0,
+		if (handle_revision_arg(sb.buf, revs, revs->revarg_flags,
 					REVARG_CANNOT_BE_FILENAME))
 			die("bad revision '%s'", sb.buf);
 	}
diff --git a/revision.h b/revision.h
index 9d977cd3cc..c55147e7f2 100644
--- a/revision.h
+++ b/revision.h
@@ -156,6 +156,9 @@ struct rev_info {
 	 * - Return 0 to continue revision.c's normal processing of the
 	 *   line (after possibly munging the provided strbuf).
 	 *
+	 *   Change "revarg_flags" to affect the subsequent handling
+	 *   in handle_revision_arg()
+	 *
 	 * - Return 1 to indicate that the line is fully processed,
          *   moving onto the next line (if any)
 	 *
@@ -165,6 +168,7 @@ struct rev_info {
 	 * around.
 	 */
 	rev_info_stdin_line_func handle_stdin_line;
+	int revarg_flags;
 	void *stdin_line_priv;
 
 	/* topo-sort */
@@ -422,8 +426,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 			const char * const usagestr[]);
 #define REVARG_CANNOT_BE_FILENAME 01
 #define REVARG_COMMITTISH 02
-int handle_revision_arg(const char *arg, struct rev_info *revs,
-			int flags, unsigned revarg_opt);
 
 /**
  * Reset the flags used by the revision walking api. You can use this to do
-- 
2.32.0.rc3.434.gd8aed1f08a7

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> @@ -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;
> -

OK, so this shows the primary value of the entire series---we
managed to lose handwritten loop around a logic that gets migrated
into a callback function that has been used to read revisions from
the standard input elsewhere in the system.

All four patches are cleanly explained and nicely done.

Thanks.

[PATCH v2 0/4] pack-objects: use revision.c's --stdin parsing

[Ævar Arnfjörð Bjarmason]

A re-roll of
https://lore.kernel.org/git/cover-0.4-0000000000-20210608T121008Z-avarab@gmail.com/

Fixes whitespace issues in v1, and uses the named enums I added
instead of their implicit values in the documentation. I also
conflated BREAK and CONTINUE in that same explanation.

Ævar Arnfjörð Bjarmason (4):
  upload-pack: run is_repository_shallow() before setup_revisions()
  revision.h: unify "disable_stdin" and "read_from_stdin"
  pack-objects.c: do stdin parsing via revision.c's API
  pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

 builtin/am.c           |  4 +--
 builtin/blame.c        |  2 +-
 builtin/diff-tree.c    |  2 +-
 builtin/pack-objects.c | 64 +++++++++++++++++++---------------------
 builtin/rev-list.c     |  2 +-
 revision.c             | 35 +++++++++++++++++++---
 revision.h             | 66 ++++++++++++++++++++++++++++++++++++++----
 sequencer.c            |  4 +--
 8 files changed, 129 insertions(+), 50 deletions(-)

Range-diff against v1:
1:  c56a302e09 = 1:  6a8b20a7cf upload-pack: run is_repository_shallow() before setup_revisions()
2:  002e0f1cf7 = 2:  d88b2c0410 revision.h: unify "disable_stdin" and "read_from_stdin"
3:  1a9eb26587 ! 3:  d433d7b24a pack-objects.c: do stdin parsing via revision.c's API
    @@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,
     +			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;
    @@ revision.h: struct rev_info {
     +	 * When reading from stdin (see "stdin_handling" above) define
     +	 * a handle_stdin_line function to consume the lines.
     +	 *
    -+	 * - Return 0 to continue revision.c's normal processing of the
    -+	 *   line (after possibly munging the provided strbuf).
    ++	 * - Return REV_INFO_STDIN_LINE_PROCESS to continue
    ++	 *   revision.c's normal processing of the line (after
    ++	 *   possibly munging the provided strbuf).
     +	 *
    -+	 * - Return 1 to indicate that the line is fully processed,
    -+         *   moving onto the next line (if any)
    ++	 * - 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 2 to process no further lines.
    ++	 * - 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.
4:  15a3a5d047 ! 4:  e59a06c314 pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
    @@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,
     
      ## revision.h ##
     @@ revision.h: struct rev_info {
    - 	 * - Return 0 to continue revision.c's normal processing of the
    - 	 *   line (after possibly munging the provided strbuf).
    + 	 *   revision.c's normal processing of the line (after
    + 	 *   possibly munging the provided strbuf).
      	 *
     +	 *   Change "revarg_flags" to affect the subsequent handling
     +	 *   in handle_revision_arg()
     +	 *
    - 	 * - Return 1 to indicate that the line is fully processed,
    -          *   moving onto the next line (if any)
    + 	 * - 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
    ++	 *
    ++	 * - Return REV_INFO_STDIN_LINE_BREAK to indicate that the
    + 	 *   line is fully processed, moving onto the next line (if
    + 	 *   any)
      	 *
     @@ revision.h: struct rev_info {
      	 * around.
-- 
2.32.0.571.gdba276db2c

[PATCH v2 1/4] upload-pack: run is_repository_shallow() before setup_revisions()

[Ævar Arnfjörð Bjarmason]

Move the is_repository_shallow() added in b790e0f67cd (upload-pack:
send shallow info over stdin to pack-objects, 2014-03-11) to above
setup_revisions().

Running is_repository_shallow() before setup_revisions() doesn't
matter now, but in subsequent commits we'll make the code that
followed setup_revisions() happen inside a callback in that
function. This isolated change documents that re-arranging this part
of the code is OK in isolation.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index de00adbb9e..1fbaa34f91 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3757,11 +3757,12 @@ static void get_object_list(int ac, const char **av)
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
-	setup_revisions(ac, av, &revs, &s_r_opt);
 
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
 
+	setup_revisions(ac, av, &revs, &s_r_opt);
+
 	save_warning = warn_on_object_refname_ambiguity;
 	warn_on_object_refname_ambiguity = 0;
 
-- 
2.32.0.571.gdba276db2c

[PATCH v2 2/4] revision.h: unify "disable_stdin" and "read_from_stdin"

[Ævar Arnfjörð Bjarmason]

In 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03) we
added the "disable_stdin" flag, and then much later in
a12cbe23ef7 (rev-list: make empty --stdin not an error, 2018-08-22) we
gained a "read_from_stdin" flag.

The interaction between these is more subtle than they might appear at
first sight, as noted in a12cbe23ef7. "read_stdin" is not the inverse
of "disable_stdin", rather we read stdin if we see the "--stdin"
option.

The "read" is intended to understood as "I read it", not "you should
read it". Let's avoid this confusion by using "consume" and "consumed"
instead, i.e. a word whose present and past tense isn't the same.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/am.c        |  4 ++--
 builtin/blame.c     |  2 +-
 builtin/diff-tree.c |  2 +-
 builtin/rev-list.c  |  2 +-
 revision.c          |  4 ++--
 revision.h          | 23 ++++++++++++++++++++---
 sequencer.c         |  4 ++--
 7 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81..3a6c8455b4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
 	repo_init_revisions(the_repository, &rev_info, NULL);
 	rev_info.diff = 1;
 	rev_info.abbrev = 0;
-	rev_info.disable_stdin = 1;
+	rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
 	rev_info.show_root_diff = 1;
 	rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev_info.no_commit_id = 1;
@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)
 	fp = xfopen(am_path(state, "patch"), "w");
 	repo_init_revisions(the_repository, &rev_info, NULL);
 	rev_info.diff = 1;
-	rev_info.disable_stdin = 1;
+	rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
 	rev_info.no_commit_id = 1;
 	rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev_info.diffopt.use_color = 0;
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9a..c9f66c58c4 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		argv[argc - 1] = "--";
 	}
 
-	revs.disable_stdin = 1;
+	revs.stdin_handling = REV_INFO_STDIN_IGNORE;
 	setup_revisions(argc, argv, &revs, NULL);
 	if (!revs.pending.nr && is_bare_repository()) {
 		struct commit *head_commit;
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index f33d30d57b..fc548ebe16 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 		die(_("index file corrupt"));
 	opt->abbrev = 0;
 	opt->diff = 1;
-	opt->disable_stdin = 1;
+	opt->stdin_handling = REV_INFO_STDIN_IGNORE;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.tweak = diff_tree_tweak_rev;
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 7677b1af5a..88bd9ef954 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -651,7 +651,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
 	     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
 	      !revs.pending.nr) &&
-	     !revs.rev_input_given && !revs.read_from_stdin) ||
+	     !revs.rev_input_given && !revs.consumed_stdin_per_option) ||
 	    revs.diff)
 		usage(rev_list_usage);
 
diff --git a/revision.c b/revision.c
index 8140561b6c..69b3812093 100644
--- a/revision.c
+++ b/revision.c
@@ -2741,11 +2741,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 
 			if (!strcmp(arg, "--stdin")) {
-				if (revs->disable_stdin) {
+				if (revs->stdin_handling == REV_INFO_STDIN_IGNORE) {
 					argv[left++] = arg;
 					continue;
 				}
-				if (revs->read_from_stdin++)
+				if (revs->consumed_stdin_per_option++)
 					die("--stdin given twice?");
 				read_revisions_from_stdin(revs, &prune_data);
 				continue;
diff --git a/revision.h b/revision.h
index 17698cb51a..475c8ed785 100644
--- a/revision.h
+++ b/revision.h
@@ -86,6 +86,11 @@ struct rev_cmdline_info {
 struct oidset;
 struct topo_walk_info;
 
+enum rev_info_stdin {
+	REV_INFO_STDIN_CONSUME_ON_OPTION = 0,
+	REV_INFO_STDIN_IGNORE,
+};
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -114,9 +119,22 @@ struct rev_info {
 	int rev_input_given;
 
 	/*
-	 * Whether we read from stdin due to the --stdin option.
+	 * How should we handle seeing --stdin?
+	 *
+	 * Defaults to reading if we see it with
+	 * REV_INFO_STDIN_CONSUME_ON_OPTION.
+	 *
+	 * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided
+	 * --stdin option.
+	 */
+	enum rev_info_stdin stdin_handling;
+
+	/*
+	 * Did we read from stdin due to stdin_handling ==
+	 * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
+	 * option?
 	 */
-	int read_from_stdin;
+	int consumed_stdin_per_option;
 
 	/* topo-sort */
 	enum rev_sort_order sort_order;
@@ -216,7 +234,6 @@ struct rev_info {
 			date_mode_explicit:1,
 			preserve_subject:1,
 			encode_email_headers:1;
-	unsigned int	disable_stdin:1;
 	/* --show-linear-break */
 	unsigned int	track_linear:1,
 			track_first_time:1,
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38..4e73bd79d6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,
 	log_tree_opt.abbrev = 0;
 	log_tree_opt.diff = 1;
 	log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
-	log_tree_opt.disable_stdin = 1;
+	log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
 	log_tree_opt.no_commit_id = 1;
 	log_tree_opt.diffopt.file = fopen(buf.buf, "w");
 	log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,
 			log_tree_opt.diff = 1;
 			log_tree_opt.diffopt.output_format =
 				DIFF_FORMAT_DIFFSTAT;
-			log_tree_opt.disable_stdin = 1;
+			log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
 
 			if (read_oneliner(&buf, rebase_path_orig_head(), 0) &&
 			    !get_oid(buf.buf, &orig) &&
-- 
2.32.0.571.gdba276db2c

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

[Ævar Arnfjörð Bjarmason]

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(-)

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;
 
-- 
2.32.0.571.gdba276db2c

[PATCH v2 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

[Ævar Arnfjörð Bjarmason]

Continue the work started in the preceding commit of porting
pack-objects.c over to the new handle_stdin_line callback pattern. The
common case for the users of this API is to do some of their own
parsing or munging, and then have handle_revision_arg() handle the
rest.

The existing users of the --stdin parsing always wanted a flag of "0"
to be passed to handle_revision_arg(), but pack-objects.c wants to set
custom flags. Let's support this common case by having a
"revarg_flags" member in the "rev_info" struct.

This allows us to return REV_INFO_STDIN_LINE_PROCESS in the new
get_object_list_handle_stdin_line() instead of
REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now
pass down the right flag for us.

This also means that we can make the handle_revision_arg() function
static. Now that the only external user of the API has been migrated
over to the callback mechanism nothing external to revision.c needs to
call handle_revision_arg() anymore.

That handle_revision_arg() function was made public in a combination
of 5d6f0935e6d (revision.c: allow injecting revision parameters after
setup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run
rev-list equivalent internally., 2006-09-04).

This change leaves the briefly-used in preceding commits
"void *stdin_line_priv" without any in-tree user, as
builtin/pack-objects.c could be ported over to our new "revarg_flags"
common case.

I'm leaving that "void *stdin_line_priv" in place anyway for two
reasons:

 1. It's a common pattern to allow such a "void *" to be used for
    callback data, so we might as well follow that pattern here in
    anticipation of a future in-tree user.

 2. I have patches for such an in-tree user already in a series
    that'll be submitted after this one.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 9 ++-------
 revision.c             | 4 ++--
 revision.h             | 8 ++++++--
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index cbb01f2b2d..0ab2d10853 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3748,7 +3748,6 @@ 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;
 
@@ -3756,7 +3755,7 @@ static enum rev_info_stdin_line get_object_list_handle_stdin_line(
 		return REV_INFO_STDIN_LINE_BREAK;
 	if (*line == '-') {
 		if (!strcmp(line, "--not")) {
-			*flags ^= UNINTERESTING;
+			revs->revarg_flags ^= UNINTERESTING;
 			write_bitmap_index = 0;
 			return REV_INFO_STDIN_LINE_CONTINUE;
 		}
@@ -3770,9 +3769,7 @@ static enum rev_info_stdin_line get_object_list_handle_stdin_line(
 		}
 		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;
+	return REV_INFO_STDIN_LINE_PROCESS;
 }
 
 static void get_object_list(int ac, const char **av)
@@ -3781,7 +3778,6 @@ static void get_object_list(int ac, const char **av)
 	struct setup_revision_opt s_r_opt = {
 		.allow_exclude_promisor_objects = 1,
 	};
-	int flags = 0;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
@@ -3791,7 +3787,6 @@ static void get_object_list(int ac, const char **av)
 
 	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);
 
 	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
diff --git a/revision.c b/revision.c
index 2ca2c38447..ad94a646f7 100644
--- a/revision.c
+++ b/revision.c
@@ -2089,7 +2089,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 	return 0;
 }
 
-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
 {
 	int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);
 	if (!ret)
@@ -2145,7 +2145,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 			}
 			die("options not supported in --stdin mode");
 		}
-		if (handle_revision_arg(sb.buf, revs, 0,
+		if (handle_revision_arg(sb.buf, revs, revs->revarg_flags,
 					REVARG_CANNOT_BE_FILENAME))
 			die("bad revision '%s'", sb.buf);
 	}
diff --git a/revision.h b/revision.h
index 5df1523a6c..c473567d82 100644
--- a/revision.h
+++ b/revision.h
@@ -157,11 +157,16 @@ struct rev_info {
 	 *   revision.c's normal processing of the line (after
 	 *   possibly munging the provided strbuf).
 	 *
+	 *   Change "revarg_flags" to affect the subsequent handling
+	 *   in handle_revision_arg()
+	 *
 	 * - 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
+	 *
+	 * - Return REV_INFO_STDIN_LINE_BREAK to indicate that the
 	 *   line is fully processed, moving onto the next line (if
 	 *   any)
 	 *
@@ -169,6 +174,7 @@ struct rev_info {
 	 * around.
 	 */
 	rev_info_stdin_line_func handle_stdin_line;
+	int revarg_flags;
 	void *stdin_line_priv;
 
 	/* topo-sort */
@@ -426,8 +432,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 			const char * const usagestr[]);
 #define REVARG_CANNOT_BE_FILENAME 01
 #define REVARG_COMMITTISH 02
-int handle_revision_arg(const char *arg, struct rev_info *revs,
-			int flags, unsigned revarg_opt);
 
 /**
  * Reset the flags used by the revision walking api. You can use this to do
-- 
2.32.0.571.gdba276db2c

Re: [PATCH v2 2/4] revision.h: unify "disable_stdin" and "read_from_stdin"

[Emily Shaffer]

On Thu, Jun 17, 2021 at 12:57:35PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> In 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03) we
> added the "disable_stdin" flag, and then much later in
> a12cbe23ef7 (rev-list: make empty --stdin not an error, 2018-08-22) we
> gained a "read_from_stdin" flag.
> 
> The interaction between these is more subtle than they might appear at
> first sight, as noted in a12cbe23ef7. "read_stdin" is not the inverse
> of "disable_stdin", rather we read stdin if we see the "--stdin"
> option.
> 
> The "read" is intended to understood as "I read it", not "you should
> read it". Let's avoid this confusion by using "consume" and "consumed"
> instead, i.e. a word whose present and past tense isn't the same.

Unfortunately, I still find your disambiguation text to be ambiguous...

> diff --git a/revision.c b/revision.c
> index 8140561b6c..69b3812093 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2741,11 +2741,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  			}
>  
>  			if (!strcmp(arg, "--stdin")) {
> -				if (revs->disable_stdin) {
> +				if (revs->stdin_handling == REV_INFO_STDIN_IGNORE) {
>  					argv[left++] = arg;
>  					continue;
>  				}
> -				if (revs->read_from_stdin++)
> +				if (revs->consumed_stdin_per_option++)
>  					die("--stdin given twice?");
>  				read_revisions_from_stdin(revs, &prune_data);
>  				continue;

Eeh. I know this is not logic introduced by your patch, BUT your patch
does change the semantics of the replacement to ->disable_stdin from a
bool to an enum. There is an implicit assumption here - "if
stdin_handling isn't REV_INFO_STDIN_IGNORE, then it must be
REV_INFO_CONSUME_ON_OPTION." That's true during this patch, but becomes
false in the next patch in this series, and it didn't look to me like
this section of code was changed accordingly.
[snip]
> +	/*
> +	 * Did we read from stdin due to stdin_handling ==
> +	 * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
> +	 * option?
>  	 */
> -	int read_from_stdin;
> +	int consumed_stdin_per_option;

And indeed, the comment here confirms the implicit assumption made in
the code. Based on this comment, I'd expect the implementation to
explicitly check that stdin_handling == CONSUME_ON_OPTION.

 - Emily

Re: [PATCH v2 2/4] revision.h: unify "disable_stdin" and "read_from_stdin"

[Jonathan Tan]

About the subject:

> revision.h: unify "disable_stdin" and "read_from_stdin"

Hmm...there is no unification, I think? Both of these remain distinct -
it's just that both were renamed and one was changed into an enum.

> @@ -114,9 +119,22 @@ struct rev_info {
>  	int rev_input_given;
>  
>  	/*
> -	 * Whether we read from stdin due to the --stdin option.
> +	 * How should we handle seeing --stdin?
> +	 *
> +	 * Defaults to reading if we see it with
> +	 * REV_INFO_STDIN_CONSUME_ON_OPTION.
> +	 *
> +	 * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided
> +	 * --stdin option.
> +	 */
> +	enum rev_info_stdin stdin_handling;

This was changed to an enum, because (looking at the next patches) we
want to add an entry to it. Maybe mention it here - at the very least,
this will help reviewers check that the addition of extra entries to the
enum in future commits will not negatively affect functionality
introduced in this commit.

> +	/*
> +	 * Did we read from stdin due to stdin_handling ==
> +	 * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
> +	 * option?
>  	 */
> -	int read_from_stdin;
> +	int consumed_stdin_per_option;

The usage of "per" here seems correct to me, but it's not the first
meaning that springs to mind. Could this just be called
"consumed_stdin"?

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

[Jonathan Tan]

> 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.

[PATCH v3 0/4] pack-objects: use revision.c's --stdin parsing

[Ævar Arnfjörð Bjarmason]

This v3 fixes issues noted by Emily Shaffer & Jonathan Tan on v2,
thanks both!

The REV_INFO_STDIN_LINE_BREAK is gone, it was just a result of some
WIP hacking, but I was too close to the code to notice. The "!len"
check in pack-objects.c was also redundant by moving to the revision.c
API. I have a parallel just-submitted series[1] that tests for that,
this version passes tests when combined with those more exhaustive
tests.

There were various other nits & suggestions that I either addressed by
changing the relevant code / documentation as suggested, or (e.g. in
the case of the "--not" parsing) explained in commit message(s) why it
made sense to leave those special-cases in place.

1. https://lore.kernel.org/git/cover-0.2-00000000000-20210621T145819Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (4):
  upload-pack: run is_repository_shallow() before setup_revisions()
  revision.h: refactor "disable_stdin" and "read_from_stdin"
  pack-objects.c: do stdin parsing via revision.c's API
  pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

 builtin/am.c           |  4 +--
 builtin/blame.c        |  2 +-
 builtin/diff-tree.c    |  2 +-
 builtin/pack-objects.c | 61 +++++++++++++++++++-----------------------
 builtin/rev-list.c     |  2 +-
 revision.c             | 38 +++++++++++++++++++++-----
 revision.h             | 59 ++++++++++++++++++++++++++++++++++++----
 sequencer.c            |  4 +--
 8 files changed, 119 insertions(+), 53 deletions(-)

Range-diff against v2:
1:  6a8b20a7cf3 = 1:  3840ac28e8d upload-pack: run is_repository_shallow() before setup_revisions()
2:  d88b2c04102 ! 2:  6f69644b808 revision.h: unify "disable_stdin" and "read_from_stdin"
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    revision.h: unify "disable_stdin" and "read_from_stdin"
    +    revision.h: refactor "disable_stdin" and "read_from_stdin"
     
    -    In 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03) we
    -    added the "disable_stdin" flag, and then much later in
    -    a12cbe23ef7 (rev-list: make empty --stdin not an error, 2018-08-22) we
    -    gained a "read_from_stdin" flag.
    +    Change the two "disable_stdin" and "read_from_stdin" flags to an enum,
    +    in preparation for a subsequent commit adding more flags.
     
         The interaction between these is more subtle than they might appear at
         first sight, as noted in a12cbe23ef7. "read_stdin" is not the inverse
    @@ Commit message
         read it". Let's avoid this confusion by using "consume" and "consumed"
         instead, i.e. a word whose present and past tense isn't the same.
     
    +    See 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03)
    +    where we added the "disable_stdin" flag, and a12cbe23ef7 (rev-list:
    +    make empty --stdin not an error, 2018-08-22) for the addition of the
    +    "read_from_stdin" flag.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/am.c ##
    @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr
      	     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
      	      !revs.pending.nr) &&
     -	     !revs.rev_input_given && !revs.read_from_stdin) ||
    -+	     !revs.rev_input_given && !revs.consumed_stdin_per_option) ||
    ++	     !revs.rev_input_given && !revs.consumed_stdin) ||
      	    revs.diff)
      		usage(rev_list_usage);
      
    @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *re
      
      			if (!strcmp(arg, "--stdin")) {
     -				if (revs->disable_stdin) {
    -+				if (revs->stdin_handling == REV_INFO_STDIN_IGNORE) {
    ++				switch (revs->stdin_handling) {
    ++				case REV_INFO_STDIN_IGNORE:
      					argv[left++] = arg;
      					continue;
    ++				case REV_INFO_STDIN_CONSUME_ON_OPTION:
    ++					if (revs->consumed_stdin++)
    ++						die("--stdin given twice?");
    ++					read_revisions_from_stdin(revs, &prune_data);
    ++					continue;
      				}
     -				if (revs->read_from_stdin++)
    -+				if (revs->consumed_stdin_per_option++)
    - 					die("--stdin given twice?");
    - 				read_revisions_from_stdin(revs, &prune_data);
    - 				continue;
    +-					die("--stdin given twice?");
    +-				read_revisions_from_stdin(revs, &prune_data);
    +-				continue;
    + 			}
    + 
    + 			if (!strcmp(arg, "--end-of-options")) {
     
      ## revision.h ##
     @@ revision.h: struct rev_cmdline_info {
    @@ revision.h: struct rev_info {
     -	 * Whether we read from stdin due to the --stdin option.
     +	 * How should we handle seeing --stdin?
     +	 *
    -+	 * Defaults to reading if we see it with
    -+	 * REV_INFO_STDIN_CONSUME_ON_OPTION.
    ++	 * Defaults to REV_INFO_STDIN_CONSUME_ON_OPTION where we'll
    ++	 * attempt to read it if we see the --stdin option.
     +	 *
    -+	 * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided
    -+	 * --stdin option.
    ++	 * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin
    ++	 * option.
     +	 */
     +	enum rev_info_stdin stdin_handling;
     +
    @@ revision.h: struct rev_info {
     +	 * option?
      	 */
     -	int read_from_stdin;
    -+	int consumed_stdin_per_option;
    ++	int consumed_stdin;
      
      	/* topo-sort */
      	enum rev_sort_order sort_order;
3:  d433d7b24a3 ! 3:  943b1b4c12a pack-objects.c: do stdin parsing via revision.c's API
    @@ Metadata
      ## Commit message ##
         pack-objects.c: do stdin parsing via revision.c's API
     
    -    Change the fgets(..., stdin) parsing in pack-objects.c to use a
    -    now-extended version of the rev_info stdin parsing API.
    +    Extend the rev_info stdin parsing API to support hooking into its
    +    read_revisions_from_stdin() function, and change the custom stdin
    +    parsing in pack-objects.c to use it.
     
    -    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).
    +    The pack-objects.c code 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
    +    rev-list an option to read revs from the standard input., 2006-09-05).
    +    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).
     
    @@ builtin/pack-objects.c: static void mark_bitmap_preferred_tips(void)
     +{
     +	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;
    @@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,
      			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;
     @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
    + 
    + 			if (!strcmp(arg, "--stdin")) {
    + 				switch (revs->stdin_handling) {
    ++				case REV_INFO_STDIN_ALWAYS_READ:
    + 				case REV_INFO_STDIN_IGNORE:
    + 					argv[left++] = arg;
    + 					continue;
    +@@ revision.c: 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).
    ++	 * We're asked to ALWAYS_READ from stdin, but no --stdin
    ++	 * option (or "consumed_stdin" would be set).
     +	 */
    -+	if (revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ &&
    -+	    !revs->consumed_stdin_per_option)
    ++	if (!revs->consumed_stdin &&
    ++	    revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)
     +		read_revisions_from_stdin(revs, &prune_data);
     +
      	if (prune_data.nr) {
    @@ revision.h: struct topo_walk_info;
      
     +enum rev_info_stdin_line {
     +	REV_INFO_STDIN_LINE_PROCESS,
    -+	REV_INFO_STDIN_LINE_BREAK,
     +	REV_INFO_STDIN_LINE_CONTINUE,
     +};
     +
    @@ revision.h: struct topo_walk_info;
      	struct commit_list *commits;
     @@ revision.h: struct rev_info {
      	 *
    - 	 * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided
    - 	 * --stdin option.
    + 	 * Can be set to REV_INFO_STDIN_IGNORE to ignore the --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.
    @@ revision.h: struct rev_info {
      
     @@ revision.h: struct rev_info {
      	 */
    - 	int consumed_stdin_per_option;
    + 	int consumed_stdin;
      
     +	/*
     +	 * When reading from stdin (see "stdin_handling" above) define
    @@ revision.h: struct rev_info {
     +	 *   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)
4:  e59a06c3148 ! 4:  34750ab81cf pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
    @@ Commit message
         REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now
         pass down the right flag for us.
     
    -    This also means that we can make the handle_revision_arg() function
    -    static. Now that the only external user of the API has been migrated
    -    over to the callback mechanism nothing external to revision.c needs to
    -    call handle_revision_arg() anymore.
    +    I considered making the "--not" parsing be another flag handled by the
    +    revision.c API itself, but since there's only one caller who wants
    +    this, and the "write_bitmap_index = 0" case is more specific than
    +    marking things UNINTERESTING I think it's better to handle it with a
    +    more general mechanism.
    +
    +    These changes means that we can make the handle_revision_arg()
    +    function static. Now that the only external user of the API has been
    +    migrated over to the callback mechanism nothing external to revision.c
    +    needs to call handle_revision_arg() anymore.
     
         That handle_revision_arg() function was made public in a combination
         of 5d6f0935e6d (revision.c: allow injecting revision parameters after
    @@ builtin/pack-objects.c: static void mark_bitmap_preferred_tips(void)
      {
     -	int *flags = stdin_line_priv;
      	char *line = line_sb->buf;
    - 	size_t len = line_sb->len;
      
    -@@ builtin/pack-objects.c: static enum rev_info_stdin_line get_object_list_handle_stdin_line(
    - 		return REV_INFO_STDIN_LINE_BREAK;
    - 	if (*line == '-') {
    - 		if (!strcmp(line, "--not")) {
    +-	if (*line == '-') {
    +-		if (!strcmp(line, "--not")) {
     -			*flags ^= UNINTERESTING;
    -+			revs->revarg_flags ^= UNINTERESTING;
    - 			write_bitmap_index = 0;
    - 			return REV_INFO_STDIN_LINE_CONTINUE;
    - 		}
    -@@ builtin/pack-objects.c: static enum rev_info_stdin_line get_object_list_handle_stdin_line(
    - 		}
    +-			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;
    +-		}
    ++	if (*line != '-')
    ++		return REV_INFO_STDIN_LINE_PROCESS;
    ++
    ++	if (!strcmp(line, "--not")) {
    ++		revs->revarg_flags ^= UNINTERESTING;
    ++		write_bitmap_index = 0;
    ++		return REV_INFO_STDIN_LINE_CONTINUE;
    ++	} else 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;
    ++	} else {
      		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;
    -+	return REV_INFO_STDIN_LINE_PROCESS;
      }
      
      static void get_object_list(int ac, const char **av)
    @@ revision.h: struct rev_info {
     +	 *   Change "revarg_flags" to affect the subsequent handling
     +	 *   in handle_revision_arg()
     +	 *
    - 	 * - 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
    -+	 *
    -+	 * - Return REV_INFO_STDIN_LINE_BREAK to indicate that the
      	 *   line is fully processed, moving onto the next line (if
      	 *   any)
    - 	 *
     @@ revision.h: struct rev_info {
      	 * around.
      	 */
-- 
2.32.0.599.g3967b4fa4ac

[PATCH v3 1/4] upload-pack: run is_repository_shallow() before setup_revisions()

[Ævar Arnfjörð Bjarmason]

Move the is_repository_shallow() added in b790e0f67cd (upload-pack:
send shallow info over stdin to pack-objects, 2014-03-11) to above
setup_revisions().

Running is_repository_shallow() before setup_revisions() doesn't
matter now, but in subsequent commits we'll make the code that
followed setup_revisions() happen inside a callback in that
function. This isolated change documents that re-arranging this part
of the code is OK in isolation.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 65579e09fe0..a4e2ae059c0 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3767,11 +3767,12 @@ static void get_object_list(int ac, const char **av)
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
-	setup_revisions(ac, av, &revs, &s_r_opt);
 
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
 
+	setup_revisions(ac, av, &revs, &s_r_opt);
+
 	save_warning = warn_on_object_refname_ambiguity;
 	warn_on_object_refname_ambiguity = 0;
 
-- 
2.32.0.599.g3967b4fa4ac

[PATCH v3 2/4] revision.h: refactor "disable_stdin" and "read_from_stdin"

[Ævar Arnfjörð Bjarmason]

Change the two "disable_stdin" and "read_from_stdin" flags to an enum,
in preparation for a subsequent commit adding more flags.

The interaction between these is more subtle than they might appear at
first sight, as noted in a12cbe23ef7. "read_stdin" is not the inverse
of "disable_stdin", rather we read stdin if we see the "--stdin"
option.

The "read" is intended to understood as "I read it", not "you should
read it". Let's avoid this confusion by using "consume" and "consumed"
instead, i.e. a word whose present and past tense isn't the same.

See 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03)
where we added the "disable_stdin" flag, and a12cbe23ef7 (rev-list:
make empty --stdin not an error, 2018-08-22) for the addition of the
"read_from_stdin" flag.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/am.c        |  4 ++--
 builtin/blame.c     |  2 +-
 builtin/diff-tree.c |  2 +-
 builtin/rev-list.c  |  2 +-
 revision.c          | 12 +++++++-----
 revision.h          | 23 ++++++++++++++++++++---
 sequencer.c         |  4 ++--
 7 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81b..3a6c8455b47 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
 	repo_init_revisions(the_repository, &rev_info, NULL);
 	rev_info.diff = 1;
 	rev_info.abbrev = 0;
-	rev_info.disable_stdin = 1;
+	rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
 	rev_info.show_root_diff = 1;
 	rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev_info.no_commit_id = 1;
@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)
 	fp = xfopen(am_path(state, "patch"), "w");
 	repo_init_revisions(the_repository, &rev_info, NULL);
 	rev_info.diff = 1;
-	rev_info.disable_stdin = 1;
+	rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
 	rev_info.no_commit_id = 1;
 	rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev_info.diffopt.use_color = 0;
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9af..c9f66c58c46 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		argv[argc - 1] = "--";
 	}
 
-	revs.disable_stdin = 1;
+	revs.stdin_handling = REV_INFO_STDIN_IGNORE;
 	setup_revisions(argc, argv, &revs, NULL);
 	if (!revs.pending.nr && is_bare_repository()) {
 		struct commit *head_commit;
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index f33d30d57bf..fc548ebe166 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 		die(_("index file corrupt"));
 	opt->abbrev = 0;
 	opt->diff = 1;
-	opt->disable_stdin = 1;
+	opt->stdin_handling = REV_INFO_STDIN_IGNORE;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.tweak = diff_tree_tweak_rev;
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 7677b1af5a4..524632ba328 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -651,7 +651,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
 	     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
 	      !revs.pending.nr) &&
-	     !revs.rev_input_given && !revs.read_from_stdin) ||
+	     !revs.rev_input_given && !revs.consumed_stdin) ||
 	    revs.diff)
 		usage(rev_list_usage);
 
diff --git a/revision.c b/revision.c
index 8140561b6c7..a65f9b89e99 100644
--- a/revision.c
+++ b/revision.c
@@ -2741,14 +2741,16 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 
 			if (!strcmp(arg, "--stdin")) {
-				if (revs->disable_stdin) {
+				switch (revs->stdin_handling) {
+				case REV_INFO_STDIN_IGNORE:
 					argv[left++] = arg;
 					continue;
+				case REV_INFO_STDIN_CONSUME_ON_OPTION:
+					if (revs->consumed_stdin++)
+						die("--stdin given twice?");
+					read_revisions_from_stdin(revs, &prune_data);
+					continue;
 				}
-				if (revs->read_from_stdin++)
-					die("--stdin given twice?");
-				read_revisions_from_stdin(revs, &prune_data);
-				continue;
 			}
 
 			if (!strcmp(arg, "--end-of-options")) {
diff --git a/revision.h b/revision.h
index 17698cb51ac..79a3421cd1f 100644
--- a/revision.h
+++ b/revision.h
@@ -86,6 +86,11 @@ struct rev_cmdline_info {
 struct oidset;
 struct topo_walk_info;
 
+enum rev_info_stdin {
+	REV_INFO_STDIN_CONSUME_ON_OPTION = 0,
+	REV_INFO_STDIN_IGNORE,
+};
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -114,9 +119,22 @@ struct rev_info {
 	int rev_input_given;
 
 	/*
-	 * Whether we read from stdin due to the --stdin option.
+	 * How should we handle seeing --stdin?
+	 *
+	 * Defaults to REV_INFO_STDIN_CONSUME_ON_OPTION where we'll
+	 * attempt to read it if we see the --stdin option.
+	 *
+	 * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin
+	 * option.
+	 */
+	enum rev_info_stdin stdin_handling;
+
+	/*
+	 * Did we read from stdin due to stdin_handling ==
+	 * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
+	 * option?
 	 */
-	int read_from_stdin;
+	int consumed_stdin;
 
 	/* topo-sort */
 	enum rev_sort_order sort_order;
@@ -216,7 +234,6 @@ struct rev_info {
 			date_mode_explicit:1,
 			preserve_subject:1,
 			encode_email_headers:1;
-	unsigned int	disable_stdin:1;
 	/* --show-linear-break */
 	unsigned int	track_linear:1,
 			track_first_time:1,
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..4e73bd79d69 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,
 	log_tree_opt.abbrev = 0;
 	log_tree_opt.diff = 1;
 	log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
-	log_tree_opt.disable_stdin = 1;
+	log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
 	log_tree_opt.no_commit_id = 1;
 	log_tree_opt.diffopt.file = fopen(buf.buf, "w");
 	log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,
 			log_tree_opt.diff = 1;
 			log_tree_opt.diffopt.output_format =
 				DIFF_FORMAT_DIFFSTAT;
-			log_tree_opt.disable_stdin = 1;
+			log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
 
 			if (read_oneliner(&buf, rebase_path_orig_head(), 0) &&
 			    !get_oid(buf.buf, &orig) &&
-- 
2.32.0.599.g3967b4fa4ac

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

[Ævar Arnfjörð Bjarmason]

Extend the rev_info stdin parsing API to support hooking into its
read_revisions_from_stdin() function, and change the custom stdin
parsing in pack-objects.c to use it.

The pack-objects.c code 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).
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 | 63 ++++++++++++++++++++----------------------
 revision.c             | 22 +++++++++++++++
 revision.h             | 30 ++++++++++++++++++++
 3 files changed, 82 insertions(+), 33 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a4e2ae059c0..f8cae6c305a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3755,15 +3755,40 @@ 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;
+
+	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;
@@ -3771,39 +3796,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 a65f9b89e99..f55b3a1caf4 100644
--- a/revision.c
+++ b/revision.c
@@ -2119,6 +2119,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 		int len = sb.len;
 		if (!len)
 			break;
+
+		if (revs->handle_stdin_line) {
+			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_CONTINUE:
+				continue;
+			}
+		}
+
 		if (sb.buf[0] == '-') {
 			if (len == 2 && sb.buf[1] == '-') {
 				seen_dashdash = 1;
@@ -2742,6 +2755,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 			if (!strcmp(arg, "--stdin")) {
 				switch (revs->stdin_handling) {
+				case REV_INFO_STDIN_ALWAYS_READ:
 				case REV_INFO_STDIN_IGNORE:
 					argv[left++] = arg;
 					continue;
@@ -2789,6 +2803,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		}
 	}
 
+	/*
+	 * We're asked to ALWAYS_READ from stdin, but no --stdin
+	 * option (or "consumed_stdin" would be set).
+	 */
+	if (!revs->consumed_stdin &&
+	    revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)
+		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 79a3421cd1f..ff7994bb13d 100644
--- a/revision.h
+++ b/revision.h
@@ -89,8 +89,17 @@ 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_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 +135,9 @@ struct rev_info {
 	 *
 	 * Can be set to REV_INFO_STDIN_IGNORE to ignore the --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 +148,24 @@ struct rev_info {
 	 */
 	int consumed_stdin;
 
+	/*
+	 * 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_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;
 
-- 
2.32.0.599.g3967b4fa4ac

[PATCH v3 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

[Ævar Arnfjörð Bjarmason]

Continue the work started in the preceding commit of porting
pack-objects.c over to the new handle_stdin_line callback pattern. The
common case for the users of this API is to do some of their own
parsing or munging, and then have handle_revision_arg() handle the
rest.

The existing users of the --stdin parsing always wanted a flag of "0"
to be passed to handle_revision_arg(), but pack-objects.c wants to set
custom flags. Let's support this common case by having a
"revarg_flags" member in the "rev_info" struct.

This allows us to return REV_INFO_STDIN_LINE_PROCESS in the new
get_object_list_handle_stdin_line() instead of
REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now
pass down the right flag for us.

I considered making the "--not" parsing be another flag handled by the
revision.c API itself, but since there's only one caller who wants
this, and the "write_bitmap_index = 0" case is more specific than
marking things UNINTERESTING I think it's better to handle it with a
more general mechanism.

These changes means that we can make the handle_revision_arg()
function static. Now that the only external user of the API has been
migrated over to the callback mechanism nothing external to revision.c
needs to call handle_revision_arg() anymore.

That handle_revision_arg() function was made public in a combination
of 5d6f0935e6d (revision.c: allow injecting revision parameters after
setup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run
rev-list equivalent internally., 2006-09-04).

This change leaves the briefly-used in preceding commits
"void *stdin_line_priv" without any in-tree user, as
builtin/pack-objects.c could be ported over to our new "revarg_flags"
common case.

I'm leaving that "void *stdin_line_priv" in place anyway for two
reasons:

 1. It's a common pattern to allow such a "void *" to be used for
    callback data, so we might as well follow that pattern here in
    anticipation of a future in-tree user.

 2. I have patches for such an in-tree user already in a series
    that'll be submitted after this one.

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

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f8cae6c305a..50f703e1795 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3758,28 +3758,25 @@ 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;
 
-	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;
-		}
+	if (*line != '-')
+		return REV_INFO_STDIN_LINE_PROCESS;
+
+	if (!strcmp(line, "--not")) {
+		revs->revarg_flags ^= UNINTERESTING;
+		write_bitmap_index = 0;
+		return REV_INFO_STDIN_LINE_CONTINUE;
+	} else 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;
+	} else {
 		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)
@@ -3788,7 +3785,6 @@ static void get_object_list(int ac, const char **av)
 	struct setup_revision_opt s_r_opt = {
 		.allow_exclude_promisor_objects = 1,
 	};
-	int flags = 0;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
@@ -3798,7 +3794,6 @@ static void get_object_list(int ac, const char **av)
 
 	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);
 
 	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
diff --git a/revision.c b/revision.c
index f55b3a1caf4..17121f40c0b 100644
--- a/revision.c
+++ b/revision.c
@@ -2089,7 +2089,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 	return 0;
 }
 
-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
 {
 	int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);
 	if (!ret)
@@ -2139,7 +2139,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 			}
 			die("options not supported in --stdin mode");
 		}
-		if (handle_revision_arg(sb.buf, revs, 0,
+		if (handle_revision_arg(sb.buf, revs, revs->revarg_flags,
 					REVARG_CANNOT_BE_FILENAME))
 			die("bad revision '%s'", sb.buf);
 	}
diff --git a/revision.h b/revision.h
index ff7994bb13d..2b96c05ced8 100644
--- a/revision.h
+++ b/revision.h
@@ -156,6 +156,9 @@ struct rev_info {
 	 *   revision.c's normal processing of the line (after
 	 *   possibly munging the provided strbuf).
 	 *
+	 *   Change "revarg_flags" to affect the subsequent handling
+	 *   in handle_revision_arg()
+	 *
 	 * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
 	 *   line is fully processed, moving onto the next line (if
 	 *   any)
@@ -164,6 +167,7 @@ struct rev_info {
 	 * around.
 	 */
 	rev_info_stdin_line_func handle_stdin_line;
+	int revarg_flags;
 	void *stdin_line_priv;
 
 	/* topo-sort */
@@ -421,8 +425,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 			const char * const usagestr[]);
 #define REVARG_CANNOT_BE_FILENAME 01
 #define REVARG_COMMITTISH 02
-int handle_revision_arg(const char *arg, struct rev_info *revs,
-			int flags, unsigned revarg_opt);
 
 /**
  * Reset the flags used by the revision walking api. You can use this to do
-- 
2.32.0.599.g3967b4fa4ac

Re: [PATCH v3 2/4] revision.h: refactor "disable_stdin" and "read_from_stdin"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the two "disable_stdin" and "read_from_stdin" flags to an enum,
> in preparation for a subsequent commit adding more flags.
>
> The interaction between these is more subtle than they might appear at
> first sight, as noted in a12cbe23ef7. "read_stdin" is not the inverse
> of "disable_stdin", rather we read stdin if we see the "--stdin"
> option.
>
> The "read" is intended to understood as "I read it", not "you should

"I've read it already" would have been easier to understand.

>  			if (!strcmp(arg, "--stdin")) {
> -				if (revs->disable_stdin) {
> +				switch (revs->stdin_handling) {
> +				case REV_INFO_STDIN_IGNORE:
>  					argv[left++] = arg;
>  					continue;
> +				case REV_INFO_STDIN_CONSUME_ON_OPTION:
> +					if (revs->consumed_stdin++)
> +						die("--stdin given twice?");
> +					read_revisions_from_stdin(revs, &prune_data);
> +					continue;
>  				}

More importantly, consumed_stdin will start at zero and if it is
found to be non-zero here, it is an immediate error, hence ...

> +	/*
> +	 * Did we read from stdin due to stdin_handling ==
> +	 * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
> +	 * option?
>  	 */
> -	int read_from_stdin;
> +	int consumed_stdin;

... it does not have to be a full integer.
Just a single-bit would do.

But none of the above is grave enough to require an update.

Re: [PATCH v3 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> @@ -3758,28 +3758,25 @@ 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;
>  
> -	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;
> -		}
> +	if (*line != '-')
> +		return REV_INFO_STDIN_LINE_PROCESS;
> +
> +	if (!strcmp(line, "--not")) {
> +		revs->revarg_flags ^= UNINTERESTING;
> +		write_bitmap_index = 0;
> +		return REV_INFO_STDIN_LINE_CONTINUE;
> +	} else 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;
> +	} else {
>  		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;
>  }

Now, this does make things slightly more modular (specifically, it
no longer is necessary for handle_revision_arg() to be visible
outside revision.c).

Having said that, it is not immediately obvious if the amount of
code churn by 3/4 and 4/4 is justified.  This still looks more like
"we can add a new API, so we added it" than "we needed a new API for
such and such reasons---now we can do this more easily then before".

It's not wrong per-se, but I am not all that impressed.  Sorry.

Re: [PATCH v3 3/4] pack-objects.c: do stdin parsing via revision.c's API

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Extend the rev_info stdin parsing API to support hooking into its
> read_revisions_from_stdin() function, and change the custom stdin
> parsing in pack-objects.c to use it.

This step, while it might not be introducing new bugs, does not look
anything more than just adding yet another new API for the sake of
adding one (especially, the number of lines added vs removed does
not look so impressive).

Perhaps we'll see why it makes sense in the next step?

>  builtin/pack-objects.c | 63 ++++++++++++++++++++----------------------
>  revision.c             | 22 +++++++++++++++
>  revision.h             | 30 ++++++++++++++++++++
>  3 files changed, 82 insertions(+), 33 deletions(-)

[PATCH v4 0/5] revision.[ch]: add --stdin parsing API, use in pack-objects

[Ævar Arnfjörð Bjarmason]

A re-roll of v3[1], this v4 fixes specific issues noted there, but
more importantly I've tried to address Junio's comments to the effect
of "what's the point of this churny thing?"" in [2].

As the new 3/5 explains what I really wanted this for is a new feature
for "git bundle create" that I have working locally, which allows it
to do:

    $ printf "e83c5163316f89bfbde7\trefs/heads/first-git-dot-git-commit\n" |
    git bundle create initial.bundle --stdin

As well as things like:

    $ git for-each-ref 'refs/remotes/origin' |
    sed 's!\trefs/remotes/origin/!\trefs/heads/!' |
    git bundle create origin.bundle --stdin

As elaborated on in 3/5 for that it's much better to have an API like
this, but since those changes also needed what I've got in
ab/bundle-updates and ab/bundle-doc (well, not that one really, but
the merge conflict would be painful otherwise) I split this series off
so I wouldn't end up with another 10-20 patch monster.

Junio: I hope that'll convence you to accept this, I can also submit
that "monster", but it'll be harder to read & review sinc so much of
it (e.g. this series, and doc refactoring) will be needed just to get
to the point of implementing that feature.

1. https://lore.kernel.org/git/cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com/
2. https://lore.kernel.org/git/xmqqfswoa1ku.fsf@gitster.g/

Ævar Arnfjörð Bjarmason (5):
  upload-pack: run is_repository_shallow() before setup_revisions()
  revision.h: refactor "disable_stdin" and "read_from_stdin"
  revision.[ch]: add a "handle_stdin_line" API
  pack-objects.c: do stdin parsing via revision.c's API
  pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

 builtin/am.c           |  4 +--
 builtin/blame.c        |  2 +-
 builtin/diff-tree.c    |  2 +-
 builtin/pack-objects.c | 61 +++++++++++++++++++-----------------------
 builtin/rev-list.c     |  2 +-
 revision.c             | 39 ++++++++++++++++++++++-----
 revision.h             | 59 ++++++++++++++++++++++++++++++++++++----
 sequencer.c            |  4 +--
 8 files changed, 120 insertions(+), 53 deletions(-)

Range-diff against v3:
1:  dd2efd1cfb9 < -:  ----------- pack-objects tests: cover blindspots in stdin handling
2:  a9702132385 < -:  ----------- pack-objects: fix segfault in --stdin-packs option
3:  3840ac28e8d = 1:  8b2872227c5 upload-pack: run is_repository_shallow() before setup_revisions()
4:  6f69644b808 ! 2:  630feb06650 revision.h: refactor "disable_stdin" and "read_from_stdin"
    @@ Commit message
         of "disable_stdin", rather we read stdin if we see the "--stdin"
         option.
     
    -    The "read" is intended to understood as "I read it", not "you should
    -    read it". Let's avoid this confusion by using "consume" and "consumed"
    -    instead, i.e. a word whose present and past tense isn't the same.
    +    The "read" is intended to understood as "I've read it already", not
    +    "you should read it". Let's avoid this confusion by using "consume"
    +    and "consumed" instead, i.e. a word whose present and past tense isn't
    +    the same.
     
         See 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03)
         where we added the "disable_stdin" flag, and a12cbe23ef7 (rev-list:
    @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *re
      					argv[left++] = arg;
      					continue;
     +				case REV_INFO_STDIN_CONSUME_ON_OPTION:
    -+					if (revs->consumed_stdin++)
    ++					if (revs->consumed_stdin)
     +						die("--stdin given twice?");
     +					read_revisions_from_stdin(revs, &prune_data);
    ++					revs->consumed_stdin = 1;
     +					continue;
      				}
     -				if (revs->read_from_stdin++)
    @@ revision.h: struct rev_cmdline_info {
      struct topo_walk_info;
      
     +enum rev_info_stdin {
    -+	REV_INFO_STDIN_CONSUME_ON_OPTION = 0,
    ++	REV_INFO_STDIN_CONSUME_ON_OPTION,
     +	REV_INFO_STDIN_IGNORE,
     +};
     +
    @@ revision.h: struct rev_info {
     +	 * option?
      	 */
     -	int read_from_stdin;
    -+	int consumed_stdin;
    ++	int consumed_stdin:1;
      
      	/* topo-sort */
      	enum rev_sort_order sort_order;
-:  ----------- > 3:  60ef49df3b8 revision.[ch]: add a "handle_stdin_line" API
5:  943b1b4c12a ! 4:  ebffeb5891f pack-objects.c: do stdin parsing via revision.c's API
    @@ Metadata
      ## Commit message ##
         pack-objects.c: do stdin parsing via revision.c's API
     
    -    Extend the rev_info stdin parsing API to support hooking into its
    -    read_revisions_from_stdin() function, and change the custom stdin
    -    parsing in pack-objects.c to use it.
    +    Use the new "handle_stdin_line" API in revision.c to parse stdin in
    +    pack-objects.c, instead of using custom pack-objects.c-specific code
    +    to do so.
    +
    +    This means that 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.
     
         The pack-objects.c code being refactored away here was first added in
         Linus's c323ac7d9c5 (git-pack-objects: create a packed object
    @@ Commit message
         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
    +    For now we've just made setup_revisions() 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.
     
    @@ builtin/pack-objects.c: static void get_object_list(int ac, const char **av)
      	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
      		return;
      
    -
    - ## revision.c ##
    -@@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,
    - 		int len = sb.len;
    - 		if (!len)
    - 			break;
    -+
    -+		if (revs->handle_stdin_line) {
    -+			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_CONTINUE:
    -+				continue;
    -+			}
    -+		}
    -+
    - 		if (sb.buf[0] == '-') {
    - 			if (len == 2 && sb.buf[1] == '-') {
    - 				seen_dashdash = 1;
    -@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
    - 
    - 			if (!strcmp(arg, "--stdin")) {
    - 				switch (revs->stdin_handling) {
    -+				case REV_INFO_STDIN_ALWAYS_READ:
    - 				case REV_INFO_STDIN_IGNORE:
    - 					argv[left++] = arg;
    - 					continue;
    -@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
    - 		}
    - 	}
    - 
    -+	/*
    -+	 * We're asked to ALWAYS_READ from stdin, but no --stdin
    -+	 * option (or "consumed_stdin" would be set).
    -+	 */
    -+	if (!revs->consumed_stdin &&
    -+	    revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)
    -+		read_revisions_from_stdin(revs, &prune_data);
    -+
    - 	if (prune_data.nr) {
    - 		/*
    - 		 * If we need to introduce the magic "a lone ':' means no
    -
    - ## revision.h ##
    -@@ revision.h: 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_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;
    -@@ revision.h: struct rev_info {
    - 	 *
    - 	 * Can be set to REV_INFO_STDIN_IGNORE to ignore the --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;
    - 
    -@@ revision.h: struct rev_info {
    - 	 */
    - 	int consumed_stdin;
    - 
    -+	/*
    -+	 * 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_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;
    - 
6:  34750ab81cf ! 5:  0c049aee67c pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
    @@ Commit message
             anticipation of a future in-tree user.
     
          2. I have patches for such an in-tree user already in a series
    -        that'll be submitted after this one.
    +        that'll be submitted after this one. See the reference to "git
    +        bundle create --stdin" in the commit that added the "handle_stdin_line"
    +        API.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
-- 
2.32.0.636.g43e71d69cff

[PATCH v4 1/5] upload-pack: run is_repository_shallow() before setup_revisions()

[Ævar Arnfjörð Bjarmason]

Move the is_repository_shallow() added in b790e0f67cd (upload-pack:
send shallow info over stdin to pack-objects, 2014-03-11) to above
setup_revisions().

Running is_repository_shallow() before setup_revisions() doesn't
matter now, but in subsequent commits we'll make the code that
followed setup_revisions() happen inside a callback in that
function. This isolated change documents that re-arranging this part
of the code is OK in isolation.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index df49f656b96..3639d0379ed 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3774,11 +3774,12 @@ static void get_object_list(int ac, const char **av)
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
-	setup_revisions(ac, av, &revs, &s_r_opt);
 
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
 
+	setup_revisions(ac, av, &revs, &s_r_opt);
+
 	save_warning = warn_on_object_refname_ambiguity;
 	warn_on_object_refname_ambiguity = 0;
 
-- 
2.32.0.636.g43e71d69cff

[PATCH v4 2/5] revision.h: refactor "disable_stdin" and "read_from_stdin"

[Ævar Arnfjörð Bjarmason]

Change the two "disable_stdin" and "read_from_stdin" flags to an enum,
in preparation for a subsequent commit adding more flags.

The interaction between these is more subtle than they might appear at
first sight, as noted in a12cbe23ef7. "read_stdin" is not the inverse
of "disable_stdin", rather we read stdin if we see the "--stdin"
option.

The "read" is intended to understood as "I've read it already", not
"you should read it". Let's avoid this confusion by using "consume"
and "consumed" instead, i.e. a word whose present and past tense isn't
the same.

See 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03)
where we added the "disable_stdin" flag, and a12cbe23ef7 (rev-list:
make empty --stdin not an error, 2018-08-22) for the addition of the
"read_from_stdin" flag.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/am.c        |  4 ++--
 builtin/blame.c     |  2 +-
 builtin/diff-tree.c |  2 +-
 builtin/rev-list.c  |  2 +-
 revision.c          | 13 ++++++++-----
 revision.h          | 23 ++++++++++++++++++++---
 sequencer.c         |  4 ++--
 7 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81b..3a6c8455b47 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
 	repo_init_revisions(the_repository, &rev_info, NULL);
 	rev_info.diff = 1;
 	rev_info.abbrev = 0;
-	rev_info.disable_stdin = 1;
+	rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
 	rev_info.show_root_diff = 1;
 	rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev_info.no_commit_id = 1;
@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)
 	fp = xfopen(am_path(state, "patch"), "w");
 	repo_init_revisions(the_repository, &rev_info, NULL);
 	rev_info.diff = 1;
-	rev_info.disable_stdin = 1;
+	rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
 	rev_info.no_commit_id = 1;
 	rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev_info.diffopt.use_color = 0;
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9af..c9f66c58c46 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		argv[argc - 1] = "--";
 	}
 
-	revs.disable_stdin = 1;
+	revs.stdin_handling = REV_INFO_STDIN_IGNORE;
 	setup_revisions(argc, argv, &revs, NULL);
 	if (!revs.pending.nr && is_bare_repository()) {
 		struct commit *head_commit;
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index f33d30d57bf..fc548ebe166 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 		die(_("index file corrupt"));
 	opt->abbrev = 0;
 	opt->diff = 1;
-	opt->disable_stdin = 1;
+	opt->stdin_handling = REV_INFO_STDIN_IGNORE;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.tweak = diff_tree_tweak_rev;
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 7677b1af5a4..524632ba328 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -651,7 +651,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
 	     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
 	      !revs.pending.nr) &&
-	     !revs.rev_input_given && !revs.read_from_stdin) ||
+	     !revs.rev_input_given && !revs.consumed_stdin) ||
 	    revs.diff)
 		usage(rev_list_usage);
 
diff --git a/revision.c b/revision.c
index cddd0542a65..f30b9d40af4 100644
--- a/revision.c
+++ b/revision.c
@@ -2742,14 +2742,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 
 			if (!strcmp(arg, "--stdin")) {
-				if (revs->disable_stdin) {
+				switch (revs->stdin_handling) {
+				case REV_INFO_STDIN_IGNORE:
 					argv[left++] = arg;
 					continue;
+				case REV_INFO_STDIN_CONSUME_ON_OPTION:
+					if (revs->consumed_stdin)
+						die("--stdin given twice?");
+					read_revisions_from_stdin(revs, &prune_data);
+					revs->consumed_stdin = 1;
+					continue;
 				}
-				if (revs->read_from_stdin++)
-					die("--stdin given twice?");
-				read_revisions_from_stdin(revs, &prune_data);
-				continue;
 			}
 
 			if (!strcmp(arg, "--end-of-options")) {
diff --git a/revision.h b/revision.h
index 5c5510d4220..a942b1250ad 100644
--- a/revision.h
+++ b/revision.h
@@ -86,6 +86,11 @@ struct rev_cmdline_info {
 struct oidset;
 struct topo_walk_info;
 
+enum rev_info_stdin {
+	REV_INFO_STDIN_CONSUME_ON_OPTION,
+	REV_INFO_STDIN_IGNORE,
+};
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -114,9 +119,22 @@ struct rev_info {
 	int rev_input_given;
 
 	/*
-	 * Whether we read from stdin due to the --stdin option.
+	 * How should we handle seeing --stdin?
+	 *
+	 * Defaults to REV_INFO_STDIN_CONSUME_ON_OPTION where we'll
+	 * attempt to read it if we see the --stdin option.
+	 *
+	 * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin
+	 * option.
+	 */
+	enum rev_info_stdin stdin_handling;
+
+	/*
+	 * Did we read from stdin due to stdin_handling ==
+	 * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
+	 * option?
 	 */
-	int read_from_stdin;
+	int consumed_stdin:1;
 
 	/* topo-sort */
 	enum rev_sort_order sort_order;
@@ -216,7 +234,6 @@ struct rev_info {
 			date_mode_explicit:1,
 			preserve_subject:1,
 			encode_email_headers:1;
-	unsigned int	disable_stdin:1;
 	/* --show-linear-break */
 	unsigned int	track_linear:1,
 			track_first_time:1,
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..4e73bd79d69 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,
 	log_tree_opt.abbrev = 0;
 	log_tree_opt.diff = 1;
 	log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
-	log_tree_opt.disable_stdin = 1;
+	log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
 	log_tree_opt.no_commit_id = 1;
 	log_tree_opt.diffopt.file = fopen(buf.buf, "w");
 	log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,
 			log_tree_opt.diff = 1;
 			log_tree_opt.diffopt.output_format =
 				DIFF_FORMAT_DIFFSTAT;
-			log_tree_opt.disable_stdin = 1;
+			log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
 
 			if (read_oneliner(&buf, rebase_path_orig_head(), 0) &&
 			    !get_oid(buf.buf, &orig) &&
-- 
2.32.0.636.g43e71d69cff

[PATCH v4 3/5] revision.[ch]: add a "handle_stdin_line" API

[Ævar Arnfjörð Bjarmason]

Extend the rev_info stdin parsing API to support hooking into its
read_revisions_from_stdin() function, in the next commit we'll change
the the custom stdin parsing in pack-objects.c to use it..

For that use-case adding API is barely justified. We'll be able to
make the handle_revision_arg() static in exchange for it, and we'll
avoid the duplicate dance around setting "save_warning" and
"warn_on_object_refname_ambiguity", but we could just continue to do
that ourselves in builtin/pack-objects.c

The real reason to add this is for a change not part of this
series. We'll soon teach "git bundle create" to accept
revision/refname pairs on stdin, and thus do away with the limitation
of it being impossible to create bundles with ref tips that don't
correspond to the state of the current repository. I.e. this will
work:

    $ printf "e83c5163316f89bfbde7\trefs/heads/first-git-dot-git-commit\n" |
    git bundle create initial.bundle --stdin

As well as things like:

    $ git for-each-ref 'refs/remotes/origin' |
    sed 's!\trefs/remotes/origin/!\trefs/heads/!' |
    git bundle create origin.bundle --stdin

In order to do that we'll need to modify the lines we consume on stdin
revision.c (which bundle.c uses already), and be assured that that
stripping extra bundle-specific data from them is the only change in
behavior.

That change will be much more complex if bundle.c needs to start doing
its own stdin parsing again outside of revision.c, it was recently
converted to use revision.c's parsing in 5bb0fd2cab5 (bundle:
arguments can be read from stdin, 2021-01-11)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 revision.c | 22 ++++++++++++++++++++++
 revision.h | 30 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/revision.c b/revision.c
index f30b9d40af4..557b7f113a9 100644
--- a/revision.c
+++ b/revision.c
@@ -2120,6 +2120,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 		int len = sb.len;
 		if (!len)
 			break;
+
+		if (revs->handle_stdin_line) {
+			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_CONTINUE:
+				continue;
+			}
+		}
+
 		if (sb.buf[0] == '-') {
 			if (len == 2 && sb.buf[1] == '-') {
 				seen_dashdash = 1;
@@ -2743,6 +2756,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 			if (!strcmp(arg, "--stdin")) {
 				switch (revs->stdin_handling) {
+				case REV_INFO_STDIN_ALWAYS_READ:
 				case REV_INFO_STDIN_IGNORE:
 					argv[left++] = arg;
 					continue;
@@ -2791,6 +2805,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		}
 	}
 
+	/*
+	 * We're asked to ALWAYS_READ from stdin, but no --stdin
+	 * option (or "consumed_stdin" would be set).
+	 */
+	if (!revs->consumed_stdin &&
+	    revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)
+		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 a942b1250ad..52241c84c5b 100644
--- a/revision.h
+++ b/revision.h
@@ -89,8 +89,17 @@ struct topo_walk_info;
 enum rev_info_stdin {
 	REV_INFO_STDIN_CONSUME_ON_OPTION,
 	REV_INFO_STDIN_IGNORE,
+	REV_INFO_STDIN_ALWAYS_READ,
 };
 
+enum rev_info_stdin_line {
+	REV_INFO_STDIN_LINE_PROCESS,
+	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 +135,9 @@ struct rev_info {
 	 *
 	 * Can be set to REV_INFO_STDIN_IGNORE to ignore the --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 +148,24 @@ struct rev_info {
 	 */
 	int consumed_stdin:1;
 
+	/*
+	 * 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_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;
 
-- 
2.32.0.636.g43e71d69cff

[PATCH v4 4/5] pack-objects.c: do stdin parsing via revision.c's API

[Ævar Arnfjörð Bjarmason]

Use the new "handle_stdin_line" API in revision.c to parse stdin in
pack-objects.c, instead of using custom pack-objects.c-specific code
to do so.

This means that 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.

The pack-objects.c code 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).
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).

For now we've just made setup_revisions() 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 | 63 ++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3639d0379ed..595adc89c12 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3762,15 +3762,40 @@ 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;
+
+	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;
@@ -3778,39 +3803,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;
 
-- 
2.32.0.636.g43e71d69cff

[PATCH v4 5/5] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

[Ævar Arnfjörð Bjarmason]

Continue the work started in the preceding commit of porting
pack-objects.c over to the new handle_stdin_line callback pattern. The
common case for the users of this API is to do some of their own
parsing or munging, and then have handle_revision_arg() handle the
rest.

The existing users of the --stdin parsing always wanted a flag of "0"
to be passed to handle_revision_arg(), but pack-objects.c wants to set
custom flags. Let's support this common case by having a
"revarg_flags" member in the "rev_info" struct.

This allows us to return REV_INFO_STDIN_LINE_PROCESS in the new
get_object_list_handle_stdin_line() instead of
REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now
pass down the right flag for us.

I considered making the "--not" parsing be another flag handled by the
revision.c API itself, but since there's only one caller who wants
this, and the "write_bitmap_index = 0" case is more specific than
marking things UNINTERESTING I think it's better to handle it with a
more general mechanism.

These changes means that we can make the handle_revision_arg()
function static. Now that the only external user of the API has been
migrated over to the callback mechanism nothing external to revision.c
needs to call handle_revision_arg() anymore.

That handle_revision_arg() function was made public in a combination
of 5d6f0935e6d (revision.c: allow injecting revision parameters after
setup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run
rev-list equivalent internally., 2006-09-04).

This change leaves the briefly-used in preceding commits
"void *stdin_line_priv" without any in-tree user, as
builtin/pack-objects.c could be ported over to our new "revarg_flags"
common case.

I'm leaving that "void *stdin_line_priv" in place anyway for two
reasons:

 1. It's a common pattern to allow such a "void *" to be used for
    callback data, so we might as well follow that pattern here in
    anticipation of a future in-tree user.

 2. I have patches for such an in-tree user already in a series
    that'll be submitted after this one. See the reference to "git
    bundle create --stdin" in the commit that added the "handle_stdin_line"
    API.

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

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 595adc89c12..9f1d7efeb61 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3765,28 +3765,25 @@ 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;
 
-	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;
-		}
+	if (*line != '-')
+		return REV_INFO_STDIN_LINE_PROCESS;
+
+	if (!strcmp(line, "--not")) {
+		revs->revarg_flags ^= UNINTERESTING;
+		write_bitmap_index = 0;
+		return REV_INFO_STDIN_LINE_CONTINUE;
+	} else 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;
+	} else {
 		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)
@@ -3795,7 +3792,6 @@ static void get_object_list(int ac, const char **av)
 	struct setup_revision_opt s_r_opt = {
 		.allow_exclude_promisor_objects = 1,
 	};
-	int flags = 0;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
@@ -3805,7 +3801,6 @@ static void get_object_list(int ac, const char **av)
 
 	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);
 
 	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
diff --git a/revision.c b/revision.c
index 557b7f113a9..f910b7882ae 100644
--- a/revision.c
+++ b/revision.c
@@ -2090,7 +2090,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 	return 0;
 }
 
-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
 {
 	int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);
 	if (!ret)
@@ -2140,7 +2140,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 			}
 			die("options not supported in --stdin mode");
 		}
-		if (handle_revision_arg(sb.buf, revs, 0,
+		if (handle_revision_arg(sb.buf, revs, revs->revarg_flags,
 					REVARG_CANNOT_BE_FILENAME))
 			die("bad revision '%s'", sb.buf);
 	}
diff --git a/revision.h b/revision.h
index 52241c84c5b..3da56333960 100644
--- a/revision.h
+++ b/revision.h
@@ -156,6 +156,9 @@ struct rev_info {
 	 *   revision.c's normal processing of the line (after
 	 *   possibly munging the provided strbuf).
 	 *
+	 *   Change "revarg_flags" to affect the subsequent handling
+	 *   in handle_revision_arg()
+	 *
 	 * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
 	 *   line is fully processed, moving onto the next line (if
 	 *   any)
@@ -164,6 +167,7 @@ struct rev_info {
 	 * around.
 	 */
 	rev_info_stdin_line_func handle_stdin_line;
+	int revarg_flags;
 	void *stdin_line_priv;
 
 	/* topo-sort */
@@ -422,8 +426,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 			const char * const usagestr[]);
 #define REVARG_CANNOT_BE_FILENAME 01
 #define REVARG_COMMITTISH 02
-int handle_revision_arg(const char *arg, struct rev_info *revs,
-			int flags, unsigned revarg_opt);
 
 /**
  * Reset the flags used by the revision walking api. You can use this to do
-- 
2.32.0.636.g43e71d69cff

Re: [PATCH v4 2/5] revision.h: refactor "disable_stdin" and "read_from_stdin"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +	/*
> +	 * Did we read from stdin due to stdin_handling ==
> +	 * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
> +	 * option?
>  	 */
> -	int read_from_stdin;
> +	int consumed_stdin:1;

Will make this into "unsigned int" while queuing, as 1-bit signed
bitfield would not make much sense.

[PATCH v5 0/5] add --stdin parsing API, use in pack-objects

[Ævar Arnfjörð Bjarmason]

A trivial re-roll of v4[1] on top of
gitster/bc/rev-list-without-commit-line, which it had a conflict
with. Also includes the s/int/unsigned int/ fixup that Junio applied
on the v4 he picked up.

https://lore.kernel.org/git/cover-0.5-00000000000-20210709T105850Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (5):
  upload-pack: run is_repository_shallow() before setup_revisions()
  revision.h: refactor "disable_stdin" and "read_from_stdin"
  revision.[ch]: add a "handle_stdin_line" API
  pack-objects.c: do stdin parsing via revision.c's API
  pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

 builtin/am.c           |  4 +--
 builtin/blame.c        |  2 +-
 builtin/diff-tree.c    |  2 +-
 builtin/pack-objects.c | 61 +++++++++++++++++++-----------------------
 builtin/rev-list.c     |  2 +-
 revision.c             | 39 ++++++++++++++++++++++-----
 revision.h             | 60 +++++++++++++++++++++++++++++++++++++----
 sequencer.c            |  4 +--
 8 files changed, 121 insertions(+), 53 deletions(-)

Range-diff against v4:
1:  f2cc9695306 = 1:  b0c7ec31ca9 upload-pack: run is_repository_shallow() before setup_revisions()
2:  50c3b81820b ! 2:  4e5e6620245 revision.h: refactor "disable_stdin" and "read_from_stdin"
    @@ revision.h: struct rev_info {
     +	 * option?
      	 */
     -	int read_from_stdin;
    -+	int consumed_stdin:1;
    ++	unsigned int consumed_stdin:1;
      
      	/* topo-sort */
      	enum rev_sort_order sort_order;
     @@ revision.h: struct rev_info {
    - 			date_mode_explicit:1,
      			preserve_subject:1,
    - 			encode_email_headers:1;
    + 			encode_email_headers:1,
    + 			include_header:1;
     -	unsigned int	disable_stdin:1;
    ++
      	/* --show-linear-break */
      	unsigned int	track_linear:1,
      			track_first_time:1,
3:  0339af8c39a ! 3:  e3d24bd6e8a revision.[ch]: add a "handle_stdin_line" API
    @@ revision.h: struct rev_info {
      
     @@ revision.h: struct rev_info {
      	 */
    - 	int consumed_stdin:1;
    + 	unsigned int consumed_stdin:1;
      
     +	/*
     +	 * When reading from stdin (see "stdin_handling" above) define
4:  d8080b7fd9c = 4:  4787490a90f pack-objects.c: do stdin parsing via revision.c's API
5:  32a172aa80a = 5:  a8b0976649a pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v5 1/5] upload-pack: run is_repository_shallow() before setup_revisions()

[Ævar Arnfjörð Bjarmason]

Move the is_repository_shallow() added in b790e0f67cd (upload-pack:
send shallow info over stdin to pack-objects, 2014-03-11) to above
setup_revisions().

Running is_repository_shallow() before setup_revisions() doesn't
matter now, but in subsequent commits we'll make the code that
followed setup_revisions() happen inside a callback in that
function. This isolated change documents that re-arranging this part
of the code is OK in isolation.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index de00adbb9e0..1fbaa34f91b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3757,11 +3757,12 @@ static void get_object_list(int ac, const char **av)
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
-	setup_revisions(ac, av, &revs, &s_r_opt);
 
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
 
+	setup_revisions(ac, av, &revs, &s_r_opt);
+
 	save_warning = warn_on_object_refname_ambiguity;
 	warn_on_object_refname_ambiguity = 0;
 
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v5 2/5] revision.h: refactor "disable_stdin" and "read_from_stdin"

[Ævar Arnfjörð Bjarmason]

Change the two "disable_stdin" and "read_from_stdin" flags to an enum,
in preparation for a subsequent commit adding more flags.

The interaction between these is more subtle than they might appear at
first sight, as noted in a12cbe23ef7. "read_stdin" is not the inverse
of "disable_stdin", rather we read stdin if we see the "--stdin"
option.

The "read" is intended to understood as "I've read it already", not
"you should read it". Let's avoid this confusion by using "consume"
and "consumed" instead, i.e. a word whose present and past tense isn't
the same.

See 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03)
where we added the "disable_stdin" flag, and a12cbe23ef7 (rev-list:
make empty --stdin not an error, 2018-08-22) for the addition of the
"read_from_stdin" flag.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/am.c        |  4 ++--
 builtin/blame.c     |  2 +-
 builtin/diff-tree.c |  2 +-
 builtin/rev-list.c  |  2 +-
 revision.c          | 13 ++++++++-----
 revision.h          | 24 +++++++++++++++++++++---
 sequencer.c         |  4 ++--
 7 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81b..3a6c8455b47 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
 	repo_init_revisions(the_repository, &rev_info, NULL);
 	rev_info.diff = 1;
 	rev_info.abbrev = 0;
-	rev_info.disable_stdin = 1;
+	rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
 	rev_info.show_root_diff = 1;
 	rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev_info.no_commit_id = 1;
@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)
 	fp = xfopen(am_path(state, "patch"), "w");
 	repo_init_revisions(the_repository, &rev_info, NULL);
 	rev_info.diff = 1;
-	rev_info.disable_stdin = 1;
+	rev_info.stdin_handling = REV_INFO_STDIN_IGNORE;
 	rev_info.no_commit_id = 1;
 	rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev_info.diffopt.use_color = 0;
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9af..c9f66c58c46 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		argv[argc - 1] = "--";
 	}
 
-	revs.disable_stdin = 1;
+	revs.stdin_handling = REV_INFO_STDIN_IGNORE;
 	setup_revisions(argc, argv, &revs, NULL);
 	if (!revs.pending.nr && is_bare_repository()) {
 		struct commit *head_commit;
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index f33d30d57bf..fc548ebe166 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 		die(_("index file corrupt"));
 	opt->abbrev = 0;
 	opt->diff = 1;
-	opt->disable_stdin = 1;
+	opt->stdin_handling = REV_INFO_STDIN_IGNORE;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.tweak = diff_tree_tweak_rev;
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 36cb909ebaa..f1778833401 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -666,7 +666,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
 	     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
 	      !revs.pending.nr) &&
-	     !revs.rev_input_given && !revs.read_from_stdin) ||
+	     !revs.rev_input_given && !revs.consumed_stdin) ||
 	    revs.diff)
 		usage(rev_list_usage);
 
diff --git a/revision.c b/revision.c
index 8140561b6c7..50909339a59 100644
--- a/revision.c
+++ b/revision.c
@@ -2741,14 +2741,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 
 			if (!strcmp(arg, "--stdin")) {
-				if (revs->disable_stdin) {
+				switch (revs->stdin_handling) {
+				case REV_INFO_STDIN_IGNORE:
 					argv[left++] = arg;
 					continue;
+				case REV_INFO_STDIN_CONSUME_ON_OPTION:
+					if (revs->consumed_stdin)
+						die("--stdin given twice?");
+					read_revisions_from_stdin(revs, &prune_data);
+					revs->consumed_stdin = 1;
+					continue;
 				}
-				if (revs->read_from_stdin++)
-					die("--stdin given twice?");
-				read_revisions_from_stdin(revs, &prune_data);
-				continue;
 			}
 
 			if (!strcmp(arg, "--end-of-options")) {
diff --git a/revision.h b/revision.h
index 7464434f600..99458cc0647 100644
--- a/revision.h
+++ b/revision.h
@@ -86,6 +86,11 @@ struct rev_cmdline_info {
 struct oidset;
 struct topo_walk_info;
 
+enum rev_info_stdin {
+	REV_INFO_STDIN_CONSUME_ON_OPTION,
+	REV_INFO_STDIN_IGNORE,
+};
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -114,9 +119,22 @@ struct rev_info {
 	int rev_input_given;
 
 	/*
-	 * Whether we read from stdin due to the --stdin option.
+	 * How should we handle seeing --stdin?
+	 *
+	 * Defaults to REV_INFO_STDIN_CONSUME_ON_OPTION where we'll
+	 * attempt to read it if we see the --stdin option.
+	 *
+	 * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin
+	 * option.
+	 */
+	enum rev_info_stdin stdin_handling;
+
+	/*
+	 * Did we read from stdin due to stdin_handling ==
+	 * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
+	 * option?
 	 */
-	int read_from_stdin;
+	unsigned int consumed_stdin:1;
 
 	/* topo-sort */
 	enum rev_sort_order sort_order;
@@ -217,7 +235,7 @@ struct rev_info {
 			preserve_subject:1,
 			encode_email_headers:1,
 			include_header:1;
-	unsigned int	disable_stdin:1;
+
 	/* --show-linear-break */
 	unsigned int	track_linear:1,
 			track_first_time:1,
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..4e73bd79d69 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,
 	log_tree_opt.abbrev = 0;
 	log_tree_opt.diff = 1;
 	log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
-	log_tree_opt.disable_stdin = 1;
+	log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
 	log_tree_opt.no_commit_id = 1;
 	log_tree_opt.diffopt.file = fopen(buf.buf, "w");
 	log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,
 			log_tree_opt.diff = 1;
 			log_tree_opt.diffopt.output_format =
 				DIFF_FORMAT_DIFFSTAT;
-			log_tree_opt.disable_stdin = 1;
+			log_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;
 
 			if (read_oneliner(&buf, rebase_path_orig_head(), 0) &&
 			    !get_oid(buf.buf, &orig) &&
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v5 3/5] revision.[ch]: add a "handle_stdin_line" API

[Ævar Arnfjörð Bjarmason]

Extend the rev_info stdin parsing API to support hooking into its
read_revisions_from_stdin() function, in the next commit we'll change
the the custom stdin parsing in pack-objects.c to use it..

For that use-case adding API is barely justified. We'll be able to
make the handle_revision_arg() static in exchange for it, and we'll
avoid the duplicate dance around setting "save_warning" and
"warn_on_object_refname_ambiguity", but we could just continue to do
that ourselves in builtin/pack-objects.c

The real reason to add this is for a change not part of this
series. We'll soon teach "git bundle create" to accept
revision/refname pairs on stdin, and thus do away with the limitation
of it being impossible to create bundles with ref tips that don't
correspond to the state of the current repository. I.e. this will
work:

    $ printf "e83c5163316f89bfbde7\trefs/heads/first-git-dot-git-commit\n" |
    git bundle create initial.bundle --stdin

As well as things like:

    $ git for-each-ref 'refs/remotes/origin' |
    sed 's!\trefs/remotes/origin/!\trefs/heads/!' |
    git bundle create origin.bundle --stdin

In order to do that we'll need to modify the lines we consume on stdin
revision.c (which bundle.c uses already), and be assured that that
stripping extra bundle-specific data from them is the only change in
behavior.

That change will be much more complex if bundle.c needs to start doing
its own stdin parsing again outside of revision.c, it was recently
converted to use revision.c's parsing in 5bb0fd2cab5 (bundle:
arguments can be read from stdin, 2021-01-11)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 revision.c | 22 ++++++++++++++++++++++
 revision.h | 30 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/revision.c b/revision.c
index 50909339a59..3f6ab834aff 100644
--- a/revision.c
+++ b/revision.c
@@ -2119,6 +2119,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 		int len = sb.len;
 		if (!len)
 			break;
+
+		if (revs->handle_stdin_line) {
+			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_CONTINUE:
+				continue;
+			}
+		}
+
 		if (sb.buf[0] == '-') {
 			if (len == 2 && sb.buf[1] == '-') {
 				seen_dashdash = 1;
@@ -2742,6 +2755,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 			if (!strcmp(arg, "--stdin")) {
 				switch (revs->stdin_handling) {
+				case REV_INFO_STDIN_ALWAYS_READ:
 				case REV_INFO_STDIN_IGNORE:
 					argv[left++] = arg;
 					continue;
@@ -2790,6 +2804,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		}
 	}
 
+	/*
+	 * We're asked to ALWAYS_READ from stdin, but no --stdin
+	 * option (or "consumed_stdin" would be set).
+	 */
+	if (!revs->consumed_stdin &&
+	    revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)
+		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 99458cc0647..644b7c8a217 100644
--- a/revision.h
+++ b/revision.h
@@ -89,8 +89,17 @@ struct topo_walk_info;
 enum rev_info_stdin {
 	REV_INFO_STDIN_CONSUME_ON_OPTION,
 	REV_INFO_STDIN_IGNORE,
+	REV_INFO_STDIN_ALWAYS_READ,
 };
 
+enum rev_info_stdin_line {
+	REV_INFO_STDIN_LINE_PROCESS,
+	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 +135,9 @@ struct rev_info {
 	 *
 	 * Can be set to REV_INFO_STDIN_IGNORE to ignore the --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 +148,24 @@ struct rev_info {
 	 */
 	unsigned int consumed_stdin:1;
 
+	/*
+	 * 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_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;
 
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v5 4/5] pack-objects.c: do stdin parsing via revision.c's API

[Ævar Arnfjörð Bjarmason]

Use the new "handle_stdin_line" API in revision.c to parse stdin in
pack-objects.c, instead of using custom pack-objects.c-specific code
to do so.

This means that 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.

The pack-objects.c code 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).
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).

For now we've just made setup_revisions() 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 | 63 ++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1fbaa34f91b..35d5247f85a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3745,15 +3745,40 @@ 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;
+
+	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 +3786,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;
 
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v5 5/5] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS

[Ævar Arnfjörð Bjarmason]

Continue the work started in the preceding commit of porting
pack-objects.c over to the new handle_stdin_line callback pattern. The
common case for the users of this API is to do some of their own
parsing or munging, and then have handle_revision_arg() handle the
rest.

The existing users of the --stdin parsing always wanted a flag of "0"
to be passed to handle_revision_arg(), but pack-objects.c wants to set
custom flags. Let's support this common case by having a
"revarg_flags" member in the "rev_info" struct.

This allows us to return REV_INFO_STDIN_LINE_PROCESS in the new
get_object_list_handle_stdin_line() instead of
REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now
pass down the right flag for us.

I considered making the "--not" parsing be another flag handled by the
revision.c API itself, but since there's only one caller who wants
this, and the "write_bitmap_index = 0" case is more specific than
marking things UNINTERESTING I think it's better to handle it with a
more general mechanism.

These changes means that we can make the handle_revision_arg()
function static. Now that the only external user of the API has been
migrated over to the callback mechanism nothing external to revision.c
needs to call handle_revision_arg() anymore.

That handle_revision_arg() function was made public in a combination
of 5d6f0935e6d (revision.c: allow injecting revision parameters after
setup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run
rev-list equivalent internally., 2006-09-04).

This change leaves the briefly-used in preceding commits
"void *stdin_line_priv" without any in-tree user, as
builtin/pack-objects.c could be ported over to our new "revarg_flags"
common case.

I'm leaving that "void *stdin_line_priv" in place anyway for two
reasons:

 1. It's a common pattern to allow such a "void *" to be used for
    callback data, so we might as well follow that pattern here in
    anticipation of a future in-tree user.

 2. I have patches for such an in-tree user already in a series
    that'll be submitted after this one. See the reference to "git
    bundle create --stdin" in the commit that added the "handle_stdin_line"
    API.

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

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 35d5247f85a..06a085a9a2a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3748,28 +3748,25 @@ 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;
 
-	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;
-		}
+	if (*line != '-')
+		return REV_INFO_STDIN_LINE_PROCESS;
+
+	if (!strcmp(line, "--not")) {
+		revs->revarg_flags ^= UNINTERESTING;
+		write_bitmap_index = 0;
+		return REV_INFO_STDIN_LINE_CONTINUE;
+	} else 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;
+	} else {
 		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)
@@ -3778,7 +3775,6 @@ static void get_object_list(int ac, const char **av)
 	struct setup_revision_opt s_r_opt = {
 		.allow_exclude_promisor_objects = 1,
 	};
-	int flags = 0;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
@@ -3788,7 +3784,6 @@ static void get_object_list(int ac, const char **av)
 
 	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);
 
 	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
diff --git a/revision.c b/revision.c
index 3f6ab834aff..4164a4fcd11 100644
--- a/revision.c
+++ b/revision.c
@@ -2089,7 +2089,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 	return 0;
 }
 
-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
 {
 	int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);
 	if (!ret)
@@ -2139,7 +2139,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 			}
 			die("options not supported in --stdin mode");
 		}
-		if (handle_revision_arg(sb.buf, revs, 0,
+		if (handle_revision_arg(sb.buf, revs, revs->revarg_flags,
 					REVARG_CANNOT_BE_FILENAME))
 			die("bad revision '%s'", sb.buf);
 	}
diff --git a/revision.h b/revision.h
index 644b7c8a217..fd7fdbe42e8 100644
--- a/revision.h
+++ b/revision.h
@@ -156,6 +156,9 @@ struct rev_info {
 	 *   revision.c's normal processing of the line (after
 	 *   possibly munging the provided strbuf).
 	 *
+	 *   Change "revarg_flags" to affect the subsequent handling
+	 *   in handle_revision_arg()
+	 *
 	 * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the
 	 *   line is fully processed, moving onto the next line (if
 	 *   any)
@@ -164,6 +167,7 @@ struct rev_info {
 	 * around.
 	 */
 	rev_info_stdin_line_func handle_stdin_line;
+	int revarg_flags;
 	void *stdin_line_priv;
 
 	/* topo-sort */
@@ -423,8 +427,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 			const char * const usagestr[]);
 #define REVARG_CANNOT_BE_FILENAME 01
 #define REVARG_COMMITTISH 02
-int handle_revision_arg(const char *arg, struct rev_info *revs,
-			int flags, unsigned revarg_opt);
 
 /**
  * Reset the flags used by the revision walking api. You can use this to do
-- 
2.32.0.956.g6b0c84ceda8