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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 8, 2021, 22:21 UTC
Message-ID: <xmqqmtqwa1l4.fsf@gitster.g>
In-Reply-To: <patch-4.4-34750ab81cf-20210621T150651Z-avarab@gmail.com>

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

Show 42 quoted lines

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