Re: [PATCH 2/2] status: improve rebase todo list parsing

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Elijah

Thanks for the review, all you suggestions look sensible to me, I'll 
send a re-roll.

Phillip

On 22/04/2026 01:32, Elijah Newren wrote:
> On Mon, Apr 20, 2026 at 8:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When there is rebase in progress "git status" displays the last couple
>> of completed and the next couple of pending commands from the todo
>> list. When it does this is tries to abbreviate the object ids of
> 
> is tries => it tries ?
> 
> [...]
>> @@ -1363,6 +1363,51 @@ static int split_commit_in_progress(struct wt_status *s)
>>          free(rebase_orig_head);
>>
>>          return split_in_progress;
>> +}
>> +
>> +static void abbrev_oid_in_line(struct repository *r,
>> +                              struct strbuf *line, char **pp)
>> +{
>> +       char *p = *pp;
>> +       char *end_of_object_name, saved;
>> +       const char *abbrev;
>> +       struct object_id oid;
>> +       bool have_oid;
> 
> I'll put "thinking out loud" text in square brackets below...
> 
>> +
>> +       p += strspn(p, " \t");
>> +       end_of_object_name = p + strcspn(p, " \t");
> 
> [Advances p after whitespace, marks the end of the object with the
> next whitespace after that.]
> 
>> +       /*
>> +        * The for "merge" and "reset" the object name may be a label or
> 
> The for => For ?
> 
>> +        * ref rather than a hex object id. Only abbreviate the object
>> +        * name if it is a hex object id.
>> +        */
>> +       for (const char *q = p; q < end_of_object_name; q++) {
>> +               if (!isxdigit(*q))
>> +                       goto out;
>> +       }
> 
> 
> 
>> +       saved = *end_of_object_name;
>> +       *end_of_object_name = '\0';
>> +       have_oid = !repo_get_oid(r, p, &oid);
>> +       *end_of_object_name = saved;
> 
> [Tries to resolve the token, doing NUL-termination and restore dance.]
> 
>> +       if (!have_oid)
>> +               goto out; /* object name was a label */
> 
> 
>> +       abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
>> +       if (!starts_with(p, abbrev))
>> +               goto out; /* object name was a refname containing only xdigits */
> 
> [Ensures what we have is an oid rather than a branch name that can be
> resolved to an oid]
> 
>> +       p += strlen(abbrev);
>> +       strbuf_remove(line, p - line->buf, end_of_object_name - p);
>> +       end_of_object_name = p;
> 
> [Splice out a bunch of characters in the middle?]
> 
>> +out:
>> +       *pp = end_of_object_name;
>> +}
> 
> I had a hard time following the logic in the function and trying to
> figure out what it was doing.  I went line by line but had no mental
> model to follow.  When I got to the comment that is now above
> format_todo_line(), I suddenly understood, but without it, all the
> code was hard to follow.  Maybe a small comment at the beginning of
> the function along the lines of
> 
>   /*
>    * If the whitespace-delimited token starting at or just after *pp is a
>    * full hex object id that resolves uniquely, rewrite it in place to
>    * its default abbreviation, shrinking `line` accordingly. On return
>    * *pp points one past the (possibly abbreviated) token. Leaves both
>    * `line` and *pp-advanced-past-the-token unchanged in all other cases
>    * (non-hex token, unresolvable, or a refname that happens to consist
>    * only of hex digits).
>    */
> 
> ?  (Assuming I'm understanding correctly, of course.)
> 
>> +
>> +static void skip_dash_c(char **pp) {
> 
> Move the brace to the next line?
> 
>> +       char *p = *pp;
>> +
>> +       p += strspn(p, " \t");
>> +       /* The (void) cast is required to silence -Wunused_value */
> 
> -Wunused_value => -Wunused-value ?
> 
>> +       (void)(skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p));
>> +       *pp = p;
>>   }
>>
>>   /*
>> @@ -1371,29 +1416,57 @@ static int split_commit_in_progress(struct wt_status *s)
>>    * into
>>    * "pick d6a2f03 some message"
>>    *
>> - * The function assumes that the line does not contain useless spaces
>> - * before or after the command.
>> + * Returns false on comment lines, true otherwise
>>    */
>> -static void abbrev_oid_in_line(struct repository *r, struct strbuf *line)
>> +static bool format_todo_line(struct repository *r, struct strbuf *line)
>>   {
>> -       struct string_list split = STRING_LIST_INIT_DUP;
>> -       struct object_id oid;
>> -
>> -       if (starts_with(line->buf, "exec ") ||
>> -           starts_with(line->buf, "x ") ||
>> -           starts_with(line->buf, "label ") ||
>> -           starts_with(line->buf, "l "))
>> -               return;
>> -
>> -       if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
>> -           !repo_get_oid(r, split.items[1].string, &oid)) {
>> -               strbuf_reset(line);
>> -               strbuf_addf(line, "%s ", split.items[0].string);
>> -               strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
>> -               for (size_t i = 2; i < split.nr; i++)
>> -                       strbuf_addf(line, " %s", split.items[i].string);
>> +       enum todo_command cmd;
>> +       char *p = line->buf;
>> +
>> +       if (!sequencer_parse_todo_command((const char**)&p, &cmd))
>> +               return true; /* keep invalid lines */
>> +
>> +       switch (cmd) {
>> +       case TODO_COMMENT:
>> +               return false;
>> +
>> +       case TODO_MERGE:
>> +               skip_dash_c(&p);
>> +               while (true) {
>> +                       p += strspn(p, " \t");
>> +                       if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
>> +                               break;
>> +                       abbrev_oid_in_line(r, line, &p);
>> +               }
>> +               break;
>> +
>> +       case TODO_FIXUP:
>> +               skip_dash_c(&p);
>> +               /* fallthrough */
>> +       case TODO_DROP:
>> +       case TODO_EDIT:
>> +       case TODO_PICK:
>> +       case TODO_RESET:
>> +       case TODO_REVERT:
>> +       case TODO_REWORD:
>> +       case TODO_SQUASH:
>> +               abbrev_oid_in_line(r, line, &p);
>> +               break;
>> +
>> +       /*
>> +        * Avoid "default" and instead list all the other commands so
>> +        * that -Wswitch warns if a new command is added without handling
>> +        * it in this function.
>> +        */
> 
> Nice. :-)
> 
>> +       case TODO_BREAK:
>> +       case TODO_EXEC:
>> +       case TODO_LABEL:
>> +       case TODO_NOOP:
>> +       case TODO_UPDATE_REF:
>> +               break;
>>          }
>> -       string_list_clear(&split, 0);
>> +
>> +       return true;
>>   }
>>
>>   static int read_rebase_todolist(struct repository *r, const char *fname, struct string_list *lines)
>> @@ -1411,13 +1484,9 @@ static int read_rebase_todolist(struct repository *r, const char *fname, struct
>>                            repo_git_path_replace(r, &buf, "%s", fname));
>>          }
>>          while (!strbuf_getline_lf(&buf, f)) {
>> -               if (starts_with(buf.buf, comment_line_str))
>> -                       continue;
>>                  strbuf_trim(&buf);
>> -               if (!buf.len)
>> -                       continue;
>> -               abbrev_oid_in_line(r, &buf);
>> -               string_list_append(lines, buf.buf);
>> +               if (format_todo_line(r, &buf))
>> +                       string_list_append(lines, buf.buf);
>>          }
>>          fclose(f);
>>
>> --
>> 2.54.0.rc1.174.gd833f386ac5.dirty
> 
> Other than the minor comments above, this looks like a nice cleanup.