From: Stefan Sperling <stsp@stsp.name>
Subject: Re: tog diff view search
To: Tracey Emery <tracey@traceyemery.net>
Cc: gameoftrees@openbsd.org
Date: Sat, 1 Feb 2020 18:41:43 +0100

On Sat, Feb 01, 2020 at 09:54:07AM -0700, Tracey Emery wrote:
> This would be part 2 to enable search functionality. Thoughts?
> 
> +const struct got_error *
> +diff_dump_filestream_info(size_t *filesize, int *nlines,

As mentioned in my other response, I would prefer a more generic
name which doesn't tie this functionality to diff.

> +	fseek(infile, 0, SEEK_END);
> +	len = ftell(infile) + 1;
> +	fseek(infile, 0, SEEK_SET);

All the above calls are missing return value checks, why?
Let's please not get into a habit of putting code without such checks in.
Every time we do that, we'll have to go back and fix it.

As an aside, I wish there was an easier way of getting the file size from
an open FILE *. With a file descriptor we could use fstat(2), but the diff
code makes it hard for us to use a file desciptor instead of FILE *.
I might try to revisit that later.

> +
> +	if (len == 0)
> +		return NULL;
> +	if ((buf = calloc(len, sizeof(char *))) == NULL)
> +		return got_error_from_errno("calloc");
> +
> +	fread(buf, 1, len, infile);

Missing return value check.

> +
> +	i = 0;
> +	if (line_offsets && nlines) {
> +		if (*line_offsets == NULL) {
> +			/* Have some data but perhaps no '\n'. */
> +			noffsets = 1;
> +			*nlines = 1;
> +			*line_offsets = calloc(1, sizeof(**line_offsets));
> +			if (*line_offsets == NULL)
> +				return got_error_from_errno("malloc");
> +				/* Skip forward over end of first line. */
> +			while (i < len) {
> +				if (buf[i] == '\n')
> +					break;
> +				i++;
> +			}
> +		}
> +		/* Scan '\n' offsets in remaining chunk of data. */
> +		while (i < len) {
> +			if (buf[i] != '\n') {
> +				i++;
> +				continue;
> +			}
> +			(*nlines)++;
> +			if (noffsets < *nlines) {
> +				off_t *o = recallocarray(*line_offsets,
> +				    noffsets, *nlines,
> +				    sizeof(**line_offsets));
> +				if (o == NULL) {
> +					free(*line_offsets);
> +					*line_offsets = NULL;
> +					return got_error_from_errno(
> +					    "recallocarray");
> +				}
> +				*line_offsets = o;
> +				noffsets = *nlines;
> +			}
> +			off = i + 1;
> +			(*line_offsets)[*nlines - 1] = off;
> +			i++;
> +		}
> +	}
> +
> +	if (fflush(infile) != 0)
> +		return got_error_from_errno("fflush");
> +	rewind(infile);
> +
> +	if (filesize)
> +		*filesize = len;
> +
> +	return NULL;
> +}
> +
>  static const struct got_error *
>  create_diff(struct tog_diff_view_state *s)
>  {
>  	const struct got_error *err = NULL;
> -	FILE *f = NULL;
>  	int obj_type;
>  
> -	f = got_opentemp();
> -	if (f == NULL) {

What if s->f is not NULL here? Can that happen?
If so we should close the old file before opening a new one...

> +	s->f = got_opentemp();
> +	if (s->f == NULL) {
>  		err = got_error_from_errno("got_opentemp");
>  		goto done;
>  	}
> -	if (s->f && fclose(s->f) != 0) {

... like the old code did here.

> -		err = got_error_from_errno("fclose");
> -		goto done;
> -	}
> -	s->f = f;
>  
>  	if (s->id1)
>  		err = got_object_get_type(&obj_type, s->repo, s->id1);
> @@ -2882,11 +2963,11 @@ create_diff(struct tog_diff_view_state *s)
>  	switch (obj_type) {
>  	case GOT_OBJ_TYPE_BLOB:
>  		err = got_diff_objects_as_blobs(s->id1, s->id2, NULL, NULL,
> -		    s->diff_context, 0, s->repo, f);
> +		    s->diff_context, 0, s->repo, s->f);
>  		break;
>  	case GOT_OBJ_TYPE_TREE:
>  		err = got_diff_objects_as_trees(s->id1, s->id2, "", "",
> -		    s->diff_context, 0, s->repo, f);
> +		    s->diff_context, 0, s->repo, s->f);
>  		break;
>  	case GOT_OBJ_TYPE_COMMIT: {
>  		const struct got_object_id_queue *parent_ids;
> @@ -2898,13 +2979,13 @@ create_diff(struct tog_diff_view_state *s)
>  			break;
>  		/* Show commit info if we're diffing to a parent/root commit. */
>  		if (s->id1 == NULL)
> -			write_commit_info(s->id2, s->refs, s->repo, f);
> +			write_commit_info(s->id2, s->refs, s->repo, s->f);

There's a missing error check here, too, in the existing code.
This would be a good opportinity to add one :-)

>  		else {
>  			parent_ids = got_object_commit_get_parent_ids(commit2);
>  			SIMPLEQ_FOREACH(pid, parent_ids, entry) {
>  				if (got_object_id_cmp(s->id1, pid->id) == 0) {
>  					write_commit_info(s->id2, s->refs,

Same.

> -					    s->repo, f);
> +					    s->repo, s->f);
>  					break;

The rest looks good to me, thanks!