[3/9] diff: avoid generating unused hunk header lines
diff mbox series

Message ID 20181102063606.GC31216@sigill.intra.peff.net
State New
Headers show
Series
  • [1/9] xdiff: provide a separate emit callback for hunks
Related show

Commit Message

Jeff King Nov. 2, 2018, 6:36 a.m. UTC
Some callers of xdi_diff_outf() do not look at the generated hunk header
lines at all. By plugging in a no-op hunk callback, this tells xdiff not
to even bother formatting them.

This patch introduces a stock no-op callback and uses it with a few
callers whose line callbacks explicitly ignore hunk headers (because
they look only for +/- lines).

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c             | 4 ++--
 diffcore-pickaxe.c | 3 ++-
 xdiff-interface.c  | 6 ++++++
 xdiff-interface.h  | 6 ++++++
 4 files changed, 16 insertions(+), 3 deletions(-)

Comments

Stefan Beller Nov. 2, 2018, 7:50 p.m. UTC | #1
> +/*
> + * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL
> + * one just sends the hunk line to the line_fn callback).
> + */
> +void discard_hunk_line(void *, long, long, long, long, const char *, long);

Recently we had the discussion on style and naming things.
On the one hand I don't know what these 4 different longs do,
so I'd wish for some descriptive variable names in here.
On the other hand the docs explain clearly why I don't need
to care (a no-op ignores all of the parameters, no need
to take care of their order)

So to revive that discussion, I would strongly prefer
to have *some* names there, for the sake of a
simply described coding style without many exceptions
(especially those exceptions that rely on judgement).

Today I read [1], which describes the analog in the
mechanical world: To evolve and have more impact
you need tighter requirements on some parts. And
I would roughly translate that to our use case as
not having to worry about style (it's ironic I even type
out this email... if we could just run clang format or
some other tightly controlling formatter/linter, I'd be
much happier as our focus should be elsewhere,
such as UX or performance).

Apart from that, I read the whole series, and found
it a pleasant read.

Thanks,
Stefan

[1] https://www.nybooks.com/articles/2018/10/25/precision-accuracy-perfectionism/
Jeff King Nov. 2, 2018, 8:15 p.m. UTC | #2
On Fri, Nov 02, 2018 at 12:50:05PM -0700, Stefan Beller wrote:

> > +/*
> > + * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL
> > + * one just sends the hunk line to the line_fn callback).
> > + */
> > +void discard_hunk_line(void *, long, long, long, long, const char *, long);
> 
> Recently we had the discussion on style and naming things.
> On the one hand I don't know what these 4 different longs do,
> so I'd wish for some descriptive variable names in here.
> On the other hand the docs explain clearly why I don't need
> to care (a no-op ignores all of the parameters, no need
> to take care of their order)

Right, I actually did have the same thought while writing it. And ended
up following that line of reasoning (since it's just a placeholder, it
doesn't matter).  But I'm not opposed to putting in the names.

> So to revive that discussion, I would strongly prefer
> to have *some* names there, for the sake of a
> simply described coding style without many exceptions
> (especially those exceptions that rely on judgement).

Fair enough.

Squashable patch is below; it goes on 34c829621e (diff: avoid generating
unused hunk header lines, 2018-11-02).

Junio, let me know if you'd prefer a re-send of the series, but it
doesn't look necessary otherwise (so far).

> Apart from that, I read the whole series, and found
> it a pleasant read.

Thanks!

diff --git a/xdiff-interface.h b/xdiff-interface.h
index 7b0ccbdd1d..8af245eed9 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -39,7 +39,9 @@ extern int git_xmerge_style;
  * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL
  * one just sends the hunk line to the line_fn callback).
  */
