Re: [PATCHv2 7/7] git grep: honor textconv by default

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 29, 2013, 15:04 UTC
Message-ID: <7vy5c1l6nb.fsf@alter.siamese.dyndns.org>
In-Reply-To: <517E37A9.8040609@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

Show 5 quoted lines

>> It should be possible to have a tri-state for the --[no-]textconv
>> option: unset, set to true or set to false. But the code sharing between
>> log, show and diff might make that non-trivial.
>
> Right now it's a diffopt bit...

I wonder if you can do something along the lines of the attached patch. The following discussion assumes that your default wants textconv for generating patches, and no textconv for showing blobs, which is the case your "it is a bit" becomes an issue.

The basic structure is that:

 * There is an extra "opt->touched_flags" that keeps track of all
   the fields that have been touched by DIFF_OPT_SET and
   DIFF_OPT_CLR;

 * You may continue setting the default values to the flags, like
   commands in the "log" family do in cmd_log_init_defaults(), but
   after you finished setting the defaults, you clear the
   touched_flags field;

 * And then you let the usual callchain to call diff_opt_parse(),
   allowing the opt->flags be set or unset, while keeping track of
   which bits the user touched;

 * There is an optional callback "opt->set_default" that is called
   at the very beginning to lets you inspect touched_flags and
   update opt->flags appropriately, before the remainder of the
   diffcore machinery is set up, taking the opt->flags value into
   account.

Your "git show" could start out with ALLOW_TEXTCONV set, but notice explicit requests to --[no-]textconv from the command line in your set_default() callback. And then when it deals with a blob, check if the user touched ALLOW_TEXTCONV and appropriately act on that knowledge.

There would be three cases in your set_default callback:

 * flags has ALLOW_TEXTCONV set, and the bit was touched: the user
   explicitly said --textconv because she wants blobs to be mangled;

 * flags has ALLOW_TEXTCONV set, and the bit was not touched: the
   user did not say --textconv; do not mangle blobs;

 * flags has ALLOW_TEXTCONV unset; the user did not say --textconv,
   or explicitly said --no-textconv; do not mangle blobs.

The set_default callback can also be used to adjust defaults for fields that are not handled by the DIFF_OPT_SET/CLR/TST, by the way. You can remember the address of the default value you fed to a string field before entering the callchain to diff_opt_parse(), and in your set_default callback see if the value is still pointing at the same piece of memory (in which case the user did not touch it).

 builtin/log.c | 1 +
 diff.c        | 3 +++
 diff.h        | 7 +++++--
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 6e56a50..c62ecd1 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -91,6 +91,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 
 	if (default_date_mode)
 		rev->date_mode = parse_date_format(default_date_mode);
+	rev->diffopt.touched_flags = 0;
 }
 
 static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
diff --git a/diff.c b/diff.c
index f0b3e7c..7c24872 100644
--- a/diff.c
+++ b/diff.c
@@ -3213,6 +3213,9 @@ void diff_setup_done(struct diff_options *options)
 {
 	int count = 0;
 
+	if (options->set_default)
+		options->set_default(options);
+
 	if (options->output_format & DIFF_FORMAT_NAME)
 		count++;
 	if (options->output_format & DIFF_FORMAT_NAME_STATUS)
diff --git a/diff.h b/diff.h
index 78b4091..5c2f878 100644
--- a/diff.h
+++ b/diff.h
@@ -87,8 +87,8 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
 
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
-#define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
-#define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)
+#define DIFF_OPT_SET(opts, flag)    (((opts)->flags |= DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))
+#define DIFF_OPT_CLR(opts, flag)    (((opts)->flags &= ~DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))
 #define DIFF_XDL_TST(opts, flag)    ((opts)->xdl_opts & XDF_##flag)
 #define DIFF_XDL_SET(opts, flag)    ((opts)->xdl_opts |= XDF_##flag)
 #define DIFF_XDL_CLR(opts, flag)    ((opts)->xdl_opts &= ~XDF_##flag)
@@ -109,6 +109,7 @@ struct diff_options {
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
 	unsigned flags;
+	unsigned touched_flags;
 	int use_color;
 	int context;
 	int interhunkcontext;
@@ -145,6 +146,8 @@ struct diff_options {
 	/* to support internal diff recursion by --follow hack*/
 	int found_follow;
 
+	void (*set_default)(struct diff_options *);
+
 	FILE *file;
 	int close_file;