On Tuesday 26 April 2011, Junio C Hamano wrote:
> Johan Herland <johan@xxxxxxxxxxx> 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).
> > +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@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html