On Mon, Mar 14, 2022 at 09:39:46PM +0100, Omar Polo wrote:
> As recently pointed out by stsp, got_patch abuses some callbacks.
> 
> The attached diff solves the situation by introducing a
> got_patch_process_cb callback that gets invoked after every patch.  This
> also remove the hackish printf("M") I added in the modified-only case.
> 
> I've got ready a follow-up diff to extend the callback to handle also
> the "patch applied with offset" case, but wanted to get a feedback on
> the direction first.
> 
> regress is still passing.
> 
> ok?
> 
> -----------------------------------------------
> commit 70e6ccc6e3da8f8faf26a3c0d0970e9ae07285f9 (pscb)
> from: Omar Polo <op@omarpolo.com>
> date: Mon Mar 14 19:17:27 2022 UTC
>  
>  introduce got_patch_progress_cb
>  
>  in got_patch I made use of callbacks that are intended for other got
>  operations.  This introduce a proper got_patch specific progress
>  callback that gets invoked when got_patch fisishes to process a patch.

s/fisishes/finishes/

> blob - 5f28ffc619f1b4e32ba37ff8e5361c636009ba74
> blob + 2c3744d0ad739fd94ee5702c1684aa55ca80e8e6
> --- include/got_patch.h
> +++ include/got_patch.h
> @@ -15,6 +15,14 @@
>   */
>  
>  /*
> + * A callback that gets invoked during the patch application.
> + *
> + * Receives the old and new path and the mode of the change (A, M or D.)

To keep the terminology consistent:

s/and the mode of the change/and a status code/

> + */
> +typedef const struct got_error *(*got_patch_progress_cb)(void *,
> +    const char *, const char *, unsigned char);
> +
> +/*
>   * Apply the (already opened) patch to the repository and register the
>   * status of the added and removed files.
>   *

> blob - 6e5ec0f2143d39ff9eeca89060522c5155706a77
> blob + e12ead90351f918d666094a55081a5fafa0ba42c
> --- lib/patch.c
> +++ lib/patch.c
> @@ -66,6 +66,8 @@ struct got_patch_hunk {
>  };
>  
>  struct got_patch {
> +	got_patch_progress_cb cb;
> +	void	*arg;
>  	int	 nop;
>  	char	*old;
>  	char	*new;
> @@ -565,10 +567,25 @@ check_file_status(struct got_patch *p, int file_rename
>  }
>  
>  static const struct got_error *
> +patch_delete(void *arg, unsigned char status, unsigned char staged_status,
> +    const char *path)
> +{
> +	struct got_patch *p = arg;

Again just a hint for style consistency. Feel free to ignore, I could
just change this myself later.

I would define a separate struct for callback-specific arguments.
Something like this:

struct patch_delete_args {
	got_patch_progress_cb progress_cb;
	void	*progress_arg;
};

And apply_patch() would receive progress_cb, progress_arg arguments,
and on its stack it would declare:

	struct patch_delete_args pda;

and later:

	pda.progress_cb = progress_cb;
	pda.progress_arg = progress_arg;
	err = got_worktree_schedule_delete(worktree, &oldpaths,
 	    0, NULL, patch_delete, &pda, repo, 0, 0);

I like it that way because it makes it easy to see which arguments a
callback is using. Whereas if you pass the entire 'struct got patch'
the callback has access to this entire struct and could have side
effects and whatnot.

> +
> +	return p->cb(p->arg, path, NULL, status);
> +}
> +
> +static const struct got_error *
> +patch_add(void *arg, unsigned char status, const char *path)
> +{
> +	struct got_patch *p = arg;
> +
> +	return p->cb(p->arg, NULL, path, status);
> +}
> +
> +static const struct got_error *
>  apply_patch(struct got_worktree *worktree, struct got_repository *repo,
> -    struct got_patch *p, got_worktree_delete_cb delete_cb, void *delete_arg,
> -    got_worktree_checkout_cb add_cb, void *add_arg, got_cancel_cb cancel_cb,
> -    void *cancel_arg)
> +    struct got_patch *p, got_cancel_cb cancel_cb, void *cancel_arg)
>  {
>  	const struct got_error *err = NULL;
>  	struct got_pathlist_head oldpaths, newpaths;
> @@ -617,7 +634,7 @@ apply_patch(struct got_worktree *worktree, struct got_
>  
>  	if (p->old != NULL && p->new == NULL) {
>  		err = got_worktree_schedule_delete(worktree, &oldpaths,
> -		    0, NULL, delete_cb, delete_arg, repo, 0, 0);
> +		    0, NULL, patch_delete, p, repo, 0, 0);
>  		goto done;
>  	}
>