Re: [PATCH] xdiff: rename "struct group" to "struct xdlgroup"

[Jeff King]

On Tue, Sep 27, 2016 at 03:18:02PM +0200, Michael Haggerty wrote:

> > Let's resolve by giving the xdiff variant a scoped name,
> > which is closer to other xdiff types anyway (e.g.,
> > xdlfile_t, though note that xdiff is fond if typedefs when
> > Git usually is not).
> 
> Makes sense to me. I didn't try to adhere to xdiff conventions too
> tightly because I don't think that project is alive anymore, so I don't
> expect we'll be upstreaming anything [1]. But this change definitely
> makes sense.

Yeah, TBH I don't really care about following xdiff coding conventions.
They're pretty far from our own, and at this point I think xdiff is
basically just an imported part of our code base. Mostly my rationale
was that it's not too terribly out of place to give it an "xdl" name,
and it happens to solve my other problem, too. :)

> [1] Though I've since learned that libgit2 also bases their diff code on
> xdiff, so if we avoid changing things gratuitously there is more chance
> that our two projects can benefit from each other's improvements
> whenever they are also licensed compatibly.

I'd agree on not changing things gratuitously.

-Peff

Re: [PATCH] xdiff: rename "struct group" to "struct xdlgroup"

[Stefan Beller]

On Mon, Sep 26, 2016 at 9:37 PM, Jeff King  wrote:
> Commit e8adf23 (xdl_change_compact(): introduce the concept
> of a change group, 2016-08-22) added a "struct group" type
> to xdiff/xdiffi.c. But the POSIX system header "grp.h"
> already defines "struct group" (it is part of the getgrnam
> interface). This happens to work because the new type is
> local to xdiffi.c, and the xdiff code includes a relatively
> small set of system headers. But it will break compilation
> if xdiff ever switches to using git-compat-util.h.  It can
> also probably cause confusion with tools that look at the
> whole code base, like coccinelle or ctags.
>
> Let's resolve by giving the xdiff variant a scoped name,
> which is closer to other xdiff types anyway (e.g.,
> xdlfile_t, though note that xdiff is fond if typedefs when
> Git usually is not).

Makes sense!

Thanks,
Stefan

>
> Signed-off-by: Jeff King 
> ---
> I didn't rename the functions, which have no conflict, but that would
> also be closer to xdiff's usual style. I don't know how far it is worth
> going; maybe this patch is even already too far.
>
> I noticed because I have a patch series which switches xdiff
> to git-compat-util, to try to use the st_* macros there.
>
>  xdiff/xdiffi.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index 67c1ccc..760fbb6 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -708,7 +708,7 @@ static int score_cmp(struct split_score *s1, struct 
> split_score *s2)
>   * Note that loops that are testing for changed lines in xdf->rchg do not 
> need
>   * index bounding since the array is prepared with a zero at position -1 and 
> N.
>   */
> -struct group {
> +struct xdlgroup {
> /*
>  * The index of the first changed line in the group, or the index of
>  * the unchanged line above which the (empty) group is located.
> @@ -725,7 +725,7 @@ struct group {
>  /*
>   * Initialize g to point at the first group in xdf.
>   */
> -static void group_init(xdfile_t *xdf, struct group *g)
> +static void group_init(xdfile_t *xdf, struct xdlgroup *g)
>  {
> g->start = g->end = 0;
> while (xdf->rchg[g->end])
> @@ -736,7 +736,7 @@ static void group_init(xdfile_t *xdf, struct group *g)
>   * Move g to describe the next (possibly empty) group in xdf and return 0. 
> If g
>   * is already at the end of the file, do nothing and return -1.
>   */
> -static inline int group_next(xdfile_t *xdf, struct group *g)
> +static inline int group_next(xdfile_t *xdf, struct xdlgroup *g)
>  {
> if (g->end == xdf->nrec)
> return -1;
> @@ -752,7 +752,7 @@ static inline int group_next(xdfile_t *xdf, struct group 
> *g)
>   * Move g to describe the previous (possibly empty) group in xdf and return 
> 0.
>   * If g is already at the beginning of the file, do nothing and return -1.
>   */
> -static inline int group_previous(xdfile_t *xdf, struct group *g)
> +static inline int group_previous(xdfile_t *xdf, struct xdlgroup *g)
>  {
> if (g->start == 0)
> return -1;
> @@ -769,7 +769,7 @@ static inline int group_previous(xdfile_t *xdf, struct 
> group *g)
>   * following group, expand this group to include it. Return 0 on success or 
> -1
>   * if g cannot be slid down.
>   */
> -static int group_slide_down(xdfile_t *xdf, struct group *g, long flags)
> +static int group_slide_down(xdfile_t *xdf, struct xdlgroup *g, long flags)
>  {
> if (g->end < xdf->nrec &&
> recs_match(xdf->recs[g->start], xdf->recs[g->end], flags)) {
> @@ -790,7 +790,7 @@ static int group_slide_down(xdfile_t *xdf, struct group 
> *g, long flags)
>   * into a previous group, expand this group to include it. Return 0 on 
> success
>   * or -1 if g cannot be slid up.
>   */
> -static int group_slide_up(xdfile_t *xdf, struct group *g, long flags)
> +static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g, long flags)
>  {
> if (g->start > 0 &&
> recs_match(xdf->recs[g->start - 1], xdf->recs[g->end - 1], 
> flags)) {
> @@ -818,7 +818,7 @@ static void xdl_bug(const char *msg)
>   * size.
>   */
>  int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
> -   struct group g, go;
> +   struct xdlgroup g, go;
> long earliest_end, end_matching_other;
> long groupsize;
> unsigned int blank_lines;
> --
> 2.10.0.492.g14f803f

Re: [PATCH] xdiff: rename "struct group" to "struct xdlgroup"

[Michael Haggerty]

On 09/27/2016 06:37 AM, Jeff King wrote:
> Commit e8adf23 (xdl_change_compact(): introduce the concept
> of a change group, 2016-08-22) added a "struct group" type
> to xdiff/xdiffi.c. But the POSIX system header "grp.h"
> already defines "struct group" (it is part of the getgrnam
> interface). This happens to work because the new type is
> local to xdiffi.c, and the xdiff code includes a relatively
> small set of system headers. But it will break compilation
> if xdiff ever switches to using git-compat-util.h.  It can
> also probably cause confusion with tools that look at the
> whole code base, like coccinelle or ctags.
> 
> Let's resolve by giving the xdiff variant a scoped name,
> which is closer to other xdiff types anyway (e.g.,
> xdlfile_t, though note that xdiff is fond if typedefs when
> Git usually is not).

Makes sense to me. I didn't try to adhere to xdiff conventions too
tightly because I don't think that project is alive anymore, so I don't
expect we'll be upstreaming anything [1]. But this change definitely
makes sense.

Thanks,
Michael

[1] Though I've since learned that libgit2 also bases their diff code on
xdiff, so if we avoid changing things gratuitously there is more chance
that our two projects can benefit from each other's improvements
whenever they are also licensed compatibly.