| Message ID | 20181102063606.GC31216@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] xdiff: provide a separate emit callback for hunks | expand |
> +/* > + * 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/
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
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.
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.
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(-)