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

Re: [PATCH v2 09/14] range-diff: split lines manually

From
Thomas Gummerer <t.gummerer@gmail.com>
Date
Jul 8, 2019, 11:24 UTC
Message-ID
<20190708112415.GB16825@hank.intra.tgummerer.com>
In-Reply-To
<nycvar.QRO.7.76.6.1907052052060.44@tvgsbejvaqbjf.bet>
On 07/05, Johannes Schindelin wrote:
Show 11 quoted lines
> Hi Thomas,
> 
> 
> On Fri, 5 Jul 2019, Thomas Gummerer wrote:
> 
> > Currently range-diff uses the 'strbuf_getline()' function for doing
> > its line by line processing.  In a future patch we want to do parts of
> > that parsing using the 'parse_git_header()' function, which does
> 
> If you like my suggestion in patch 7/14, this commit message needs to talk
> about the new name, too.

Thanks for the reminder here! I do indeed like the new name, but would probably have forgotten to change it in the commit message here.

Show 61 quoted lines
> > requires reading parts of the input from that function, which doesn't
> 
> s/requires/require/
> 
> > use strbufs.
> >
> > Switch range-diff to do our own line by line parsing, so we can re-use
> > the parse_git_header function later.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >
> > Longer term it might be better to have both range-diff and apply code
> > use strbufs.  However I didn't feel it's worth making that change for
> > this patch series.
> 
> Makes sense.
> 
> >  range-diff.c | 69 +++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 39 insertions(+), 30 deletions(-)
> >
> > diff --git a/range-diff.c b/range-diff.c
> > index 9242b8975f..916afa44c0 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -24,6 +24,17 @@ struct patch_util {
> >  	struct object_id oid;
> >  };
> >
> > +static unsigned long linelen(const char *buffer, unsigned long size)
> 
> Shouldn't this be `size_t`?
> 
> > +{
> > +	unsigned long len = 0;
> 
> Likewise.
> 
> > +	while (size--) {
> > +		len++;
> > +		if (*buffer++ == '\n')
> > +			break;
> > +	}
> > +	return len;
> 
> How about
> 
> 	const char *eol = memchr(buffer, '\n', size);
> 
> 	return !eol ? size : eol + 1 - buffer;
> 
> instead?
> 
> For an extra brownie point, you could even rename this function to
> `find_end_of_line()` and replace the LF by a NUL:
> 
> 	if (!eol)
> 		return size;
> 
> 	*eol = '\0';
> 	return eol + 1 - buffer;
I like this, thank you!
Show 14 quoted lines
> > +}
> > +
> >  /*
> >   * Reads the patches into a string list, with the `util` field being populated
> >   * as struct object_id (will need to be free()d).
> > @@ -31,10 +42,12 @@ struct patch_util {
> >  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 buf = STRBUF_INIT, file = STRBUF_INIT;
> 
> This puzzled me. I'd like to suggest s/file/contents/
Thanks, will change.
Show 22 quoted lines
> >  	struct patch_util *util = NULL;
> >  	int in_header = 1;
> > +	char *line;
> > +	int offset, len;
> > +	size_t size;
> >
> >  	argv_array_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
> >  			"--reverse", "--date-order", "--decorate=no",
> > @@ -54,17 +67,15 @@ static int read_patches(const char *range, struct string_list *list)
> >
> >  	if (start_command(&cp))
> >  		return error_errno(_("could not start `log`"));
> > -	in = fdopen(cp.out, "r");
> > -	if (!in) {
> > -		error_errno(_("could not read `log` output"));
> > -		finish_command(&cp);
> > -		return -1;
> > -	}
> > +	strbuf_read(&file, cp.out, 0);
> 
> Shouldn't we handle a negative return value here, erroring out with "could
> not read `log` output" as before?

Yeah, that was an oversight, we should definitely still handle errors here.

Show 13 quoted lines
> >
> > -	while (strbuf_getline(&line, in) != EOF) {
> > +	line = strbuf_detach(&file, &size);
> 
> I strongly suspect this to leak, given that `line` is subsequently
> advanced, and there is no backup copy.
> 
> Maybe
> 
> 	line = file.buf;
> 	size = file.len;
> 
> would make more sense here?
Hmm good point, that makes more sense indeed.
Show 19 quoted lines
> > +	for (offset = 0; size > 0; offset += len, size -= len, line += len) {
> >  		const char *p;
> >
> > -		if (skip_prefix(line.buf, "commit ", &p)) {
> > +		len = linelen(line, size);
> > +		line[len - 1] = '\0';
> > +		if (skip_prefix(line, "commit ", &p)) {
> >  			if (util) {
> >  				string_list_append(list, buf.buf)->util = util;
> >  				strbuf_reset(&buf);
> > @@ -75,8 +86,6 @@ static int read_patches(const char *range, struct string_list *list)
> >  				free(util);
> >  				string_list_clear(list, 1);
> >  				strbuf_release(&buf);
> > -				strbuf_release(&line);
> > -				fclose(in);
> 
> We should release the file contents in `file` (or `contents`, if you like
> my suggestions) here.

Yeah, I thought it was no longer necessary because of the 'strbuf_detach()' earlier, but that obviously leaks in a different way as you pointed out. Will release 'contents' here and below.

Show 84 quoted lines
> >  				finish_command(&cp);
> >  				return -1;
> >  			}
> > @@ -85,26 +94,28 @@ static int read_patches(const char *range, struct string_list *list)
> >  			continue;
> >  		}
> >
> > -		if (starts_with(line.buf, "diff --git")) {
> > +		if (starts_with(line, "diff --git")) {
> >  			in_header = 0;
> >  			strbuf_addch(&buf, '\n');
> >  			if (!util->diff_offset)
> >  				util->diff_offset = buf.len;
> >  			strbuf_addch(&buf, ' ');
> > -			strbuf_addbuf(&buf, &line);
> > +			strbuf_addstr(&buf, line);
> >  		} else if (in_header) {
> > -			if (starts_with(line.buf, "Author: ")) {
> > -				strbuf_addbuf(&buf, &line);
> > +			if (starts_with(line, "Author: ")) {
> > +				strbuf_addstr(&buf, line);
> >  				strbuf_addstr(&buf, "\n\n");
> > -			} else if (starts_with(line.buf, "    ")) {
> > -				strbuf_rtrim(&line);
> > -				strbuf_addbuf(&buf, &line);
> > +			} else if (starts_with(line, "    ")) {
> > +				p = line + len - 2;
> > +				while (isspace(*p) && p >= line)
> > +					p--;
> > +				strbuf_add(&buf, line, p - line + 1);
> >  				strbuf_addch(&buf, '\n');
> >  			}
> >  			continue;
> > -		} else if (starts_with(line.buf, "@@ "))
> > +		} else if (starts_with(line, "@@ "))
> >  			strbuf_addstr(&buf, "@@");
> > -		else if (!line.buf[0] || starts_with(line.buf, "index "))
> > +		else if (!line[0] || starts_with(line, "index "))
> >  			/*
> >  			 * A completely blank (not ' \n', which is context)
> >  			 * line is not valid in a diff.  We skip it
> > @@ -117,25 +128,23 @@ static int read_patches(const char *range, struct string_list *list)
> >  			 * we are not interested.
> >  			 */
> >  			continue;
> > -		else if (line.buf[0] == '>') {
> > +		else if (line[0] == '>') {
> >  			strbuf_addch(&buf, '+');
> > -			strbuf_add(&buf, line.buf + 1, line.len - 1);
> > -		} else if (line.buf[0] == '<') {
> > +			strbuf_addstr(&buf, line + 1);
> > +		} else if (line[0] == '<') {
> >  			strbuf_addch(&buf, '-');
> > -			strbuf_add(&buf, line.buf + 1, line.len - 1);
> > -		} else if (line.buf[0] == '#') {
> > +			strbuf_addstr(&buf, line + 1);
> > +		} else if (line[0] == '#') {
> >  			strbuf_addch(&buf, ' ');
> > -			strbuf_add(&buf, line.buf + 1, line.len - 1);
> > +			strbuf_addstr(&buf, line + 1);
> >  		} else {
> >  			strbuf_addch(&buf, ' ');
> > -			strbuf_addbuf(&buf, &line);
> > +			strbuf_addstr(&buf, line);
> >  		}
> >
> >  		strbuf_addch(&buf, '\n');
> >  		util->diffsize++;
> >  	}
> > -	fclose(in);
> > -	strbuf_release(&line);
> 
> We should release the file contents we previously read via `strbuf_read()` here.
> 
> Ciao,
> Dscho
> 
> >
> >  	if (util)
> >  		string_list_append(list, buf.buf)->util = util;
> > --
> > 2.22.0.510.g264f2c817a
> >
> >
Previous: Johannes SchindelinNext: Thomas Gummerer
Message 14 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.