Re: [PATCH 1/2] Make xdiff_outf_{init,release} interface

From: Brian Downing <bdowning@lavos.net>
Date: Aug 14, 2008, 02:06 UTC
Message-ID: <20080814020614.GD4396@lavos.net>
In-Reply-To: <7vljz0iftm.fsf@gitster.siamese.dyndns.org>

On Wed, Aug 13, 2008 at 05:46:29PM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> Brian Downing <bdowning@lavos.net> writes:
> > @@ -103,6 +110,10 @@ int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
> >  	return 0;
> >  }
> >  
> > +void xdiff_outf_release(void *priv_)
> > +{
> > +}
> > +
> 
> It might make it more clear to have this function take a pointer to
> "struct xdiff_emit_state", which is always the first member of the
> callback private data structure.

That makes the call sites (slightly) more complicated, in that instead
of:
	xdiff_outf_release(&state);
you'd want:
	xdiff_outf_release(&state.xm);

That was not the typical usage before, in that it said "ecb.priv =
&state" rather than "ecb.priv = &state.xm", and I used the void *
argument to mirror that, but I can change it if it'd be preferable.

Show 6 quoted lines

> Although I wish xdi_diff() could do the necessary clean-up immediately
> before it returns (so that the caller did not have to do anything
> special), it is not possible to do so cleanly, because there are
> "outf" implementations other than xdiff_outf that do not even use
> "struct xdiff_emit_state" in their callbacks.  So I think your patch
> makes sense.

Well, I could do something like:

	if (xecb->outf == xdiff_outf)
		/* xdiff_outf cleanup */

at the end of xdi_diff, but that's... kind of horrible I think.

For that matter, I could just make an xdi_outf_diff function that would take the state in addition to the other xdi_diff arguments and go ahead and set it up, do the diff, and tear it down in one step. Maybe that would be better if it works for everywhere this style of diff needs to be called.

Another question:  should I go ahead and make xdiff_outf itself static?

-bcd