On Thu, Mar 17, 2022 at 03:43:48PM +0100, Omar Polo wrote:
> Stefan Sperling <stsp@stsp.name> wrote:
> > >  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.
> 
> I tried to allocate and copy the hunks as per original suggestion and to
> avoid passing a lot of arguments to the callback.  i prefer to avoid the
> extra allocation too and in the updated diff below I'm doing as you
> suggest (that is passing the hunk info as parameters to the function and
> calling it in a loop)
> 
> > > @@ -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
> 
> wops, i forget about it.  Fixed.
> 
> (there are some other instances of `foo+1' in the code, i'll get rid of
> them in a follow-up commit)
> 
> > >  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.
> 
> agreed!
> 

Thanks, I like it much better!

ok stsp@

(small nit: rename 'mode' to 'status' in patch_progress)