git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 12/14] range-diff: add section header instead of diff header

From
Thomas Gummerer <t.gummerer@gmail.com>
Date
Jul 8, 2019, 11:44 UTC
Message-ID
<20190708114422.GC16825@hank.intra.tgummerer.com>
In-Reply-To
<nycvar.QRO.7.76.6.1907052114480.44@tvgsbejvaqbjf.bet>
On 07/05, Johannes Schindelin wrote:
Show 25 quoted lines
> >  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!
Show 13 quoted lines
> > +			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.

Show 6 quoted lines
> > +			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.
Show 18 quoted lines
> > @@ -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.

Show 27 quoted lines
> >  			 */
> >  			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.

Show 16 quoted lines
> > @@ -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.

Show 18 quoted lines
> >  	:<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 :)

Show 103 quoted lines
> >  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
> >
> >
Previous: Johannes SchindelinNext: Johannes Schindelin
Message 20 of 61 in “output improvements for git range-diff”
  1. 00/14 output improvements for git range-diffThomas Gummerer, Jul 5, 2019
  2. 01/14 apply: replace marc.info link with public-inboxThomas Gummerer, Jul 5, 2019
  3. 03/14 apply: only pass required data to git_header_nameThomas Gummerer, Jul 5, 2019
  4. 02/14 apply: only pass required data to skip_tree_prefixThomas Gummerer, Jul 5, 2019
  5. 04/14 apply: only pass required data to check_header_lineThomas Gummerer, Jul 5, 2019
  6. 05/14 apply: only pass required data to find_name_*Thomas Gummerer, Jul 5, 2019
  7. 07/14 apply: make parse_git_header publicThomas Gummerer, Jul 5, 2019
  8. Johannes SchindelinJul 5, 2019
  9. 06/14 apply: only pass required data to gitdiff_* functionsThomas Gummerer, Jul 5, 2019
  10. Johannes SchindelinJul 5, 2019
  11. 08/14 range-diff: fix function parameter indentationThomas Gummerer, Jul 5, 2019
  12. 09/14 range-diff: split lines manuallyThomas Gummerer, Jul 5, 2019
  13. Johannes SchindelinJul 5, 2019
  14. Thomas GummererJul 8, 2019
  15. 10/14 range-diff: don't remove funcname from inner diffThomas Gummerer, Jul 5, 2019
  16. Johannes SchindelinJul 5, 2019
  17. 11/14 range-diff: suppress line count in outer diffThomas Gummerer, Jul 5, 2019
  18. 12/14 range-diff: add section header instead of diff headerThomas Gummerer, Jul 5, 2019
  19. Johannes SchindelinJul 5, 2019
  20. Thomas GummererJul 8, 2019
  21. Johannes SchindelinJul 8, 2019
  22. 13/14 range-diff: add filename to inner diffThomas Gummerer, Jul 5, 2019
  23. 14/14 range-diff: add headers to the outer hunk headerThomas Gummerer, Jul 5, 2019
  24. Johannes SchindelinJul 5, 2019
  25. Thomas GummererJul 8, 2019
  26. 00/14 output improvements for git range-diffThomas Gummerer, Jul 8, 2019
  27. 01/14 apply: replace marc.info link with public-inboxThomas Gummerer, Jul 8, 2019
  28. 02/14 apply: only pass required data to skip_tree_prefixThomas Gummerer, Jul 8, 2019
  29. 03/14 apply: only pass required data to git_header_nameThomas Gummerer, Jul 8, 2019
  30. 04/14 apply: only pass required data to check_header_lineThomas Gummerer, Jul 8, 2019
  31. 05/14 apply: only pass required data to find_name_*Thomas Gummerer, Jul 8, 2019
  32. 06/14 apply: only pass required data to gitdiff_* functionsThomas Gummerer, Jul 8, 2019
  33. 07/14 apply: make parse_git_header publicThomas Gummerer, Jul 8, 2019
  34. Junio C HamanoJul 9, 2019
  35. Thomas GummererJul 9, 2019
  36. Junio C HamanoJul 9, 2019
  37. Thomas GummererJul 10, 2019
  38. 09/14 range-diff: split lines manuallyThomas Gummerer, Jul 8, 2019
  39. 08/14 range-diff: fix function parameter indentationThomas Gummerer, Jul 8, 2019
  40. 10/14 range-diff: don't remove funcname from inner diffThomas Gummerer, Jul 8, 2019
  41. 11/14 range-diff: suppress line count in outer diffThomas Gummerer, Jul 8, 2019
  42. 12/14 range-diff: add section header instead of diff headerThomas Gummerer, Jul 8, 2019
  43. 14/14 range-diff: add headers to the outer hunk headerThomas Gummerer, Jul 8, 2019
  44. 13/14 range-diff: add filename to inner diffThomas Gummerer, Jul 8, 2019
  45. 00/14 output improvements for git range-diffThomas Gummerer, Jul 11, 2019
  46. 01/14 apply: replace marc.info link with public-inboxThomas Gummerer, Jul 11, 2019
  47. 02/14 apply: only pass required data to skip_tree_prefixThomas Gummerer, Jul 11, 2019
  48. 04/14 apply: only pass required data to check_header_lineThomas Gummerer, Jul 11, 2019
  49. 03/14 apply: only pass required data to git_header_nameThomas Gummerer, Jul 11, 2019
  50. 05/14 apply: only pass required data to find_name_*Thomas Gummerer, Jul 11, 2019
  51. 06/14 apply: only pass required data to gitdiff_* functionsThomas Gummerer, Jul 11, 2019
  52. 08/14 range-diff: fix function parameter indentationThomas Gummerer, Jul 11, 2019
  53. 07/14 apply: make parse_git_diff_header publicThomas Gummerer, Jul 11, 2019
  54. 09/14 range-diff: split lines manuallyThomas Gummerer, Jul 11, 2019
  55. 10/14 range-diff: don't remove funcname from inner diffThomas Gummerer, Jul 11, 2019
  56. 12/14 range-diff: add section header instead of diff headerThomas Gummerer, Jul 11, 2019
  57. 11/14 range-diff: suppress line count in outer diffThomas Gummerer, Jul 11, 2019
  58. 13/14 range-diff: add filename to inner diffThomas Gummerer, Jul 11, 2019
  59. 14/14 range-diff: add headers to the outer hunk headerThomas Gummerer, Jul 11, 2019
  60. Ramsay JonesJul 11, 2019
  61. Johannes SchindelinJul 12, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.