git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
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
>
>

  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).