Jeff King <p...@peff.net> writes:
> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
