diff mbox series

[v3,02/11] diff: export diffstat interface

Message ID c7a377890d84849ea2f63099cfc081420a4de15d.1563289115.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series git add -i: add a rudimentary version in C (supporting only status and help so far) | expand

Commit Message

Linus Arver via GitGitGadget July 16, 2019, 2:58 p.m. UTC 
From: Daniel Ferreira <bnmvco@gmail.com>

Make the diffstat interface (namely, the diffstat_t struct and
compute_diffstat) no longer be internal to diff.c and allow it to be used
by other parts of git.

This is helpful for code that may want to easily extract information
from files using the diff machinery, while flushing it differently from
how the show_* functions used by diff_flush() do it. One example is the
builtin implementation of git-add--interactive's status.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff.c | 37 +++++++++++++++----------------------
 diff.h | 19 +++++++++++++++++++
 2 files changed, 34 insertions(+), 22 deletions(-)

Comments

Junio C Hamano July 31, 2019, 5:59 p.m. UTC | #1 
"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?

>  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
Johannes Schindelin Aug. 27, 2019, 9:22 a.m. UTC | #2 
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
>
diff mbox series

Patch

diff --git a/diff.c b/diff.c
index 1ee04e321b..03d6f00d97 100644
--- a/diff.c
+++ b/diff.c
@@ -2489,22 +2489,6 @@  static void pprint_rename(struct strbuf *name, const char *a, const char *b)
 	}
 }
 
-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;
-};
-
 static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat,
 					  const char *name_a,
 					  const char *name_b)
@@ -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);
+	}
+}
+
 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