git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ben Peart <peartben@gmail.com>
To: "Eckhard Maaß" <eckhard.s.maass@googlemail.com>
Cc: Elijah Newren <newren@gmail.com>,
	Ben Peart <Ben.Peart@microsoft.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>,
	"peff@peff.net" <peff@peff.net>,
	"gitster@pobox.com" <gitster@pobox.com>,
	"pclouds@gmail.com" <pclouds@gmail.com>,
	"vmiklos@frugalware.org" <vmiklos@frugalware.org>,
	Kevin Willford <kewillf@microsoft.com>,
	"Johannes.Schindelin@gmx.de" <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v1 1/2] merge: Add merge.renames config setting
Date: Tue, 24 Apr 2018 12:53:47 -0400	[thread overview]
Message-ID: <ccb85580-e0cc-f1e6-6667-ea89ac90106f@gmail.com> (raw)
In-Reply-To: <20180423213228.GA20391@esm>



On 4/23/2018 5:32 PM, Eckhard Maaß wrote:
> On Mon, Apr 23, 2018 at 09:15:09AM -0400, Ben Peart wrote:
>> In commit 2a2ac926547 when merge.renamelimit was added, it was decided to
>> have separate settings for merge and diff to give users the ability to
>> control that behavior.  In this particular case, it will default to the
>> value of diff.renamelimit when it isn't set.  That isn't consistent with the
>> other merge settings.
> 
> However, it seems like a desirable way to do it.

I'm just one opinion among many but I personally believe the cascading 
settings are complicated enough just with the various config files and 
command line options and which overwrite the others.  I'd rather not 
complicate them further by having settings inherited from one feature 
(diff) to another (merge).

There are currently ~15 merge specific config settings and only 
merge.renamelimit currently does this inheritance.  That said, at least 
one other person thought it was a good idea. :)

