Stefan Sperling <stsp@stsp.name> wrote:
> On Mon, Mar 14, 2022 at 09:39:46PM +0100, Omar Polo wrote:
> > [...]
> > 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/

woops

> > 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/

fixed

> > + */
> > +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.

agreed.  I've added a patch_args struct since the add and delete
callbacks need the exact same information (function pointer and data.)
We can always split into two struct in the future if needed.

-----------------------------------------------
commit 41a42854417ad3ff76ae876d306a2ff3e545b95f
from: Omar Polo <op@omarpolo.com>
date: Mon Mar 14 21:14:54 2022 UTC
 
 introduce got_patch_progress_cb
 
 in got_patch I've used callbacks that are intended for other operations.
 This introduce a proper got_patch specific progress callback that is
 invoked after processing a patch.  It also drops the hackish printf in
 the modified case.
 
diff 463cdc6089041a9ec1747b58910c2f9be8bb6cd7 e66632530bcb4288db158ee8003246b04e1b85cb
blob - d595de198f4880c3458bb395b994e889acf68df3
blob + 541f9eac1d2cad4c888bcf3da0eb9a7a1632c750
--- got/got.c
+++ got/got.c
@@ -7186,6 +7186,17 @@ patch_from_stdin(int *patchfd)
 }
 
 static const struct got_error *
+patch_progress(void *arg, const char *old, const char *new, unsigned char mode)
+{
+	const char *path = new == NULL ? old : new;
+
+	while (*path == '/')
+		path++;
+	printf("%c  %s\n", mode, path);
+	return NULL;
+}
+
+static const struct got_error *
 cmd_patch(int argc, char *argv[])
 {
 	const struct got_error *error = NULL, *close_error = NULL;
@@ -7247,8 +7258,8 @@ cmd_patch(int argc, char *argv[])
 		err(1, "pledge");
 #endif
 
-	error = got_patch(patchfd, worktree, repo, nop, &print_remove_status,
-	    NULL, &add_progress, NULL, check_cancelled, NULL);
+	error = got_patch(patchfd, worktree, repo, nop, &patch_progress,
+	    NULL, check_cancelled, NULL);
 
 done:
 	if (repo) {
blob - 5f28ffc619f1b4e32ba37ff8e5361c636009ba74
blob + 391c0cf0a69178e759d03ba7c752062f5752dfc6
--- 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 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.
  *
@@ -22,5 +30,4 @@
  */
 const struct got_error *
 got_patch(int, struct got_worktree *, struct got_repository *, int,
-    got_worktree_delete_cb, void *, got_worktree_checkout_cb, void *,
-    got_cancel_cb, void *);
+    got_patch_progress_cb, void *, got_cancel_cb, void *);
blob - 6e5ec0f2143d39ff9eeca89060522c5155706a77
blob + a10f54062e0e80aa2a49665b6f36f51f610d9350
--- lib/patch.c
+++ lib/patch.c
@@ -66,12 +66,19 @@ struct got_patch_hunk {
 };
 
 struct got_patch {
+	got_patch_progress_cb cb;
+	void	*arg;
 	int	 nop;
 	char	*old;
 	char	*new;
 	STAILQ_HEAD(, got_patch_hunk) head;
 };
 
+struct patch_args {
+	got_patch_progress_cb progress_cb;
+	void	*progress_arg;
+};
+
 static const struct got_error *
 send_patch(struct imsgbuf *ibuf, int fd)
 {
@@ -565,13 +572,29 @@ 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 patch_args *pa = arg;
+
+	return pa->progress_cb(pa->progress_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;
+	struct patch_args pa;
 	int file_renamed = 0;
 	char *oldpath = NULL, *newpath = NULL;
 	char *tmppath = NULL, *template = NULL;
@@ -615,9 +638,12 @@ apply_patch(struct got_worktree *worktree, struct got_
 	if (p->nop)
 		goto done;
 
+	pa.progress_cb = p->cb;
+	pa.progress_arg = p->arg;
+
 	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, &pa, repo, 0, 0);
 		goto done;
 	}
 
@@ -628,15 +654,16 @@ apply_patch(struct got_worktree *worktree, struct got_
 
 	if (file_renamed) {
 		err = got_worktree_schedule_delete(worktree, &oldpaths,
-		    0, NULL, delete_cb, delete_arg, repo, 0, 0);
+		    0, NULL, patch_delete, &pa, repo, 0, 0);
 		if (err == NULL)
 			err = got_worktree_schedule_add(worktree, &newpaths,
-			    add_cb, add_arg, repo, 1);
+			    patch_add, p, repo, 1);
 	} else if (p->old == NULL)
 		err = got_worktree_schedule_add(worktree, &newpaths,
-		    add_cb, add_arg, repo, 1);
+		    patch_add, &pa, repo, 1);
 	else
-		printf("M  %s\n", oldpath); /* XXX */
+		err = pa.progress_cb(pa.progress_arg, oldpath, newpath,
+		    GOT_STATUS_MODIFY);
 
 done:
 	if (err != NULL && newpath != NULL && (file_renamed || p->old == NULL))
@@ -654,9 +681,8 @@ done:
 
 const struct got_error *
 got_patch(int fd, struct got_worktree *worktree, struct got_repository *repo,
-    int nop, 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)
+    int nop, got_patch_progress_cb progress_cb, void *progress_arg,
+    got_cancel_cb cancel_cb, void *cancel_arg)
 {
 	const struct got_error *err = NULL;
 	struct imsgbuf *ibuf;
@@ -704,9 +730,10 @@ got_patch(int fd, struct got_worktree *worktree, struc
 		if (err || done)
 			break;
 
+		p.cb = progress_cb;
+		p.arg = progress_arg;
 		p.nop = nop;
-		err = apply_patch(worktree, repo, &p, delete_cb, delete_arg,
-		    add_cb, add_arg, cancel_cb, cancel_arg);
+		err = apply_patch(worktree, repo, &p, cancel_cb, cancel_arg);
 		patch_free(&p);
 		if (err)
 			break;