-void discard_hunk_line(void *, long, long, long, long, const char *, long);
+void discard_hunk_line(void *priv,
+		       long ob, long on, long nb, long nn,
+		       const char *func, long funclen);
 
 /*
  * Compare the strings l1 with l2 which are of size s1 and s2 respectively.

-Peff
Junio C Hamano Nov. 3, 2018, 12:32 a.m. UTC | #3
Jeff King <peff@peff.net> writes:

>> to have *some* names there, for the sake of a
>> simply described coding style without many exceptions
>> (especially those exceptions that rely on judgement).
>
> Fair enough.

For the record, there aren't "many" exceptions to the rule that was
suggested earlier: if you refer to parameters by name in the comment
to explain the interface, name them and use these names [*1*].

> Squashable patch is below; it goes on 34c829621e (diff: avoid generating
> unused hunk header lines, 2018-11-02).
>
> Junio, let me know if you'd prefer a re-send of the series, but it
> doesn't look necessary otherwise (so far).

Giving them proper names would serve as a readily usable sample for
those who write new hunk-line callbacks that do not ignore them, so
what you wrote is good.  Let me squash it in.

Thanks.  If this were to add bunch of "unused$n" names to the
parameters to this no-op function, only to "have *some* names
there", I would have said that it is not even worth the time to
discuss it, though.

> diff --git a/xdiff-interface.h b/xdiff-interface.h
> index 7b0ccbdd1d..8af245eed9 100644
> --- a/xdiff-interface.h
> +++ b/xdiff-interface.h
> @@ -39,7 +39,9 @@ extern int git_xmerge_style;
>   * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL
>   * one just sends the hunk line to the line_fn callback).
>   */
> -void discard_hunk_line(void *, long, long, long, long, const char *, long);
> +void discard_hunk_line(void *priv,
> +		       long ob, long on, long nb, long nn,
> +		       const char *func, long funclen);
>  
>  /*
>   * Compare the strings l1 with l2 which are of size s1 and s2 respectively.
>
> -Peff

[Footnote]

*1* You may say that the "if you refer to parameters by name" part
relies on judgment, but I think it is a good thing---it makes poor
judgment stand out.

When describing a function that takes two parameters of the same
type, for example, you could explain the interface as "take two ints
and return true if first int is smaller than the second int" in
order to leave the parameters unnamed, but if you gave such a
description, it is so obvious that you are _avoiding_ names.  Not
using names when you do not have to is good, but nobody with half an
intelligence would think it is good to give a bad description just
to avoid using names.

Patch
diff mbox series

diff --git a/diff.c b/diff.c
index 07be5879e5..d84356e007 100644
--- a/diff.c
+++ b/diff.c
@@ -3637,8 +3637,8 @@  static void builtin_diffstat(const char *name_a, const char *name_b,
 		xpp.anchors_nr = o->anchors_nr;
 		xecfg.ctxlen = o->context;
 		xecfg.interhunkctxlen = o->interhunkcontext;
-		if (xdi_diff_outf(&mf1, &mf2, NULL, diffstat_consume,
-				  diffstat, &xpp, &xecfg))
+		if (xdi_diff_outf(&mf1, &mf2, discard_hunk_line,
+				  diffstat_consume, diffstat, &xpp, &xecfg))
 			die("unable to generate diffstat for %s", one->path);
 	}
 
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 58df35670a..69fc55ea1e 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -62,7 +62,8 @@  static int diff_grep(mmfile_t *one, mmfile_t *two,
 	ecbdata.hit = 0;
 	xecfg.ctxlen = o->context;
 	xecfg.interhunkctxlen = o->interhunkcontext;
-	if (xdi_diff_outf(one, two, NULL, diffgrep_consume, &ecbdata, &xpp, &xecfg))
+	if (xdi_diff_outf(one, two, discard_hunk_line, diffgrep_consume,
+			  &ecbdata, &xpp, &xecfg))
 		return 0;
 	return ecbdata.hit;
 }
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 16d37ce6be..2622981d97 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -157,6 +157,12 @@  int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co
 	return xdl_diff(&a, &b, xpp, xecfg, xecb);
 }
 
+void discard_hunk_line(void *priv,
+		       long ob, long on, long nb, long nn,
+		       const char *func, long funclen)
+{
+}
+
 int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
 		  xdiff_emit_hunk_fn hunk_fn,
 		  xdiff_emit_line_fn line_fn,
diff --git a/xdiff-interface.h b/xdiff-interface.h
index 2dbe2feb19..7b0ccbdd1d 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -35,6 +35,12 @@  extern void xdiff_clear_find_func(xdemitconf_t *xecfg);
 extern int git_xmerge_config(const char *var, const char *value, void *cb);
 extern int git_xmerge_style;
 
+/*
+ * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL
+ * one just sends the hunk line to the line_fn callback).
+ */
+void discard_hunk_line(void *, long, long, long, long, const char *, long);
+
 /*
  * Compare the strings l1 with l2 which are of size s1 and s2 respectively.
  * Returns 1 if the strings are deemed equal, 0 otherwise.