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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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