From: Stefan Beller <sbeller@google.com>
To: Marc Branchaud <marcnarc@xiplink.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
Michael Haggerty <mhagger@alum.mit.edu>,
Jeff King <peff@peff.net>
Subject: Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.
Date: Fri, 28 Apr 2017 10:34:15 -0700 [thread overview]
Message-ID: <CAGZ79kbUqVfz+6Y0XkTL7FCZfaD+2YRMZ_v0vP8-DOFhWc+ELw@mail.gmail.com> (raw)
In-Reply-To: <20170427205037.1787-1-marcnarc@xiplink.com>
On Thu, Apr 27, 2017 at 1:50 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
> So here's my attempt at fixing this.
>
> The thing I was missing is that init_revisions() calls diff_setup(), which
> sets the xdl options. It's therefore necessary to have the
> diff_indent_heuristic flag set before calling init_revisions().
>
> A naive way to get the indentHeuristic config option respected in the
> diff-* plumbing commands is to make them use the git_diff_heuristic_config()
> callback right at the start of their main cmd functions.
>
> But I did not like that for two reasons:
>
> * It would make these commands invoke git_config() twice.
>
> * It doesn't avoid the problem if/when someone creates a new diff-something
> plumbing command, and forgets to set the diff_indent_heuristic flag before
> calling init_revisions().
>
> So instead I chose to make the indentHeuristic option part of diff's basic
> configuration, and in each of the diff plumbing commands I moved the call to
> git_config() before the call to init_revisions().
>
> This still doesn't really future-proof things for possible new diff plumbing
> commands, because someone could still invoke init_revisions() before setting
> up diff's basic configuration. But I don't see an obvious way of ensuring
> that the diff_indent_heuristic flag is respected regardless of when
> diff_setup() is invoked.
>
The feature was included in v2.11 (released 2016-11-29) and we got no
negative feedback. Quite the opposite, all feedback we got, was positive.
This could be explained by having the feature as an experimental feature
and users who would be broken by it, did not test it yet or did not speak up.
So I'd propose to turn it on by default and anyone negatively impacted by that
could then use the config to turn it off for themselves (including plumbing).
Something like this, maybe?
---8<---
From 58d9a1ef135eff9f85b165986e4bc13479914f8e Mon Sep 17 00:00:00 2001
From: Stefan Beller <sbeller@google.com>
Date: Thu, 27 Apr 2017 14:26:59 -0700
Subject: [PATCH] diff.c: enable indent heuristic by default
The feature was included in v2.11 (released 2016-11-29) and we got no
negative feedback. Quite the opposite, all feedback we got was positive.
Turn it on by default. users who dislike the feature can turn it off
using by setting diff.compactionHeuristic (which includes plumbing
commands, see prior patches)
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff --git a/diff.c b/diff.c
index 11eef1c85d..7f301c1b62 100644
--- a/diff.c
+++ b/diff.c
@@ -27,7 +27,7 @@
#endif
static int diff_detect_rename_default;
-static int diff_indent_heuristic; /* experimental */
+static int diff_indent_heuristic = 1;
static int diff_rename_limit_default = 400;
static int diff_suppress_blank_empty;
static int diff_use_color_default = -1;
next prev parent reply other threads:[~2017-04-28 17:34 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-25 17:21 BUG: diff-{index,files,tree} (and git-gui) do not respect the diff.indentHeuristic config setting Marc Branchaud
2017-04-25 21:27 ` Jeff King
2017-04-27 20:50 ` [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic Marc Branchaud
2017-04-27 20:50 ` [PATCH 1/2] Make the indent heuristic part of diff's basic configuration Marc Branchaud
2017-04-28 7:59 ` Jeff King
2017-04-27 20:50 ` [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions Marc Branchaud
2017-04-28 8:06 ` Jeff King
2017-05-01 1:01 ` Junio C Hamano
2017-05-01 5:17 ` Jeff King
2017-05-01 5:29 ` Jeff King
2017-04-28 7:56 ` [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic Jeff King
2017-04-28 17:34 ` Stefan Beller [this message]
2017-04-28 22:04 ` Jeff King
2017-04-28 22:13 ` Stefan Beller
2017-05-01 10:34 ` Ævar Arnfjörð Bjarmason
2017-05-09 3:16 ` Jeff King
2017-05-09 4:06 ` Junio C Hamano
2017-05-09 7:58 ` Ævar Arnfjörð Bjarmason
2017-05-09 8:04 ` Jeff King
2017-04-28 22:33 ` [PATCHv2 0/3] " Marc Branchaud
2017-04-28 22:33 ` [PATCHv2 1/3] diff: make the indent heuristic part of diff's basic configuration Marc Branchaud
2017-04-28 22:33 ` [PATCHv2 2/3] diff: have the diff-* builtins configure diff before initializing revisions Marc Branchaud
2017-04-28 22:33 ` [PATCH 3/3] diff: enable indent heuristic by default Marc Branchaud
2017-04-29 12:40 ` [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic Jeff King
2017-04-29 13:14 ` Jeff King
2017-05-01 1:11 ` Junio C Hamano
2017-05-01 5:15 ` Jeff King
2017-05-01 23:25 ` Junio C Hamano
2017-05-01 22:13 ` [PATCHv3 0/4] " Marc Branchaud
2017-05-01 22:13 ` [PATCHv3 1/4] diff: make the indent heuristic part of diff's basic configuration Marc Branchaud
2017-05-01 22:13 ` [PATCHv3 2/4] diff: have the diff-* builtins configure diff before initializing revisions Marc Branchaud
2017-05-01 22:13 ` [PATCHv3 3/4] diff: enable indent heuristic by default Marc Branchaud
2017-05-01 22:20 ` Jeff King
2017-05-01 22:13 ` [PATCHv3 4/4] add--interactive: drop diff.indentHeuristic handling Marc Branchaud
2017-05-01 22:18 ` [PATCHv3 0/4] Make diff plumbing commands respect the indentHeuristic Stefan Beller
2017-04-29 13:15 ` [PATCH 4/3] add--interactive: drop diff.indentHeuristic handling Jeff King
2017-04-30 3:26 ` [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic Michael Haggerty
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=CAGZ79kbUqVfz+6Y0XkTL7FCZfaD+2YRMZ_v0vP8-DOFhWc+ELw@mail.gmail.com \
--to=sbeller@google.com \
--cc=git@vger.kernel.org \
--cc=marcnarc@xiplink.com \
--cc=mhagger@alum.mit.edu \
--cc=peff@peff.net \
/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).