> 
> Maybe let me throw in some code for discussion (test and documentation
> is missing, mainly to form an idea what the change in options should
> be). I admit the patch below is concerned only with diff.renames, but
> whatever we come up with for merge should be reflected there, too,
> doesn't it >
> Greetings,
> Eckhard
> 
> -- >8 --
> 
>  From e8a88111f2aaf338a4c19e83251c7178f7152129 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Eckhard=20S=2E=20Maa=C3=9F?= <eckhard.s.maass@gmail.com>
> Date: Sun, 22 Apr 2018 23:29:08 +0200
> Subject: [PATCH] diff: enhance diff.renames to be able to set rename score
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Signed-off-by: Eckhard S. Maaß <eckhard.s.maass@gmail.com>
> ---
>   diff.c | 35 ++++++++++++++++++++++++++++-------
>   1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index 1289df4b1f..a3cedad5cf 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -30,6 +30,7 @@
>   #endif
>   
>   static int diff_detect_rename_default;
> +static int diff_rename_score_default;
>   static int diff_indent_heuristic = 1;
>   static int diff_rename_limit_default = 400;
>   static int diff_suppress_blank_empty;
> @@ -177,13 +178,33 @@ static int parse_submodule_params(struct diff_options *options, const char *valu
>   	return 0;
>   }
>   
> +int parse_rename_score(const char **cp_p);
> +
> +static int git_config_rename_score(const char *value)
> +{
> +	int parsed_rename_score = parse_rename_score(&value);
> +	if (parsed_rename_score == -1)
> +		return error("invalid argument to diff.renamescore: %s", value);
> +	diff_rename_score_default = parsed_rename_score;
> +	return 0;
> +}
> +
>   static int git_config_rename(const char *var, const char *value)
>   {
> -	if (!value)
> -		return DIFF_DETECT_RENAME;
> -	if (!strcasecmp(value, "copies") || !strcasecmp(value, "copy"))
> -		return  DIFF_DETECT_COPY;
> -	return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
> +	if (!value) {
> +		diff_detect_rename_default = DIFF_DETECT_RENAME;
> +		return 0;
> +	}
> +	if (skip_to_optional_arg(value, "copies", &value) || skip_to_optional_arg(value, "copy", &value)) {
> +		diff_detect_rename_default = DIFF_DETECT_COPY;
> +		return git_config_rename_score(value);
> +	}
> +	if (skip_to_optional_arg(value, "renames", &value) || skip_to_optional_arg(value, "rename", &value)) {
> +		diff_detect_rename_default = DIFF_DETECT_RENAME;
> +		return git_config_rename_score(value);
> +	}
> +	diff_detect_rename_default = git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
> +	return 0;
>   }
>   
>   long parse_algorithm_value(const char *value)
> @@ -307,8 +328,7 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
>   		return 0;
>   	}
>   	if (!strcmp(var, "diff.renames")) {
> -		diff_detect_rename_default = git_config_rename(var, value);
> -		return 0;
> +		return git_config_rename(var, value);
>   	}
>   	if (!strcmp(var, "diff.autorefreshindex")) {
>   		diff_auto_refresh_index = git_config_bool(var, value);
> @@ -4116,6 +4136,7 @@ void diff_setup(struct diff_options *options)
>   	options->add_remove = diff_addremove;
>   	options->use_color = diff_use_color_default;
>   	options->detect_rename = diff_detect_rename_default;
> +	options->rename_score = diff_rename_score_default;
>   	options->xdl_opts |= diff_algorithm;
>   	if (diff_indent_heuristic)
>   		DIFF_XDL_SET(options, INDENT_HEURISTIC);
> 

  reply	other threads:[~2018-04-24 16:53 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-20 13:36 [PATCH v1 0/2] add additional config settings for merge Ben Peart
2018-04-20 13:36 ` [PATCH v1 1/2] merge: Add merge.renames config setting Ben Peart
2018-04-20 17:02   ` Elijah Newren
2018-04-20 17:26     ` Elijah Newren
2018-04-23 12:57       ` Ben Peart
2018-04-20 17:59     ` Ben Peart
2018-04-20 18:34       ` Elijah Newren
2018-04-21  4:23         ` Junio C Hamano
2018-04-23 16:00           ` Ben Peart
2018-04-23 23:23             ` Junio C Hamano
2018-04-24 11:58               ` Johannes Schindelin
2018-04-24 17:47                 ` Elijah Newren
2018-04-25  8:20                   ` Johannes Schindelin
2018-04-22 12:07         ` Eckhard Maaß
2018-04-23 13:15           ` Ben Peart
2018-04-23 21:32             ` Eckhard Maaß
2018-04-24 16:53               ` Ben Peart [this message]
2018-04-23 13:22         ` Ben Peart
2018-04-20 13:36 ` [PATCH v1 2/2] merge: Add merge.aggressive " Ben Peart
2018-04-20 17:22   ` Elijah Newren
2018-04-24 16:45     ` Ben Peart
2018-04-24 17:36       ` Elijah Newren
2018-04-24 23:57       ` Junio C Hamano
2018-04-25 14:47         ` Ben Peart
2018-04-20 17:34 ` [PATCH v1 0/2] add additional config settings for merge Elijah Newren
2018-04-20 18:19   ` Ben Peart
2018-04-24 17:11 ` [PATCH v2 " Ben Peart
2018-04-24 17:11   ` [PATCH v2 1/2] merge: Add merge.renames config setting Ben Peart
2018-04-24 18:11     ` Elijah Newren
2018-04-24 18:59     ` Elijah Newren
2018-04-24 20:31       ` Ben Peart
2018-04-25 16:01         ` Elijah Newren
2018-04-24 17:11   ` [PATCH v2 2/2] merge: Add merge.aggressive " Ben Peart
2018-04-25  0:13   ` [PATCH v2 0/2] add additional config settings for merge Junio C Hamano
2018-04-25 15:22     ` Ben Peart
2018-04-26  1:48       ` Junio C Hamano
2018-04-26 20:52 ` [PATCH v3 0/3] add merge.renames config setting Ben Peart
2018-04-26 20:52   ` [PATCH v3 1/3] merge: update documentation for {merge,diff}.renameLimit Ben Peart
2018-04-26 23:11     ` Elijah Newren
2018-04-26 23:23       ` Jonathan Tan
2018-04-26 20:52   ` [PATCH v3 2/3] merge: Add merge.renames config setting Ben Peart
2018-04-26 22:52     ` Elijah Newren
2018-04-27  0:54       ` Ben Peart
2018-04-27  2:23         ` Junio C Hamano
2018-04-27  3:28           ` Elijah Newren
2018-04-27  7:23             ` Johannes Schindelin
2018-04-27 14:32               ` Elijah Newren
2018-04-27 18:37           ` Eckhard Maaß
2018-04-27 20:23             ` Elijah Newren
2018-04-30  8:03               ` Eckhard Maaß
2018-04-30 16:54                 ` Elijah Newren
2018-04-27  4:17         ` Elijah Newren
2018-04-27 18:19         ` Elijah Newren
2018-04-30 13:11           ` Ben Peart
2018-04-30 16:12             ` Re: Elijah Newren
2018-05-02 14:33               ` Re: Ben Peart
2018-04-26 20:52   ` [PATCH v3 3/3] merge: pass aggressive when rename detection is turned off Ben Peart
2018-04-26 23:00     ` Elijah Newren
2018-04-26 22:08   ` [PATCH v3 0/3] add merge.renames config setting Elijah Newren
2018-05-02 16:01 ` [PATCH v4 0/3] add additional config settings for merge Ben Peart
2018-05-02 16:01   ` [PATCH v4 1/3] merge: update documentation for {merge,diff}.renameLimit Ben Peart
2018-05-02 16:01   ` [PATCH v4 2/3] merge: Add merge.renames config setting Ben Peart
2018-05-04  3:07     ` Junio C Hamano
2018-05-02 16:01   ` [PATCH v4 3/3] merge: pass aggressive when rename detection is turned off Ben Peart
2018-05-02 17:20   ` [PATCH v4 0/3] add additional config settings for merge Elijah Newren

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=ccb85580-e0cc-f1e6-6667-ea89ac90106f@gmail.com \
    --to=peartben@gmail.com \
    --cc=Ben.Peart@microsoft.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=eckhard.s.maass@googlemail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kewillf@microsoft.com \
    --cc=newren@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=vmiklos@frugalware.org \
    --subject='Re: [PATCH v1 1/2] merge: Add merge.renames config setting' \
    /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

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