Hi,
Thanks for the feedback. I'll be sending a patch with the updates shortly!
On 12/06/17 11:35 AM, Junio C Hamano wrote:
> liam Beguin <liambeguin@xxxxxxxxx> writes:
>
>> +static int stash_count_refs(struct object_id *ooid, struct object_id *noid,
>> + const char *email, timestamp_t timestamp, int tz,
>> + const char *message, void *cb_data)
>> +{
>> + int *c = cb_data;
>> + (*c)++;
>> + return 0;
>> +}
>
> Count up, and tell the caller to keep going by returning 0. That
> sounds sane.
>
>> +static void wt_longstatus_print_stash_summary(struct wt_status *s)
>> +{
>> + int stash_count = 0;
>> +
>> + for_each_reflog_ent("refs/stash", stash_count_refs, &stash_count);
>
> And do so with a counter initialized to 0. Also sane.
>
>> + if (stash_count > 0)
>> + status_printf_ln(s, GIT_COLOR_NORMAL,
>> + Q_("Your stash currently has %d commit",
>> + "Your stash currently has %d commits", stash_count),
>> + stash_count);
>
> Conceptually, the contents of the stash are *not* commits, even
> though the implementation happens to use a commit to represent each
> stash entry. Perhaps "has %d entry/entries" is an improvement, but
> a quick scanning of an early part of "git stash --help" tells me
> that
what's different between a stash and a commit?
>
> You have 1 stash / You have 4 stashes
>
> would be the best, as the documentation calls each entry "a stash".
> E.g. "list" is explained to list "the stashes", and "show <stash>"
> is explained to show the changes recorded in "the stash".
>
>> +}
>> +
>> static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncommitted)
>> {
>> struct child_process sm_summary = CHILD_PROCESS_INIT;
>> @@ -1536,6 +1557,7 @@ static void wt_longstatus_print(struct wt_status *s)
>> const char *branch_color = color(WT_STATUS_ONBRANCH, s);
>> const char *branch_status_color = color(WT_STATUS_HEADER, s);
>> struct wt_status_state state;
>> + int show_stash = 0;
>>
>> memset(&state, 0, sizeof(state));
>> wt_status_get_state(&state,
>> @@ -1641,6 +1663,8 @@ static void wt_longstatus_print(struct wt_status *s)
>> } else
>> printf(_("nothing to commit, working tree clean\n"));
>> }
>> + if (!git_config_get_bool("status.showStash", &show_stash) && show_stash)
>> + wt_longstatus_print_stash_summary(s);
>> }
>
> Try to get "status.showstash" as a boolean, and only when it
> succeeds and the value is true, give this extra info (i.e. when the
> variable does not exist, do not complain and do not show). Sounds
> sensible.
>
> Overall the logic looks good to me; just the phrasing is
> questionable, relative to the existing documentation.
>
> Thanks.
>
Thanks,
- Liam