Re: [PATCH 4/6] Add config variable for specifying default --dirstat behavior
- From
Johan Herland <johan@herland.net>
- Date
- Apr 27, 2011, 02:02 UTC
- Message-ID
- <201104270402.17685.johan@herland.net>
- In-Reply-To
- <7vtydlezfj.fsf@alter.siamese.dyndns.org>
On Tuesday 26 April 2011, Junio C Hamano wrote:
Show 17 quoted lines
> Johan Herland <johan@herland.net> writes:
> > +static void init_default_diff_options()
> > +{
> > + static int initialized = 0;
> > + if (initialized)
> > + return;
> > +
> > + default_diff_options.dirstat_percent = 3;
> > +
> > + initialized = 1;
> > +}
>
> This smells fishy on two counts.
>
> . The rest of the diff machinery is designed to be callable multiple
> times by calling diff_setup(), and there should be no place for any
> call-once function like this one.True. I needed it because the hardcoded "options->dirstat_percent = 3" in diff_setup() would overwrite the "diff.dirstat=10" stored in default_diff_options.dirstat_percent. Instead, I needed the fallback "3" to be stored in default_diff_options.dirstat_percent before "diff.dirstat" was parsed.
> . Why is dirstat-percent _so_ special that it is the only one that has > to be initialized this way, when the function name implies that this > is the central place to control the initialization of all diff > related options?
Once I added init_default_diff_options(), I did in fact try to move the other hardcoded diff options from diff_setup(). However, I ended up with a lot of test failures, so I quickly gave up on that.
In the upcoming re-roll, I have solved the problem in a different way (using a static variable to store the default dirstat percentage).
Show 12 quoted lines
> > +static void dirstat_opt_args(struct diff_options *options, const char
> > *args) +{
> > + const char *p = args;
> > + while (*p) {
> > + if (!prefixcmp(p, "changes")) {
> > + ...
> > + }
> > + }
> > +}
>
> Please move this part to the previous patch in your reroll. This helper
> is what the previous patch should have been written with.Will do.
...Johan
-- Johan Herland, <johan@herland.net> www.herland.net