On Wed, Apr 18, 2018 at 8:55 PM, Samuel Lijin <sxlijin@xxxxxxxxx> wrote:
> Thanks for the quick review!
>
> On Wed, Apr 18, 2018 at 11:38 AM, Martin Ågren <martin.agren@xxxxxxxxx> wrote:
>> Hi Samuel,
>>
>> Welcome back. :-)
>>
>> On 18 April 2018 at 05:06, Samuel Lijin <sxlijin@xxxxxxxxx> 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
