From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 30 Apr 2026 13:54:03 GMT
Subject: Re: [PATCH v3 3/5] name-rev: factor code for sharing with a new command
Message-ID: <8016697f-9eb7-4c75-be87-d9479186919c@gmail.com>
In-Reply-To: <V3_name-rev_factor.66d@msgid.xyz>

Hi Kristoffer

On 28/04/2026 23:25, kristofferhaugsbakk@fastmail.com wrote:
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
> 
> @@ -516,6 +534,7 @@ static void name_rev_line(char *p, struct name_ref_data *data)
>   
>   	for (p_start = p; *p; p++) {
>   #define ishex(x) (isdigit((x)) || ((x) >= 'a' && (x) <= 'f'))
> +	start:
>   		if (!ishex(*p)) {
>   			counter = 0;
>   		} else if (++counter == hexsz &&
> @@ -524,25 +543,32 @@ static void name_rev_line(char *p, struct name_ref_data *data)
>   			const char *name = NULL;
>   			char c = *(p + 1);
>   			int p_len = p - p_start + 1;
> +			struct object *o = NULL;
> +			int oid_ret = 1;
>   
>   			counter = 0;
>   
>   			*(p + 1) = 0;
> -			if (!repo_get_oid(the_repository, p - (hexsz - 1), &oid)) {
> -				struct object *o =
> -					lookup_object(the_repository, &oid);
> +			oid_ret = repo_get_oid(the_repository, p - (hexsz - 1), &oid);

It would be safer to restore *(p + 1) here rather that relying on each 
case block to do it.

			*(p + 1) = c;
> +
> +			switch (cmd->type) {
> +			case NAME_REV:
> +				if (!oid_ret)
> +					o = lookup_object(the_repository, &oid);
>   				if (o)
>   					name = get_rev_name(o, &buf);
> +				*(p + 1) = c;
> +				if (!name)
> +					goto start;

The pre-image uses "continue" which will increment p - why the change in 
behavior?

Thanks

Phillip

> +				if (cmd->u.name_only)
> +					printf("%.*s%s", p_len - hexsz, p_start, name);
> +				else
> +					printf("%.*s (%s)", p_len, p_start, name);
> +				break;
> +			default:
> +				BUG("uncovered case: %d", cmd->type);
>   			}
> -			*(p + 1) = c;
> -
> -			if (!name)
> -				continue;
>   
> -			if (data->name_only)
> -				printf("%.*s%s", p_len - hexsz, p_start, name);
> -			else
> -				printf("%.*s (%s)", p_len, p_start, name);
>   			p_start = p + 1;
>   		}
>   	}
> @@ -567,6 +593,7 @@ int cmd_name_rev(int argc,
>   #endif
>   	int all = 0, annotate_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
>   	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
> +	struct command cmd;
>   	struct option opts[] = {
>   		OPT_BOOL(0, "name-only", &data.name_only, N_("print only ref-based names (no object names)")),
>   		OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")),
> @@ -596,6 +623,7 @@ int cmd_name_rev(int argc,
>   	init_commit_rev_name(&rev_names);
>   	repo_config(the_repository, git_default_config, NULL);
>   	argc = parse_options(argc, argv, prefix, opts, name_rev_usage, 0);
> +	init_name_rev_command(&cmd, data.name_only);
>   
>   #ifndef WITH_BREAKING_CHANGES
>   	if (transform_stdin) {
> @@ -663,7 +691,7 @@ int cmd_name_rev(int argc,
>   
>   		while (strbuf_getline(&sb, stdin) != EOF) {
>   			strbuf_addch(&sb, '\n');
> -			name_rev_line(sb.buf, &data);
> +			name_rev_line(sb.buf, &cmd);
>   		}
>   		strbuf_release(&sb);
>   	} else if (all) {