Skip to content

Makefile update libgit.a: Include xdiff and reftable in libgit.a #2065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: next
Choose a base branch
from
+23 −39
Open

Makefile update libgit.a: Include xdiff and reftable in libgit.a #2065

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b535a45
make: delete XDIFF_LIB, add xdiff to LIB_OBJS
ezekielnewren Sep 20, 2025
9031610
make: delete REFTABLE_LIB, add reftable to LIB_OBJS
ezekielnewren Sep 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 23 additions & 39 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,8 +918,6 @@ export PYTHON_PATH
TEST_SHELL_PATH = $(SHELL_PATH)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

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?

Patrick

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

Incidentally this would slightly reduce the patch noise and make the
result more merge friendly when other topics are in flight that
touch these (like adding a new file or two to REFTABLE_OBJS), but
with the movement of these lines in [1/3], that benefit is
diminished.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

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.

Patrick

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ezekiel Newren wrote (reply to this):

On Thu, Oct 2, 2025 at 9:33 AM Patrick Steinhardt <ps@pks.im> wrote:
> 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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ezekiel Newren wrote (reply to this):

On Wed, Oct 1, 2025 at 11:47 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> 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.

If I understand correctly, the PHONY target xdiff-objs is to make sure
that xdiff/lib.a is always built. But if it's folded into libgit.a
then its purpose becomes moot. I will update my commit message to make
this clear.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Ezekiel Newren <ezekielnewren@gmail.com> writes:

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

With step [1/3] shuffling lines around, the advantage of keeping
XDIFF_OBJS and REFTABLE_OBJS to make the result merge-friendly
disappeared, so I am perfectly fine to throw everything into
LIB_OBJS.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ezekiel Newren wrote (reply to this):

On Thu, Oct 2, 2025 at 1:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ezekiel Newren <ezekielnewren@gmail.com> writes:
>
> > I will go with your preference Junio. Do you prefer your way or Patrick's way?
>
> With step [1/3] shuffling lines around, the advantage of keeping
> XDIFF_OBJS and REFTABLE_OBJS to make the result merge-friendly
> disappeared, so I am perfectly fine to throw everything into
> LIB_OBJS.

Sounds good. I'll do that in my next version.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Wed, Oct 01, 2025 at 06:02:28PM +0000, Ezekiel Newren via GitGitGadget wrote:
> diff --git a/Makefile b/Makefile
> index d89ba03286..4c63045443 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1411,9 +1410,9 @@ REFTABLE_OBJS += reftable/system.o
>  REFTABLE_OBJS += reftable/table.o
>  REFTABLE_OBJS += reftable/tree.o
>  REFTABLE_OBJS += reftable/writer.o
> +LIB_OBJS += $(REFTABLE_OBJS)
>  
> -# reftable lib may in turn depend on what is in libgit.a
> -GITLIBS = common-main.o $(LIB_FILE) $(REFTABLE_LIB) $(LIB_FILE)
> +GITLIBS = common-main.o $(LIB_FILE)
>  EXTLIBS =
>  
>  GIT_USER_AGENT = git/$(GIT_VERSION)

Same question here as on the preceding commit: do we even need
REFTABLE_OBJS anymore?

Other than that these patches look sensible to me, thanks. Even without
Rust they simplify our build infra a bit, so I think that landing them
independently of Rust is a good thing.

Thanks!

Patrick

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Oct 01, 2025 at 06:02:28PM +0000, Ezekiel Newren via GitGitGadget wrote:
>> diff --git a/Makefile b/Makefile
>> index d89ba03286..4c63045443 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1411,9 +1410,9 @@ REFTABLE_OBJS += reftable/system.o
>>  REFTABLE_OBJS += reftable/table.o
>>  REFTABLE_OBJS += reftable/tree.o
>>  REFTABLE_OBJS += reftable/writer.o
>> +LIB_OBJS += $(REFTABLE_OBJS)
>>  
>> -# reftable lib may in turn depend on what is in libgit.a
>> -GITLIBS = common-main.o $(LIB_FILE) $(REFTABLE_LIB) $(LIB_FILE)
>> +GITLIBS = common-main.o $(LIB_FILE)
>>  EXTLIBS =
>>  
>>  GIT_USER_AGENT = git/$(GIT_VERSION)
>
> Same question here as on the preceding commit: do we even need
> REFTABLE_OBJS anymore?

Same answer as before.

> Other than that these patches look sensible to me, thanks. Even without
> Rust they simplify our build infra a bit, so I think that landing them
> independently of Rust is a good thing.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ezekiel Newren wrote (reply to this):

On Wed, Oct 1, 2025 at 11:49 PM Patrick Steinhardt <ps@pks.im> wrote:
> Other than that these patches look sensible to me, thanks. Even without
> Rust they simplify our build infra a bit, so I think that landing them
> independently of Rust is a good thing.

I agree. I've been trying to find and fix things that don't depend on
Rust, but will need to be changed to make the adoption of Rust
smoother.

Thanks for your feedback.


