Re: [PATCH v3 02/11] diff: export diffstat interface

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Wed, 31 Jul 2019, Junio C Hamano wrote:

> "Daniel Ferreira via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > @@ -6273,12 +6257,7 @@ void diff_flush(struct diff_options *options)
> >  	    dirstat_by_line) {
> >  		struct diffstat_t diffstat;
> >
> > -		memset(&diffstat, 0, sizeof(struct diffstat_t));
> > -		for (i = 0; i < q->nr; i++) {
> > -			struct diff_filepair *p = q->queue[i];
> > -			if (check_pair_status(p))
> > -				diff_flush_stat(p, options, &diffstat);
> > -		}
> > +		compute_diffstat(options, &diffstat, q);
> >  		if (output_format & DIFF_FORMAT_NUMSTAT)
> >  			show_numstat(&diffstat, options);
> >  		if (output_format & DIFF_FORMAT_DIFFSTAT)
> > @@ -6611,6 +6590,20 @@ static int is_submodule_ignored(const char *path, struct diff_options *options)
> >  	return ignored;
> >  }
> >
> > +void compute_diffstat(struct diff_options *options,
> > +		      struct diffstat_t *diffstat,
> > +		      struct diff_queue_struct *q)
> > +{
> > +	int i;
> > +
> > +	memset(diffstat, 0, sizeof(struct diffstat_t));
> > +	for (i = 0; i < q->nr; i++) {
> > +		struct diff_filepair *p = q->queue[i];
> > +		if (check_pair_status(p))
> > +			diff_flush_stat(p, options, diffstat);
> > +	}
> > +}
>
> Hmm, (1) clearing diffstat struct to initialize, (2) looping over
> diff_queue to compute stat for each path, (3) using diffstat
> information and then (4) finally freeing the diffstat info is the
> bog-standard sequence of the user of this API.  Merging step (1) and
> (2) may probably be OK (iow, I do not think of a use pattern for
> future users where being able to do some custom things between steps
> (1) and (2) would be useful), which is this function is about.  (3)
> is what the user of this API would do, but shouldn't (4) be exported
> at the same time, if we are making (1+2) as an external API?

Good point.

It _also_ hints at the fact that we're not releasing the memory properly
after running the diffstat in the built-in `add -i`.

Will fix,
Dscho

>
> >  void diff_addremove(struct diff_options *options,
> >  		    int addremove, unsigned mode,
> >  		    const struct object_id *oid,
> > diff --git a/diff.h b/diff.h
> > index b680b377b2..34fc658946 100644
> > --- a/diff.h
> > +++ b/diff.h
> > @@ -244,6 +244,22 @@ void diff_emit_submodule_error(struct diff_options *o, const char *err);
> >  void diff_emit_submodule_pipethrough(struct diff_options *o,
> >  				     const char *line, int len);
> >
> > +struct diffstat_t {
> > +	int nr;
> > +	int alloc;
> > +	struct diffstat_file {
> > +		char *from_name;
> > +		char *name;
> > +		char *print_name;
> > +		const char *comments;
> > +		unsigned is_unmerged:1;
> > +		unsigned is_binary:1;
> > +		unsigned is_renamed:1;
> > +		unsigned is_interesting:1;
> > +		uintmax_t added, deleted;
> > +	} **files;
> > +};
> > +
> >  enum color_diff {
> >  	DIFF_RESET = 0,
> >  	DIFF_CONTEXT = 1,
> > @@ -333,6 +349,9 @@ void diff_change(struct diff_options *,
> >
> >  struct diff_filepair *diff_unmerge(struct diff_options *, const char *path);
> >
> > +void compute_diffstat(struct diff_options *options, struct diffstat_t *diffstat,
> > +		      struct diff_queue_struct *q);
> > +
> >  #define DIFF_SETUP_REVERSE      	1
> >  #define DIFF_SETUP_USE_SIZE_CACHE	4
>