git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: mmr15@case.edu
Cc: git@vger.kernel.org, Matthew Ruffalo <matthew.ruffalo@case.edu>
Subject: Re: [PATCH 2/3] diffstat: Use new diffstat config values
Date: Wed, 08 Dec 2010 21:54:07 -0800	[thread overview]
Message-ID: <7vwrnjh4ow.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <1291776263-16320-2-git-send-email-matthew.ruffalo@case.edu> (mmr's message of "Tue\,  7 Dec 2010 21\:44\:22 -0500")

mmr15@case.edu writes:

> This required removing the diffstat options from 'struct diff_options'
> and adding these values as static ints in diff.c. This preserves the
> style of "config options are static ints, command-line options are
> diff_options members". stat_opt now directly sets the global options.

It is not that I do not trust/believe it, but I am very unhappy with the
above "This required".

The diff callchain was designed to be a highly reusable library and has
been kept callable multiple times with different settings in a single
program by passing different "struct diff_options".  The above sounds like
a rather huge regression.

Aren't there any way to avoid this?  Why do these two options need to be
any different from other variables (e.g. a_prefix, context) that can be
set from the config and can be overridden by the command line options
while having a built-in fallback default values?

In general, the callflow of each git subcommand looks like this:

 (1) find $GIT_DIR;
 (2) read $GIT_DIR/config and friends;
 (3) parse command line options;
 (4) decide what the user asked us to do and do it.

I would imagine that the following should do what you want:

 * declare two static int variables, stat_name_width_default and
   stat_width_default, that are initialized to 80/50 at compile time;

 * add code to git_diff_ui_config() to update these two *_default
   variables in step (2) above;

 * add code to diff_setup() to initialize opt->stat_name_width and
   opt->stat_width from these two *_default variables;

 * add code to diff_opt_parse() to update opt->stat_name_width and
   opt->stat_width from the command line parameters.

Then follow cmd_diff() in diff.c to make sure the above is sufficient.
Observe that:

 - The first thing cmd_diff() does is to read the config;
 - then init_revisions() will call diff_setup();
 - then setup_revisions() will call into diff_opt_parse().

  reply	other threads:[~2010-12-09  5:54 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-11-28 23:51 [PATCH/RFC 2/3] diffstat: Use new diff.stat config values Matthew Ruffalo
2010-11-29 20:24 ` Junio C Hamano
2010-12-08  2:44   ` [PATCH 1/3] diffstat width: #define defaults in diff.h mmr15
2010-12-08  2:44     ` [PATCH 2/3] diffstat: Use new diffstat config values mmr15
2010-12-09  5:54       ` Junio C Hamano [this message]
2010-12-08  2:44     ` [PATCH 3/3] Add documentation for new diffstat config options mmr15

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7vwrnjh4ow.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=matthew.ruffalo@case.edu \
    --cc=mmr15@case.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).