Re: [PATCH] diff: add interhunk context config option

From: Pranit Bauva <pranit.bauva@gmail.com>
Date: Jan 3, 2017, 06:19 UTC
Message-ID: <CAFZEwPMLk5KTeLR9zygU6ZH5zN7TLnQzmJVpyCNig8FrYcOazw@mail.gmail.com>
In-Reply-To: <20170102233114.20778-1-vegard.nossum@oracle.com>

Hey Vegard,

On Tue, Jan 3, 2017 at 5:01 AM, Vegard Nossum <vegard.nossum@oracle.com> wrote:

Show 21 quoted lines

> The --inter-hunk-context= option was added in commit 6d0e674a5754
> ("diff: add option to show context between close hunks"). This patch
> allows configuring a default for this option.
>
> Cc: René Scharfe <l.s.r@web.de>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  Documentation/diff-options.txt | 2 ++
>  diff.c                         | 8 ++++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index e6215c372..163b65444 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -511,6 +511,8 @@ endif::git-format-patch[]
>  --inter-hunk-context=<lines>::
>         Show the context between diff hunks, up to the specified number
>         of lines, thereby fusing hunks that are close to each other.
> +       Defaults to `diff.interhunkcontext` or 0 if the config option
> +       is unset.

Seems good. But I think you have missed adding the documentation of this to Documentation/diff-config.txt . Generally whatever config variable one adds, is added to Documentation/config.txt but in this case, there exists a separate Documentation/diff-config.txt for all "diff" related things which is included in Documentation/config.txt . Also, you need to link the link both the parts of this documentation. Please refer to commit id aaab84203 so as to know what I am talking about.

Show 34 quoted lines

>  -W::
>  --function-context::
> diff --git a/diff.c b/diff.c
> index 84dba60c4..40b4e6afe 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -33,6 +33,7 @@ static int diff_rename_limit_default = 400;
>  static int diff_suppress_blank_empty;
>  static int diff_use_color_default = -1;
>  static int diff_context_default = 3;
> +static int diff_interhunk_context_default = 0;
>  static const char *diff_word_regex_cfg;
>  static const char *external_diff_cmd_cfg;
>  static const char *diff_order_file_cfg;
> @@ -248,6 +249,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
>                         return -1;
>                 return 0;
>         }
> +       if (!strcmp(var, "diff.interhunkcontext")) {
> +               diff_interhunk_context_default = git_config_int(var, value);
> +               if (diff_interhunk_context_default < 0)
> +                       return -1;
> +               return 0;
> +       }
>         if (!strcmp(var, "diff.renames")) {
>                 diff_detect_rename_default = git_config_rename(var, value);
>                 return 0;
> @@ -3371,6 +3378,7 @@ void diff_setup(struct diff_options *options)
>         options->rename_limit = -1;
>         options->dirstat_permille = diff_dirstat_permille_default;
>         options->context = diff_context_default;
> +       options->interhunkcontext = diff_interhunk_context_default;
>         options->ws_error_highlight = ws_error_highlight_default;
>         DIFF_OPT_SET(options, RENAME_EMPTY);

On a first look, it seems that we can overwrite the default config values by using a different command line argument which is good.

Also, tests are missing. It seems that t/t4032 might be a good place to add those tests.

Rest all is quite good! :)

Regards, Pranit Bauva