Re: [PATCH] Clarify text filter merge conflict reduction docs

Eyvind Bernhardsen [off-list ref] writes:

Signed-off-by: Eyvind Bernhardsen <redacted>
---
How does this look?

Looks Ok (I didn't read _this_ patch but read a squashed-in result),
thanks.

+If you have added attributes to a file that cause...
+...To prevent these unnecessary merge conflicts,

This naturally calls for an optimization idea, doesn't it?

I wonder if ll_merge should gain another flag bit to disable the calls to
normalize_file(), so that the whole thing can be skipped when the caller
somehow knows .gitattributes files that govern the path didn't change.

That won't be a trivial optimization and my gut feeling is that it
shouldn't be part of this series.

I do however wonder if this should be initially introduced as an
experimental feature, guarded with a configuration option for brave souls
to try it out, and flip the feature on by default after we gain confidence
in it, both in performance and in correctness.

-- >8 --
Introduce "double conversion during merge" more gradually

This marks the recent improvement to the merge machinery that helps people
who changed their mind between CRLF/LF an opt in feature, so that we can
more easily release it early to everybody, without fear of breaking the
majority of users (read: on POSIX) that don't need it.

Signed-off-by: Junio C Hamano <redacted>
---
 Documentation/config.txt        |   10 ++++++++++
 Documentation/gitattributes.txt |    5 +++--
 cache.h                         |    1 +
 config.c                        |    5 +++++
 environment.c                   |    1 +
 ll-merge.c                      |    8 +++++---
 6 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4c49104..ad2a27e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -538,6 +538,16 @@ core.sparseCheckout::
 	Enable "sparse checkout" feature. See section "Sparse checkout" in
 	linkgit:git-read-tree[1] for more information.
 
+core.doubleConvert::
+	Tell git that canonical representation of files in the repository
+	has changed over time (e.g. earlier commits record text files
+	with CRLF line endings, but recent ones use LF line endings).  In
+	such a repository, git is forced to convert the data recorded in
+	commits twice before performing a merge to reduce unnecessary
+	conflicts.  For more information, see section
+	"Merging branches with differing checkin/checkout attributes" in
+	linkgit:gitattributes[5].
+
 add.ignore-errors::
 	Tells 'git add' to continue adding files when some files cannot be
 	added due to indexing errors. Equivalent to the '--ignore-errors'

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 22400c1..504d5ca 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt

@@ -351,9 +351,10 @@ clean/smudge filter or text/eol/ident attributes, merging anything
 where the attribute is not in place would normally cause merge
 conflicts.
 
-To prevent these unnecessary merge conflicts, git runs a virtual
+To prevent these unnecessary merge conflicts, git can be told to run a virtual
 check-out and check-in of all three stages of a file when resolving a
-three-way merge.  This prevents changes caused by check-in conversion
+three-way merge by setting `core.doubleConvert` configuration variable.
+This prevents changes caused by check-in conversion
 from causing spurious merge conflicts when a converted file is merged
 with an unconverted file.
 

diff --git a/cache.h b/cache.h
index aa725b0..217f1e9 100644
--- a/cache.h
+++ b/cache.h

@@ -551,6 +551,7 @@ extern int read_replace_refs;
 extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
+extern int core_ll_merge_double_convert;
 
 enum safe_crlf {
 	SAFE_CRLF_FALSE = 0,

diff --git a/config.c b/config.c
index cdcf583..bea054c 100644
--- a/config.c
+++ b/config.c

@@ -595,6 +595,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.doubleconvert")) {
+		core_ll_merge_double_convert = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }

diff --git a/environment.c b/environment.c
index 83d38d3..a8f04e7 100644
--- a/environment.c
+++ b/environment.c

@@ -53,6 +53,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_apply_sparse_checkout;
+int core_ll_merge_double_convert;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;

diff --git a/ll-merge.c b/ll-merge.c
index 28c6f54..8831631 100644
--- a/ll-merge.c
+++ b/ll-merge.c

@@ -344,9 +344,11 @@ int ll_merge(mmbuffer_t *result_buf,
 	const struct ll_merge_driver *driver;
 	int virtual_ancestor = flag & 01;
 
-	normalize_file(ancestor, path);
-	normalize_file(ours, path);
-	normalize_file(theirs, path);
+	if (core_ll_merge_double_convert) {
+		normalize_file(ancestor, path);
+		normalize_file(ours, path);
+		normalize_file(theirs, path);
+	}
 	if (!git_path_check_merge(path, check)) {
 		ll_driver_name = check[0].value;
 		if (check[1].value) {

On 29. juni 2010, at 18.19, Junio C Hamano wrote:

I do however wonder if this should be initially introduced as an
experimental feature, guarded with a configuration option for brave souls
to try it out, and flip the feature on by default after we gain confidence
in it, both in performance and in correctness.

Agreed, and thanks.  Messing with the merge machinery unnerves me a little.

Shouldn't the normalization in merge-recursive be conditional too?

Something like this squashed into the delete/modify patch:

--8<--

diff --git a/merge-recursive.c b/merge-recursive.c
index a2c174f..49bd3d2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -1079,6 +1079,8 @@ static int normalized_eq(const unsigned char *a_sha,
 {
        struct strbuf a = STRBUF_INIT;
        struct strbuf b = STRBUF_INIT;
+       if (!core_ll_merge_double_convert)
+               return 0;
        int ret = 0;
        if (a_sha && b_sha &&
            read_sha1_strbuf(a_sha, path, &a) &&
--8<--

Sorry about the whitespace-damaged inline diff, I'll redo the series if necessary.
-- 
Eyvind

On 29. juni 2010, at 18.19, Junio C Hamano wrote:

Eyvind Bernhardsen [off-list ref] writes:

[...]

quoted

+If you have added attributes to a file that cause...
+...To prevent these unnecessary merge conflicts,

This naturally calls for an optimization idea, doesn't it?

I wonder if ll_merge should gain another flag bit to disable the calls to
normalize_file(), so that the whole thing can be skipped when the caller
somehow knows .gitattributes files that govern the path didn't change.

That won't be a trivial optimization and my gut feeling is that it
shouldn't be part of this series.

Are you thinking that we could check changes in .gitattributes during a merge and only turn on normalization for those files where relevant attributes have changed?  I like it, but I agree with your gut, especially since normalization has to be enabled manually.

I do however wonder if this should be initially introduced as an
experimental feature, guarded with a configuration option for brave souls
to try it out, and flip the feature on by default after we gain confidence
in it, both in performance and in correctness.

My fix to add the configuration option to the delete/modify patch yesterday was pretty bad, sorry.  My only excuse is that I was in a hurry, I'll resend the series tonight with a better fix.
-- 
Eyvind Bernhardsen