From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got_patch_progress_cb: handle offsets and errors too
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Thu, 17 Mar 2022 14:05:18 +0100

On Thu, Mar 17, 2022 at 12:11:06PM +0100, Omar Polo wrote:
> > You do not have to omit the ,1 cases, I would just print them.
> > It does not hurt to write ,1 explicitly.
> > I doubt everyone will know that ,1 is implied if omitted.
> 
> (I for one didn't know ",1" is implied before taking a look at
> usr.bin/patch.)
> 
> the rationale for hiding the ",1" anyway was to ease the grep-ability;
> i'm expecting most (all?) programs that produce diffs to hide the ,1.

Hmm, fair enough. Maybe it makes sense to match the output of diff(1).
We can reconsider this later. 

I have a few small nits.
But I would be OK with this going in as it is.  We can refine later.

>  static const struct got_error *
> -patch_progress(void *arg, const char *old, const char *new, unsigned char mode)
> +patch_progress(void *arg, const char *old, const char *new,
> +    unsigned char mode, struct got_hunk *hunks, size_t n,
> +    const struct got_error *error)
>  {
>  	const char *path = new == NULL ? old : new;
> +	size_t i;
>  
>  	while (*path == '/')
>  		path++;
>  	printf("%c  %s\n", mode, path);
> +
> +	if (error != NULL)
> +		fprintf(stderr, "%s: %s\n", getprogname(), error->msg);
> +
> +	for (i = 0; i < n; ++i) {
> +		printf("@@ -%ld,%ld +%ld,%ld @@ ", hunks[i].old_from,
> +		    hunks[i].old_lines, hunks[i].new_from, hunks[i].new_lines);
> +		if (hunks[i].error != NULL)
> +			printf("%s\n", hunks[i].error->msg);
> +		else
> +			printf("applied with offset %ld\n", hunks[i].offset);
> +	}

If the progress() function of lib/patch.c was looping over hunks and
called the patch_progress() with information about each hunk separately,
you could save yourself the trouble of allocating a temporary array.
This patch_progress() function could simply check which arguments
are valid (non-NULL, non-zero), and print them if so.

> @@ -462,6 +468,9 @@ patch_file(struct got_patch *p, const char *path, FILE
>  		if (err != NULL)
>  			goto done;
>  
> +		if (lineno+1 != h->old_from)
> +			h->offset = lineno+1 - h->old_from;

Spaces around operators with two operands, please: lineno + 1

>  static const struct got_error *
> +progress(struct patch_args *pa, const char *old, const char *new,

I think report_progress() would be a better name for this function.

> +    unsigned char status, const struct got_error *orig_error)
> +{
> +	const struct got_error *err = NULL;
> +	struct got_patch_hunk *h;
> +	struct got_hunk *hunks = NULL;
> +	size_t n = 0;
> +
> +	STAILQ_FOREACH(h, pa->head, entries)
> +		if (h->offset != 0 || h->err != NULL)
> +			n++;

I would use braces around the above FOREACH loop.

However, my suggestion further above would make this above loop unnecessary.
You would do something like this here:

	STAILQ_FOREACH(h, pa->head, entries) {
		err = pa->progress_cb(pa->progress_arg, old, new, status,
		    h->old_from, h->old_lines, h->new_from, h->new_lines,
		    h->offset, orig_error);
	}