On Wed, Mar 16, 2022 at 07:30:12PM +0100, Omar Polo wrote:
> blob - 6e4f19aa3e9703b1e7a3b57f3d76fe41a2b801ac
> blob + 453af5ad8978ba78acf573f3beb5d5f0d2092688
> --- got/got.c
> +++ got/got.c
> @@ -7186,13 +7186,32 @@ patch_from_stdin(int *patchfd)
>  }
>  
>  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, const struct got_patch_hunk_head *head)
>  {
> +	struct got_patch_hunk *h;
>  	const char *path = new == NULL ? old : new;
>  
>  	while (*path == '/')
>  		path++;
>  	printf("%c  %s\n", mode, path);
> +
> +	STAILQ_FOREACH(h, head, entries) {
> +		if (h->offset != 0 || h->err != NULL) {
> +			printf("@@ -%ld", h->old_from);
> +			if (h->old_lines != 1)
> +				printf(",%ld", h->old_lines);
> +			printf(" +%ld", h->new_from);
> +			if (h->new_lines != 1)
> +				printf(",%ld", h->new_lines);
> +			printf(" @@ ");
> +		}
> +		if (h->offset != 0)
> +			printf("applied with offset %ld\n", h->offset);
> +		if (h->err != NULL)
> +			printf("%s\n", h->err->msg);

Instead of running so many printf calls, can we build a string and
do just one printf?

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.

> +	}
> +
>  	return NULL;
>  }
>  
> blob - 391c0cf0a69178e759d03ba7c752062f5752dfc6
> blob + 5b5f8d153e01e5595219bcda45f025ba7289bbbb
> --- include/got_patch.h
> +++ include/got_patch.h
> @@ -14,13 +14,28 @@
>   * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>   */
>  
> +STAILQ_HEAD(got_patch_hunk_head, got_patch_hunk);
> +struct got_patch_hunk {
> +	STAILQ_ENTRY(got_patch_hunk) entries;
> +	const struct got_error *err;
> +	long	offset;
> +	long	old_from;
> +	long	old_lines;
> +	long	new_from;
> +	long	new_lines;
> +	size_t	len;
> +	size_t	cap;
> +	char	**lines;
> +};

If a callback messes with the STAILQ part of the struct, the code
in lib/patch.c could crash.
Are you sure you want to send this internal struct to the progress callback,
instead of a smaller struct that has copies of the required fields?

> +
>  /*
>   * A callback that gets invoked during the patch application.
>   *
> - * Receives the old and new path and a status code.
> + * Receives the old and new path, a status code and the hunks.
>   */
>  typedef const struct got_error *(*got_patch_progress_cb)(void *,
> -    const char *, const char *, unsigned char);
> +    const char *, const char *, unsigned char,
> +    const struct got_patch_hunk_head *);
>  
>  /*
>   * Apply the (already opened) patch to the repository and register the
> blob - 58d4125553be6fd56d2408805b9c001d9c08c698
> blob + 9eab8fa2fd1714e2fa85193a6843cc5480466f4d
> --- lib/patch.c
> +++ lib/patch.c
> @@ -54,26 +54,16 @@
>  
>  #define MIN(a, b) ((a) < (b) ? (a) : (b))
>  
> -struct got_patch_hunk {
> -	STAILQ_ENTRY(got_patch_hunk) entries;
> -	long	old_from;
> -	long	old_lines;
> -	long	new_from;
> -	long	new_lines;
> -	size_t	len;
> -	size_t	cap;
> -	char	**lines;
> -};
> -
>  struct got_patch {
>  	char	*old;
>  	char	*new;
> -	STAILQ_HEAD(, got_patch_hunk) head;
> +	struct got_patch_hunk_head head;
>  };
>  
>  struct patch_args {
>  	got_patch_progress_cb progress_cb;
>  	void	*progress_arg;
> +	struct got_patch_hunk_head *head;
>  };
>  
>  static const struct got_error *
> @@ -344,11 +334,8 @@ test_hunk(FILE *orig, struct got_patch_hunk *h)
>  		case '-':
>  			linelen = getline(&line, &linesize, orig);
>  			if (linelen == -1) {
> -				if (ferror(orig))
> -					err = got_error_from_errno("getline");

Possible errors from getline() are EINVAL and EOVERFLOW, and various
others including ENOMEM.
Is it really fine to hide any such error behind GOT_ERR_PATCH_DONT_APPLY?

> -				else
> -					err = got_error(
> -					    GOT_ERR_PATCH_DONT_APPLY);
> +				err = got_ferror(orig,
> +				    GOT_ERR_PATCH_DONT_APPLY);
>  				goto done;
>  			}
>  			if (strcmp(h->lines[i]+1, line)) {