* [PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output
2010-03-15 7:47 ` [PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3 Jonathan Nieder
@ 2010-03-15 7:49 ` Jonathan Nieder
2010-03-15 8:10 ` Junio C Hamano
2010-03-15 7:51 ` [PATCH 02/10] merge-file --diff3: add a label for ancestor Jonathan Nieder
` (8 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 7:49 UTC (permalink / raw)
To: git; +Cc: Stefan Monnier
The ‘git checkout --conflict=diff3’ command can be used to
present conflicts hunks including text from the common ancestor:
<<<<<<< ours
ourside
|||||||
original
=======
theirside
>>>>>>> theirs
The added information is helpful for resolving merges by hand, and
merge tools can usually grok it because it is very similar to the
output from diff3 -m.
A subtle change can help more tools to understand the output. ‘diff3’
includes the name of the merge base on the ||||||| line of the output,
and some tools misparse the conflict hunks without it. Add a new
xmp->ancestor parameter to xdl_merge() for use with conflict style
XDL_MERGE_DIFF3 as a label on the ||||||| line for any conflict hunks.
If xmp->ancestor is NULL, the output format is unchanged. ‘git
checkout’ and other users within git all use NULL for this parameter,
so this change only provides unexposed plumbing for a fix: it does not
affect the outward behavior of git in any way.
Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
xdiff/xdiff.h | 1 +
xdiff/xmerge.c | 16 ++++++++++++++--
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 3f6229e..abaf960 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -117,6 +117,7 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
typedef struct s_xmparam {
xpparam_t xpp;
int marker_size;
+ const char *ancestor;
} xmparam_t;
#define DEFAULT_CONFLICT_MARKER_SIZE 7
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 8cbe45e..3a0ae14 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -144,11 +144,13 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
xdfenv_t *xe2, const char *name2,
+ const char *name3,
int size, int i, int style,
xdmerge_t *m, char *dest, int marker_size)
{
int marker1_size = (name1 ? strlen(name1) + 1 : 0);
int marker2_size = (name2 ? strlen(name2) + 1 : 0);
+ int marker3_size = (name3 ? strlen(name3) + 1 : 0);
int j;
if (marker_size <= 0)
@@ -178,10 +180,15 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
if (style == XDL_MERGE_DIFF3) {
/* Shared preimage */
if (!dest) {
- size += marker_size + 1;
+ size += marker_size + 1 + marker3_size;
} else {
for (j = 0; j < marker_size; j++)
dest[size++] = '|';
+ if (marker3_size) {
+ dest[size] = ' ';
+ memcpy(dest + size + 1, name3, marker3_size - 1);
+ size += marker3_size;
+ }
dest[size++] = '\n';
}
size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
@@ -216,6 +223,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
xdfenv_t *xe2, const char *name2,
+ const char *ancestor_name,
int favor,
xdmerge_t *m, char *dest, int style,
int marker_size)
@@ -228,6 +236,7 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
if (m->mode == 0)
size = fill_conflict_hunk(xe1, name1, xe2, name2,
+ ancestor_name,
size, i, style, m, dest,
marker_size);
else if (m->mode == 1)
@@ -397,6 +406,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
int flags, xmparam_t const *xmp, mmbuffer_t *result) {
xdmerge_t *changes, *c;
xpparam_t const *xpp = &xmp->xpp;
+ const char * const ancestor_name = xmp->ancestor;
int i0, i1, i2, chg0, chg1, chg2;
int level = flags & XDL_MERGE_LEVEL_MASK;
int style = flags & XDL_MERGE_STYLE_MASK;
@@ -534,6 +544,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
if (result) {
int marker_size = xmp->marker_size;
int size = xdl_fill_merge_buffer(xe1, name1, xe2, name2,
+ ancestor_name,
favor, changes, NULL, style,
marker_size);
result->ptr = xdl_malloc(size);
@@ -542,7 +553,8 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
return -1;
}
result->size = size;
- xdl_fill_merge_buffer(xe1, name1, xe2, name2, favor, changes,
+ xdl_fill_merge_buffer(xe1, name1, xe2, name2,
+ ancestor_name, favor, changes,
result->ptr, style, marker_size);
}
return xdl_cleanup_merge(changes);
--
1.7.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output
2010-03-15 7:49 ` [PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output Jonathan Nieder
@ 2010-03-15 8:10 ` Junio C Hamano
2010-03-15 8:35 ` Jonathan Nieder
0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2010-03-15 8:10 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Stefan Monnier
I think this patch itself makes sense, but if you were to add one name to
xmparam structure, wouldn't it make sense to store all three names in
there?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output
2010-03-15 8:10 ` Junio C Hamano
@ 2010-03-15 8:35 ` Jonathan Nieder
2010-03-15 8:37 ` Junio C Hamano
0 siblings, 1 reply; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 8:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Stefan Monnier
Junio C Hamano wrote:
> I think this patch itself makes sense, but if you were to add one name to
> xmparam structure, wouldn't it make sense to store all three names in
> there?
Good idea. Will do.
Ciao,
Jonathan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output
2010-03-15 8:35 ` Jonathan Nieder
@ 2010-03-15 8:37 ` Junio C Hamano
0 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2010-03-15 8:37 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git, Stefan Monnier
Jonathan Nieder <jrnieder@gmail.com> writes:
> Junio C Hamano wrote:
>
>> I think this patch itself makes sense, but if you were to add one name to
>> xmparam structure, wouldn't it make sense to store all three names in
>> there?
>
> Good idea. Will do.
That would also mean that existing name1 and name2 arguments many
functions (starting from xdl_merge()) have will go.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 02/10] merge-file --diff3: add a label for ancestor
2010-03-15 7:47 ` [PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3 Jonathan Nieder
2010-03-15 7:49 ` [PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output Jonathan Nieder
@ 2010-03-15 7:51 ` Jonathan Nieder
2010-03-15 7:52 ` [PATCH 03/10] ll_merge(): add ancestor label parameter for diff3-style output Jonathan Nieder
` (7 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 7:51 UTC (permalink / raw)
To: git; +Cc: Stefan Monnier
git merge-file --diff3 can be used to present conflicts hunks
including text from the common ancestor.
The added information can be very helpful for resolving a merge by
hand, and merge tools can usually grok it because it looks like output
from diff3 -m. Unfortunately, some cannot reportedly, because ‘diff3’
includes a label for the merge base on the ||||||| line and some tools
cannot parse conflict hunks without such a label. So write the
base-name as passed in a -L option (or the name of the “ancestor” file
by default) on that line.
git rerere will not have trouble parsing this output, since instead of
looking for a newline, it looks for whitespace after the |||||||
marker. No other code in git tries to parse conflict hunks.
Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This requires a slight change when merging with next. A test was
added to t6023-merge-file.sh for the longer conflict markers;
the corresponding line in that new test needs to have new5.txt
added as well.
builtin/merge-file.c | 1 +
t/t6023-merge-file.sh | 2 +-
2 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 1e70073..0ff8b03 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -72,6 +72,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
argv[i]);
}
+ xmp.ancestor = names[1];
ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2],
&xmp, XDL_MERGE_FLAGS(level, style, favor), &result);
diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index d605024..16c3c69 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -181,7 +181,7 @@ et nihil mihi deerit;
In loco pascuae ibi me collocavit;
super aquam refectionis educavit me.
-|||||||
+||||||| new5.txt
et nihil mihi deerit.
In loco pascuae ibi me collocavit,
super aquam refectionis educavit me;
--
1.7.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 03/10] ll_merge(): add ancestor label parameter for diff3-style output
2010-03-15 7:47 ` [PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3 Jonathan Nieder
2010-03-15 7:49 ` [PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output Jonathan Nieder
2010-03-15 7:51 ` [PATCH 02/10] merge-file --diff3: add a label for ancestor Jonathan Nieder
@ 2010-03-15 7:52 ` Jonathan Nieder
2010-03-15 7:55 ` [PATCH 04/10] checkout --conflict=diff3: add a label for ancestor Jonathan Nieder
` (6 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 7:52 UTC (permalink / raw)
To: git; +Cc: Stefan Monnier
Various commands using the ll_merge() function present conflict hunks
in output something like what ‘diff3 -m’ produces if the
merge.conflictstyle configuration option is set to diff3.
Unfortunately, the format is not fully compatible yet. ‘diff3’
includes the name of the merge base on the ||||||| line of the output,
and some tools are reported to misparse the conflict hunks without it.
Add a new ancestor_label parameter to ll_merge() to give callers the
power to rectify this situation. Let all callers pass NULL as that
parameter for now to avoid distracting changes in behavior when
testing this interface change.
Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
A change to unexposed plumbing only.
builtin/checkout.c | 2 +-
ll-merge.c | 20 +++++++++++---------
ll-merge.h | 2 +-
merge-file.c | 2 +-
merge-recursive.c | 2 +-
rerere.c | 4 ++--
6 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index acefaaf..d67f809 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -149,7 +149,7 @@ static int checkout_merged(int pos, struct checkout *state)
read_mmblob(&ours, active_cache[pos+1]->sha1);
read_mmblob(&theirs, active_cache[pos+2]->sha1);
- status = ll_merge(&result_buf, path, &ancestor,
+ status = ll_merge(&result_buf, path, &ancestor, NULL,
&ours, "ours", &theirs, "theirs", 0);
free(ancestor.ptr);
free(ours.ptr);
diff --git a/ll-merge.c b/ll-merge.c
index 4c7f11b..68fa1e6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -15,7 +15,7 @@ struct ll_merge_driver;
typedef int (*ll_merge_fn)(const struct ll_merge_driver *,
mmbuffer_t *result,
const char *path,
- mmfile_t *orig,
+ mmfile_t *orig, const char *orig_name,
mmfile_t *src1, const char *name1,
mmfile_t *src2, const char *name2,
int flag,
@@ -36,7 +36,7 @@ struct ll_merge_driver {
static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
mmbuffer_t *result,
const char *path_unused,
- mmfile_t *orig,
+ mmfile_t *orig, const char *orig_name,
mmfile_t *src1, const char *name1,
mmfile_t *src2, const char *name2,
int flag, int marker_size)
@@ -57,7 +57,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
mmbuffer_t *result,
const char *path,
- mmfile_t *orig,
+ mmfile_t *orig, const char *orig_name,
mmfile_t *src1, const char *name1,
mmfile_t *src2, const char *name2,
int flag, int marker_size)
@@ -73,12 +73,14 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
path, name1, name2);
return ll_binary_merge(drv_unused, result,
path,
- orig, src1, name1,
+ orig, orig_name,
+ src1, name1,
src2, name2,
flag, marker_size);
}
memset(&xmp, 0, sizeof(xmp));
+ xmp.ancestor = orig_name;
if (git_xmerge_style >= 0)
style = git_xmerge_style;
if (marker_size > 0)
@@ -93,7 +95,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
static int ll_union_merge(const struct ll_merge_driver *drv_unused,
mmbuffer_t *result,
const char *path_unused,
- mmfile_t *orig,
+ mmfile_t *orig, const char *orig_name,
mmfile_t *src1, const char *name1,
mmfile_t *src2, const char *name2,
int flag, int marker_size)
@@ -106,7 +108,7 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,
saved_style = git_xmerge_style;
git_xmerge_style = 0;
status = ll_xdl_merge(drv_unused, result, path_unused,
- orig, src1, NULL, src2, NULL,
+ orig, orig_name, src1, NULL, src2, NULL,
flag, marker_size);
git_xmerge_style = saved_style;
if (status <= 0)
@@ -165,7 +167,7 @@ static void create_temp(mmfile_t *src, char *path)
static int ll_ext_merge(const struct ll_merge_driver *fn,
mmbuffer_t *result,
const char *path,
- mmfile_t *orig,
+ mmfile_t *orig, const char *orig_name,
mmfile_t *src1, const char *name1,
mmfile_t *src2, const char *name2,
int flag, int marker_size)
@@ -356,7 +358,7 @@ static int git_path_check_merge(const char *path, struct git_attr_check check[2]
int ll_merge(mmbuffer_t *result_buf,
const char *path,
- mmfile_t *ancestor,
+ mmfile_t *ancestor, const char *ancestor_label,
mmfile_t *ours, const char *our_label,
mmfile_t *theirs, const char *their_label,
int flag)
@@ -378,7 +380,7 @@ int ll_merge(mmbuffer_t *result_buf,
driver = find_ll_merge_driver(ll_driver_name);
if (virtual_ancestor && driver->recursive)
driver = find_ll_merge_driver(driver->recursive);
- return driver->fn(driver, result_buf, path, ancestor,
+ return driver->fn(driver, result_buf, path, ancestor, ancestor_label,
ours, our_label, theirs, their_label,
flag, marker_size);
}
diff --git a/ll-merge.h b/ll-merge.h
index 5788922..57754cc 100644
--- a/ll-merge.h
+++ b/ll-merge.h
@@ -7,7 +7,7 @@
int ll_merge(mmbuffer_t *result_buf,
const char *path,
- mmfile_t *ancestor,
+ mmfile_t *ancestor, const char *ancestor_label,
mmfile_t *ours, const char *our_label,
mmfile_t *theirs, const char *their_label,
int flag);
diff --git a/merge-file.c b/merge-file.c
index fd34d76..07cc0c9 100644
--- a/merge-file.c
+++ b/merge-file.c
@@ -30,7 +30,7 @@ static void *three_way_filemerge(const char *path, mmfile_t *base, mmfile_t *our
int merge_status;
mmbuffer_t res;
- merge_status = ll_merge(&res, path, base,
+ merge_status = ll_merge(&res, path, base, NULL,
our, ".our", their, ".their", 0);
if (merge_status < 0)
return NULL;
diff --git a/merge-recursive.c b/merge-recursive.c
index 195ebf9..3b2cc9d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -640,7 +640,7 @@ static int merge_3way(struct merge_options *o,
read_mmblob(&src1, a->sha1);
read_mmblob(&src2, b->sha1);
- merge_status = ll_merge(result_buf, a->path, &orig,
+ merge_status = ll_merge(result_buf, a->path, &orig, NULL,
&src1, name1, &src2, name2,
(!!o->call_depth) | (favor << 1));
diff --git a/rerere.c b/rerere.c
index a59f74f..f221bed 100644
--- a/rerere.c
+++ b/rerere.c
@@ -319,7 +319,7 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
if (!mmfile[i].ptr && !mmfile[i].size)
mmfile[i].ptr = xstrdup("");
}
- ll_merge(&result, path, &mmfile[0],
+ ll_merge(&result, path, &mmfile[0], NULL,
&mmfile[1], "ours",
&mmfile[2], "theirs", 0);
for (i = 0; i < 3; i++)
@@ -376,7 +376,7 @@ static int merge(const char *name, const char *path)
ret = 1;
goto out;
}
- ret = ll_merge(&result, path, &base, &cur, "", &other, "", 0);
+ ret = ll_merge(&result, path, &base, NULL, &cur, "", &other, "", 0);
if (!ret) {
FILE *f = fopen(path, "w");
if (!f)
--
1.7.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 04/10] checkout --conflict=diff3: add a label for ancestor
2010-03-15 7:47 ` [PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3 Jonathan Nieder
` (2 preceding siblings ...)
2010-03-15 7:52 ` [PATCH 03/10] ll_merge(): add ancestor label parameter for diff3-style output Jonathan Nieder
@ 2010-03-15 7:55 ` Jonathan Nieder
2010-03-15 8:20 ` Junio C Hamano
2010-03-15 7:56 ` [PATCH 05/10] merge_file(): add comment explaining behavior wrt conflict style Jonathan Nieder
` (5 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 7:55 UTC (permalink / raw)
To: git; +Cc: Stefan Monnier
git checkout --conflict=diff3 can be used to present conflicts hunks
including text from the common ancestor:
<<<<<<< ours
ourside
|||||||
original
=======
theirside
>>>>>>> theirs
The added information can be very helpful for resolving a merge by
hand, and merge tools can usually understand it without trouble
because it looks like output from ‘diff3 -m’.
Unfortunately, not all can: ‘diff3’ includes a label for the merge
base on the ||||||| line and it seems some tools cannot parse conflict
hunks without such a label. Humans could use help in interpreting the
output, too. So mark the start of the text from the common ancestor
with the label “||||||| original”.
git rerere will not have trouble parsing this output, since instead of
looking for a newline, it looks for whitespace after the |||||||
marker. No other code in git tries to parse conflict hunks.
Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Since “original” a good name for the common ancestor? I also
considered “base” and “ancestor”; the latter is too jargon-y for my
taste, but “base” seems all right.
builtin/checkout.c | 2 +-
t/t7201-co.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index d67f809..c60c3e0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -149,7 +149,7 @@ static int checkout_merged(int pos, struct checkout *state)
read_mmblob(&ours, active_cache[pos+1]->sha1);
read_mmblob(&theirs, active_cache[pos+2]->sha1);
- status = ll_merge(&result_buf, path, &ancestor, NULL,
+ status = ll_merge(&result_buf, path, &ancestor, "original",
&ours, "ours", &theirs, "theirs", 0);
free(ancestor.ptr);
free(ours.ptr);
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index d20ed61..ebfa5ca 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -481,7 +481,7 @@ test_expect_success 'checkout with --merge, in diff3 -m style' '
(
echo "<<<<<<< ours"
echo ourside
- echo "|||||||"
+ echo "||||||| original"
echo original
echo "======="
echo theirside
@@ -525,7 +525,7 @@ test_expect_success 'checkout --conflict=diff3' '
(
echo "<<<<<<< ours"
echo ourside
- echo "|||||||"
+ echo "||||||| original"
echo original
echo "======="
echo theirside
--
1.7.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 04/10] checkout --conflict=diff3: add a label for ancestor
2010-03-15 7:55 ` [PATCH 04/10] checkout --conflict=diff3: add a label for ancestor Jonathan Nieder
@ 2010-03-15 8:20 ` Junio C Hamano
2010-03-15 8:32 ` Jonathan Nieder
0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2010-03-15 8:20 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Stefan Monnier
Jonathan Nieder <jrnieder@gmail.com> writes:
> git checkout --conflict=diff3 can be used to present conflicts hunks
> including text from the common ancestor:
>
> <<<<<<< ours
> ourside
> |||||||
> original
> =======
> theirside
> >>>>>>> theirs
>
> The added information can be very helpful for resolving a merge by
> hand, and merge tools can usually understand it without trouble
> because it looks like output from ‘diff3 -m’.
>
> Unfortunately, not all can: ‘diff3’ includes a label for the merge
> base on the ||||||| line and it seems some tools cannot parse conflict
> hunks without such a label. Humans could use help in interpreting the
> output, too. So mark the start of the text from the common ancestor
> with the label “||||||| original”.
>
> git rerere will not have trouble parsing this output, since instead of
> looking for a newline, it looks for whitespace after the |||||||
> marker.
Missing:
"... and adding the extra label will not affect the computed the conflict
identifier, so existing rerere database will not be invalidated with this
change either".
I didn't verified the above claim, but if it does not hold true, then we
need to think the transition strategy. I don't expect a problem, though.
> Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
This "Reported" feels very odd for a feature enhancement ("Requested"
would be more appropriate) not a bugfix.
> ---
> Since “original” a good name for the common ancestor? I also
> considered “base” and “ancestor”; the latter is too jargon-y for my
> taste, but “base” seems all right.
Yeah, base sounds good. Even though at the lowest level, a merge is a
merge between two equals, people tend to think of the contents of their
own side "original" (vs the merge outcome "result").
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 04/10] checkout --conflict=diff3: add a label for ancestor
2010-03-15 8:20 ` Junio C Hamano
@ 2010-03-15 8:32 ` Jonathan Nieder
0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 8:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Stefan Monnier
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> git rerere will not have trouble parsing this output, since instead of
>> looking for a newline, it looks for whitespace after the |||||||
>> marker.
>
> Missing:
>
> "... and adding the extra label will not affect the computed the conflict
> identifier, so existing rerere database will not be invalidated with this
> change either".
>
> I didn't verified the above claim, but if it does not hold true, then we
> need to think the transition strategy. I don't expect a problem, though.
It holds. handle_path() in rerere.c actually recreates the conflict hunk
after parsing it:
rerere_io_putconflict('<', marker_size, io);
rerere_io_putmem(one.buf, one.len, io);
rerere_io_putconflict('=', marker_size, io);
rerere_io_putmem(two.buf, two.len, io);
rerere_io_putconflict('>', marker_size, io);
This is a piece of defensive programing by Dscho that has lasted since
rerere was first made builtin. It shelters rerere from changes in the
format of the conflict markers.
>> Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>
> This "Reported" feels very odd for a feature enhancement ("Requested"
> would be more appropriate) not a bugfix.
Makes sense, thanks.
>> ---
>> Since “original” a good name for the common ancestor? I also
>> considered “base” and “ancestor”; the latter is too jargon-y for my
>> taste, but “base” seems all right.
>
> Yeah, base sounds good. Even though at the lowest level, a merge is a
> merge between two equals, people tend to think of the contents of their
> own side "original" (vs the merge outcome "result").
Sound good. “base” it is, then.
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 05/10] merge_file(): add comment explaining behavior wrt conflict style
2010-03-15 7:47 ` [PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3 Jonathan Nieder
` (3 preceding siblings ...)
2010-03-15 7:55 ` [PATCH 04/10] checkout --conflict=diff3: add a label for ancestor Jonathan Nieder
@ 2010-03-15 7:56 ` Jonathan Nieder
2010-03-15 8:00 ` [PATCH 06/10] merge_trees(): add ancestor label parameter for diff3-style output Jonathan Nieder
` (4 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 7:56 UTC (permalink / raw)
To: git; +Cc: Stefan Monnier
The merge_file() function is a helper for ‘git read-tree’, which does
not respect the merge.conflictstyle option, so there is no need to
worry about its ancestor_name parameter for ll_merge(). Add a comment
to this effect.
Signed-off-by: Jonathan Nieder <jrnieder@mgila.com>
---
There are more non-changes (e.g., in builtin/merge-tree.c), but none
of them seemed nonobvious like this one.
merge-file.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/merge-file.c b/merge-file.c
index 07cc0c9..c336c93 100644
--- a/merge-file.c
+++ b/merge-file.c
@@ -30,6 +30,12 @@ static void *three_way_filemerge(const char *path, mmfile_t *base, mmfile_t *our
int merge_status;
mmbuffer_t res;
+ /*
+ * This function is only used by cmd_merge_tree, which
+ * does not respect the merge.conflictstyle option.
+ * There is no need to worry about a label for the
+ * common ancestor.
+ */
merge_status = ll_merge(&res, path, base, NULL,
our, ".our", their, ".their", 0);
if (merge_status < 0)
--
1.7.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 06/10] merge_trees(): add ancestor label parameter for diff3-style output
2010-03-15 7:47 ` [PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3 Jonathan Nieder
` (4 preceding siblings ...)
2010-03-15 7:56 ` [PATCH 05/10] merge_file(): add comment explaining behavior wrt conflict style Jonathan Nieder
@ 2010-03-15 8:00 ` Jonathan Nieder
2010-03-15 8:00 ` [PATCH 07/10] tests: document format of conflicts from checkout -m Jonathan Nieder
` (3 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 8:00 UTC (permalink / raw)
To: git; +Cc: Stefan Monnier
Commands using the merge_trees() machinery will present conflict hunks
in output something like what ‘diff3 -m’ produces if the
merge.conflictstyle configuration option is set to diff3.
Unfortunately, the format is not fully compatible yet. ‘diff3’
includes the name of the merge base on the ||||||| line of the output,
and tools can misparse the conflict hunks without it. Add a new
o->ancestor parameter to merge_trees() for use as a label for the
ancestor in conflict hunks to give callers the power to rectify this
situation.
If o->ancestor is NULL, the output format is as before. All callers
pass NULL, so this patch does not affect the outward behavior of git.
If o->ancestor is non-NULL and both branches renamed the base file
to the same name, that name is included in the conflict hunk labels.
Even if o->ancestor is NULL I think this would be a good change, but
this patch only does it in the non-NULL case to ensure the output
format does not change where it might matter.
Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Another no-op.
merge-recursive.c | 11 ++++++++---
merge-recursive.h | 1 +
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 3b2cc9d..017cafd 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -608,7 +608,7 @@ static int merge_3way(struct merge_options *o,
const char *branch2)
{
mmfile_t orig, src1, src2;
- char *name1, *name2;
+ char *base_name, *name1, *name2;
int merge_status;
int favor;
@@ -628,10 +628,15 @@ static int merge_3way(struct merge_options *o,
}
}
- if (strcmp(a->path, b->path)) {
+ if (strcmp(a->path, b->path) ||
+ (o->ancestor != NULL && strcmp(a->path, one->path) != 0)) {
+ base_name = o->ancestor == NULL ? NULL :
+ xstrdup(mkpath("%s:%s", o->ancestor, one->path));
name1 = xstrdup(mkpath("%s:%s", branch1, a->path));
name2 = xstrdup(mkpath("%s:%s", branch2, b->path));
} else {
+ base_name = o->ancestor == NULL ? NULL :
+ xstrdup(mkpath("%s", o->ancestor));
name1 = xstrdup(mkpath("%s", branch1));
name2 = xstrdup(mkpath("%s", branch2));
}
@@ -640,7 +645,7 @@ static int merge_3way(struct merge_options *o,
read_mmblob(&src1, a->sha1);
read_mmblob(&src2, b->sha1);
- merge_status = ll_merge(result_buf, a->path, &orig, NULL,
+ merge_status = ll_merge(result_buf, a->path, &orig, base_name,
&src1, name1, &src2, name2,
(!!o->call_depth) | (favor << 1));
diff --git a/merge-recursive.h b/merge-recursive.h
index be8410a..d1192f5 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -4,6 +4,7 @@
#include "string-list.h"
struct merge_options {
+ const char *ancestor;
const char *branch1;
const char *branch2;
enum {
--
1.7.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 07/10] tests: document format of conflicts from checkout -m
2010-03-15 7:47 ` [PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3 Jonathan Nieder
` (5 preceding siblings ...)
2010-03-15 8:00 ` [PATCH 06/10] merge_trees(): add ancestor label parameter for diff3-style output Jonathan Nieder
@ 2010-03-15 8:00 ` Jonathan Nieder
2010-03-15 8:01 ` [PATCH 08/10] checkout -m --conflict=diff3: add a label for ancestor Jonathan Nieder
` (2 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 8:00 UTC (permalink / raw)
To: git; +Cc: Stefan Monnier
We are about to change the format of the conflict hunks that ‘checkout
--merge’ writes. Add tests checking the current behavior first.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t7201-co.sh | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 61 insertions(+), 4 deletions(-)
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index ebfa5ca..970a460 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -11,10 +11,12 @@ Test switching across them.
! [master] Initial A one, A two
* [renamer] Renamer R one->uno, M two
! [side] Side M one, D two, A three
- ---
- + [side] Side M one, D two, A three
- * [renamer] Renamer R one->uno, M two
- +*+ [master] Initial A one, A two
+ ! [simple] Simple D one, M two
+ ----
+ + [simple] Simple D one, M two
+ + [side] Side M one, D two, A three
+ * [renamer] Renamer R one->uno, M two
+ +*++ [master] Initial A one, A two
'
@@ -52,6 +54,11 @@ test_expect_success setup '
git update-index --add --remove one two three &&
git commit -m "Side M one, D two, A three" &&
+ git checkout -b simple master &&
+ rm -f one &&
+ fill a c e > two &&
+ git commit -a -m "Simple D one, M two" &&
+
git checkout master
'
@@ -166,6 +173,56 @@ test_expect_success 'checkout -m with merge conflict' '
! test -s current
'
+test_expect_success 'format of merge conflict from checkout -m' '
+
+ git checkout -f master && git clean -f &&
+
+ fill b d > two &&
+ git checkout -m simple &&
+
+ git ls-files >current &&
+ fill same two two two >expect &&
+ test_cmp current expect &&
+
+ cat <<-EOF >expect &&
+ <<<<<<< simple
+ a
+ c
+ e
+ =======
+ b
+ d
+ >>>>>>> local
+ EOF
+ test_cmp two expect
+'
+
+test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
+
+ git checkout -f master && git reset --hard && git clean -f &&
+
+ fill b d > two &&
+ git checkout --merge --conflict=diff3 simple &&
+
+ cat <<-EOF >expect &&
+ <<<<<<< simple
+ a
+ c
+ e
+ |||||||
+ a
+ b
+ c
+ d
+ e
+ =======
+ b
+ d
+ >>>>>>> local
+ EOF
+ test_cmp two expect
+'
+
test_expect_success 'checkout to detach HEAD (with advice declined)' '
git config advice.detachedHead false &&
--
1.7.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 08/10] checkout -m --conflict=diff3: add a label for ancestor
2010-03-15 7:47 ` [PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3 Jonathan Nieder
` (6 preceding siblings ...)
2010-03-15 8:00 ` [PATCH 07/10] tests: document format of conflicts from checkout -m Jonathan Nieder
@ 2010-03-15 8:01 ` Jonathan Nieder
2010-03-15 8:05 ` [PATCH 09/10] cherry-pick: " Jonathan Nieder
2010-03-15 8:07 ` [PATCH 10/10] merge-recursive: " Jonathan Nieder
9 siblings, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 8:01 UTC (permalink / raw)
To: git; +Cc: Stefan Monnier
git checkout --merge --conflict=diff3 can be used to present conflicts
hunks including text from the common ancestor.
The added information can be very helpful for resolving a merge by
hand, and many merge tools can usually grok it because it is very
similar to the output from ‘diff3 -m’.
Unfortunately, some tools reportedly cannot parse the conflict hunks
because of a small difference from diff3: diff3 -m includes a label
for the merge base on the ||||||| line. Humans can benefit from a
cue when learning to interpreting the format, too. So mark the start
of the text from the old branch with a label based on the branch’s
name.
git rerere will not have trouble parsing this output, since instead of
looking for a newline, it looks for whitespace after the |||||||
marker. No other code in git tries to parse conflict hunks.
Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/checkout.c | 1 +
t/t7201-co.sh | 2 +-
2 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index c60c3e0..ebc5891 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -439,6 +439,7 @@ static int merge_working_tree(struct checkout_opts *opts,
ret = reset_tree(new->commit->tree, opts, 1);
if (ret)
return ret;
+ o.ancestor = old->name;
o.branch1 = new->name;
o.branch2 = "local";
merge_trees(&o, new->commit->tree, work,
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 970a460..ae14b19 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -209,7 +209,7 @@ test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
a
c
e
- |||||||
+ ||||||| master
a
b
c
--
1.7.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 09/10] cherry-pick: add a label for ancestor
2010-03-15 7:47 ` [PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3 Jonathan Nieder
` (7 preceding siblings ...)
2010-03-15 8:01 ` [PATCH 08/10] checkout -m --conflict=diff3: add a label for ancestor Jonathan Nieder
@ 2010-03-15 8:05 ` Jonathan Nieder
2010-03-15 8:07 ` [PATCH 10/10] merge-recursive: " Jonathan Nieder
9 siblings, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 8:05 UTC (permalink / raw)
To: git; +Cc: Stefan Monnier
git cherry-pick and git revert will present conflict hunks in output
something like what ‘diff3 -m’ produces if the merge.conflictstyle
configuration option is set to diff3.
Unfortunately, some tools reportedly cannot parse the conflict hunks
because of a small difference from diff3: diff3 -m includes a label
for the merge base on the ||||||| line. Humans parsing the conflict
hunks by hand might want to know what the common ancestor represents,
too. So mark the start of the text from the parent of the commit
being cherrypicked (resp. from the commit being reverted) with the
label "parent".
It would be nicer to use a more informative label, and that might
happen in the future.
git rerere will not have trouble parsing this output, since instead of
looking for a newline, it looks for whitespace after the |||||||
marker. No other code in git tries to parse conflict hunks.
Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
In the revert case, this gives
<<<<<<< HEAD
foo
||||||| parent
bar
=======
baz
>>>>>>> ab78c93 "Make some change we would like to revert"
This is a bit misleading, since actually “baz” is from the commit
to revert’s parent and “bar” is from the commit to revert.
Probably the labels should be switched. I didn’t do this because
it is late and it would have been a larger change from the current
behavior.
It also would be nice to suppress the ancestor and do a two-way
merge when cherry-picking a root commit. I haven’t yet looked into
how hard this would be.
builtin/revert.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index eff5268..a61dd9a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -372,6 +372,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
read_cache();
init_merge_options(&o);
+ o.ancestor = base ? "parent" : "(empty tree)";
o.branch1 = "HEAD";
o.branch2 = oneline;
--
1.7.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 10/10] merge-recursive: add a label for ancestor
2010-03-15 7:47 ` [PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3 Jonathan Nieder
` (8 preceding siblings ...)
2010-03-15 8:05 ` [PATCH 09/10] cherry-pick: " Jonathan Nieder
@ 2010-03-15 8:07 ` Jonathan Nieder
2010-03-15 8:29 ` Junio C Hamano
2010-03-15 13:18 ` Stefan Monnier
9 siblings, 2 replies; 23+ messages in thread
From: Jonathan Nieder @ 2010-03-15 8:07 UTC (permalink / raw)
To: git; +Cc: Stefan Monnier
git merge-recursive (and hence git merge) will present conflict hunks
in output something like what ‘diff3 -m’ produces if the
merge.conflictstyle configuration option is set to diff3.
Unfortunately, some tools reportedly cannot parse the conflict hunks
because of a small difference from diff3: diff3 -m includes a label
for the merge base on the ||||||| line. Humans unfamiliar with the
format would do a better job with a label, too. Mark the start of the
text from the merge bases with the heading "||||||| merged common
ancestors".
It would be nicer to use a more informative label, and that might
happen in the future.
git rerere does not have trouble parsing this output, since instead of
looking for a newline, it looks for whitespace after the |||||||
marker. No other code in git tries to parse conflict hunks.
Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I would be interested to know which merge tool chokes on |||||||. ;)
As it is, since I haven’t experienced the mechanical misbehavior, I
am targetting humans.
That’s the end of the series. Thanks for reading.
Good night,
Jonathan
merge-recursive.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 017cafd..917397c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1347,6 +1347,7 @@ int merge_recursive(struct merge_options *o,
if (!o->call_depth)
read_cache();
+ o->ancestor = "merged common ancestors";
clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,
&mrtree);
--
1.7.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 10/10] merge-recursive: add a label for ancestor
2010-03-15 8:07 ` [PATCH 10/10] merge-recursive: " Jonathan Nieder
@ 2010-03-15 8:29 ` Junio C Hamano
2010-03-15 13:18 ` Stefan Monnier
1 sibling, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2010-03-15 8:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Stefan Monnier
Jonathan Nieder <jrnieder@gmail.com> writes:
> I would be interested to know which merge tool chokes on |||||||. ;)
> As it is, since I haven’t experienced the mechanical misbehavior, I
> am targetting humans.
Even if there weren't any broken tools, I think targetting humans is a
good thing to do, as long as we don't break ourselves (including old
rerere database).
> That’s the end of the series. Thanks for reading.
Thanks for writing. I think this is generally a good thing to have,
and from my cursory look I didn't see anything glaringly wrong except for
the one I already commented on the first patch.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 10/10] merge-recursive: add a label for ancestor
2010-03-15 8:07 ` [PATCH 10/10] merge-recursive: " Jonathan Nieder
2010-03-15 8:29 ` Junio C Hamano
@ 2010-03-15 13:18 ` Stefan Monnier
1 sibling, 0 replies; 23+ messages in thread
From: Stefan Monnier @ 2010-03-15 13:18 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git
> I would be interested to know which merge tool chokes on |||||||. ;)
In my case, it was smerge-mode.el (a minor mode included in Emacs since
Emacs-22, which highlights such conflicts and tries to help you resolve
them).
Stefan
PS: As you may have guessed, I'm the primary author of this
Emacs package.
^ permalink raw reply [flat|nested] 23+ messages in thread