Re: [RFC PATCH 7/7] diff/am: enhance diff format to use */~ for moved lines
On Sat, Aug 4, 2018 at 10:15 AM Junio C Hamano wrote: > > Stefan Beller writes: > > > Try it out via > > ./git-format-patch --mark-moved 15ef69314d^..15ef69314d > > to see if you like it. > > > > This separates the coloring decision from the detection of moved lines. > > When giving --mark-moved, move detection is still performed and the output > > markers are adjusted to */~ for new and old code. > > > > git-apply and git-am will also accept these patches by rewriting those > > signs back to +/-. > > > > Signed-off-by: Stefan Beller > > --- > > This does not have anything to do with the range-diff topic, but > would stand on its own merit. Yes. I should have emphasized this more in the cover letter. This is more a "while at it" thing, that is easy to do due to the refactoring in previous patches. > I have a mixed feeling about this. Me, too. > If you need to convince "GNU patch" maintainers to accept these two > signs, then probably it is not worth the battle of incompatiblity. > If it is truly a worthy innovation, they would follow suit, which is > how they learned to take our renaming diffs without us prodding > them. I just do not get the gut feeling that it would happen for > this particular thing, and I am not convinced myself enough to sell > this to "patch" maintainers and expect to be taken seriously. ok. > When reviewing anything complex that would be helped by moved code > highlighting, I do not think a normal person would choose to review > such a change only inside MUA. I certainly won't. I'd rather apply > the patch and view it within a larger context than the piece of > e-mail that was originally sent offers, with better tools like -W > and --color-moved applied locally. So in that sense, I do not think > I'd appreciate lines that begin with '~'/'*' as different kind of > '-'/'+', as helpful hints; at least until my eyes get used to them, > they would only appear as distraction. My use case would be patches that are *not* complex, but still shuffling lots of code around, e.g. reordering functions/paragraphs in a file. > In other words, I have this nagging suspicion that people who > suggested to you that this would help in e-mail workflow are > misguided and they do not understand e-mail workflow in the first > place, but perhaps it is just me. There are no other people that suggested this. It was really just a quick shot "while at it" as we had the refactoring in place that enables this, and I think for trivial patches (non-complex, but lots of changes) it *may* be beneficial. But it is more for corner cases, I guess. Stefan
Re: [RFC PATCH 7/7] diff/am: enhance diff format to use */~ for moved lines
Stefan Beller writes: > Try it out via > ./git-format-patch --mark-moved 15ef69314d^..15ef69314d > to see if you like it. > > This separates the coloring decision from the detection of moved lines. > When giving --mark-moved, move detection is still performed and the output > markers are adjusted to */~ for new and old code. > > git-apply and git-am will also accept these patches by rewriting those > signs back to +/-. > > Signed-off-by: Stefan Beller > --- This does not have anything to do with the range-diff topic, but would stand on its own merit. I have a mixed feeling about this. If you need to convince "GNU patch" maintainers to accept these two signs, then probably it is not worth the battle of incompatiblity. If it is truly a worthy innovation, they would follow suit, which is how they learned to take our renaming diffs without us prodding them. I just do not get the gut feeling that it would happen for this particular thing, and I am not convinced myself enough to sell this to "patch" maintainers and expect to be taken seriously. When reviewing anything complex that would be helped by moved code highlighting, I do not think a normal person would choose to review such a change only inside MUA. I certainly won't. I'd rather apply the patch and view it within a larger context than the piece of e-mail that was originally sent offers, with better tools like -W and --color-moved applied locally. So in that sense, I do not think I'd appreciate lines that begin with '~'/'*' as different kind of '-'/'+', as helpful hints; at least until my eyes get used to them, they would only appear as distraction. In other words, I have this nagging suspicion that people who suggested to you that this would help in e-mail workflow are misguided and they do not understand e-mail workflow in the first place, but perhaps it is just me. Thanks.
[RFC PATCH 7/7] diff/am: enhance diff format to use */~ for moved lines
Try it out via
./git-format-patch --mark-moved 15ef69314d^..15ef69314d
to see if you like it.
This separates the coloring decision from the detection of moved lines.
When giving --mark-moved, move detection is still performed and the output
markers are adjusted to */~ for new and old code.
git-apply and git-am will also accept these patches by rewriting those
signs back to +/-.
Signed-off-by: Stefan Beller
---
apply.c | 12
diff.c | 21 +
diff.h | 5 -
3 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/apply.c b/apply.c
index 23a0f25ded8..cc42a4fa02a 100644
--- a/apply.c
+++ b/apply.c
@@ -2900,6 +2900,12 @@ static int apply_one_fragment(struct apply_state *state,
ws_blank_line(patch + 1, plen, ws_rule))
is_blank_context = 1;
/* fallthrough */
+ case '~':
+ /*
+* For now ignore moved line indicators and apply
+* as a regular old line
+*/
+ /* fallthrough */
case '-':
memcpy(old, patch + 1, plen);
add_line_info(, old, plen,
@@ -2908,6 +2914,12 @@ static int apply_one_fragment(struct apply_state *state,
if (first == '-')
break;
/* fallthrough */
+ case '*':
+ /*
+* For now ignore moved line indicators and apply
+* as a regular new line
+*/
+ /* fallthrough */
case '+':
/* --no-add does not add new lines */
if (first == '+' && state->no_add)
diff --git a/diff.c b/diff.c
index 56bab011df7..8e39e77229f 100644
--- a/diff.c
+++ b/diff.c
@@ -1043,6 +1043,9 @@ static const char *determine_line_color(struct
diff_options *o,
const int off = (eds->s == DIFF_SYMBOL_PLUS) ?
DIFF_FILE_NEW_MOVED - DIFF_FILE_OLD_MOVED : 0;
+ if (!o->color_moved)
+ goto default_color;
+
switch (flags & (DIFF_SYMBOL_MOVED_LINE |
DIFF_SYMBOL_MOVED_LINE_ALT |
DIFF_SYMBOL_MOVED_LINE_UNINTERESTING)) {
@@ -1063,6 +1066,7 @@ static const char *determine_line_color(struct
diff_options *o,
set = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED + off);
break;
default:
+default_color:
set = (eds->s == DIFF_SYMBOL_PLUS) ?
diff_get_color_opt(o, DIFF_FILE_NEW):
diff_get_color_opt(o, DIFF_FILE_OLD);
@@ -1152,6 +1156,9 @@ static void emit_diff_symbol_from_struct(struct
diff_options *o,
first = o->output_indicators[OI_NEW] ?
o->output_indicators[OI_NEW] : "+";
+ if (o->output_indicators[OI_MOVED_NEW] &&
+ (flags & DIFF_SYMBOL_MOVED_LINE))
+ first = "*";
emit_line_ws_markup(o, set_sign, set, reset, first, line, len,
flags & DIFF_SYMBOL_CONTENT_WS_MASK,
flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
@@ -1176,6 +1183,9 @@ static void emit_diff_symbol_from_struct(struct
diff_options *o,
}
first = o->output_indicators[OI_OLD] ?
o->output_indicators[OI_OLD] : "-";
+ if (o->output_indicators[OI_MOVED_NEW] &&
+ (flags & DIFF_SYMBOL_MOVED_LINE))
+ first = "~";
emit_line_ws_markup(o, set_sign, set, reset, first, line, len,
flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
break;
@@ -4795,6 +4805,7 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "--no-color"))
options->use_color = 0;
else if (!strcmp(arg, "--color-moved")) {
+ options->color_moved = 1;
if (diff_color_moved_default)
options->markup_moved = diff_color_moved_default;
if (options->markup_moved == COLOR_MOVED_NO)
@@ -4806,6 +4817,16 @@ int diff_opt_parse(struct diff_options *options,
if (cm < 0)
die("bad --color-moved argument: %s", arg);
options->markup_moved = cm;
+ options->color_moved = 1;
+ } else if (skip_prefix(arg, "--mark-moved", )) {
+ /*
+* NEEDSWORK:
+* Once merged with 51da15eb230 (diff.c: add a blocks mode for
+* moved code detection, 2018-07-16), make it COLOR_MOVED_BLOCKS
+*/
+ options->markup_moved = COLOR_MOVED_PLAIN;
+
