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

[Samuel Lijin <sxlijin@gmail.com>]

On Wed, Apr 18, 2018 at 8:55 PM, Samuel Lijin <sxlijin@gmail.com> wrote:
> Thanks for the quick review!
>
> On Wed, Apr 18, 2018 at 11:38 AM, Martin Ågren <martin.agren@gmail.com> wrote:
>> 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.
>
> That definitely works better. Will fix when I reroll.
>
>>>  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.
>
> I originally ordered it the way I did because in the constify-first
> scenario, "fix t7501" and "const-ify wt_longstatus_print_updated"
> seemed like two logically separate patches to me (which would have
> made the patch series three patches instead of two). I'm happy to
> reroll in whichever fashion if people care strongly though.
>
>> Martin