LIB_FILE = libgit.a
XDIFF_LIB = xdiff/lib.a
REFTABLE_LIB = reftable/libreftable.a

GENERATED_H += command-list.h
GENERATED_H += config-list.h
Expand Down Expand Up @@ -1137,6 +1135,19 @@ LIB_OBJS += refs/iterator.o
LIB_OBJS += refs/packed-backend.o
LIB_OBJS += refs/ref-cache.o
LIB_OBJS += refspec.o
LIB_OBJS += reftable/basics.o
LIB_OBJS += reftable/error.o
LIB_OBJS += reftable/block.o
LIB_OBJS += reftable/blocksource.o
LIB_OBJS += reftable/iter.o
LIB_OBJS += reftable/merged.o
LIB_OBJS += reftable/pq.o
LIB_OBJS += reftable/record.o
LIB_OBJS += reftable/stack.o
LIB_OBJS += reftable/system.o
LIB_OBJS += reftable/table.o
LIB_OBJS += reftable/tree.o
LIB_OBJS += reftable/writer.o
LIB_OBJS += remote.o
LIB_OBJS += replace-object.o
LIB_OBJS += repo-settings.o
Expand Down Expand Up @@ -1209,6 +1220,13 @@ LIB_OBJS += write-or-die.o
LIB_OBJS += ws.o
LIB_OBJS += wt-status.o
LIB_OBJS += xdiff-interface.o
LIB_OBJS += xdiff/xdiffi.o
LIB_OBJS += xdiff/xemit.o
LIB_OBJS += xdiff/xhistogram.o
LIB_OBJS += xdiff/xmerge.o
LIB_OBJS += xdiff/xpatience.o
LIB_OBJS += xdiff/xprepare.o
LIB_OBJS += xdiff/xutils.o

BUILTIN_OBJS += builtin/add.o
BUILTIN_OBJS += builtin/am.o
Expand Down Expand Up @@ -1390,8 +1408,7 @@ CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o

UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o

# xdiff and reftable libs may in turn depend on what is in libgit.a
GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
GITLIBS = common-main.o $(LIB_FILE)
EXTLIBS =

GIT_USER_AGENT = git/$(GIT_VERSION)
Expand Down Expand Up @@ -2723,30 +2740,6 @@ reconfigure config.mak.autogen: config.status
.PHONY: reconfigure # This is a convenience target.
endif

XDIFF_OBJS += xdiff/xdiffi.o
XDIFF_OBJS += xdiff/xemit.o
XDIFF_OBJS += xdiff/xhistogram.o
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)

REFTABLE_OBJS += reftable/basics.o
REFTABLE_OBJS += reftable/error.o
REFTABLE_OBJS += reftable/block.o
REFTABLE_OBJS += reftable/blocksource.o
REFTABLE_OBJS += reftable/iter.o
REFTABLE_OBJS += reftable/merged.o
REFTABLE_OBJS += reftable/pq.o
REFTABLE_OBJS += reftable/record.o
REFTABLE_OBJS += reftable/stack.o
REFTABLE_OBJS += reftable/system.o
REFTABLE_OBJS += reftable/table.o
REFTABLE_OBJS += reftable/tree.o
REFTABLE_OBJS += reftable/writer.o

TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))

.PHONY: test-objs
Expand All @@ -2767,9 +2760,8 @@ OBJECTS += $(GIT_OBJS)
OBJECTS += $(SCALAR_OBJS)
OBJECTS += $(PROGRAM_OBJS)
OBJECTS += $(TEST_OBJS)
OBJECTS += $(XDIFF_OBJS)
OBJECTS += $(FUZZ_OBJS)
OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
OBJECTS += $(REFTABLE_TEST_OBJS)
OBJECTS += $(UNIT_TEST_OBJS)
OBJECTS += $(CLAR_TEST_OBJS)
OBJECTS += $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
Expand Down Expand Up @@ -2921,12 +2913,6 @@ scalar$X: scalar.o GIT-LDFLAGS $(GITLIBS)
$(LIB_FILE): $(LIB_OBJS)
$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^

$(XDIFF_LIB): $(XDIFF_OBJS)
$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^

$(REFTABLE_LIB): $(REFTABLE_OBJS)
$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^

export DEFAULT_EDITOR DEFAULT_PAGER

Documentation/GIT-EXCLUDED-PROGRAMS: FORCE
Expand Down Expand Up @@ -3765,7 +3751,7 @@ clean: profile-clean coverage-clean cocciclean
$(RM) git.rc git.res
$(RM) $(OBJECTS)
$(RM) headless-git.o
$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB)
$(RM) $(LIB_FILE)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
$(RM) $(TEST_PROGRAMS)
$(RM) $(FUZZ_PROGRAMS)
Expand Down Expand Up @@ -3958,8 +3944,6 @@ endif

LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o
LIBGIT_PUB_OBJS += libgit.a
LIBGIT_PUB_OBJS += reftable/libreftable.a
LIBGIT_PUB_OBJS += xdiff/lib.a

LIBGIT_PARTIAL_EXPORT = contrib/libgit-sys/partial_symbol_export.o

Expand Down