* [PATCH 1/2] xdl_fill_merge_buffer(): separate out a too deeply nested function @ 2008-08-29 0:09 Junio C Hamano 2008-08-29 0:18 ` [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2008-08-29 0:09 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git This simply moves code around to make a separate function that prepares a single conflicted hunk with markers into the buffer. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * I am sending this out because I need an extra set of eyeballs and a bit of insight into this code from you for the second part, which is still an early RFC/WIP, and it depends on this refactoring. xdiff/xmerge.c | 121 ++++++++++++++++++++++++++++++++----------------------- 1 files changed, 70 insertions(+), 51 deletions(-) diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c index 82b3573..6ffaa4f 100644 --- a/xdiff/xmerge.c +++ b/xdiff/xmerge.c @@ -113,65 +113,84 @@ static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest) return size; } -static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1, - xdfenv_t *xe2, const char *name2, xdmerge_t *m, char *dest) +static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1, + xdfenv_t *xe2, const char *name2, + int size, int i, + xdmerge_t *m, char *dest) { const int marker_size = 7; int marker1_size = (name1 ? strlen(name1) + 1 : 0); int marker2_size = (name2 ? strlen(name2) + 1 : 0); - int conflict_marker_size = 3 * (marker_size + 1) - + marker1_size + marker2_size; - int size, i1, j; - - for (size = i1 = 0; m; m = m->next) { - if (m->mode == 0) { - size += xdl_recs_copy(xe1, i1, m->i1 - i1, 0, - dest ? dest + size : NULL); - if (dest) { - for (j = 0; j < marker_size; j++) - dest[size++] = '<'; - if (marker1_size) { - dest[size] = ' '; - memcpy(dest + size + 1, name1, - marker1_size - 1); - size += marker1_size; - } - dest[size++] = '\n'; - } else - size += conflict_marker_size; - size += xdl_recs_copy(xe1, m->i1, m->chg1, 1, - dest ? dest + size : NULL); - if (dest) { - for (j = 0; j < marker_size; j++) - dest[size++] = '='; - dest[size++] = '\n'; - } - size += xdl_recs_copy(xe2, m->i2, m->chg2, 1, - dest ? dest + size : NULL); - if (dest) { - for (j = 0; j < marker_size; j++) - dest[size++] = '>'; - if (marker2_size) { - dest[size] = ' '; - memcpy(dest + size + 1, name2, - marker2_size - 1); - size += marker2_size; - } - dest[size++] = '\n'; - } - } else if (m->mode == 1) - size += xdl_recs_copy(xe1, i1, m->i1 + m->chg1 - i1, 0, - dest ? dest + size : NULL); + int j; + + /* Before conflicting part */ + size += xdl_recs_copy(xe1, i, m->i1 - i, 0, + dest ? dest + size : NULL); + + if (!dest) { + size += marker_size + 1 + marker1_size; + } else { + for (j = 0; j < marker_size; j++) + dest[size++] = '<'; + if (marker1_size) { + dest[size] = ' '; + memcpy(dest + size + 1, name1, marker1_size - 1); + size += marker1_size; + } + dest[size++] = '\n'; + } + + /* Postimage from side #1 */ + size += xdl_recs_copy(xe1, m->i1, m->chg1, 1, + dest ? dest + size : NULL); + if (!dest) { + size += marker_size + 1; + } else { + for (j = 0; j < marker_size; j++) + dest[size++] = '='; + dest[size++] = '\n'; + } + + /* Postimage from side #2 */ + size += xdl_recs_copy(xe2, m->i2, m->chg2, 1, + dest ? dest + size : NULL); + if (!dest) { + size += marker_size + 1 + marker2_size; + } else { + for (j = 0; j < marker_size; j++) + dest[size++] = '>'; + if (marker2_size) { + dest[size] = ' '; + memcpy(dest + size + 1, name2, marker2_size - 1); + size += marker2_size; + } + dest[size++] = '\n'; + } + return size; +} + +static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1, + xdfenv_t *xe2, const char *name2, xdmerge_t *m, char *dest) +{ + int size, i; + + for (size = i = 0; m; m = m->next) { + if (m->mode == 0) + size = fill_conflict_hunk(xe1, name1, xe2, name2, + size, i, m, dest); + else if (m->mode == 1) + size += xdl_recs_copy(xe1, i, m->i1 + m->chg1 - i, 0, + dest ? dest + size : NULL); else if (m->mode == 2) - size += xdl_recs_copy(xe2, m->i2 - m->i1 + i1, - m->i1 + m->chg2 - i1, 0, - dest ? dest + size : NULL); + size += xdl_recs_copy(xe2, m->i2 - m->i1 + i, + m->i1 + m->chg2 - i, 0, + dest ? dest + size : NULL); else continue; - i1 = m->i1 + m->chg1; + i = m->i1 + m->chg1; } - size += xdl_recs_copy(xe1, i1, xe1->xdf2.nrec - i1, 0, - dest ? dest + size : NULL); + size += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0, + dest ? dest + size : NULL); return size; } -- 1.6.0.1.200.ge682f ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style 2008-08-29 0:09 [PATCH 1/2] xdl_fill_merge_buffer(): separate out a too deeply nested function Junio C Hamano @ 2008-08-29 0:18 ` Junio C Hamano 2008-08-29 0:34 ` Linus Torvalds 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2008-08-29 0:18 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git When showing conflicting merges, we traditionally followed RCS's merge output format. The output shows: <<<<<<< postimage from one side; ======= postimage of the other side; and >>>>>>> Some poeple find it easier to be able to understand what is going on when they can view the common ancestor's version, which is used by "diff3 -m", which shows: <<<<<<< postimage from one side; ||||||| shared preimage; ======= postimage of the other side; and >>>>>>> This is an initial step to bring that as an optional feature to git. Only "git merge-file" has been converted, with "--diff3" option. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * This is RFH/WIP partly because I tested it only on simplest cases, but more importantly I suspect this should be orthogonal to the refinement level parameter. The idea is to keep (just like <i1,chg1>, <i2,chg2> you use to point at the postimage of both sides) <i0,chg0> in xdmerge_t that point at the corresponding part of the shared preimage. I think I got the initial computation of <i0,chg0>, coalescing done in xdl_append_merge(), and use of that in xdl_fill_merge_buffer() right, but I couldn't figure out how to adjust them inside the merge refinement logic. Of course the eventual goal is to add this to merge-file.c, give a new configuration variable to allow people to use this, while teaching rerere to ignore the new '|||||||' + shared preimage section, so that existing rerere information can be reused for new conflicts in diff3 style output. builtin-merge-file.c | 6 ++- xdiff/xdiff.h | 5 +- xdiff/xmerge.c | 145 ++++++++++++++++++++++++++++++++++++++------------ 3 files changed, 119 insertions(+), 37 deletions(-) diff --git a/builtin-merge-file.c b/builtin-merge-file.c index 3605960..2f69aa1 100644 --- a/builtin-merge-file.c +++ b/builtin-merge-file.c @@ -13,6 +13,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) mmbuffer_t result = {NULL, 0}; xpparam_t xpp = {XDF_NEED_MINIMAL}; int ret = 0, i = 0, to_stdout = 0; + int level = XDL_MERGE_ZEALOUS_ALNUM; while (argc > 4) { if (!strcmp(argv[1], "-L") && i < 3) { @@ -25,6 +26,9 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) else if (!strcmp(argv[1], "-q") || !strcmp(argv[1], "--quiet")) freopen("/dev/null", "w", stderr); + else if (!strcmp(argv[1], "-3") || + !strcmp(argv[1], "--diff3")) + level = XDL_MERGE_DIFF3; else usage(merge_file_usage); argc--; @@ -46,7 +50,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) } ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2], - &xpp, XDL_MERGE_ZEALOUS_ALNUM, &result); + &xpp, level, &result); for (i = 0; i < 3; i++) free(mmfs[i].ptr); diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h index 413082e..a7b6e08 100644 --- a/xdiff/xdiff.h +++ b/xdiff/xdiff.h @@ -52,8 +52,9 @@ extern "C" { #define XDL_MERGE_MINIMAL 0 #define XDL_MERGE_EAGER 1 -#define XDL_MERGE_ZEALOUS 2 -#define XDL_MERGE_ZEALOUS_ALNUM 3 +#define XDL_MERGE_DIFF3 2 +#define XDL_MERGE_ZEALOUS 10 +#define XDL_MERGE_ZEALOUS_ALNUM 11 typedef struct s_mmfile { char *ptr; diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c index 6ffaa4f..ac6cc40 100644 --- a/xdiff/xmerge.c +++ b/xdiff/xmerge.c @@ -30,17 +30,32 @@ typedef struct s_xdmerge { * 2 = no conflict, take second. */ int mode; + /* + * These point at the respective postimages. E.g. <i1,chg1> is + * how side #1 wants to change the common ancestor; if there is no + * overlap, lines before i1 in the postimage of side #1 appear + * in the merge result as a region touched by neither side. + */ long i1, i2; long chg1, chg2; + /* + * These point at the preimage; of course there is just one + * preimage, that is from the shared common ancestor. + */ + long i0; + long chg0; } xdmerge_t; static int xdl_append_merge(xdmerge_t **merge, int mode, - long i1, long chg1, long i2, long chg2) + long i0, long chg0, + long i1, long chg1, + long i2, long chg2) { xdmerge_t *m = *merge; if (m && (i1 <= m->i1 + m->chg1 || i2 <= m->i2 + m->chg2)) { if (mode != m->mode) m->mode = 0; + m->chg0 = i0 + chg0 - m->i0; m->chg1 = i1 + chg1 - m->i1; m->chg2 = i2 + chg2 - m->i2; } else { @@ -49,6 +64,8 @@ static int xdl_append_merge(xdmerge_t **merge, int mode, return -1; m->next = NULL; m->mode = mode; + m->i0 = i0; + m->chg0 = chg0; m->i1 = i1; m->chg1 = chg1; m->i2 = i2; @@ -91,11 +108,13 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2, return 0; } -static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest) +static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add_nl, char *dest) { - xrecord_t **recs = xe->xdf2.recs + i; + xrecord_t **recs; int size = 0; + recs = (use_orig ? xe->xdf1.recs : xe->xdf2.recs) + i; + if (count < 1) return 0; @@ -113,9 +132,19 @@ static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest) return size; } +static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest) +{ + return xdl_recs_copy_0(0, xe, i, count, add_nl, dest); +} + +static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest) +{ + return xdl_recs_copy_0(1, xe, i, count, add_nl, dest); +} + static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1, xdfenv_t *xe2, const char *name2, - int size, int i, + int size, int i, int level, xdmerge_t *m, char *dest) { const int marker_size = 7; @@ -143,6 +172,20 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1, /* Postimage from side #1 */ size += xdl_recs_copy(xe1, m->i1, m->chg1, 1, dest ? dest + size : NULL); + + if (level == XDL_MERGE_DIFF3) { + /* Shared preimage */ + if (!dest) { + size += marker_size + 1; + } else { + for (j = 0; j < marker_size; j++) + dest[size++] = '|'; + dest[size++] = '\n'; + } + size += xdl_orig_copy(xe1, m->i0, m->chg0, 1, + dest ? dest + size : NULL); + } + if (!dest) { size += marker_size + 1; } else { @@ -170,14 +213,15 @@ 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, xdmerge_t *m, char *dest) + xdfenv_t *xe2, const char *name2, + xdmerge_t *m, char *dest, int level) { int size, i; for (size = i = 0; m; m = m->next) { if (m->mode == 0) size = fill_conflict_hunk(xe1, name1, xe2, name2, - size, i, m, dest); + size, i, level, m, dest); else if (m->mode == 1) size += xdl_recs_copy(xe1, i, m->i1 + m->chg1 - i, 0, dest ? dest + size : NULL); @@ -332,19 +376,31 @@ static int xdl_simplify_non_conflicts(xdfenv_t *xe1, xdmerge_t *m, } /* - * level == 0: mark all overlapping changes as conflict - * level == 1: mark overlapping changes as conflict only if not identical - * level == 2: analyze non-identical changes for minimal conflict set - * level == 3: analyze non-identical changes for minimal conflict set, but - * treat hunks not containing any letter or number as conflicting + * "Level" parameter can be: + * + * (MINIMAL): + * Mark all overlapping changes as conflict + * + * (EAGER): + * Mark overlapping changes as conflict only if not identical + * + * (DIFF3): + * Same as EAGER but show the shared preimage in the output as well + * + * (ZEALOUS): + * Analyze non-identical changes for minimal conflict set + * + * (ZEALOUS_ALNUM): + * Analyze non-identical changes for minimal conflict set, but + * treat hunks not containing any letter or number as conflicting * * returns < 0 on error, == 0 for no conflicts, else number of conflicts */ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, - xdfenv_t *xe2, xdchange_t *xscr2, const char *name2, - int level, xpparam_t const *xpp, mmbuffer_t *result) { + xdfenv_t *xe2, xdchange_t *xscr2, const char *name2, + int level, xpparam_t const *xpp, mmbuffer_t *result) { xdmerge_t *changes, *c; - int i1, i2, chg1, chg2; + int i0, i1, i2, chg0, chg1, chg2; c = changes = NULL; @@ -352,11 +408,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, if (!changes) changes = c; if (xscr1->i1 + xscr1->chg1 < xscr2->i1) { + i0 = xscr1->i1; i1 = xscr1->i2; i2 = xscr2->i2 - xscr2->i1 + xscr1->i1; + chg0 = xscr1->chg1; chg1 = xscr1->chg2; chg2 = xscr1->chg1; - if (xdl_append_merge(&c, 1, i1, chg1, i2, chg2)) { + if (xdl_append_merge(&c, 1, + i0, chg0, i1, chg1, i2, chg2)) { xdl_cleanup_merge(changes); return -1; } @@ -364,40 +423,50 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, continue; } if (xscr2->i1 + xscr2->chg1 < xscr1->i1) { + i0 = xscr2->i1; i1 = xscr1->i2 - xscr1->i1 + xscr2->i1; i2 = xscr2->i2; + chg0 = xscr2->chg1; chg1 = xscr2->chg1; chg2 = xscr2->chg2; - if (xdl_append_merge(&c, 2, i1, chg1, i2, chg2)) { + if (xdl_append_merge(&c, 2, + i0, chg0, i1, chg1, i2, chg2)) { xdl_cleanup_merge(changes); return -1; } xscr2 = xscr2->next; continue; } - if (level < 1 || xscr1->i1 != xscr2->i1 || - xscr1->chg1 != xscr2->chg1 || - xscr1->chg2 != xscr2->chg2 || - xdl_merge_cmp_lines(xe1, xscr1->i2, + if (level == XDL_MERGE_MINIMAL || + (xscr1->i1 != xscr2->i1 || + xscr1->chg1 != xscr2->chg1 || + xscr1->chg2 != xscr2->chg2) || + xdl_merge_cmp_lines(xe1, xscr1->i2, xe2, xscr2->i2, xscr1->chg2, xpp->flags)) { /* conflict */ int off = xscr1->i1 - xscr2->i1; int ffo = off + xscr1->chg1 - xscr2->chg1; + i0 = xscr1->i1; i1 = xscr1->i2; i2 = xscr2->i2; - if (off > 0) + if (off > 0) { + i0 -= off; i1 -= off; + } else i2 += off; + chg0 = xscr1->i1 + xscr1->chg1 - i0; chg1 = xscr1->i2 + xscr1->chg2 - i1; chg2 = xscr2->i2 + xscr2->chg2 - i2; - if (ffo > 0) - chg2 += ffo; - else + if (ffo < 0) { + chg0 -= ffo; chg1 -= ffo; - if (xdl_append_merge(&c, 0, i1, chg1, i2, chg2)) { + } else + chg2 += ffo; + if (xdl_append_merge(&c, 0, + i0, chg0, i1, chg1, i2, chg2)) { xdl_cleanup_merge(changes); return -1; } @@ -414,11 +483,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, while (xscr1) { if (!changes) changes = c; + i0 = xscr1->i1; i1 = xscr1->i2; i2 = xscr1->i1 + xe2->xdf2.nrec - xe2->xdf1.nrec; + chg0 = xscr1->chg1; chg1 = xscr1->chg2; chg2 = xscr1->chg1; - if (xdl_append_merge(&c, 1, i1, chg1, i2, chg2)) { + if (xdl_append_merge(&c, 1, + i0, chg0, i1, chg1, i2, chg2)) { xdl_cleanup_merge(changes); return -1; } @@ -427,11 +499,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, while (xscr2) { if (!changes) changes = c; + i0 = xscr2->i1; i1 = xscr2->i1 + xe1->xdf2.nrec - xe1->xdf1.nrec; i2 = xscr2->i2; + chg0 = xscr2->chg1; chg1 = xscr2->chg1; chg2 = xscr2->chg2; - if (xdl_append_merge(&c, 2, i1, chg1, i2, chg2)) { + if (xdl_append_merge(&c, 2, + i0, chg0, i1, chg1, i2, chg2)) { xdl_cleanup_merge(changes); return -1; } @@ -440,16 +515,17 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, if (!changes) changes = c; /* refine conflicts */ - if (level > 1 && + if (XDL_MERGE_ZEALOUS <= level && (xdl_refine_conflicts(xe1, xe2, changes, xpp) < 0 || - xdl_simplify_non_conflicts(xe1, changes, level > 2) < 0)) { + xdl_simplify_non_conflicts(xe1, changes, + level > XDL_MERGE_ZEALOUS) < 0)) { xdl_cleanup_merge(changes); return -1; } /* output */ if (result) { int size = xdl_fill_merge_buffer(xe1, name1, xe2, name2, - changes, NULL); + changes, NULL, level); result->ptr = xdl_malloc(size); if (!result->ptr) { xdl_cleanup_merge(changes); @@ -457,14 +533,15 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, } result->size = size; xdl_fill_merge_buffer(xe1, name1, xe2, name2, changes, - result->ptr); + result->ptr, level); } return xdl_cleanup_merge(changes); } -int xdl_merge(mmfile_t *orig, mmfile_t *mf1, const char *name1, - mmfile_t *mf2, const char *name2, - xpparam_t const *xpp, int level, mmbuffer_t *result) { +int xdl_merge(mmfile_t *orig, + mmfile_t *mf1, const char *name1, + mmfile_t *mf2, const char *name2, + xpparam_t const *xpp, int level, mmbuffer_t *result) { xdchange_t *xscr1, *xscr2; xdfenv_t xe1, xe2; int status; -- 1.6.0.1.200.ge682f ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style 2008-08-29 0:18 ` [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style Junio C Hamano @ 2008-08-29 0:34 ` Linus Torvalds 2008-08-29 1:07 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Linus Torvalds @ 2008-08-29 0:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, git On Thu, 28 Aug 2008, Junio C Hamano wrote: > > Some poeple find it easier to be able to understand what is going on when > they can view the common ancestor's version, which is used by "diff3 -m", > which shows: > > <<<<<<< > postimage from one side; > ||||||| > shared preimage; > ======= > postimage of the other side; and > >>>>>>> > > This is an initial step to bring that as an optional feature to git. > Only "git merge-file" has been converted, with "--diff3" option. If you have the common ancestor, why would you ever want this format, and not a nice conflict entry in the index? Anyway, that's irrelevant for my real question, which is: > /* > - * level == 0: mark all overlapping changes as conflict > - * level == 1: mark overlapping changes as conflict only if not identical > - * level == 2: analyze non-identical changes for minimal conflict set > - * level == 3: analyze non-identical changes for minimal conflict set, but > - * treat hunks not containing any letter or number as conflicting > + * "Level" parameter can be: > + * > + * (MINIMAL): > + * Mark all overlapping changes as conflict > + * > + * (EAGER): > + * Mark overlapping changes as conflict only if not identical > + * > + * (DIFF3): > + * Same as EAGER but show the shared preimage in the output as well > + * > + * (ZEALOUS): > + * Analyze non-identical changes for minimal conflict set > + * > + * (ZEALOUS_ALNUM): > + * Analyze non-identical changes for minimal conflict set, but > + * treat hunks not containing any letter or number as conflicting > * Wouldn't it be much nicer to make this a bitflag, than an enumeration of different models? In particular, why would it be wrong to want to be ZEALOUS (possibly _ALNUM) and still want DIFF3 output? IOW, I don't think these are at all disjoint sets. In fact, I think DIFF3 sounds not at all like a "level" to me, but like an output format thing, so it's in a totally different "address space". Hmm? Linus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style 2008-08-29 0:34 ` Linus Torvalds @ 2008-08-29 1:07 ` Junio C Hamano 2008-08-31 7:10 ` Junio C Hamano 2008-08-29 6:27 ` Junio C Hamano 2008-08-29 9:01 ` Junio C Hamano 2 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2008-08-29 1:07 UTC (permalink / raw) To: Linus Torvalds; +Cc: Johannes Schindelin, git Linus Torvalds <torvalds@linux-foundation.org> writes: > On Thu, 28 Aug 2008, Junio C Hamano wrote: >> >> Some poeple find it easier to be able to understand what is going on when >> they can view the common ancestor's version, which is used by "diff3 -m", >> which shows: >> >> <<<<<<< >> postimage from one side; >> ||||||| >> shared preimage; >> ======= >> postimage of the other side; and >> >>>>>>> >> >> This is an initial step to bring that as an optional feature to git. >> Only "git merge-file" has been converted, with "--diff3" option. > > If you have the common ancestor, why would you ever want this format, and > not a nice conflict entry in the index? We already have that, don't we? You can think of it as how to present that information without resorting to "diff :1:path :2:path". > Anyway, that's irrelevant for my real question, which is: ... the same thing as I already said after three-dashes, hence RFH/WIP. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style 2008-08-29 1:07 ` Junio C Hamano @ 2008-08-31 7:10 ` Junio C Hamano 2008-08-31 17:38 ` Linus Torvalds 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2008-08-31 7:10 UTC (permalink / raw) To: Linus Torvalds; +Cc: Johannes Schindelin, git Junio C Hamano <gitster@pobox.com> writes: > Linus Torvalds <torvalds@linux-foundation.org> writes: > >> On Thu, 28 Aug 2008, Junio C Hamano wrote: >>> >>> Some poeple find it easier to be able to understand what is going on when >>> they can view the common ancestor's version, which is used by "diff3 -m", >>> which shows: >>> >>> <<<<<<< >>> postimage from one side; >>> ||||||| >>> shared preimage; >>> ======= >>> postimage of the other side; and >>> >>>>>>> >>> >>> This is an initial step to bring that as an optional feature to git. >>> Only "git merge-file" has been converted, with "--diff3" option. >> >> If you have the common ancestor, why would you ever want this format, and >> not a nice conflict entry in the index? > > We already have that, don't we? You can think of it as how to present > that information without resorting to "diff :1:path :2:path". I've been using this for my own work, and here is a typical example. This happened while rebuilding 'pu', merging jc/merge-whitespace topic on top of what contains jc/better-conflict-resolution topic. In case if anybody is interested in reproducing this to follow the discussion of improving the support of conflicted merge resolution, here is how to reproduce this: $ git checkout 7ead6a7^ $ git merge ^2 In builtin-merge-file.c, there appears: ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2], <<<<<<< HEAD:builtin-merge-file.c &xpp, merge_level | merge_style, &result); ||||||| &xpp, XDL_MERGE_ZEALOUS_ALNUM, &result); ======= &xpp, XDL_MERGE_ZEALOUS_ALNUM, ws_rule, &result); >>>>>>> jc/merge-whitespace:builtin-merge-file.c for (i = 0; i < 3; i++) In this case, both branches are my code, so I do not need the additional hint to see that one branch added ws_rule parameter while the other one generalized the "merge simplify level" parameter and is now passing a bitwise OR of two variables. But if somebody else is doing the merge, it may not be so obvious how the resolution should be. On the other hand, this appears in ll-merge.c: xpparam_t xpp; <<<<<<< HEAD:ll-merge.c int style = 0; ||||||| ======= unsigned ws_rule = 0; >>>>>>> jc/merge-whitespace:ll-merge.c This is obviously useless; perhaps we can drop the "original" hunk when it is empty to save space? Another thing that helped me figure out what was going on was when merging mv/merge-recursive topic on top of next. In case if anybody is interested in reproducing this to follow the discussion of improving the support of conflicted merge resolution, here is how to reproduce this: $ git checkout ebe0e3b^ $ git merge ebe0e3b^2 This merge involves removal of quite a lot of lines to a new file merge-recursive.c from existing builtin-merge-recursive.c; the default conflict simplification (XDL_MERGE_ZEALOUS) works _extremely_ well for this merge, in that it shows this (I'll refer to this result as *1* in a later paragraph): <<<<<<< static struct commit *get_ref(const char *ref) { ... } static int merge_config(const char *var, const char *value, void *cb) { ... } ======= >>>>>>> It just shows we have _some_ changes to these two functions since the other side removed them from this file, but it does not say what the change is. However, with the "common ancestor" version, here is what I get (you need to use 'next' and "merge.conflictstyle" configuration set to "diff3" to reproduce this): <<<<<<< static struct commit *get_ref(const char *ref) { ... } static int merge_config(const char *var, const char *value, void *cb) { ... return git_xmerge_config(var, value, cb); } ||||||| static struct commit *get_ref(const char *ref) { ... } static int merge_config(const char *var, const char *value, void *cb) { ... return git_default_config(var, value, cb); } ======= >>>>>>> theirs By comparing the first and second part, we can tell that the call to git_default_config() was replaced to a call to git_xmerge_config(). And this is a crucial clue to me to make an corresponding update to the function that now lives in merge-file.c. On average, I am finding that "diff3 -m" format more irritating than useful. However, on occasions like this, I am finding it quite useful. My observation so far suggests that it would be best for me to leave the configuration "merge.conflictstyle" to the default "merge", and instead give an option to allow me to tell "git checkout -m -- $path" (which is also a new feature; it overwrites the $path by the result of a fresh merge to reproduce the conflicted state in the working tree, using the three stages recorded in the index) to use "diff3 -m" style, when I want to. That means the command sequence would look like: $ git merge mv/merge-recursive ;# git merge ebe0e3b^2 .. oops, heavily conflicted merge and I see *1*, which is not .. very useful. Let's see what is going on. .. Note that this --merge=diff3 option does not exist yet. $ git checkout --merge=diff3 -- builtin-merge-recursive.c $ emacs builtin-merge-recursive.c .. split the buffer into two, move the cursor to the line after <<< .. in one window while moving the cursor to the line after ||| line .. in the other one, and say "M-x compare-windows". Aha, there is .. a change there. Having said all that, I now realize that the way I described in my response to you is just as easy to view this particular case: $ git merge mv/merge-recursive ;# git merge ebe0e3b^2 .. oops, heavily conflicted merge and I see *1*, which is not .. very useful. Let's see what is going on. .. Note that this --merge=diff3 option does not exist yet. $ git diff :{1,2}:builtin-merge-recursive.c ;# shell expands {1,2} ;-) which exactly shows what one side changed -- instead of calling git_default_config(), the code calls git_xmerge_config(). $ git diff :{1,2}:builtin-merge-recursive.c diff --git a/:1:builtin-merge-recursive.c b/:2:builtin-merge-recursive.c index dfb363e..f3b6ede 100644 --- a/:1:builtin-merge-recursive.c +++ b/:2:builtin-merge-recursive.c @@ -1348,7 +1348,7 @@ static int merge_config(const char *var, const char *value, void *cb) merge_rename_limit = git_config_int(var, value); return 0; } - return git_default_config(var, value, cb); + return git_xmerge_config(var, value, cb); } int cmd_merge_recursive(int argc, const char **argv, const char *prefix) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style 2008-08-31 7:10 ` Junio C Hamano @ 2008-08-31 17:38 ` Linus Torvalds 2008-08-31 18:42 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Linus Torvalds @ 2008-08-31 17:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, git On Sun, 31 Aug 2008, Junio C Hamano wrote: > > On average, I am finding that "diff3 -m" format more irritating than > useful. However, on occasions like this, I am finding it quite useful. I have to say that the best feature of the standard merge is the zealous version, which apparently doesn't work well with the "diff3 -m" format. I do agree that what is often really nasty with the normal merge conflict info is when you actually need the original thing to understand why some hunk was left, and yes, a common case of that is that one side simply removed a function, and the other side had modified it. At that point the resulting conflict is often hard to understand without seeing the original, exactly because you don't see any actual "conflict", you only see one side (the other side being empty). But what really saves that situation is the combination of - 'gitk --merge <file>' - 'git diff' showing the multi-way merge and I find myself really _hating_ doing rebases because the merge helpers are so totally useless (ie "gitk --merge" at least didn't use to work across a rebase conflict because MERGE_HEAD isn't set) But the biggest problem, and the reason I _really_ detest the diff3 format, is that small merges are fairly often pretty easy to see anyway. If the conflict markers all fit in one screenful, it's generally fairly easy to see why something conflicted, because you can visually compare things. But the complicated cases are when there are bigger changes, and the conflict is over many many lines of code, and it's really hard to visually see what changed. And the diff3 format makes this worse - it not only makes the conflict 50% bigger to begin with, it moves the two conflicting versions away from each other, making that visual comparison much harder. Now, there are tools to help with that. I think various of the graphical merge tools understand the diff3 format, and then ti can really help. But I think it hurts for a lot of the _common_ cases. > My observation so far suggests that it would be best for me to leave the > configuration "merge.conflictstyle" to the default "merge", and instead > give an option to allow me to tell "git checkout -m -- $path" (which is > also a new feature; it overwrites the $path by the result of a fresh merge > to reproduce the conflicted state in the working tree, using the three > stages recorded in the index) to use "diff3 -m" style, when I want to. Now *this* I think is a great idea! The reason I think it's a great idea is that it solves so many _different_ issues (which is the mark of a really good solution): - it fixes my problem with diff3 output: the fact that it's more annoying by _default_ than it is occasionally useful. If the default isn't to do it - since by default it often hurts - but you have the option to do it when there is something confusing going on (like the "one side disappeared, why did it conflict?" case), then you have the best of both worlds - a good default with a way to dig deeper when you need to. - it fixes another totally unrelated problem: incorrect merge resolutions. Again, I find this to be fairly rare, but what git is good at is to make incremental resolutions for merge problems - you can resolve the merge in the work tree, then compile and test the result before actually committing it, and "git diff" always gives relevant and interesting output for the merge. And _occasionally_ the resolve looks obvious, but then when you compile things you notice that it doesn't work because (for example) you resolved it by removing one side (exactly because the other side was a removal), and it turned out that the conflict was adding a function that you hadn't realized was new, and was needed. And while "git diff" is fine, and you can cut-and-paste things and try to re-resolve it that way, I have occasionally decided to just do a "git reset" and re-do the whole merge. But your idea allows us to just re-do the merge for a single file. So I think we do quite well already, but your solution really does sound like a good and useful addition to the toolbox. Linus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style 2008-08-31 17:38 ` Linus Torvalds @ 2008-08-31 18:42 ` Junio C Hamano 0 siblings, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2008-08-31 18:42 UTC (permalink / raw) To: Linus Torvalds; +Cc: Johannes Schindelin, git Linus Torvalds <torvalds@linux-foundation.org> writes: > On Sun, 31 Aug 2008, Junio C Hamano wrote: >> ... >> My observation so far suggests that it would be best for me to leave the >> configuration "merge.conflictstyle" to the default "merge", and instead >> give an option to allow me to tell "git checkout -m -- $path" (which is >> also a new feature; it overwrites the $path by the result of a fresh merge >> to reproduce the conflicted state in the working tree, using the three >> stages recorded in the index) to use "diff3 -m" style, when I want to. > > Now *this* I think is a great idea! > > The reason I think it's a great idea is that it solves so many _different_ > issues (which is the mark of a really good solution): > ... > - it fixes another totally unrelated problem: incorrect merge > resolutions. I do not know if rerere often kicks in in your workflow, but occasionally I notice that I have a faulty merge recorded by it, which automatically is applied again when reproducing a merge. And it makes me deeply regret that I invented the rerere mechanism. The best solution for this issue I found so far is embarrasingly clumsy. Run "ls -tl .git/rr-cache" to find the newest one with "thisimage", make sure it is the right one, remove the directory and redo the merge. Actually, the feature of "git checkout -m -- $path" to reproduce the merge for the named path was conceived to address this "I cannot easily correct the faulty resolution that was re-applied" issue. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style 2008-08-29 0:34 ` Linus Torvalds 2008-08-29 1:07 ` Junio C Hamano @ 2008-08-29 6:27 ` Junio C Hamano 2008-08-29 9:01 ` Junio C Hamano 2 siblings, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2008-08-29 6:27 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Linus Torvalds, git Linus Torvalds <torvalds@linux-foundation.org> writes: > Anyway, that's irrelevant for my real question, which is: > ... > Wouldn't it be much nicer to make this a bitflag, than an enumeration of > different models? > > In particular, why would it be wrong to want to be ZEALOUS (possibly > _ALNUM) and still want DIFF3 output? > > IOW, I don't think these are at all disjoint sets. In fact, I think DIFF3 > sounds not at all like a "level" to me, but like an output format thing, > so it's in a totally different "address space". Ok, here is a replacement for [2/2]. This still forces XDL_MERGE_EAGER when "git merge-file --diff3" is asked for, because higher level "merge refinement" routines do not know anything about the necessary adjustment of the preimage pointers. I threw away some clean-ups to xdiff/xmerge.c to use symbolic constants when deciding what merge refinement codepaths are taken, to make it easier to review (the clean-up and style fixes should be done independently, not as part of this patch). builtin-merge-file.c | 10 ++++- xdiff/xdiff.h | 6 +++ xdiff/xmerge.c | 103 ++++++++++++++++++++++++++++++++++++++++--------- t/t6023-merge-file.sh | 37 +++++++++++++++++ 4 files changed, 135 insertions(+), 21 deletions(-) diff --git c/builtin-merge-file.c w/builtin-merge-file.c index 3605960..5b4f020 100644 --- c/builtin-merge-file.c +++ w/builtin-merge-file.c @@ -4,7 +4,7 @@ #include "xdiff-interface.h" static const char merge_file_usage[] = -"git merge-file [-p | --stdout] [-q | --quiet] [-L name1 [-L orig [-L name2]]] file1 orig_file file2"; +"git merge-file [-p | --stdout] [--diff3] [-q | --quiet] [-L name1 [-L orig [-L name2]]] file1 orig_file file2"; int cmd_merge_file(int argc, const char **argv, const char *prefix) { @@ -13,6 +13,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) mmbuffer_t result = {NULL, 0}; xpparam_t xpp = {XDF_NEED_MINIMAL}; int ret = 0, i = 0, to_stdout = 0; + int merge_level = XDL_MERGE_ZEALOUS_ALNUM; + int merge_style = 0; while (argc > 4) { if (!strcmp(argv[1], "-L") && i < 3) { @@ -25,6 +27,10 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) else if (!strcmp(argv[1], "-q") || !strcmp(argv[1], "--quiet")) freopen("/dev/null", "w", stderr); + else if (!strcmp(argv[1], "--diff3")) { + merge_style = XDL_MERGE_DIFF3; + merge_level = XDL_MERGE_EAGER; + } else usage(merge_file_usage); argc--; @@ -46,7 +52,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) } ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2], - &xpp, XDL_MERGE_ZEALOUS_ALNUM, &result); + &xpp, merge_level | merge_style, &result); for (i = 0; i < 3; i++) free(mmfs[i].ptr); diff --git c/xdiff/xdiff.h w/xdiff/xdiff.h index 413082e..deebe02 100644 --- c/xdiff/xdiff.h +++ w/xdiff/xdiff.h @@ -50,10 +50,16 @@ extern "C" { #define XDL_BDOP_CPY 2 #define XDL_BDOP_INSB 3 +/* merge simplification levels */ #define XDL_MERGE_MINIMAL 0 #define XDL_MERGE_EAGER 1 #define XDL_MERGE_ZEALOUS 2 #define XDL_MERGE_ZEALOUS_ALNUM 3 +#define XDL_MERGE_LEVEL_MASK 0x0f + +/* merge output styles */ +#define XDL_MERGE_DIFF3 0x8000 +#define XDL_MERGE_STYLE_MASK 0x8000 typedef struct s_mmfile { char *ptr; diff --git c/xdiff/xmerge.c w/xdiff/xmerge.c index 6ffaa4f..29cdbea 100644 --- c/xdiff/xmerge.c +++ w/xdiff/xmerge.c @@ -30,17 +30,32 @@ typedef struct s_xdmerge { * 2 = no conflict, take second. */ int mode; + /* + * These point at the respective postimages. E.g. <i1,chg1> is + * how side #1 wants to change the common ancestor; if there is no + * overlap, lines before i1 in the postimage of side #1 appear + * in the merge result as a region touched by neither side. + */ long i1, i2; long chg1, chg2; + /* + * These point at the preimage; of course there is just one + * preimage, that is from the shared common ancestor. + */ + long i0; + long chg0; } xdmerge_t; static int xdl_append_merge(xdmerge_t **merge, int mode, - long i1, long chg1, long i2, long chg2) + long i0, long chg0, + long i1, long chg1, + long i2, long chg2) { xdmerge_t *m = *merge; if (m && (i1 <= m->i1 + m->chg1 || i2 <= m->i2 + m->chg2)) { if (mode != m->mode) m->mode = 0; + m->chg0 = i0 + chg0 - m->i0; m->chg1 = i1 + chg1 - m->i1; m->chg2 = i2 + chg2 - m->i2; } else { @@ -49,6 +64,8 @@ static int xdl_append_merge(xdmerge_t **merge, int mode, return -1; m->next = NULL; m->mode = mode; + m->i0 = i0; + m->chg0 = chg0; m->i1 = i1; m->chg1 = chg1; m->i2 = i2; @@ -91,11 +108,13 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2, return 0; } -static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest) +static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add_nl, char *dest) { - xrecord_t **recs = xe->xdf2.recs + i; + xrecord_t **recs; int size = 0; + recs = (use_orig ? xe->xdf1.recs : xe->xdf2.recs) + i; + if (count < 1) return 0; @@ -113,9 +132,19 @@ static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest) return size; } +static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest) +{ + return xdl_recs_copy_0(0, xe, i, count, add_nl, dest); +} + +static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest) +{ + return xdl_recs_copy_0(1, xe, i, count, add_nl, dest); +} + static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1, xdfenv_t *xe2, const char *name2, - int size, int i, + int size, int i, int style, xdmerge_t *m, char *dest) { const int marker_size = 7; @@ -143,6 +172,20 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1, /* Postimage from side #1 */ size += xdl_recs_copy(xe1, m->i1, m->chg1, 1, dest ? dest + size : NULL); + + if (style == XDL_MERGE_DIFF3) { + /* Shared preimage */ + if (!dest) { + size += marker_size + 1; + } else { + for (j = 0; j < marker_size; j++) + dest[size++] = '|'; + dest[size++] = '\n'; + } + size += xdl_orig_copy(xe1, m->i0, m->chg0, 1, + dest ? dest + size : NULL); + } + if (!dest) { size += marker_size + 1; } else { @@ -170,14 +213,15 @@ 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, xdmerge_t *m, char *dest) + xdfenv_t *xe2, const char *name2, + xdmerge_t *m, char *dest, int style) { int size, i; for (size = i = 0; m; m = m->next) { if (m->mode == 0) size = fill_conflict_hunk(xe1, name1, xe2, name2, - size, i, m, dest); + size, i, style, m, dest); else if (m->mode == 1) size += xdl_recs_copy(xe1, i, m->i1 + m->chg1 - i, 0, dest ? dest + size : NULL); @@ -342,9 +386,11 @@ static int xdl_simplify_non_conflicts(xdfenv_t *xe1, xdmerge_t *m, */ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, xdfenv_t *xe2, xdchange_t *xscr2, const char *name2, - int level, xpparam_t const *xpp, mmbuffer_t *result) { + int flags, xpparam_t const *xpp, mmbuffer_t *result) { xdmerge_t *changes, *c; - int i1, i2, chg1, chg2; + int i0, i1, i2, chg0, chg1, chg2; + int level = flags & XDL_MERGE_LEVEL_MASK; + int style = flags & XDL_MERGE_STYLE_MASK; c = changes = NULL; @@ -352,11 +398,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, if (!changes) changes = c; if (xscr1->i1 + xscr1->chg1 < xscr2->i1) { + i0 = xscr1->i1; i1 = xscr1->i2; i2 = xscr2->i2 - xscr2->i1 + xscr1->i1; + chg0 = xscr1->chg1; chg1 = xscr1->chg2; chg2 = xscr1->chg1; - if (xdl_append_merge(&c, 1, i1, chg1, i2, chg2)) { + if (xdl_append_merge(&c, 1, + i0, chg0, i1, chg1, i2, chg2)) { xdl_cleanup_merge(changes); return -1; } @@ -364,11 +413,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, continue; } if (xscr2->i1 + xscr2->chg1 < xscr1->i1) { + i0 = xscr2->i1; i1 = xscr1->i2 - xscr1->i1 + xscr2->i1; i2 = xscr2->i2; + chg0 = xscr2->chg1; chg1 = xscr2->chg1; chg2 = xscr2->chg2; - if (xdl_append_merge(&c, 2, i1, chg1, i2, chg2)) { + if (xdl_append_merge(&c, 2, + i0, chg0, i1, chg1, i2, chg2)) { xdl_cleanup_merge(changes); return -1; } @@ -385,19 +437,26 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, int off = xscr1->i1 - xscr2->i1; int ffo = off + xscr1->chg1 - xscr2->chg1; + i0 = xscr1->i1; i1 = xscr1->i2; i2 = xscr2->i2; - if (off > 0) + if (off > 0) { + i0 -= off; i1 -= off; + } else i2 += off; + chg0 = xscr1->i1 + xscr1->chg1 - i0; chg1 = xscr1->i2 + xscr1->chg2 - i1; chg2 = xscr2->i2 + xscr2->chg2 - i2; if (ffo > 0) chg2 += ffo; - else + else { + chg0 -= ffo; chg1 -= ffo; - if (xdl_append_merge(&c, 0, i1, chg1, i2, chg2)) { + } + if (xdl_append_merge(&c, 0, + i0, chg0, i1, chg1, i2, chg2)) { xdl_cleanup_merge(changes); return -1; } @@ -414,11 +473,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, while (xscr1) { if (!changes) changes = c; + i0 = xscr1->i1; i1 = xscr1->i2; i2 = xscr1->i1 + xe2->xdf2.nrec - xe2->xdf1.nrec; + chg0 = xscr1->chg1; chg1 = xscr1->chg2; chg2 = xscr1->chg1; - if (xdl_append_merge(&c, 1, i1, chg1, i2, chg2)) { + if (xdl_append_merge(&c, 1, + i0, chg0, i1, chg1, i2, chg2)) { xdl_cleanup_merge(changes); return -1; } @@ -427,11 +489,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, while (xscr2) { if (!changes) changes = c; + i0 = xscr2->i1; i1 = xscr2->i1 + xe1->xdf2.nrec - xe1->xdf1.nrec; i2 = xscr2->i2; + chg0 = xscr2->chg1; chg1 = xscr2->chg1; chg2 = xscr2->chg2; - if (xdl_append_merge(&c, 2, i1, chg1, i2, chg2)) { + if (xdl_append_merge(&c, 2, + i0, chg0, i1, chg1, i2, chg2)) { xdl_cleanup_merge(changes); return -1; } @@ -449,7 +514,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, /* output */ if (result) { int size = xdl_fill_merge_buffer(xe1, name1, xe2, name2, - changes, NULL); + changes, NULL, style); result->ptr = xdl_malloc(size); if (!result->ptr) { xdl_cleanup_merge(changes); @@ -457,14 +522,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1, } result->size = size; xdl_fill_merge_buffer(xe1, name1, xe2, name2, changes, - result->ptr); + result->ptr, style); } return xdl_cleanup_merge(changes); } int xdl_merge(mmfile_t *orig, mmfile_t *mf1, const char *name1, mmfile_t *mf2, const char *name2, - xpparam_t const *xpp, int level, mmbuffer_t *result) { + xpparam_t const *xpp, int flags, mmbuffer_t *result) { xdchange_t *xscr1, *xscr2; xdfenv_t xe1, xe2; int status; @@ -501,7 +566,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, const char *name1, } else { status = xdl_do_merge(&xe1, xscr1, name1, &xe2, xscr2, name2, - level, xpp, result); + flags, xpp, result); } xdl_free_script(xscr1); xdl_free_script(xscr2); diff --git c/t/t6023-merge-file.sh w/t/t6023-merge-file.sh index 42620e0..f3484e3 100755 --- c/t/t6023-merge-file.sh +++ w/t/t6023-merge-file.sh @@ -161,4 +161,41 @@ test_expect_success 'ZEALOUS_ALNUM' ' ' +cat >expect <<\EOF +Dominus regit me, +<<<<<<< new8.txt +et nihil mihi deerit; + + + + +In loco pascuae ibi me collocavit; +super aquam refectionis educavit me. +||||||| +et nihil mihi deerit. +In loco pascuae ibi me collocavit, +super aquam refectionis educavit me; +======= +et nihil mihi deerit, + + + + +In loco pascuae ibi me collocavit -- +super aquam refectionis educavit me, +>>>>>>> new9.txt +animam meam convertit, +deduxit me super semitas jusitiae, +propter nomen suum. +Nam et si ambulavero in medio umbrae mortis, +non timebo mala, quoniam TU mecum es: +virga tua et baculus tuus ipsa me consolata sunt. +EOF + +test_expect_success '"diff3 -m" style output' ' + test_must_fail git merge-file -p --diff3 \ + new8.txt new5.txt new9.txt >actual && + test_cmp expect actual +' + test_done ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style 2008-08-29 0:34 ` Linus Torvalds 2008-08-29 1:07 ` Junio C Hamano 2008-08-29 6:27 ` Junio C Hamano @ 2008-08-29 9:01 ` Junio C Hamano 2 siblings, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2008-08-29 9:01 UTC (permalink / raw) To: Linus Torvalds; +Cc: Johannes Schindelin, git Linus Torvalds <torvalds@linux-foundation.org> writes: > On Thu, 28 Aug 2008, Junio C Hamano wrote: >> >> Some poeple find it easier to be able to understand what is going on when >> they can view the common ancestor's version, which is used by "diff3 -m", >> which shows: >> >> <<<<<<< >> postimage from one side; >> ||||||| >> shared preimage; >> ======= >> postimage of the other side; and >> >>>>>>> > ... > Wouldn't it be much nicer to make this a bitflag, than an enumeration of > different models? > > In particular, why would it be wrong to want to be ZEALOUS (possibly > _ALNUM) and still want DIFF3 output? > > IOW, I don't think these are at all disjoint sets. In fact, I think DIFF3 > sounds not at all like a "level" to me, but like an output format thing, > so it's in a totally different "address space". > > Hmm? I've thought about this a bit more, and I think "diff3 -m" style output would not make sense for anything more aggressive than XDL_MERGE_EAGER, because of the way how the merge reduction works. Suppose a common ancestor (shared preimage) is modified to postimage #1 and #2 (each letter represents one line): ##### postimage#1: 1234ABCDE789 | / | / preimage: 123456789 | \ postimage#2: 1234AXYE789 #### XDL_MERGE_MINIMAL and XDL_MERGE_EAGER would: (1) find the s/56/ABCDE/ done on one side and s/56/AXYE/ done on the other side, (2) notice that they touch an overlapping area, and (3) mark it as a conflict, "ABCDE vs AXYE". The difference between the two algorithm is that EAGER drops the hunk if the postimages match (i.e. both sides modified the same way), while MINIMAL keeps it. There is no other operation performed to the hunk. As the result, lines marked with "#" in the above picure will be in the RCS merge style output. We can sanely say that the part from the preimage that corresponds to these conflicting changes is "567", which is what "diff3 -m" style output adds to it. Now, XDL_MERGE_ZEALOUS postprocesses output XDL_MERGE_EAGER would have produced, by looking at the differences between the changes two postimages made. It notices that both sides start their new contents with "A", and excludes it from the output (it also excludes "E" for the same reason). The conflict that used to be "ABCDE vs AXYE" is now "BCD vs XY". There could even be matching part between two postimages in the middle. Instead of one side rewriting the shared "56" to "ABCDE" and the other side to "AXYE", imagine the case where the postimages are "ABCDE" and "AXCYE", in which case instead of having one conflicted hunk "BCD vs XY", you would have two conflicting hunks "B vs X" and "D vs Y". In either case, once you reduce "ABCDE vs AXYE" to "BCD vs XY" (or "ABCDE vs AXCYE" to "B vs X" and "D vs Y"), there is no part from the preimage that corresponds to the conflicting change made in both postimages anymore. In other words, conflict reduced by ZEALOUS algorithm cannot be expressed in "diff3 -m" style. You can still have "diff3 -m" style with MINIMAL or EAGER, but I think MINIMAL is pointless and using "diff3 -m" style with it is even more so. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-08-31 18:43 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-08-29 0:09 [PATCH 1/2] xdl_fill_merge_buffer(): separate out a too deeply nested function Junio C Hamano 2008-08-29 0:18 ` [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style Junio C Hamano 2008-08-29 0:34 ` Linus Torvalds 2008-08-29 1:07 ` Junio C Hamano 2008-08-31 7:10 ` Junio C Hamano 2008-08-31 17:38 ` Linus Torvalds 2008-08-31 18:42 ` Junio C Hamano 2008-08-29 6:27 ` Junio C Hamano 2008-08-29 9:01 ` Junio C Hamano
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).