* [RFC PATCH 1/4] range-diff: fix function parameter indentation
2019-04-14 21:09 ` [RFC PATCH 0/4] output improvements for git range-diff Thomas Gummerer
@ 2019-04-14 21:09 ` Thomas Gummerer
2019-04-14 21:09 ` [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff Thomas Gummerer
` (3 subsequent siblings)
4 siblings, 0 replies; 29+ messages in thread
From: Thomas Gummerer @ 2019-04-14 21:09 UTC (permalink / raw)
To: git; +Cc: Duy Nguyen, Johannes Schindelin, Junio C Hamano, Thomas Gummerer
Fix the indentation of the function parameters for a couple of
functions, to match the style in the rest of the file.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
range-diff.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index 48b0e1b4ce..9242b8975f 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -148,7 +148,7 @@ static int read_patches(const char *range, struct string_list *list)
}
static int patch_util_cmp(const void *dummy, const struct patch_util *a,
- const struct patch_util *b, const char *keydata)
+ const struct patch_util *b, const char *keydata)
{
return strcmp(a->diff, keydata ? keydata : b->diff);
}
@@ -373,7 +373,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
}
static void patch_diff(const char *a, const char *b,
- struct diff_options *diffopt)
+ struct diff_options *diffopt)
{
diff_queue(&diff_queued_diff,
get_filespec("a", a), get_filespec("b", b));
--
2.21.0.593.g511ec345e1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff
2019-04-14 21:09 ` [RFC PATCH 0/4] output improvements for git range-diff Thomas Gummerer
2019-04-14 21:09 ` [RFC PATCH 1/4] range-diff: fix function parameter indentation Thomas Gummerer
@ 2019-04-14 21:09 ` Thomas Gummerer
2019-04-14 23:21 ` Eric Sunshine
2019-04-15 12:53 ` Johannes Schindelin
2019-04-14 21:09 ` [RFC PATCH 3/4] range-diff: add section header instead of diff header Thomas Gummerer
` (2 subsequent siblings)
4 siblings, 2 replies; 29+ messages in thread
From: Thomas Gummerer @ 2019-04-14 21:09 UTC (permalink / raw)
To: git; +Cc: Duy Nguyen, Johannes Schindelin, Junio C Hamano, Thomas Gummerer
When postprocessing the inner diff in range-diff, we currently replace
the whole hunk header line with just "@@". This matches how 'git
tbdiff' used to handle hunk headers as well.
Most likely this is being done because line numbers in the hunk header
are not relevant without other changes. They can for example easily
change if a range is rebased, and lines are added/removed before a
change that we actually care about in our ranges.
However it can still be useful to have the function name that 'git
diff' extracts as additional context for the change.
Note that it is not guaranteed that the hunk header actually shows up
in the range-diff, and this change only aims to improve the case where
a hunk header would already be included in the final output.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
range-diff.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index 9242b8975f..f365141ade 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
strbuf_addch(&buf, '\n');
}
continue;
- } else if (starts_with(line.buf, "@@ "))
- strbuf_addstr(&buf, "@@");
- else if (!line.buf[0] || starts_with(line.buf, "index "))
+ } else if (starts_with(line.buf, "@@ ")) {
+ char *skip_lineno = strstr(line.buf + 3, "@@");
+ strbuf_remove(&line, 0, skip_lineno - line.buf);
+ strbuf_addch(&buf, ' ');
+ strbuf_addbuf(&buf, &line);
+ } else if (!line.buf[0] || starts_with(line.buf, "index "))
/*
* A completely blank (not ' \n', which is context)
* line is not valid in a diff. We skip it
--
2.21.0.593.g511ec345e1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff
2019-04-14 21:09 ` [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff Thomas Gummerer
@ 2019-04-14 23:21 ` Eric Sunshine
2019-04-15 12:54 ` Johannes Schindelin
2019-04-15 12:53 ` Johannes Schindelin
1 sibling, 1 reply; 29+ messages in thread
From: Eric Sunshine @ 2019-04-14 23:21 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: Git List, Duy Nguyen, Johannes Schindelin, Junio C Hamano
On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> [...]
> However it can still be useful to have the function name that 'git
> diff' extracts as additional context for the change.
> [...]
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/range-diff.c b/range-diff.c
> @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> + } else if (starts_with(line.buf, "@@ ")) {
> + char *skip_lineno = strstr(line.buf + 3, "@@");
> + strbuf_remove(&line, 0, skip_lineno - line.buf);
It makes me a bit uncomfortable that this is not checking for NULL
return from strstr() before doing pointer arithmetic (even though the
input is presumably machine-generated).
if (!skip_lineno)
BUG(...);
might be appropriate.
> + strbuf_addch(&buf, ' ');
> + strbuf_addbuf(&buf, &line);
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff
2019-04-14 23:21 ` Eric Sunshine
@ 2019-04-15 12:54 ` Johannes Schindelin
2019-04-15 18:56 ` Thomas Gummerer
0 siblings, 1 reply; 29+ messages in thread
From: Johannes Schindelin @ 2019-04-15 12:54 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Thomas Gummerer, Git List, Duy Nguyen, Junio C Hamano
Hi Eric,
On Sun, 14 Apr 2019, Eric Sunshine wrote:
> On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > [...]
> > However it can still be useful to have the function name that 'git
> > diff' extracts as additional context for the change.
> > [...]
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > diff --git a/range-diff.c b/range-diff.c
> > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> > + } else if (starts_with(line.buf, "@@ ")) {
> > + char *skip_lineno = strstr(line.buf + 3, "@@");
> > + strbuf_remove(&line, 0, skip_lineno - line.buf);
>
> It makes me a bit uncomfortable that this is not checking for NULL
> return from strstr() before doing pointer arithmetic (even though the
> input is presumably machine-generated).
>
> if (!skip_lineno)
> BUG(...);
Good point, but maybe we should not go so far as to declare this a bug,
and fall back to removing everything bug the initial two `at` characters
instead?
Thanks,
Dscho
>
> might be appropriate.
>
> > + strbuf_addch(&buf, ' ');
> > + strbuf_addbuf(&buf, &line);
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff
2019-04-15 12:54 ` Johannes Schindelin
@ 2019-04-15 18:56 ` Thomas Gummerer
2019-04-17 11:52 ` Johannes Schindelin
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Gummerer @ 2019-04-15 18:56 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Eric Sunshine, Git List, Duy Nguyen, Junio C Hamano
On 04/15, Johannes Schindelin wrote:
> Hi Eric,
>
> On Sun, 14 Apr 2019, Eric Sunshine wrote:
>
> > On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > [...]
> > > However it can still be useful to have the function name that 'git
> > > diff' extracts as additional context for the change.
> > > [...]
> > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > ---
> > > diff --git a/range-diff.c b/range-diff.c
> > > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> > > + } else if (starts_with(line.buf, "@@ ")) {
> > > + char *skip_lineno = strstr(line.buf + 3, "@@");
> > > + strbuf_remove(&line, 0, skip_lineno - line.buf);
> >
> > It makes me a bit uncomfortable that this is not checking for NULL
> > return from strstr() before doing pointer arithmetic (even though the
> > input is presumably machine-generated).
> >
> > if (!skip_lineno)
> > BUG(...);
>
> Good point, but maybe we should not go so far as to declare this a bug,
> and fall back to removing everything bug the initial two `at` characters
> instead?
I like declaring this a bug. We are after all parsing
machine-generated output, that does come from git (which is why I
neglected the NULL checking in the first place). If that second "@@"
is not there it's definitely a bug somewhere in the diff machinery,
I'd say.
Note that the "@@" also couldn't come from anywhere else, the diff
header has a well defined format and so does the metadata. The diff
itself is prefixed with '<', '>' and '#' in this case, and the commit
message is also prefixed with four spaces. So if this breaks
somewhere I'd rather hear about it loudly, than let users potentially
get wrong output because we missed something somewhere.
> >
> > might be appropriate.
> >
> > > + strbuf_addch(&buf, ' ');
> > > + strbuf_addbuf(&buf, &line);
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff
2019-04-15 18:56 ` Thomas Gummerer
@ 2019-04-17 11:52 ` Johannes Schindelin
2019-04-24 18:15 ` Thomas Gummerer
0 siblings, 1 reply; 29+ messages in thread
From: Johannes Schindelin @ 2019-04-17 11:52 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: Eric Sunshine, Git List, Duy Nguyen, Junio C Hamano
Hi Thomas,
On Mon, 15 Apr 2019, Thomas Gummerer wrote:
> On 04/15, Johannes Schindelin wrote:
>
> > On Sun, 14 Apr 2019, Eric Sunshine wrote:
> >
> > > On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > > [...]
> > > > However it can still be useful to have the function name that 'git
> > > > diff' extracts as additional context for the change.
> > > > [...]
> > > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > > ---
> > > > diff --git a/range-diff.c b/range-diff.c
> > > > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> > > > + } else if (starts_with(line.buf, "@@ ")) {
> > > > + char *skip_lineno = strstr(line.buf + 3, "@@");
> > > > + strbuf_remove(&line, 0, skip_lineno - line.buf);
> > >
> > > It makes me a bit uncomfortable that this is not checking for NULL
> > > return from strstr() before doing pointer arithmetic (even though the
> > > input is presumably machine-generated).
> > >
> > > if (!skip_lineno)
> > > BUG(...);
> >
> > Good point, but maybe we should not go so far as to declare this a bug,
> > and fall back to removing everything bug the initial two `at` characters
> > instead?
>
> I like declaring this a bug. We are after all parsing
> machine-generated output, that does come from git (which is why I
> neglected the NULL checking in the first place). If that second "@@"
> is not there it's definitely a bug somewhere in the diff machinery,
> I'd say.
Ah, but you do know about the micro-project I proposed to optionally feed
an mbox to `range-diff`, right?
The idea behind my proposal is that this would make it possible to
generate a range-diff between the patches on public-inbox and the commits
that actually made it into Junio's `pu`...
> Note that the "@@" also couldn't come from anywhere else, the diff
> header has a well defined format and so does the metadata. The diff
> itself is prefixed with '<', '>' and '#' in this case, and the commit
> message is also prefixed with four spaces. So if this breaks
> somewhere I'd rather hear about it loudly, than let users potentially
> get wrong output because we missed something somewhere.
Agreed. But I could imagine that `die()`ing here would be the more
appropriate way to holler loudly ;-)
Ciao,
Dscho
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff
2019-04-17 11:52 ` Johannes Schindelin
@ 2019-04-24 18:15 ` Thomas Gummerer
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Gummerer @ 2019-04-24 18:15 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Eric Sunshine, Git List, Duy Nguyen, Junio C Hamano
On 04/17, Johannes Schindelin wrote:
> Hi Thomas,
>
> On Mon, 15 Apr 2019, Thomas Gummerer wrote:
> > I like declaring this a bug. We are after all parsing
> > machine-generated output, that does come from git (which is why I
> > neglected the NULL checking in the first place). If that second "@@"
> > is not there it's definitely a bug somewhere in the diff machinery,
> > I'd say.
>
> Ah, but you do know about the micro-project I proposed to optionally feed
> an mbox to `range-diff`, right?
>
> The idea behind my proposal is that this would make it possible to
> generate a range-diff between the patches on public-inbox and the commits
> that actually made it into Junio's `pu`...
I had forgotten about that, and was only looking at what the code
currently does.
> > Note that the "@@" also couldn't come from anywhere else, the diff
> > header has a well defined format and so does the metadata. The diff
> > itself is prefixed with '<', '>' and '#' in this case, and the commit
> > message is also prefixed with four spaces. So if this breaks
> > somewhere I'd rather hear about it loudly, than let users potentially
> > get wrong output because we missed something somewhere.
>
> Agreed. But I could imagine that `die()`ing here would be the more
> appropriate way to holler loudly ;-)
Yup, in the light of the potential microproject, I'm fine with just
'die()'ing here. If we'd get into this situation right now, there
would probably be a bunch of other tests failing as well, so we might
as well make it a little more future proof. Thanks.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff
2019-04-14 21:09 ` [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff Thomas Gummerer
2019-04-14 23:21 ` Eric Sunshine
@ 2019-04-15 12:53 ` Johannes Schindelin
2019-04-15 18:57 ` Thomas Gummerer
1 sibling, 1 reply; 29+ messages in thread
From: Johannes Schindelin @ 2019-04-15 12:53 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git, Duy Nguyen, Junio C Hamano
Hi Thomas,
On Sun, 14 Apr 2019, Thomas Gummerer wrote:
> When postprocessing the inner diff in range-diff, we currently replace
> the whole hunk header line with just "@@". This matches how 'git
> tbdiff' used to handle hunk headers as well.
>
> Most likely this is being done because line numbers in the hunk header
> are not relevant without other changes. They can for example easily
> change if a range is rebased, and lines are added/removed before a
> change that we actually care about in our ranges.
>
> However it can still be useful to have the function name that 'git
> diff' extracts as additional context for the change.
>
> Note that it is not guaranteed that the hunk header actually shows up
> in the range-diff, and this change only aims to improve the case where
> a hunk header would already be included in the final output.
Makes sense.
> diff --git a/range-diff.c b/range-diff.c
> index 9242b8975f..f365141ade 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> strbuf_addch(&buf, '\n');
> }
> continue;
> - } else if (starts_with(line.buf, "@@ "))
> - strbuf_addstr(&buf, "@@");
> - else if (!line.buf[0] || starts_with(line.buf, "index "))
> + } else if (starts_with(line.buf, "@@ ")) {
> + char *skip_lineno = strstr(line.buf + 3, "@@");
Rather than using the magic constant "3", it would probably make sense to
declare `skip_lineno` outside of the `if` construct, and use
`skip_prefix(line.buf, "@@ ", &skip_lineno)` instead of
`starts_with(...)`.
We *will*, however, want to have a safeguard against `strstr()` not
finding anything. Maybe re-use the `p` variable that we already have, and
do this instead:
} else if (skip_prefix(line.buf, "@@ ", &p) &&
(p = strstr(p, "@@"))) {
> + strbuf_remove(&line, 0, skip_lineno - line.buf);
> + strbuf_addch(&buf, ' ');
Shorter:
strbuf_splice(&line, 0, p - line.buf, " ", 1);
(assuming that you accept my suggestion to use `p` instead of
`skip_lineno`...)
Thanks,
Dscho
> + strbuf_addbuf(&buf, &line);
> + } else if (!line.buf[0] || starts_with(line.buf, "index "))
> /*
> * A completely blank (not ' \n', which is context)
> * line is not valid in a diff. We skip it
> --
> 2.21.0.593.g511ec345e1
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff
2019-04-15 12:53 ` Johannes Schindelin
@ 2019-04-15 18:57 ` Thomas Gummerer
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Gummerer @ 2019-04-15 18:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Duy Nguyen, Junio C Hamano
On 04/15, Johannes Schindelin wrote:
> Hi Thomas,
>
>
> On Sun, 14 Apr 2019, Thomas Gummerer wrote:
> > diff --git a/range-diff.c b/range-diff.c
> > index 9242b8975f..f365141ade 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> > strbuf_addch(&buf, '\n');
> > }
> > continue;
> > - } else if (starts_with(line.buf, "@@ "))
> > - strbuf_addstr(&buf, "@@");
> > - else if (!line.buf[0] || starts_with(line.buf, "index "))
> > + } else if (starts_with(line.buf, "@@ ")) {
> > + char *skip_lineno = strstr(line.buf + 3, "@@");
>
> Rather than using the magic constant "3", it would probably make sense to
> declare `skip_lineno` outside of the `if` construct, and use
> `skip_prefix(line.buf, "@@ ", &skip_lineno)` instead of
> `starts_with(...)`.
I like this suggestion.
> We *will*, however, want to have a safeguard against `strstr()` not
> finding anything. Maybe re-use the `p` variable that we already have, and
> do this instead:
>
> } else if (skip_prefix(line.buf, "@@ ", &p) &&
> (p = strstr(p, "@@"))) {
>
> > + strbuf_remove(&line, 0, skip_lineno - line.buf);
> > + strbuf_addch(&buf, ' ');
>
> Shorter:
>
> strbuf_splice(&line, 0, p - line.buf, " ", 1);
>
> (assuming that you accept my suggestion to use `p` instead of
> `skip_lineno`...)
And this is also much nicer, thanks!
> Thanks,
> Dscho
>
> > + strbuf_addbuf(&buf, &line);
> > + } else if (!line.buf[0] || starts_with(line.buf, "index "))
> > /*
> > * A completely blank (not ' \n', which is context)
> > * line is not valid in a diff. We skip it
> > --
> > 2.21.0.593.g511ec345e1
> >
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* [RFC PATCH 3/4] range-diff: add section header instead of diff header
2019-04-14 21:09 ` [RFC PATCH 0/4] output improvements for git range-diff Thomas Gummerer
2019-04-14 21:09 ` [RFC PATCH 1/4] range-diff: fix function parameter indentation Thomas Gummerer
2019-04-14 21:09 ` [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff Thomas Gummerer
@ 2019-04-14 21:09 ` Thomas Gummerer
2019-04-14 23:29 ` Eric Sunshine
2019-04-14 21:09 ` [RFC PATCH 4/4] range-diff: add section headers to the outer hunk header Thomas Gummerer
2019-04-15 12:47 ` [RFC PATCH 0/4] output improvements for git range-diff Johannes Schindelin
4 siblings, 1 reply; 29+ messages in thread
From: Thomas Gummerer @ 2019-04-14 21:09 UTC (permalink / raw)
To: git; +Cc: Duy Nguyen, Johannes Schindelin, Junio C Hamano, Thomas Gummerer
Currently range-diff keeps the diff header of the inner diff
intact (apart from stripping lines starting with index). This diff
header is somewhat useful, especially when files get different
names in different ranges.
However there is no real need to keep the whole diff header for that.
The main reason we currently do that is probably because it is easy to
do.
Introduce a new range diff hunk header, that's enclosed by "##",
similar to how line numbers in diff hunks are enclosed by "@@", and
give human readable information of what exactly happened to the file,
including the file name.
At this point, this is only a marginal improvement in readability of
the range-diff output. More interestingly however, this allows us to
add these range diff hunk headers to the outer diffs hunk headers
using a custom userdiff pattern, which should help making the
range-diff more readable.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
range-diff.c | 34 ++++++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index f365141ade..aa466060ef 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -33,6 +33,7 @@ static int read_patches(const char *range, struct string_list *list)
struct child_process cp = CHILD_PROCESS_INIT;
FILE *in;
struct strbuf buf = STRBUF_INIT, line = STRBUF_INIT;
+ struct strbuf filename_a = STRBUF_INIT;
struct patch_util *util = NULL;
int in_header = 1;
@@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)
strbuf_addch(&buf, '\n');
if (!util->diff_offset)
util->diff_offset = buf.len;
- strbuf_addch(&buf, ' ');
- strbuf_addbuf(&buf, &line);
+ } else if (starts_with(line.buf, "--- ")) {
+ if (!strcmp(line.buf, "--- /dev/null"))
+ strbuf_remove(&line, 0, 4);
+ else
+ strbuf_remove(&line, 0, 6);
+ strbuf_rtrim(&line);
+ strbuf_reset(&filename_a);
+ strbuf_addbuf(&filename_a, &line);
+ } else if (starts_with(line.buf, "+++ ")) {
+ strbuf_addstr(&buf, " ## ");
+ if (!strcmp(line.buf, "--- /dev/null"))
+ strbuf_remove(&line, 0, 4);
+ else
+ strbuf_remove(&line, 0, 6);
+ strbuf_rtrim(&line);
+ if (!strcmp(filename_a.buf, "/dev/null")) {
+ strbuf_addstr(&buf, "new file ");
+ strbuf_addbuf(&buf, &line);
+ } else if (!strcmp(line.buf, "/dev/null")) {
+ strbuf_addstr(&buf, "removed file ");
+ strbuf_addbuf(&buf, &line);
+ } else if (strbuf_cmp(&filename_a, &line)) {
+ strbuf_addstr(&buf, "renamed file ");
+ strbuf_addbuf(&buf, &filename_a);
+ strbuf_addstr(&buf, " -> ");
+ strbuf_addbuf(&buf, &line);
+ } else {
+ strbuf_addstr(&buf, "modified file ");
+ strbuf_addbuf(&buf, &line);
+ }
+ strbuf_addstr(&buf, " ##\n");
} else if (in_header) {
if (starts_with(line.buf, "Author: ")) {
strbuf_addbuf(&buf, &line);
--
2.21.0.593.g511ec345e1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header
2019-04-14 21:09 ` [RFC PATCH 3/4] range-diff: add section header instead of diff header Thomas Gummerer
@ 2019-04-14 23:29 ` Eric Sunshine
2019-04-15 6:28 ` Johannes Sixt
2019-04-15 13:01 ` Johannes Schindelin
0 siblings, 2 replies; 29+ messages in thread
From: Eric Sunshine @ 2019-04-14 23:29 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: Git List, Duy Nguyen, Johannes Schindelin, Junio C Hamano
On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> [...]
> Introduce a new range diff hunk header, that's enclosed by "##",
> similar to how line numbers in diff hunks are enclosed by "@@", and
> give human readable information of what exactly happened to the file,
> including the file name.
> [...]
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/range-diff.c b/range-diff.c
> @@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)
> + } else if (starts_with(line.buf, "--- ")) {
> + if (!strcmp(line.buf, "--- /dev/null"))
> + strbuf_remove(&line, 0, 4);
> + else
> + strbuf_remove(&line, 0, 6);
> + strbuf_rtrim(&line);
> + strbuf_reset(&filename_a);
> + strbuf_addbuf(&filename_a, &line);
> + } else if (starts_with(line.buf, "+++ ")) {
At this point, we know that line.buf starts with "+++"...
> + strbuf_addstr(&buf, " ## ");
> + if (!strcmp(line.buf, "--- /dev/null"))
so, it seems unlikely that it's ever going to match "--- /dev/null".
> + strbuf_remove(&line, 0, 4);
> + if (!strcmp(filename_a.buf, "/dev/null")) {
> + strbuf_addstr(&buf, "new file ");
> + strbuf_addbuf(&buf, &line);
> + } else if (!strcmp(line.buf, "/dev/null")) {
> + strbuf_addstr(&buf, "removed file ");
> + strbuf_addbuf(&buf, &line);
> + } else if (strbuf_cmp(&filename_a, &line)) {
> + strbuf_addstr(&buf, "renamed file ");
> + strbuf_addbuf(&buf, &filename_a);
> + strbuf_addstr(&buf, " -> ");
> + strbuf_addbuf(&buf, &line);
> + } else {
> + strbuf_addstr(&buf, "modified file ");
> + strbuf_addbuf(&buf, &line);
> + }
All of these disposition strings end with "file", which seems
redundant. Short and sweet "new", "removed", "renamed", "modified"
provide just as much useful information.
Also, should these strings be localizable? Alternately, rather than
using prose to describe the disposition, perhaps do so symbolically
(thus universally), say with "+", "-", "->", "*" (or ""),
respectively?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header
2019-04-14 23:29 ` Eric Sunshine
@ 2019-04-15 6:28 ` Johannes Sixt
2019-04-15 13:01 ` Johannes Schindelin
1 sibling, 0 replies; 29+ messages in thread
From: Johannes Sixt @ 2019-04-15 6:28 UTC (permalink / raw)
To: Eric Sunshine, Thomas Gummerer
Cc: Git List, Duy Nguyen, Johannes Schindelin, Junio C Hamano
Am 15.04.19 um 01:29 schrieb Eric Sunshine:
> On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>> + strbuf_remove(&line, 0, 4);
>> + if (!strcmp(filename_a.buf, "/dev/null")) {
>> + strbuf_addstr(&buf, "new file ");
>> + strbuf_addbuf(&buf, &line);
>> + } else if (!strcmp(line.buf, "/dev/null")) {
>> + strbuf_addstr(&buf, "removed file ");
>> + strbuf_addbuf(&buf, &line);
>> + } else if (strbuf_cmp(&filename_a, &line)) {
>> + strbuf_addstr(&buf, "renamed file ");
>> + strbuf_addbuf(&buf, &filename_a);
>> + strbuf_addstr(&buf, " -> ");
>> + strbuf_addbuf(&buf, &line);
>> + } else {
>> + strbuf_addstr(&buf, "modified file ");
>> + strbuf_addbuf(&buf, &line);
>> + }
>
> All of these disposition strings end with "file", which seems
> redundant. Short and sweet "new", "removed", "renamed", "modified"
> provide just as much useful information.
>
> Also, should these strings be localizable? Alternately, rather than
> using prose to describe the disposition, perhaps do so symbolically
> (thus universally), say with "+", "-", "->", "*" (or ""),
> respectively?
When the strings are translated, it would be preferable to not do this
sentence lego at all and have format strings that are filled with the
values.
Then the noise word "file" is probably not that bad, in particular, when
translators tend to not add words that they don't see in the original.
For example, German translations of "new %s" I anticipate "neues %s",
"%s (neu)", "neu %s", all of which are awkward, but translations of "new
file %s" lends itself naturally to "neue Datei %s", which is fine.
-- Hannes
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header
2019-04-14 23:29 ` Eric Sunshine
2019-04-15 6:28 ` Johannes Sixt
@ 2019-04-15 13:01 ` Johannes Schindelin
2019-04-15 19:09 ` Thomas Gummerer
1 sibling, 1 reply; 29+ messages in thread
From: Johannes Schindelin @ 2019-04-15 13:01 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Thomas Gummerer, Git List, Duy Nguyen, Junio C Hamano
Hi Eric,
On Sun, 14 Apr 2019, Eric Sunshine wrote:
> On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > [...]
> > Introduce a new range diff hunk header, that's enclosed by "##",
> > similar to how line numbers in diff hunks are enclosed by "@@", and
> > give human readable information of what exactly happened to the file,
> > including the file name.
> > [...]
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > diff --git a/range-diff.c b/range-diff.c
> > @@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)
> > + } else if (starts_with(line.buf, "--- ")) {
> > + if (!strcmp(line.buf, "--- /dev/null"))
> > + strbuf_remove(&line, 0, 4);
> > + else
> > + strbuf_remove(&line, 0, 6);
> > + strbuf_rtrim(&line);
> > + strbuf_reset(&filename_a);
> > + strbuf_addbuf(&filename_a, &line);
> > + } else if (starts_with(line.buf, "+++ ")) {
>
> At this point, we know that line.buf starts with "+++"...
>
> > + strbuf_addstr(&buf, " ## ");
> > + if (!strcmp(line.buf, "--- /dev/null"))
>
> so, it seems unlikely that it's ever going to match "--- /dev/null".
>
> > + strbuf_remove(&line, 0, 4);
> > + if (!strcmp(filename_a.buf, "/dev/null")) {
> > + strbuf_addstr(&buf, "new file ");
> > + strbuf_addbuf(&buf, &line);
> > + } else if (!strcmp(line.buf, "/dev/null")) {
> > + strbuf_addstr(&buf, "removed file ");
> > + strbuf_addbuf(&buf, &line);
> > + } else if (strbuf_cmp(&filename_a, &line)) {
> > + strbuf_addstr(&buf, "renamed file ");
> > + strbuf_addbuf(&buf, &filename_a);
> > + strbuf_addstr(&buf, " -> ");
> > + strbuf_addbuf(&buf, &line);
> > + } else {
> > + strbuf_addstr(&buf, "modified file ");
> > + strbuf_addbuf(&buf, &line);
> > + }
>
> All of these disposition strings end with "file", which seems
> redundant. Short and sweet "new", "removed", "renamed", "modified"
> provide just as much useful information.
>
> Also, should these strings be localizable?
I'd rather not.
> Alternately, rather than using prose to describe the disposition,
> perhaps do so symbolically (thus universally), say with "+", "-", "->",
> "*" (or ""), respectively?
Or maybe streamline the common case (modified) by *not* saying anything,
then? I.e.
@@ Documentation/Makefile
for a modified file,
@@ builtin/psuh.c (new)
for a new file,
@@ git-add--interactive.perl (deleted)
for a removed one, and
@@ builtin/serve.c -> t/helper/test-serve-v2.c
for a renamed one.
That should also give us a bit of wiggle room to append the function name
part of the inner hunk header, if any.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header
2019-04-15 13:01 ` Johannes Schindelin
@ 2019-04-15 19:09 ` Thomas Gummerer
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Gummerer @ 2019-04-15 19:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Eric Sunshine, Git List, Duy Nguyen, Junio C Hamano
On 04/15, Johannes Schindelin wrote:
> Hi Eric,
>
> On Sun, 14 Apr 2019, Eric Sunshine wrote:
>
> > On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > [...]
> > > Introduce a new range diff hunk header, that's enclosed by "##",
> > > similar to how line numbers in diff hunks are enclosed by "@@", and
> > > give human readable information of what exactly happened to the file,
> > > including the file name.
> > > [...]
> > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > ---
> > > diff --git a/range-diff.c b/range-diff.c
> > > @@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)
> > > + } else if (starts_with(line.buf, "--- ")) {
> > > + if (!strcmp(line.buf, "--- /dev/null"))
> > > + strbuf_remove(&line, 0, 4);
> > > + else
> > > + strbuf_remove(&line, 0, 6);
> > > + strbuf_rtrim(&line);
> > > + strbuf_reset(&filename_a);
> > > + strbuf_addbuf(&filename_a, &line);
> > > + } else if (starts_with(line.buf, "+++ ")) {
> >
> > At this point, we know that line.buf starts with "+++"...
> >
> > > + strbuf_addstr(&buf, " ## ");
> > > + if (!strcmp(line.buf, "--- /dev/null"))
> >
> > so, it seems unlikely that it's ever going to match "--- /dev/null".
Ouch yup, this is some bad copy pasta, thanks for catching!
> > > + strbuf_remove(&line, 0, 4);
> > > + if (!strcmp(filename_a.buf, "/dev/null")) {
> > > + strbuf_addstr(&buf, "new file ");
> > > + strbuf_addbuf(&buf, &line);
> > > + } else if (!strcmp(line.buf, "/dev/null")) {
> > > + strbuf_addstr(&buf, "removed file ");
> > > + strbuf_addbuf(&buf, &line);
> > > + } else if (strbuf_cmp(&filename_a, &line)) {
> > > + strbuf_addstr(&buf, "renamed file ");
> > > + strbuf_addbuf(&buf, &filename_a);
> > > + strbuf_addstr(&buf, " -> ");
> > > + strbuf_addbuf(&buf, &line);
> > > + } else {
> > > + strbuf_addstr(&buf, "modified file ");
> > > + strbuf_addbuf(&buf, &line);
> > > + }
> >
> > All of these disposition strings end with "file", which seems
> > redundant. Short and sweet "new", "removed", "renamed", "modified"
> > provide just as much useful information.
> >
> > Also, should these strings be localizable?
>
> I'd rather not.
Dunno, why do you think they should not be localizable? I'm tend to
agree with Eric that they could be made localizable, after all this
output is not supposed to be machine readable either way. I don't
have a strong opinion here though.
> > Alternately, rather than using prose to describe the disposition,
> > perhaps do so symbolically (thus universally), say with "+", "-", "->",
> > "*" (or ""), respectively?
>
> Or maybe streamline the common case (modified) by *not* saying anything,
> then? I.e.
>
> @@ Documentation/Makefile
>
> for a modified file,
>
> @@ builtin/psuh.c (new)
>
> for a new file,
>
> @@ git-add--interactive.perl (deleted)
>
> for a removed one, and
>
> @@ builtin/serve.c -> t/helper/test-serve-v2.c
>
> for a renamed one.
This looks like a good suggestion to me, thanks!
> That should also give us a bit of wiggle room to append the function name
> part of the inner hunk header, if any.
>
> Ciao,
> Dscho
^ permalink raw reply [flat|nested] 29+ messages in thread
* [RFC PATCH 4/4] range-diff: add section headers to the outer hunk header
2019-04-14 21:09 ` [RFC PATCH 0/4] output improvements for git range-diff Thomas Gummerer
` (2 preceding siblings ...)
2019-04-14 21:09 ` [RFC PATCH 3/4] range-diff: add section header instead of diff header Thomas Gummerer
@ 2019-04-14 21:09 ` Thomas Gummerer
2019-04-15 12:44 ` Johannes Schindelin
2019-04-15 12:47 ` [RFC PATCH 0/4] output improvements for git range-diff Johannes Schindelin
4 siblings, 1 reply; 29+ messages in thread
From: Thomas Gummerer @ 2019-04-14 21:09 UTC (permalink / raw)
To: git; +Cc: Duy Nguyen, Johannes Schindelin, Junio C Hamano, Thomas Gummerer
Add the section headers we introduced in the previous commit to the
outer diff's hunk headers. This makes it easier to understand which
change we are actually looking at. For example an outer hunk header
might now look like:
@@ -77,15 +78,43 @@ modified file Documentation/config/interactive.txt
while previously it would have only been
@@ -77,15 +78,43 @@
which doesn't give a lot of context for the change that follows.
For completeness also add section headers for the commit metadata and
the commit message, although they are arguably less important.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
range-diff.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index aa466060ef..15ac54f369 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -124,8 +124,10 @@ static int read_patches(const char *range, struct string_list *list)
strbuf_addstr(&buf, " ##\n");
} else if (in_header) {
if (starts_with(line.buf, "Author: ")) {
+ strbuf_addstr(&buf, " ## Metadata ##\n");
strbuf_addbuf(&buf, &line);
strbuf_addstr(&buf, "\n\n");
+ strbuf_addstr(&buf, " ## Commit message ##\n");
} else if (starts_with(line.buf, " ")) {
strbuf_rtrim(&line);
strbuf_addbuf(&buf, &line);
@@ -387,8 +389,8 @@ static void output_pair_header(struct diff_options *diffopt,
fwrite(buf->buf, buf->len, 1, diffopt->file);
}
-static struct userdiff_driver no_func_name = {
- .funcname = { "$^", 0 }
+static struct userdiff_driver section_headers = {
+ .funcname = { "^ ## (.*) ##$", REG_EXTENDED }
};
static struct diff_filespec *get_filespec(const char *name, const char *p)
@@ -400,7 +402,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
spec->size = strlen(p);
spec->should_munmap = 0;
spec->is_stdin = 1;
- spec->driver = &no_func_name;
+ spec->driver = §ion_headers;
return spec;
}
--
2.21.0.593.g511ec345e1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 4/4] range-diff: add section headers to the outer hunk header
2019-04-14 21:09 ` [RFC PATCH 4/4] range-diff: add section headers to the outer hunk header Thomas Gummerer
@ 2019-04-15 12:44 ` Johannes Schindelin
2019-04-15 18:48 ` Thomas Gummerer
0 siblings, 1 reply; 29+ messages in thread
From: Johannes Schindelin @ 2019-04-15 12:44 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git, Duy Nguyen, Junio C Hamano
Hi Thomas,
On Sun, 14 Apr 2019, Thomas Gummerer wrote:
> Add the section headers we introduced in the previous commit to the
> outer diff's hunk headers. This makes it easier to understand which
> change we are actually looking at. For example an outer hunk header
> might now look like:
>
> @@ -77,15 +78,43 @@ modified file Documentation/config/interactive.txt
>
> while previously it would have only been
>
> @@ -77,15 +78,43 @@
>
> which doesn't give a lot of context for the change that follows.
>
> For completeness also add section headers for the commit metadata and
> the commit message, although they are arguably less important.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
That makes a total lot of sense. So much so that we should protect it by a
new regression test case.
> diff --git a/range-diff.c b/range-diff.c
> index aa466060ef..15ac54f369 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -124,8 +124,10 @@ static int read_patches(const char *range, struct string_list *list)
> strbuf_addstr(&buf, " ##\n");
> } else if (in_header) {
> if (starts_with(line.buf, "Author: ")) {
> + strbuf_addstr(&buf, " ## Metadata ##\n");
> strbuf_addbuf(&buf, &line);
> strbuf_addstr(&buf, "\n\n");
> + strbuf_addstr(&buf, " ## Commit message ##\n");
Should this not rather be added when `in_header` is set to false?
The rest of the patch makes a whole lot of sense to me.
Thanks,
Dscho
> } else if (starts_with(line.buf, " ")) {
> strbuf_rtrim(&line);
> strbuf_addbuf(&buf, &line);
> @@ -387,8 +389,8 @@ static void output_pair_header(struct diff_options *diffopt,
> fwrite(buf->buf, buf->len, 1, diffopt->file);
> }
>
> -static struct userdiff_driver no_func_name = {
> - .funcname = { "$^", 0 }
> +static struct userdiff_driver section_headers = {
> + .funcname = { "^ ## (.*) ##$", REG_EXTENDED }
> };
>
> static struct diff_filespec *get_filespec(const char *name, const char *p)
> @@ -400,7 +402,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
> spec->size = strlen(p);
> spec->should_munmap = 0;
> spec->is_stdin = 1;
> - spec->driver = &no_func_name;
> + spec->driver = §ion_headers;
>
> return spec;
> }
> --
> 2.21.0.593.g511ec345e1
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 4/4] range-diff: add section headers to the outer hunk header
2019-04-15 12:44 ` Johannes Schindelin
@ 2019-04-15 18:48 ` Thomas Gummerer
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Gummerer @ 2019-04-15 18:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Duy Nguyen, Junio C Hamano
On 04/15, Johannes Schindelin wrote:
> Hi Thomas,
>
> On Sun, 14 Apr 2019, Thomas Gummerer wrote:
>
> > Add the section headers we introduced in the previous commit to the
> > outer diff's hunk headers. This makes it easier to understand which
> > change we are actually looking at. For example an outer hunk header
> > might now look like:
> >
> > @@ -77,15 +78,43 @@ modified file Documentation/config/interactive.txt
> >
> > while previously it would have only been
> >
> > @@ -77,15 +78,43 @@
> >
> > which doesn't give a lot of context for the change that follows.
> >
> > For completeness also add section headers for the commit metadata and
> > the commit message, although they are arguably less important.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
>
> That makes a total lot of sense. So much so that we should protect it by a
> new regression test case.
Yup, definitely. As mentioned in the cover letter, I punted on to
discuss and settle on an output format first, so I don't have to
duplicate a lot of work writing and re-writing the tests ;)
> > diff --git a/range-diff.c b/range-diff.c
> > index aa466060ef..15ac54f369 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -124,8 +124,10 @@ static int read_patches(const char *range, struct string_list *list)
> > strbuf_addstr(&buf, " ##\n");
> > } else if (in_header) {
> > if (starts_with(line.buf, "Author: ")) {
> > + strbuf_addstr(&buf, " ## Metadata ##\n");
> > strbuf_addbuf(&buf, &line);
> > strbuf_addstr(&buf, "\n\n");
> > + strbuf_addstr(&buf, " ## Commit message ##\n");
>
> Should this not rather be added when `in_header` is set to false?
I don't think so. The commit message counts as part of in_header, and
starts right after the rest of the commit metadata. If we would add
this after when we set in_header to false, we'd write it after the
commit message, which would make the hunk header "@@ ... @@ Metadata",
even when we show the commit message in the output.
> The rest of the patch makes a whole lot of sense to me.
Thanks!
> Thanks,
> Dscho
>
> > } else if (starts_with(line.buf, " ")) {
> > strbuf_rtrim(&line);
> > strbuf_addbuf(&buf, &line);
> > @@ -387,8 +389,8 @@ static void output_pair_header(struct diff_options *diffopt,
> > fwrite(buf->buf, buf->len, 1, diffopt->file);
> > }
> >
> > -static struct userdiff_driver no_func_name = {
> > - .funcname = { "$^", 0 }
> > +static struct userdiff_driver section_headers = {
> > + .funcname = { "^ ## (.*) ##$", REG_EXTENDED }
> > };
> >
> > static struct diff_filespec *get_filespec(const char *name, const char *p)
> > @@ -400,7 +402,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
> > spec->size = strlen(p);
> > spec->should_munmap = 0;
> > spec->is_stdin = 1;
> > - spec->driver = &no_func_name;
> > + spec->driver = §ion_headers;
> >
> > return spec;
> > }
> > --
> > 2.21.0.593.g511ec345e1
> >
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 0/4] output improvements for git range-diff
2019-04-14 21:09 ` [RFC PATCH 0/4] output improvements for git range-diff Thomas Gummerer
` (3 preceding siblings ...)
2019-04-14 21:09 ` [RFC PATCH 4/4] range-diff: add section headers to the outer hunk header Thomas Gummerer
@ 2019-04-15 12:47 ` Johannes Schindelin
2019-04-15 19:25 ` Thomas Gummerer
4 siblings, 1 reply; 29+ messages in thread
From: Johannes Schindelin @ 2019-04-15 12:47 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git, Duy Nguyen, Junio C Hamano
Hi Thomas,
On Sun, 14 Apr 2019, Thomas Gummerer wrote:
> @@ -99,10 +90,10 @@ modified file Documentation/git-revert.txt
A better example might be a .c file, as the function name is often a
pretty useful piece of information.
Read: I think it should be part of the outer hunk header.
Also, the text "modified file" takes up an awful lot of space. Maybe we do
not really need that information?
While at it, we could strip the line numbers, as this is not intended for
machine consumption, but for human consumption instead.
> [...]
> Note that this patch series doesn't modify or add any tests, and was
> just manually tested locally, thus it is still marked as RFC.
Oh, okay then ;-)
Thanks for working on this,
Dscho
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC PATCH 0/4] output improvements for git range-diff
2019-04-15 12:47 ` [RFC PATCH 0/4] output improvements for git range-diff Johannes Schindelin
@ 2019-04-15 19:25 ` Thomas Gummerer
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Gummerer @ 2019-04-15 19:25 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Duy Nguyen, Junio C Hamano
On 04/15, Johannes Schindelin wrote:
> Hi Thomas,
>
> On Sun, 14 Apr 2019, Thomas Gummerer wrote:
>
> > @@ -99,10 +90,10 @@ modified file Documentation/git-revert.txt
>
> A better example might be a .c file, as the function name is often a
> pretty useful piece of information.
Yeah, maybe with your suggestions, we could fit the function name or
some of the function name into the outer hunk header. I'll give it a
try and see how it looks.
> Read: I think it should be part of the outer hunk header.
>
> Also, the text "modified file" takes up an awful lot of space. Maybe we do
> not really need that information?
>
> While at it, we could strip the line numbers, as this is not intended for
> machine consumption, but for human consumption instead.
Yeah, that makes sense, the line numbers are really kind of pointless
in a range-diff.
> > [...]
> > Note that this patch series doesn't modify or add any tests, and was
> > just manually tested locally, thus it is still marked as RFC.
>
> Oh, okay then ;-)
>
> Thanks for working on this,
> Dscho
^ permalink raw reply [flat|nested] 29+ messages in thread