Re: [PATCH 4/6] Add config variable for specifying default --dirstat behavior

On Tuesday 26 April 2011, Junio C Hamano wrote:

Johan Herland [off-list ref] writes:

quoted

+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).

quoted

+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, [off-list ref]
www.herland.net