Re: [PATCH 2/3] make: delete XDIFF_LIB, add xdiff to LIB_OBJS

From: Ezekiel Newren <ezekielnewren@gmail.com>
Date: Oct 2, 2025, 18:50 UTC
Message-ID: <CAH=ZcbBQ2abBS5n=_OZ=qY_K=on9sBa_sK2HbbBzbwa41gWFQg@mail.gmail.com>
In-Reply-To: <aN6bL07N8Qz6USTf@pks.im>

On Thu, Oct 2, 2025 at 9:33 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 33 quoted lines

> On Thu, Oct 02, 2025 at 06:31:33AM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > On Wed, Oct 01, 2025 at 06:02:27PM +0000, Ezekiel Newren via GitGitGadget wrote:
> > >> diff --git a/Makefile b/Makefile
> > >> index e8fad803be..d89ba03286 100644
> > >> --- a/Makefile
> > >> +++ b/Makefile
> > >> @@ -1397,8 +1396,7 @@ XDIFF_OBJS += xdiff/xmerge.o
> > >>  XDIFF_OBJS += xdiff/xpatience.o
> > >>  XDIFF_OBJS += xdiff/xprepare.o
> > >>  XDIFF_OBJS += xdiff/xutils.o
> > >> -.PHONY: xdiff-objs
> > >> -xdiff-objs: $(XDIFF_OBJS)
> > >
> > > The removal of the `xdiff-objs` target isn't mentioned or justified in
> > > the commit message. I personally don't mind that this target goes away,
> > > as I don't really have a use case for it anyway. But in theory it could
> > > continue to exist. So I'd either retain it, or explain why it goes away.
> > >
> > > In case it goes away, is there still a reason to have the separate
> > > XDIFF_OBJS variable? Can't we add these objects to `LIB_OBJS` directly?
> >
> > Doing it this way lets us still keep the "logical" organization to
> > tell which object is which, even though we may lose physical
> > distinction by throwing all objects in a single library archive.
>
> Well, I guess the logical organization still exists due to all the files
> living in "xdiff/" and "reftable/", respectively. So I'm not sure that's
> a definitive win.
>
> But in any case, I don't have any strong feelings here. I mostly
> wondered whether we can simplify the build infra even further.

My preference is the same as yours Patrick. In my Introduce Rust v2 series (that I dropped) I did it the way that you described. I changed how I did things because of Junio's suggestion. I think doing it Patrick's way would be more consistent because in Meson the `libgit_sources` variable includes all C files that are part of libgit. That variable includes the sources for reftable and xdiff.

snippet from meson.build:
libgit_sources = [
  ...
  'reftable/basics.c',
  'reftable/error.c',
  'reftable/block.c',
  'reftable/blocksource.c',
  'reftable/iter.c',
  'reftable/merged.c',
  'reftable/pq.c',
  'reftable/record.c',
  'reftable/stack.c',
  'reftable/system.c',
  'reftable/table.c',
  'reftable/tree.c',
  'reftable/writer.c',
  ...
  'xdiff/xdiffi.c',
  'xdiff/xemit.c',
  'xdiff/xhistogram.c',
  'xdiff/xmerge.c',
  'xdiff/xpatience.c',
  'xdiff/xprepare.c',
  'xdiff/xutils.c',
]

I will go with your preference Junio. Do you prefer your way or Patrick's way?