Re: [PATCH v3 3/5] name-rev: factor code for sharing with a new command
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Apr 30, 2026, 13:54 UTC
- 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:
Show 24 quoted lines
> 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;
Show 10 quoted lines
> +
> + 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
Show 45 quoted lines
> + 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) {