Re: [PATCH 1/2] wt-status: turn advice_status_hints into a field of wt_status

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 12, 2013, 09:44 UTC
Message-ID: <vpqeh8u1iip.fsf@anie.imag.fr>
In-Reply-To: <20130911183519.GA24251@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

> On Wed, Sep 11, 2013 at 11:08:58AM +0200, Matthieu Moy wrote:
>
>> No behavior change in this patch, but this makes the display of status
>> hints more flexible as they can be enabled or disabled for individual
>> calls to commit.c:run_status().
>> [...]
>> +static void status_finalize(struct wt_status *s)
>> +{
>> +	determine_whence(s);
>> +	s->hints = advice_status_hints;
>> +}
>
> Is a "finalize" the right place to put the status hint tweak? I would
> expect the order for callers to be:
>
>   wt_status_prepare(&s);
>   /* manual tweaks of fields in "s" */
>   wt_status_finalize(&s);
>
> but the finalize would overwrite any tweak of the hints field. So it
> would make more sense to me for it to go in prepare().

"finalize" is indeed not the right name. I made a separate function to avoid too much code duplication between status and commit, and looked for a name comlementary to "prepare" for a function that is ran later to fill-in some fields.

> The problem is that we are doing two things in that git_config call:
>
>   1. Loading basic git-wide core config.

(Yes. This includes the advice section, so I need it for advice_status_hints)

> So the "cleanest" thing would be something like:
>
>   git_config(git_diff_ui_config, NULL);
>   wt_status_prepare(&s);

[...]

That is clean, but a bit long and it is essentially duplicated between status and commit. I went another way: put all the similar code in a common function status_init_config:

static void status_init_config(struct wt_status *s, config_fn_t fn)
{
	wt_status_prepare(s);
	gitmodules_config();
	git_config(git_status_config, s);
	determine_whence(s);
	s->hints = advice_status_hints; /* must come after git_config() */
}

We could split the git_config call, but that would not bring much benefit IMHO. In any case, it can be done very simply on top of my patch if needed later, as there is now only one call site for git_config.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/