git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Thomas Gummerer <t.gummerer@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
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: Mon, 8 Jul 2019 12:44:22 +0100	[thread overview]
Message-ID: <20190708114422.GC16825@hank.intra.tgummerer.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1907052114480.44@tvgsbejvaqbjf.bet>

On 07/05, Johannes Schindelin wrote:
> >  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.

Thanks!

> > +			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?

Yeah, it doesn't deal well with them.  We might be able to change
that, but I haven't looked into that tbh.

> > +			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`?

Will do.

> > @@ -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?

Not really, but we no longer need to ignore them explicitly here.  The
'index' lines are just another part of the diff header, which we parse
using 'parse_git_diff_header()' now, and then replace completely.

Previously we singled that line from the git diff headers out, because
they would result in a particularly noisy range diff, as the blob
hashes would change in a lot of cases, but are not really
interesting.  We could have ignored things such as the "similarity
index" as well, as for example a 1% change in the similarity wouldn't
matter that much, but I assume we just didn't because that's much less
likely to change between different versions of a patch series.

> >  			 */
> >  			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...

No, I can't think of a good way.  I'm also not sure it would be right
to remove it.  In this case it means that in the previous version this
was only called 'file', while in the new version it was renamend in
this patch to 'renamed-file', so it does give some useful information.

Not sure how else we could represent that.  If we just had a 
'## file => renamed-file ##' section header, I'd expect that 'file'
has been renamed to 'renamed-file' in both versions.

> > @@ -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?

Sure.  The actual change (in the range-diff) here is that "Also a
silly comment here!" was added to the commit message.  The diff header
is context lines after that.

We now replace the diff header with the new "section header", which is
only a single line, so we get a couple of additional lines of the
context of the subsequent inner diff.  


> >  	:<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?

It is, and it still exists.  I'm not entirely familar with the format
for fast-export/fast-import scripts.  What I did was just fast-import
the existing script, create the new refs that were required for the
tests and then fast-export'ed it again.

So not sure exactly why this changed, but the 'removed' ref still
exists :)

> >  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-08 11:44 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
2019-07-08 11:44       ` Thomas Gummerer [this message]
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=20190708114422.GC16825@hank.intra.tgummerer.com \
    --to=t.gummerer@gmail.com \
    --cc=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 \
    /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).