From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Thomas Gummerer <t.gummerer@gmail.com>
Cc: git@vger.kernel.org, Duy Nguyen <pclouds@gmail.com>,
Junio C Hamano <gitster@pobox.com>,
Eric Sunshine <sunshine@sunshineco.com>,
Johannes Sixt <j6t@kdbg.org>
Subject: Re: [PATCH v2 12/14] range-diff: add section header instead of diff header
Date: Fri, 5 Jul 2019 21:35:22 +0200 (CEST) [thread overview]
Message-ID: <nycvar.QRO.7.76.6.1907052114480.44@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190705170630.27500-13-t.gummerer@gmail.com>
Hi Thomas,
On Fri, 5 Jul 2019, Thomas Gummerer wrote:
> 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.
>
> This improves the readability of the range-diff by giving more concise
> information to the users. For example if a file was renamed in one
> iteration, but not in another, the diff of the headers would be quite
> noisy. However the diff of a single line is concise and should be
> easier to understand.
>
> Additionaly, this allows us to add these range diff section headers to
s/Additionaly/Additionally/
> the outer diffs hunk headers using a custom userdiff pattern, which
> should help making the range-diff more readable.
Makes sense.
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> range-diff.c | 35 ++++++++++++----
> t/t3206-range-diff.sh | 91 +++++++++++++++++++++++++++++++++++++++---
> t/t3206/history.export | 84 ++++++++++++++++++++++++++++++++++++--
> 3 files changed, 193 insertions(+), 17 deletions(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index b31fbab026..cc01f7f573 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -10,6 +10,7 @@
> #include "commit.h"
> #include "pretty.h"
> #include "userdiff.h"
> +#include "apply.h"
>
> struct patch_util {
> /* For the search for an exact match */
> @@ -95,12 +96,36 @@ static int read_patches(const char *range, struct string_list *list)
> }
>
> if (starts_with(line, "diff --git")) {
> + struct patch patch;
If you append ` = { 0 }` (or ` = { NULL }`, depending on the first field
of that struct, you don't need that `memset()` later.
> + struct strbuf root = STRBUF_INIT;
> + int linenr = 0;
> +
> in_header = 0;
> strbuf_addch(&buf, '\n');
> if (!util->diff_offset)
> util->diff_offset = buf.len;
> - strbuf_addch(&buf, ' ');
> - strbuf_addstr(&buf, line);
> + memset(&patch, 0, sizeof(patch));
> + line[len - 1] = '\n';
I guess `parse_git_header()` cannot handle lines ending in a NUL?
> + len = parse_git_header(&root, &linenr, 1, line,
> + len, size, &patch);
> + if (len < 0)
> + die(_("could not parse git header"));
Maybe include the line's contents, like ` '%.*s'", (int)len, line`?
> + strbuf_addstr(&buf, " ## ");
> + if (patch.is_new > 0)
> + strbuf_addf(&buf, "%s (new)", patch.new_name);
> + else if (patch.is_delete > 0)
> + strbuf_addf(&buf, "%s (deleted)", patch.old_name);
> + else if (patch.is_rename)
> + strbuf_addf(&buf, "%s => %s", patch.old_name, patch.new_name);
> + else
> + strbuf_addstr(&buf, patch.new_name);
> +
> + if (patch.new_mode && patch.old_mode &&
> + patch.old_mode != patch.new_mode)
> + strbuf_addf(&buf, " (mode change %06o => %06o)",
> + patch.old_mode, patch.new_mode);
> +
> + strbuf_addstr(&buf, " ##");
> } else if (in_header) {
> if (starts_with(line, "Author: ")) {
> strbuf_addstr(&buf, line);
> @@ -117,17 +142,13 @@ static int read_patches(const char *range, struct string_list *list)
> if (!(p = strstr(p, "@@")))
> die(_("invalid hunk header in inner diff"));
> strbuf_addstr(&buf, p);
> - } else if (!line[0] || starts_with(line, "index "))
> + } else if (!line[0])
> /*
> * A completely blank (not ' \n', which is context)
> * line is not valid in a diff. We skip it
> * silently, because this neatly handles the blank
> * separator line between commits in git-log
> * output.
> - *
> - * We also want to ignore the diff's `index` lines
> - * because they contain exact blob hashes in which
> - * we are not interested.
Oh, are we interested in them again?
> */
> continue;
> else if (line[0] == '>') {
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 9f89af7178..c277756057 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -181,6 +181,85 @@ test_expect_success 'changed commit with sm config' '
> test_cmp expected actual
> '
>
> +test_expect_success 'renamed file' '
> + git range-diff --no-color --submodule=log topic...renamed-file >actual &&
> + sed s/Z/\ /g >expected <<-EOF &&
> + 1: 4de457d = 1: f258d75 s/5/A/
> + 2: fccce22 ! 2: 017b62d s/4/A/
> + @@
> + ZAuthor: Thomas Rast <trast@inf.ethz.ch>
> + Z
> + - s/4/A/
> + + s/4/A/ + rename file
> + Z
> + - ## file ##
> + + ## file => renamed-file ##
I guess there is no good way to suppress the `- ## file ##` line in this
case? It is a bit distracting...
> + Z@@
> + Z 1
> + Z 2
> + 3: 147e64e ! 3: 3ce7af6 s/11/B/
> + @@
> + Z
> + Z s/11/B/
> + Z
> + - ## file ##
> + + ## renamed-file ##
> + Z@@ A
> + Z 8
> + Z 9
> + 4: a63e992 ! 4: 1e6226b s/12/B/
> + @@
> + Z
> + Z s/12/B/
> + Z
> + - ## file ##
> + + ## renamed-file ##
> + Z@@ A
> + Z 9
> + Z 10
> + EOF
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'file added and later removed' '
> + git range-diff --no-color --submodule=log topic...added-removed >actual &&
> + sed s/Z/\ /g >expected <<-EOF &&
> + 1: 4de457d = 1: 096b1ba s/5/A/
> + 2: fccce22 ! 2: d92e698 s/4/A/
> + @@
> + ZAuthor: Thomas Rast <trast@inf.ethz.ch>
> + Z
> + - s/4/A/
> + + s/4/A/ + new-file
> + Z
> + Z ## file ##
> + Z@@
> + @@
> + Z A
> + Z 6
> + Z 7
> + +
> + + ## new-file (new) ##
> + 3: 147e64e ! 3: 9a1db4d s/11/B/
> + @@
> + ZAuthor: Thomas Rast <trast@inf.ethz.ch>
> + Z
> + - s/11/B/
> + + s/11/B/ + remove file
> + Z
> + Z ## file ##
> + Z@@ A
> + @@
> + Z 12
> + Z 13
> + Z 14
> + +
> + + ## new-file (deleted) ##
> + 4: a63e992 = 4: fea3b5c s/12/B/
> + EOF
> + test_cmp expected actual
> +'
> +
> test_expect_success 'no commits on one side' '
> git commit --amend -m "new message" &&
> git range-diff master HEAD@{1} HEAD
> @@ -197,9 +276,9 @@ test_expect_success 'changed message' '
> Z
> + Also a silly comment here!
> +
> - Z diff --git a/file b/file
> - Z --- a/file
> - Z +++ b/file
> + Z ## file ##
> + Z@@
> + Z 1
> 3: 147e64e = 3: b9cb956 s/11/B/
> 4: a63e992 = 4: 8add5f1 s/12/B/
> EOF
> @@ -216,9 +295,9 @@ test_expect_success 'dual-coloring' '
> : <RESET>
> : <REVERSE><GREEN>+<RESET><BOLD> Also a silly comment here!<RESET>
> : <REVERSE><GREEN>+<RESET>
> - : diff --git a/file b/file<RESET>
> - : --- a/file<RESET>
> - : +++ b/file<RESET>
> + : ## file ##<RESET>
> + : <CYAN> @@<RESET>
> + : 1<RESET>
I am a bit confused where these last two lines come from all of a
sudden... They were not there before, and I do not see any code change in
this patch that would be responsible for them, either...
Could you help me understand?
> :<RED>3: 0559556 <RESET><YELLOW>!<RESET><GREEN> 3: b9cb956<RESET><YELLOW> s/11/B/<RESET>
> : <REVERSE><CYAN>@@<RESET>
> : 9<RESET>
> diff --git a/t/t3206/history.export b/t/t3206/history.export
> index b8ffff0940..7bb3814962 100644
> --- a/t/t3206/history.export
> +++ b/t/t3206/history.export
> @@ -22,8 +22,8 @@ data 51
> 19
> 20
>
> -reset refs/heads/removed
> -commit refs/heads/removed
> +reset refs/heads/renamed-file
> +commit refs/heads/renamed-file
Hmm. Is the `removed` ref no longer required by the 'removed a commit'
test case?
> mark :2
> author Thomas Rast <trast@inf.ethz.ch> 1374424921 +0200
> committer Thomas Rast <trast@inf.ethz.ch> 1374484724 +0200
> @@ -599,6 +599,82 @@ s/12/B/
> from :46
> M 100644 :28 file
>
> -reset refs/heads/removed
> -from :47
> +commit refs/heads/added-removed
> +mark :48
> +author Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1556574151 +0100
Neat ;-)
> +data 7
> +s/5/A/
> +from :2
> +M 100644 :3 file
> +
> +blob
> +mark :49
> +data 0
> +
> +commit refs/heads/added-removed
> +mark :50
> +author Thomas Rast <trast@inf.ethz.ch> 1374485024 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1556574177 +0100
> +data 18
> +s/4/A/ + new-file
> +from :48
> +M 100644 :5 file
> +M 100644 :49 new-file
> +
> +commit refs/heads/added-removed
> +mark :51
> +author Thomas Rast <trast@inf.ethz.ch> 1374485036 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1556574177 +0100
> +data 22
> +s/11/B/ + remove file
> +from :50
> +M 100644 :7 file
> +D new-file
> +
> +commit refs/heads/added-removed
> +mark :52
> +author Thomas Rast <trast@inf.ethz.ch> 1374485044 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1556574177 +0100
> +data 8
> +s/12/B/
> +from :51
> +M 100644 :9 file
> +
> +commit refs/heads/renamed-file
> +mark :53
> +author Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1556574309 +0100
> +data 7
> +s/5/A/
> +from :2
> +M 100644 :3 file
> +
> +commit refs/heads/renamed-file
> +mark :54
> +author Thomas Rast <trast@inf.ethz.ch> 1374485024 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1556574312 +0100
> +data 21
> +s/4/A/ + rename file
> +from :53
> +D file
> +M 100644 :5 renamed-file
> +
> +commit refs/heads/renamed-file
> +mark :55
> +author Thomas Rast <trast@inf.ethz.ch> 1374485036 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1556574319 +0100
> +data 8
> +s/11/B/
> +from :54
> +M 100644 :7 renamed-file
> +
> +commit refs/heads/renamed-file
> +mark :56
> +author Thomas Rast <trast@inf.ethz.ch> 1374485044 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1556574319 +0100
> +data 8
> +s/12/B/
> +from :55
> +M 100644 :9 renamed-file
I have to admit that I allowed myself not to study this script too
closely, trusting that the range-diff explains better what commit history
it creates than the fast-import script.
Thanks,
Dscho
>
> --
> 2.22.0.510.g264f2c817a
>
>
next prev parent reply other threads:[~2019-07-05 19:35 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20190414210933.20875-1-t.gummerer@gmail.com/>
2019-07-05 17:06 ` [PATCH v2 00/14] output improvements for git range-diff Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 01/14] apply: replace marc.info link with public-inbox Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 02/14] apply: only pass required data to skip_tree_prefix Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 03/14] apply: only pass required data to git_header_name Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 04/14] apply: only pass required data to check_header_line Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 05/14] apply: only pass required data to find_name_* Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 06/14] apply: only pass required data to gitdiff_* functions Thomas Gummerer
2019-07-05 18:51 ` Johannes Schindelin
2019-07-05 17:06 ` [PATCH v2 07/14] apply: make parse_git_header public Thomas Gummerer
2019-07-05 18:48 ` Johannes Schindelin
2019-07-05 17:06 ` [PATCH v2 08/14] range-diff: fix function parameter indentation Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 09/14] range-diff: split lines manually Thomas Gummerer
2019-07-05 19:05 ` Johannes Schindelin
2019-07-08 11:24 ` Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 10/14] range-diff: don't remove funcname from inner diff Thomas Gummerer
2019-07-05 19:09 ` Johannes Schindelin
2019-07-05 17:06 ` [PATCH v2 11/14] range-diff: suppress line count in outer diff Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 12/14] range-diff: add section header instead of diff header Thomas Gummerer
2019-07-05 19:35 ` Johannes Schindelin [this message]
2019-07-08 11:44 ` Thomas Gummerer
2019-07-08 13:12 ` Johannes Schindelin
2019-07-05 17:06 ` [PATCH v2 13/14] range-diff: add filename to inner diff Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 14/14] range-diff: add headers to the outer hunk header Thomas Gummerer
2019-07-05 19:48 ` [PATCH v2 00/14] output improvements for git range-diff Johannes Schindelin
2019-07-08 11:45 ` Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 " Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 01/14] apply: replace marc.info link with public-inbox Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 02/14] apply: only pass required data to skip_tree_prefix Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 03/14] apply: only pass required data to git_header_name Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 04/14] apply: only pass required data to check_header_line Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 05/14] apply: only pass required data to find_name_* Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 06/14] apply: only pass required data to gitdiff_* functions Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 07/14] apply: make parse_git_header public Thomas Gummerer
2019-07-09 19:39 ` Junio C Hamano
2019-07-09 21:23 ` Thomas Gummerer
2019-07-09 23:22 ` Junio C Hamano
2019-07-10 8:48 ` Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 08/14] range-diff: fix function parameter indentation Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 09/14] range-diff: split lines manually Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 10/14] range-diff: don't remove funcname from inner diff Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 11/14] range-diff: suppress line count in outer diff Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 12/14] range-diff: add section header instead of diff header Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 13/14] range-diff: add filename to inner diff Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 14/14] range-diff: add headers to the outer hunk header Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 00/14] output improvements for git range-diff Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 01/14] apply: replace marc.info link with public-inbox Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 02/14] apply: only pass required data to skip_tree_prefix Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 03/14] apply: only pass required data to git_header_name Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 04/14] apply: only pass required data to check_header_line Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 05/14] apply: only pass required data to find_name_* Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 06/14] apply: only pass required data to gitdiff_* functions Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 07/14] apply: make parse_git_diff_header public Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 08/14] range-diff: fix function parameter indentation Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 09/14] range-diff: split lines manually Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 10/14] range-diff: don't remove funcname from inner diff Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 11/14] range-diff: suppress line count in outer diff Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 12/14] range-diff: add section header instead of diff header Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 13/14] range-diff: add filename to inner diff Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 14/14] range-diff: add headers to the outer hunk header Thomas Gummerer
2019-07-11 22:09 ` [PATCH v4 00/14] output improvements for git range-diff Ramsay Jones
2019-07-12 10:44 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.QRO.7.76.6.1907052114480.44@tvgsbejvaqbjf.bet \
--to=johannes.schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=pclouds@gmail.com \
--cc=sunshine@sunshineco.com \
--cc=t.gummerer@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).