Re: [PATCH 2/8] ahead-behind: parse tip references

[Taylor Blau <me@ttaylorr.com>]

On Mon, Mar 06, 2023 at 02:06:32PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> Before implementing the logic to compute the ahead/behind counts, parse
> the unknown options as commits and place them in a string_list.
>
> Be sure to error out when the reference is not found.
>
> Co-authored-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>

This all looks reasonable (and forging my S-o-b here and elsewhere
throughout this series is all fine). I have seen most of this code
before at least in its final state, but the intermediate bits are new to
me.

And they all look fine and familiar, except...

> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  builtin/ahead-behind.c  | 39 +++++++++++++++++++++++++++++++++++++++
>  t/t4218-ahead-behind.sh | 10 ++++++++++
>  2 files changed, 49 insertions(+)
>
> diff --git a/builtin/ahead-behind.c b/builtin/ahead-behind.c
> index a56cc565def..c1212cc8d46 100644
> --- a/builtin/ahead-behind.c
> +++ b/builtin/ahead-behind.c
> @@ -1,16 +1,31 @@
>  #include "builtin.h"
>  #include "parse-options.h"
>  #include "config.h"
> +#include "commit.h"
>
>  static const char * const ahead_behind_usage[] = {
>  	N_("git ahead-behind --base=<ref> [ --stdin | <revs> ]"),
>  	NULL
>  };
>
> +static int handle_arg(struct string_list *tips, const char *arg)
> +{
> +	struct string_list_item *item;
> +	struct commit *c = lookup_commit_reference_by_name(arg);
> +
> +	if (!c)
> +		return error(_("could not resolve '%s'"), arg);
> +
> +	item = string_list_append(tips, arg);
> +	item->util = c;
> +	return 0;
> +}
> +
>  int cmd_ahead_behind(int argc, const char **argv, const char *prefix)
>  {
>  	const char *base_ref = NULL;
>  	int from_stdin = 0;
> +	struct string_list tips = STRING_LIST_INIT_DUP;
>
>  	struct option ahead_behind_opts[] = {
>  		OPT_STRING('b', "base", &base_ref, N_("base"), N_("base reference to process")),
> @@ -26,5 +41,29 @@ int cmd_ahead_behind(int argc, const char **argv, const char *prefix)
>
>  	git_config(git_default_config, NULL);
>
> +	if (from_stdin) {
> +		struct strbuf line = STRBUF_INIT;
> +
> +		while (strbuf_getline(&line, stdin) != EOF) {
> +			if (!line.len)
> +				break;
> +
> +			if (handle_arg(&tips, line.buf))
> +				return 1;
> +		}
> +
> +		strbuf_release(&line);
> +	} else {
> +		int i;
> +		for (i = 0; i < argc; ++i) {
> +			if (handle_arg(&tips, argv[i]))
> +				return 1;
> +		}
> +	}
> +
> +	/* Early return for no tips. */
> +	if (!tips.nr)
> +		return 0;
> +

...are we missing a call to `string_list_clear()` here on `&tips`?

Thanks,
Taylor