Re: [PATCH 1/2] commit: fix --short and --porcelain

From: Martin Ågren <martin.agren@gmail.com>
Date: Apr 18, 2018, 18:38 UTC
Message-ID: <CAN0heSqXwVR5cdMwipUdPrnbUyCU8v2GzWK=2-0_ZWoWw3SO2w@mail.gmail.com>
In-Reply-To: <20180418030655.19378-2-sxlijin@gmail.com>

Hi Samuel,

Welcome back. :-)

On 18 April 2018 at 05:06, Samuel Lijin <sxlijin@gmail.com> wrote:

Show 7 quoted lines

> Make invoking `git commit` with `--short` or `--porcelain` return status
> code zero when there is something to commit.
>
> Mark the commitable flag in the wt_status object in the call to
> `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
> and simplify the logic in the latter function to take advantage of the
> logic shifted to the former.

The subject is sort of vague about what is being fixed. Maybe "commit: fix return code of ...", or "wt-status: set `commitable` when collecting, not when printing". Or something... I can't come up with something brilliant off the top of my head.

I did not understand the first paragraph until I had read the second and peaked at the code. Maybe tell the story the other way around? Something like this:

  Mark the `commitable` flag in the wt_status object in
  `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
  and simplify the logic in the latter function to take advantage of the
  logic shifted to the former.

  This means that callers do need to actually use the printer function
  to collect the `commitable` flag -- it is sufficient to call
  `wt_status_collect()`.

  As a result, invoking `git commit` with `--short` or `--porcelain`
  results in return status code zero when there is something to commit.
  This fixes two bugs documented in our test suite.

>  t/t7501-commit.sh |  4 ++--
>  wt-status.c       | 39 +++++++++++++++++++++++++++------------
>  2 files changed, 29 insertions(+), 14 deletions(-)

I tried to find somewhere in the documentation where this bug was described (git-commit.txt or git-status.txt), but failed. So there should be nothing to update there.

Show 12 quoted lines

> +static void wt_status_mark_commitable(struct wt_status *s) {
> +       int i;
> +
> +       for (i = 0; i < s->change.nr; i++) {
> +               struct wt_status_change_data *d = (s->change.items[i]).util;
> +
> +               if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) {
> +                       s->commitable = 1;
> +                       return;
> +               }
> +       }
> +}

This helper does exactly what the old code did inside `wt_longstatus_print_updated()` with regards to `commitable`. Ok.

This function does not reset `commitable` to 0, so reusing a `struct wt_status` won't necessarily work out. I have not thought about whether such a caller would be horribly broken for other reasons...

Show 12 quoted lines

>  void wt_status_collect(struct wt_status *s)
>  {
>         wt_status_collect_changes_worktree(s);
> @@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
>                 wt_status_collect_changes_initial(s);
>         else
>                 wt_status_collect_changes_index(s);
> +
>         wt_status_collect_untracked(s);
> +
> +       wt_status_mark_commitable(s);
>  }

So whenever we `..._collect()`, `commitable` is set for us. This is the only caller of the new helper, so in order to be able to trust `commitable`, one needs to call `wt_status_collect()`. Seems a reasonable assumption to make that the caller will remember to do so before printing. (And all current users do, so we're not regressing in some user.)

Show 10 quoted lines

>  static void wt_longstatus_print_unmerged(struct wt_status *s)
> @@ -754,26 +770,25 @@ static void wt_longstatus_print_unmerged(struct wt_status *s)
>
>  static void wt_longstatus_print_updated(struct wt_status *s)
>  {
> -       int shown_header = 0;
> -       int i;
> +       if (!s->commitable) {
> +               return;
> +       }

Regarding my comment above: If you forget to `..._collect()` first, this function is a no-op.

> +
> +       wt_longstatus_print_cached_header(s);
>
> +       int i;

You should leave this variable declaration at the top of the function.

Show 23 quoted lines

>         for (i = 0; i < s->change.nr; i++) {
>                 struct wt_status_change_data *d;
>                 struct string_list_item *it;
>                 it = &(s->change.items[i]);
>                 d = it->util;
> -               if (!d->index_status ||
> -                   d->index_status == DIFF_STATUS_UNMERGED)
> -                       continue;
> -               if (!shown_header) {
> -                       wt_longstatus_print_cached_header(s);
> -                       s->commitable = 1;
> -                       shown_header = 1;
> +               if (d->index_status &&
> +                   d->index_status != DIFF_STATUS_UNMERGED) {
> +                       wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
>                 }
> -               wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
>         }
> -       if (shown_header)
> -               wt_longstatus_print_trailer(s);
> +
> +       wt_longstatus_print_trailer(s);
>  }

This rewrite matches the original logic, assuming we can trust `commitable`. The result is a function called `print()` which does not modify the struct it is given for printing. Nice. So you can make the argument a `const struct wt_status *`. Except this function uses helpers that are missing the `const`.

You fix that in patch 2/2. I would probably have made that patch as 1/2, then done this patch as 2/2 ending the commit message with something like "As a result, we can mark the argument as `const`.", or even just silently inserting the `const` for this one function. Just a thought.

Martin