Re: [PATCH v3 4/4] wt-status.c: Set the committable flag in the collect phase.

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 7, 2018, 22:01 UTC
Message-ID: <xmqqworxufuv.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180906005329.11277-5-ischis2@cox.net>

"Stephen P. Smith" <ischis2@cox.net> writes:

Show 16 quoted lines

>  void wt_status_collect(struct wt_status *s)
>  {
> +	struct wt_status_state state;
>  	wt_status_collect_changes_worktree(s);
>  
>  	if (s->is_initial)
> @@ -746,6 +752,11 @@ void wt_status_collect(struct wt_status *s)
>  	else
>  		wt_status_collect_changes_index(s);
>  	wt_status_collect_untracked(s);
> +
> +	memset(&state, 0, sizeof(state));
> +	wt_status_get_state(&state, s->branch && !strcmp(s->branch, "HEAD"));
> +	if (state.merge_in_progress && !has_unmerged(s))
> +		s->committable = 1;
>  }

I do not think this is wrong per-se, but now we have three calls to wt_status_get_state() in wt-status.c, and it smells (at least to me) that each of these callsites does so only because their callers do not give them enough information, forcing them to find the state out for themselves.

Given that the ideal paradigm to come up with the "work tree status" is to do the collection followed by printing, and among three callers of get_state(), two appear in the "printing" side of the callchain [*1*], I wonder if it makes a better organization to

 - embed struct wt_status_state in struct wt_status

 - make the new call to wt_status_get_state() added above in this
   patch to populate the wt_status_state embedded in 's'

 - change the other two callers of wt_status_get_state() in
   wt_longstatus_print() and wt_porcelain_v2_print_tracking(), both
   of which will receive 's' that has been populated by a previous
   call to wt_status_collect(), so that they do *not* call
   get_state() themselves, but instead use the result recorded in
   wt_status_state embedded in 's', which was populated by
   wt_status_collect() before they got called.

That would bring the resulting code even closer to the ideal, i.e. the "collect" phase learns _everything_ we need about the current state that is necessary in order to later show to the user, and the "print" phase does not do its own separate discovery.

What do you think?

Thanks.

[Reference]

*1* Here are the selected functions and what other functions they
    call.

    wt_status_collect()

     -> wt_status_collect_changes_initial()
     -> wt_status_collect_changes_index()
     -> wt_status_collect_untracked()
     -> wt_status_get_state()

    wt_longstatus_print()

     -> wt_status_get_state()

    wt_porcelain_v2_print_tracking()

     -> wt_status_get_state()

    wt_status_print()

     -> wt_porcelain_v2_print()
        -> wt_porcelain_v2_print_tracking()
     -> wt_longstatus_print()

    run_status()

     -> wt_status_collect()
     -> wt_status_print()

    cmd_status()

     -> wt_status_collect()
     -> wt_status_print()

    prepare_to_commit(), dry_run_commit()

     -> run_status()

    Most notably, wt_status_collect() always happens before
    wt_status_print(), which is natural because the former is to
    collect information in 's' that is used by the latter to print.

    And in various functions wt_status_print() calls indirectly, the
    two other callers of wt_status_get_state() appear.