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

["Martin Ågren" <martin.agren@gmail.com>]

Hi Samuel,

Welcome back. :-)

On 18 April 2018 at 05:06, Samuel Lijin <sxlijin@gmail.com> wrote:
> 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.

> +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...

>  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.)

>  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.

>         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