[PATCH] build: get rid of the notion of a git library

There's no libgit, and there will never be, every object file in Git is
the same, and there's wish to organize them in any way; they are *all*
for the 'git' binary and its builtin commands.

So let's shatter any hopes of ever having a library, and be clear about
it; both the top-level objects (./*.o) and the builtin objects
(./builtin/*.o) go into git.a, which is not a library, merely a
convenient way to stash objects together.

This way there will not be linking issues when top-level objects try to
access functions of builtin objects.

LIB_OBJS and LIB_H imply a library, but there isn't one, and never will
be; so give them proper names; just a bunch of headers and objects.

Signed-off-by: Felipe Contreras <redacted>
---
 Makefile | 564 ++++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 283 insertions(+), 281 deletions(-)

diff --git a/Makefile b/Makefile
index 03524d0..63451b1 100644
--- a/Makefile
+++ b/Makefile

@@ -435,8 +435,8 @@ XDIFF_OBJS =
 VCSSVN_OBJS =
 GENERATED_H =
 EXTRA_CPPFLAGS =
-LIB_H =
-LIB_OBJS =
+HEADERS =
+OBJS =
 PROGRAM_OBJS =
 PROGRAMS =
 SCRIPT_PERL =

@@ -629,270 +629,270 @@ endif
 export PERL_PATH
 export PYTHON_PATH
 
-LIB_FILE = libgit.a
+GIT_LIB = git.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
 
-LIB_H += advice.h
-LIB_H += archive.h
-LIB_H += argv-array.h
-LIB_H += attr.h
-LIB_H += bisect.h
-LIB_H += blob.h
-LIB_H += branch.h
-LIB_H += builtin.h
-LIB_H += bulk-checkin.h
-LIB_H += bundle.h
-LIB_H += cache-tree.h
-LIB_H += cache.h
-LIB_H += color.h
-LIB_H += column.h
-LIB_H += commit.h
-LIB_H += compat/bswap.h
-LIB_H += compat/cygwin.h
-LIB_H += compat/mingw.h
-LIB_H += compat/obstack.h
-LIB_H += compat/poll/poll.h
-LIB_H += compat/precompose_utf8.h
-LIB_H += compat/terminal.h
-LIB_H += compat/win32/dirent.h
-LIB_H += compat/win32/pthread.h
-LIB_H += compat/win32/syslog.h
-LIB_H += connected.h
-LIB_H += convert.h
-LIB_H += credential.h
-LIB_H += csum-file.h
-LIB_H += decorate.h
-LIB_H += delta.h
-LIB_H += diff.h
-LIB_H += diffcore.h
-LIB_H += dir.h
-LIB_H += exec_cmd.h
-LIB_H += fetch-pack.h
-LIB_H += fmt-merge-msg.h
-LIB_H += fsck.h
-LIB_H += gettext.h
-LIB_H += git-compat-util.h
-LIB_H += gpg-interface.h
-LIB_H += graph.h
-LIB_H += grep.h
-LIB_H += hash.h
-LIB_H += help.h
-LIB_H += http.h
-LIB_H += kwset.h
-LIB_H += levenshtein.h
-LIB_H += line-log.h
-LIB_H += line-range.h
-LIB_H += list-objects.h
-LIB_H += ll-merge.h
-LIB_H += log-tree.h
-LIB_H += mailmap.h
-LIB_H += merge-blobs.h
-LIB_H += merge-recursive.h
-LIB_H += mergesort.h
-LIB_H += notes-cache.h
-LIB_H += notes-merge.h
-LIB_H += notes.h
-LIB_H += object.h
-LIB_H += pack-revindex.h
-LIB_H += pack.h
-LIB_H += parse-options.h
-LIB_H += patch-ids.h
-LIB_H += pathspec.h
-LIB_H += pkt-line.h
-LIB_H += progress.h
-LIB_H += prompt.h
-LIB_H += quote.h
-LIB_H += reachable.h
-LIB_H += reflog-walk.h
-LIB_H += refs.h
-LIB_H += remote.h
-LIB_H += rerere.h
-LIB_H += resolve-undo.h
-LIB_H += revision.h
-LIB_H += run-command.h
-LIB_H += send-pack.h
-LIB_H += sequencer.h
-LIB_H += sha1-array.h
-LIB_H += sha1-lookup.h
-LIB_H += shortlog.h
-LIB_H += sideband.h
-LIB_H += sigchain.h
-LIB_H += strbuf.h
-LIB_H += streaming.h
-LIB_H += string-list.h
-LIB_H += submodule.h
-LIB_H += tag.h
-LIB_H += tar.h
-LIB_H += thread-utils.h
-LIB_H += transport.h
-LIB_H += tree-walk.h
-LIB_H += tree.h
-LIB_H += unpack-trees.h
-LIB_H += url.h
-LIB_H += userdiff.h
-LIB_H += utf8.h
-LIB_H += varint.h
-LIB_H += vcs-svn/fast_export.h
-LIB_H += vcs-svn/line_buffer.h
-LIB_H += vcs-svn/repo_tree.h
-LIB_H += vcs-svn/sliding_window.h
-LIB_H += vcs-svn/svndiff.h
-LIB_H += vcs-svn/svndump.h
-LIB_H += walker.h
-LIB_H += wildmatch.h
-LIB_H += wt-status.h
-LIB_H += xdiff-interface.h
-LIB_H += xdiff/xdiff.h
-LIB_H += xdiff/xdiffi.h
-LIB_H += xdiff/xemit.h
-LIB_H += xdiff/xinclude.h
-LIB_H += xdiff/xmacros.h
-LIB_H += xdiff/xprepare.h
-LIB_H += xdiff/xtypes.h
-LIB_H += xdiff/xutils.h
-
-LIB_OBJS += abspath.o
-LIB_OBJS += advice.o
-LIB_OBJS += alias.o
-LIB_OBJS += alloc.o
-LIB_OBJS += archive.o
-LIB_OBJS += archive-tar.o
-LIB_OBJS += archive-zip.o
-LIB_OBJS += argv-array.o
-LIB_OBJS += attr.o
-LIB_OBJS += base85.o
-LIB_OBJS += bisect.o
-LIB_OBJS += blob.o
-LIB_OBJS += branch.o
-LIB_OBJS += bulk-checkin.o
-LIB_OBJS += bundle.o
-LIB_OBJS += cache-tree.o
-LIB_OBJS += color.o
-LIB_OBJS += column.o
-LIB_OBJS += combine-diff.o
-LIB_OBJS += commit.o
-LIB_OBJS += compat/obstack.o
-LIB_OBJS += compat/terminal.o
-LIB_OBJS += config.o
-LIB_OBJS += connect.o
-LIB_OBJS += connected.o
-LIB_OBJS += convert.o
-LIB_OBJS += copy.o
-LIB_OBJS += credential.o
-LIB_OBJS += csum-file.o
-LIB_OBJS += ctype.o
-LIB_OBJS += date.o
-LIB_OBJS += decorate.o
-LIB_OBJS += diffcore-break.o
-LIB_OBJS += diffcore-delta.o
-LIB_OBJS += diffcore-order.o
-LIB_OBJS += diffcore-pickaxe.o
-LIB_OBJS += diffcore-rename.o
-LIB_OBJS += diff-delta.o
-LIB_OBJS += diff-lib.o
-LIB_OBJS += diff-no-index.o
-LIB_OBJS += diff.o
-LIB_OBJS += dir.o
-LIB_OBJS += editor.o
-LIB_OBJS += entry.o
-LIB_OBJS += environment.o
-LIB_OBJS += exec_cmd.o
-LIB_OBJS += fetch-pack.o
-LIB_OBJS += fsck.o
-LIB_OBJS += gettext.o
-LIB_OBJS += gpg-interface.o
-LIB_OBJS += graph.o
-LIB_OBJS += grep.o
-LIB_OBJS += hash.o
-LIB_OBJS += help.o
-LIB_OBJS += hex.o
-LIB_OBJS += ident.o
-LIB_OBJS += kwset.o
-LIB_OBJS += levenshtein.o
-LIB_OBJS += line-log.o
-LIB_OBJS += line-range.o
-LIB_OBJS += list-objects.o
-LIB_OBJS += ll-merge.o
-LIB_OBJS += lockfile.o
-LIB_OBJS += log-tree.o
-LIB_OBJS += mailmap.o
-LIB_OBJS += match-trees.o
-LIB_OBJS += merge.o
-LIB_OBJS += merge-blobs.o
-LIB_OBJS += merge-recursive.o
-LIB_OBJS += mergesort.o
-LIB_OBJS += name-hash.o
-LIB_OBJS += notes.o
-LIB_OBJS += notes-cache.o
-LIB_OBJS += notes-merge.o
-LIB_OBJS += object.o
-LIB_OBJS += pack-check.o
-LIB_OBJS += pack-revindex.o
-LIB_OBJS += pack-write.o
-LIB_OBJS += pager.o
-LIB_OBJS += parse-options.o
-LIB_OBJS += parse-options-cb.o
-LIB_OBJS += patch-delta.o
-LIB_OBJS += patch-ids.o
-LIB_OBJS += path.o
-LIB_OBJS += pathspec.o
-LIB_OBJS += pkt-line.o
-LIB_OBJS += preload-index.o
-LIB_OBJS += pretty.o
-LIB_OBJS += progress.o
-LIB_OBJS += prompt.o
-LIB_OBJS += quote.o
-LIB_OBJS += reachable.o
-LIB_OBJS += read-cache.o
-LIB_OBJS += reflog-walk.o
-LIB_OBJS += refs.o
-LIB_OBJS += remote.o
-LIB_OBJS += replace_object.o
-LIB_OBJS += rerere.o
-LIB_OBJS += resolve-undo.o
-LIB_OBJS += revision.o
-LIB_OBJS += run-command.o
-LIB_OBJS += send-pack.o
-LIB_OBJS += sequencer.o
-LIB_OBJS += server-info.o
-LIB_OBJS += setup.o
-LIB_OBJS += sha1-array.o
-LIB_OBJS += sha1-lookup.o
-LIB_OBJS += sha1_file.o
-LIB_OBJS += sha1_name.o
-LIB_OBJS += shallow.o
-LIB_OBJS += sideband.o
-LIB_OBJS += sigchain.o
-LIB_OBJS += strbuf.o
-LIB_OBJS += streaming.o
-LIB_OBJS += string-list.o
-LIB_OBJS += submodule.o
-LIB_OBJS += symlinks.o
-LIB_OBJS += tag.o
-LIB_OBJS += trace.o
-LIB_OBJS += transport.o
-LIB_OBJS += transport-helper.o
-LIB_OBJS += tree-diff.o
-LIB_OBJS += tree.o
-LIB_OBJS += tree-walk.o
-LIB_OBJS += unpack-trees.o
-LIB_OBJS += url.o
-LIB_OBJS += usage.o
-LIB_OBJS += userdiff.o
-LIB_OBJS += utf8.o
-LIB_OBJS += varint.o
-LIB_OBJS += version.o
-LIB_OBJS += walker.o
-LIB_OBJS += wildmatch.o
-LIB_OBJS += wrapper.o
-LIB_OBJS += write_or_die.o
-LIB_OBJS += ws.o
-LIB_OBJS += wt-status.o
-LIB_OBJS += xdiff-interface.o
-LIB_OBJS += zlib.o
+HEADERS += advice.h
+HEADERS += archive.h
+HEADERS += argv-array.h
+HEADERS += attr.h
+HEADERS += bisect.h
+HEADERS += blob.h
+HEADERS += branch.h
+HEADERS += builtin.h
+HEADERS += bulk-checkin.h
+HEADERS += bundle.h
+HEADERS += cache-tree.h
+HEADERS += cache.h
+HEADERS += color.h
+HEADERS += column.h
+HEADERS += commit.h
+HEADERS += compat/bswap.h
+HEADERS += compat/cygwin.h
+HEADERS += compat/mingw.h
+HEADERS += compat/obstack.h
+HEADERS += compat/poll/poll.h
+HEADERS += compat/precompose_utf8.h
+HEADERS += compat/terminal.h
+HEADERS += compat/win32/dirent.h
+HEADERS += compat/win32/pthread.h
+HEADERS += compat/win32/syslog.h
+HEADERS += connected.h
+HEADERS += convert.h
+HEADERS += credential.h
+HEADERS += csum-file.h
+HEADERS += decorate.h
+HEADERS += delta.h
+HEADERS += diff.h
+HEADERS += diffcore.h
+HEADERS += dir.h
+HEADERS += exec_cmd.h
+HEADERS += fetch-pack.h
+HEADERS += fmt-merge-msg.h
+HEADERS += fsck.h
+HEADERS += gettext.h
+HEADERS += git-compat-util.h
+HEADERS += gpg-interface.h
+HEADERS += graph.h
+HEADERS += grep.h
+HEADERS += hash.h
+HEADERS += help.h
+HEADERS += http.h
+HEADERS += kwset.h
+HEADERS += levenshtein.h
+HEADERS += line-log.h
+HEADERS += line-range.h
+HEADERS += list-objects.h
+HEADERS += ll-merge.h
+HEADERS += log-tree.h
+HEADERS += mailmap.h
+HEADERS += merge-blobs.h
+HEADERS += merge-recursive.h
+HEADERS += mergesort.h
+HEADERS += notes-cache.h
+HEADERS += notes-merge.h
+HEADERS += notes.h
+HEADERS += object.h
+HEADERS += pack-revindex.h
+HEADERS += pack.h
+HEADERS += parse-options.h
+HEADERS += patch-ids.h
+HEADERS += pathspec.h
+HEADERS += pkt-line.h
+HEADERS += progress.h
+HEADERS += prompt.h
+HEADERS += quote.h
+HEADERS += reachable.h
+HEADERS += reflog-walk.h
+HEADERS += refs.h
+HEADERS += remote.h
+HEADERS += rerere.h
+HEADERS += resolve-undo.h
+HEADERS += revision.h
+HEADERS += run-command.h
+HEADERS += send-pack.h
+HEADERS += sequencer.h
+HEADERS += sha1-array.h
+HEADERS += sha1-lookup.h
+HEADERS += shortlog.h
+HEADERS += sideband.h
+HEADERS += sigchain.h
+HEADERS += strbuf.h
+HEADERS += streaming.h
+HEADERS += string-list.h
+HEADERS += submodule.h
+HEADERS += tag.h
+HEADERS += tar.h
+HEADERS += thread-utils.h
+HEADERS += transport.h
+HEADERS += tree-walk.h
+HEADERS += tree.h
+HEADERS += unpack-trees.h
+HEADERS += url.h
+HEADERS += userdiff.h
+HEADERS += utf8.h
+HEADERS += varint.h
+HEADERS += vcs-svn/fast_export.h
+HEADERS += vcs-svn/line_buffer.h
+HEADERS += vcs-svn/repo_tree.h
+HEADERS += vcs-svn/sliding_window.h
+HEADERS += vcs-svn/svndiff.h
+HEADERS += vcs-svn/svndump.h
+HEADERS += walker.h
+HEADERS += wildmatch.h
+HEADERS += wt-status.h
+HEADERS += xdiff-interface.h
+HEADERS += xdiff/xdiff.h
+HEADERS += xdiff/xdiffi.h
+HEADERS += xdiff/xemit.h
+HEADERS += xdiff/xinclude.h
+HEADERS += xdiff/xmacros.h
+HEADERS += xdiff/xprepare.h
+HEADERS += xdiff/xtypes.h
+HEADERS += xdiff/xutils.h
+
+OBJS += abspath.o
+OBJS += advice.o
+OBJS += alias.o
+OBJS += alloc.o
+OBJS += archive.o
+OBJS += archive-tar.o
+OBJS += archive-zip.o
+OBJS += argv-array.o
+OBJS += attr.o
+OBJS += base85.o
+OBJS += bisect.o
+OBJS += blob.o
+OBJS += branch.o
+OBJS += bulk-checkin.o
+OBJS += bundle.o
+OBJS += cache-tree.o
+OBJS += color.o
+OBJS += column.o
+OBJS += combine-diff.o
+OBJS += commit.o
+OBJS += compat/obstack.o
+OBJS += compat/terminal.o
+OBJS += config.o
+OBJS += connect.o
+OBJS += connected.o
+OBJS += convert.o
+OBJS += copy.o
+OBJS += credential.o
+OBJS += csum-file.o
+OBJS += ctype.o
+OBJS += date.o
+OBJS += decorate.o
+OBJS += diffcore-break.o
+OBJS += diffcore-delta.o
+OBJS += diffcore-order.o
+OBJS += diffcore-pickaxe.o
+OBJS += diffcore-rename.o
+OBJS += diff-delta.o
+OBJS += diff-lib.o
+OBJS += diff-no-index.o
+OBJS += diff.o
+OBJS += dir.o
+OBJS += editor.o
+OBJS += entry.o
+OBJS += environment.o
+OBJS += exec_cmd.o
+OBJS += fetch-pack.o
+OBJS += fsck.o
+OBJS += gettext.o
+OBJS += gpg-interface.o
+OBJS += graph.o
+OBJS += grep.o
+OBJS += hash.o
+OBJS += help.o
+OBJS += hex.o
+OBJS += ident.o
+OBJS += kwset.o
+OBJS += levenshtein.o
+OBJS += line-log.o
+OBJS += line-range.o
+OBJS += list-objects.o
+OBJS += ll-merge.o
+OBJS += lockfile.o
+OBJS += log-tree.o
+OBJS += mailmap.o
+OBJS += match-trees.o
+OBJS += merge.o
+OBJS += merge-blobs.o
+OBJS += merge-recursive.o
+OBJS += mergesort.o
+OBJS += name-hash.o
+OBJS += notes.o
+OBJS += notes-cache.o
+OBJS += notes-merge.o
+OBJS += object.o
+OBJS += pack-check.o
+OBJS += pack-revindex.o
+OBJS += pack-write.o
+OBJS += pager.o
+OBJS += parse-options.o
+OBJS += parse-options-cb.o
+OBJS += patch-delta.o
+OBJS += patch-ids.o
+OBJS += path.o
+OBJS += pathspec.o
+OBJS += pkt-line.o
+OBJS += preload-index.o
+OBJS += pretty.o
+OBJS += progress.o
+OBJS += prompt.o
+OBJS += quote.o
+OBJS += reachable.o
+OBJS += read-cache.o
+OBJS += reflog-walk.o
+OBJS += refs.o
+OBJS += remote.o
+OBJS += replace_object.o
+OBJS += rerere.o
+OBJS += resolve-undo.o
+OBJS += revision.o
+OBJS += run-command.o
+OBJS += send-pack.o
+OBJS += sequencer.o
+OBJS += server-info.o
+OBJS += setup.o
+OBJS += sha1-array.o
+OBJS += sha1-lookup.o
+OBJS += sha1_file.o
+OBJS += sha1_name.o
+OBJS += shallow.o
+OBJS += sideband.o
+OBJS += sigchain.o
+OBJS += strbuf.o
+OBJS += streaming.o
+OBJS += string-list.o
+OBJS += submodule.o
+OBJS += symlinks.o
+OBJS += tag.o
+OBJS += trace.o
+OBJS += transport.o
+OBJS += transport-helper.o
+OBJS += tree-diff.o
+OBJS += tree.o
+OBJS += tree-walk.o
+OBJS += unpack-trees.o
+OBJS += url.o
+OBJS += usage.o
+OBJS += userdiff.o
+OBJS += utf8.o
+OBJS += varint.o
+OBJS += version.o
+OBJS += walker.o
+OBJS += wildmatch.o
+OBJS += wrapper.o
+OBJS += write_or_die.o
+OBJS += ws.o
+OBJS += wt-status.o
+OBJS += xdiff-interface.o
+OBJS += zlib.o
 
 BUILTIN_OBJS += builtin/add.o
 BUILTIN_OBJS += builtin/annotate.o

@@ -990,7 +990,9 @@ BUILTIN_OBJS += builtin/verify-pack.o
 BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/write-tree.o
 
-GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
+OBJS += $(BUILTIN_OBJS)
+
+GITLIBS = $(GIT_LIB) $(XDIFF_LIB)
 EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)

@@ -1365,16 +1367,16 @@ else
 endif
 endif
 ifdef NO_INET_NTOP
-	LIB_OBJS += compat/inet_ntop.o
+	OBJS += compat/inet_ntop.o
 	BASIC_CFLAGS += -DNO_INET_NTOP
 endif
 ifdef NO_INET_PTON
-	LIB_OBJS += compat/inet_pton.o
+	OBJS += compat/inet_pton.o
 	BASIC_CFLAGS += -DNO_INET_PTON
 endif
 ifndef NO_UNIX_SOCKETS
-	LIB_OBJS += unix-socket.o
-	LIB_H += unix-socket.h
+	OBJS += unix-socket.o
+	HEADERS += unix-socket.h
 	PROGRAM_OBJS += credential-cache.o
 	PROGRAM_OBJS += credential-cache--daemon.o
 endif

@@ -1397,13 +1399,13 @@ endif
 
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
-	LIB_OBJS += block-sha1/sha1.o
-	LIB_H += block-sha1/sha1.h
+	OBJS += block-sha1/sha1.o
+	HEADERS += block-sha1/sha1.h
 else
 ifdef PPC_SHA1
 	SHA1_HEADER = "ppc/sha1.h"
-	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
-	LIB_H += ppc/sha1.h
+	OBJS += ppc/sha1.o ppc/sha1ppc.o
+	HEADERS += ppc/sha1.h
 else
 ifdef APPLE_COMMON_CRYPTO
 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL

@@ -1442,7 +1444,7 @@ ifdef NO_PTHREADS
 else
 	BASIC_CFLAGS += $(PTHREAD_CFLAGS)
 	EXTLIBS += $(PTHREAD_LIBS)
-	LIB_OBJS += thread-utils.o
+	OBJS += thread-utils.o
 endif
 
 ifdef HAVE_PATHS_H

@@ -1590,7 +1592,7 @@ LIBS = $(GITLIBS) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
 	$(COMPAT_CFLAGS)
-LIB_OBJS += $(COMPAT_OBJS)
+OBJS += $(COMPAT_OBJS)
 
 # Quote for C
 

@@ -1677,7 +1679,7 @@ strip: $(PROGRAMS) git$X
 
 # The generic compilation pattern rule and automatically
 # computed header dependencies (falling back to a dependency on
-# LIB_H) are enough to describe how most targets should be built,
+# HEADERS) are enough to describe how most targets should be built,
 # but some targets are special enough to need something a little
 # different.
 #

@@ -1712,9 +1714,9 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
-git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
+git$X: git.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
-		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
+		$(ALL_LDFLAGS) $(LIBS)
 
 help.sp help.s help.o: common-cmds.h
 

@@ -1892,7 +1894,7 @@ VCSSVN_OBJS += vcs-svn/svndiff.o
 VCSSVN_OBJS += vcs-svn/svndump.o
 
 TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
-OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
+OBJECTS := $(OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
 	$(XDIFF_OBJS) \
 	$(VCSSVN_OBJS) \
 	git.o

@@ -1998,7 +2000,7 @@ else
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
-$(OBJECTS): $(LIB_H)
+$(OBJECTS): $(HEADERS)
 endif
 
 exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX

@@ -2066,8 +2068,8 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
-$(LIB_FILE): $(LIB_OBJS)
-	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(LIB_OBJS)
+$(GIT_LIB): $(OBJS)
+	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(OBJS)
 
 $(XDIFF_LIB): $(XDIFF_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(XDIFF_OBJS)

@@ -2102,7 +2104,7 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 	--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
+LOCALIZED_C := $(C_OBJ:o=c) $(HEADERS) $(GENERATED_H)
 LOCALIZED_SH := $(SCRIPT_SH)
 LOCALIZED_PERL := $(SCRIPT_PERL)
 

@@ -2480,7 +2482,7 @@ profile-clean:
 
 clean: profile-clean coverage-clean
 	$(RM) *.o *.res block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o vcs-svn/*.o \
-		builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
+		builtin/*.o $(GIT_LIB) $(XDIFF_LIB) $(VCSSVN_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) -r bin-wrappers $(dep_dirs)

-- 
1.8.3.698.g079b096

Felipe Contreras wrote:

There's no libgit, and there will never be, every object file in Git is
the same, and there's wish to organize them in any way; they are *all*
for the 'git' binary and its builtin commands.

Nice joke patch to illustrate your point ;)

On a more serious note, please be a little more patient while everyone
copes with what you're attempting.  I've already made it clear that
I'm in favor of moving forward with your plan to lib'ify git.  The
problem is that you're sending your changes in fragmented comments and
diffs, and nobody is able to piece together what the big picture is.

Please write one cogent email (preferably with code included)
explaining your plan.

On Sat, Jun 8, 2013 at 1:02 PM, Ramkumar Ramachandra [off-list ref] wrote:

Felipe Contreras wrote:

quoted

There's no libgit, and there will never be, every object file in Git is
the same, and there's wish to organize them in any way; they are *all*
for the 'git' binary and its builtin commands.

Nice joke patch to illustrate your point ;)

It's not a joke. This is seriously the direction the others say is the
correct one.

One direction or the other, the problem that top-level objects can't
access code from builtin objects must be fixed. And if the others
don't want to fix it by properly splitting code between library-like
objects, and builtin objects, there's only one other way to fix it;
this way.

On a more serious note, please be a little more patient while everyone
copes with what you're attempting.

I don't think patience will help. What do you suggest? Wait until the
problem fixes itself? (I'll be waiting until the end times). Wait
until somebody changed their opinion by themselves? (I don't see that
happening).

I've already made it clear that
I'm in favor of moving forward with your plan to lib'ify git.

Unfortunately you are the only one.

The
problem is that you're sending your changes in fragmented comments and
diffs, and nobody is able to piece together what the big picture is.

Please write one cogent email (preferably with code included)
explaining your plan.

The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.

But to be honest, I don't really care, all I want is the problem of
the bogus split to be solved. One way to solve it is going the proper
library way, but the other is to stash everything together into git.a.
Both ways solve the problem.

Give this a try:

--- a/sequencer.c
+++ b/sequencer.c

@@ -14,6 +14,7 @@
 #include "merge-recursive.h"
 #include "refs.h"
 #include "argv-array.h"
+#include "builtin.h"

 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"

@@ -102,6 +103,17 @@ static int has_conforming_footer(struct strbuf

*sb, struct strbuf *sob,
        return 1;
 }

+static void copy_notes(const char *name)
+{
+       struct notes_rewrite_cfg *cfg;
+
+       cfg = init_copy_notes_for_rewrite(name);
+       if (!cfg)
+               return;
+
+       finish_copy_notes_for_rewrite(cfg);
+}
+
 static void remove_sequencer_state(void)
 {
        struct strbuf seq_dir = STRBUF_INIT;

@@ -997,6 +1009,8 @@ static int pick_commits(struct commit_list

*todo_list, struct replay_opts *opts)
                        return res;
        }

+       copy_notes("cherry-pick");
+
        /*
         * Sequence of picks finished successfully; cleanup by
         * removing the .git/sequencer directory

What happens?

libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'

It is not the first time, nor the last that top-level code needs
builtin code, and the solution is easy; organize the code. Alas, this
simple solution reject on the basis that we shouldn't organize the
code, because the code is not meant to be organized.

Cheers.

-- 
Felipe Contreras

Felipe Contreras wrote:

This is a good example: yes, I'm convinced that the code does need to
be reorganized.  Please resend your {sequencer.c ->
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.

On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:

Felipe Contreras wrote:

quoted

The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.

Give this a try:

--- a/sequencer.c
+++ b/sequencer.c

libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'

This is a good example: yes, I'm convinced that the code does need to
be reorganized.  Please resend your {sequencer.c ->
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.

Why should sequencer.c move into builtin/ to solve this?  Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?

On Sun, Jun 9, 2013 at 10:12 AM, John Keeping [off-list ref] wrote:

On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:

quoted

Felipe Contreras wrote:

quoted

The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.

Give this a try:

--- a/sequencer.c
+++ b/sequencer.c

libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'

This is a good example: yes, I'm convinced that the code does need to
be reorganized.  Please resend your {sequencer.c ->
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.

Why should sequencer.c move into builtin/ to solve this?  Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?

Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.

-- 
Felipe Contreras

On Sun, Jun 9, 2013 at 9:56 AM, Ramkumar Ramachandra [off-list ref] wrote:

Felipe Contreras wrote:

quoted

The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.

Give this a try:

--- a/sequencer.c
+++ b/sequencer.c

libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'

This is a good example: yes, I'm convinced that the code does need to
be reorganized.  Please resend your {sequencer.c ->
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.

I already explained this multiple times; code from ./*.o can't access
code from ./builtin/*.o.

-- 
Felipe Contreras

On Sun, Jun 09, 2013 at 10:40:32AM -0500, Felipe Contreras wrote:

On Sun, Jun 9, 2013 at 10:12 AM, John Keeping [off-list ref] wrote:

quoted

On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:

quoted

Felipe Contreras wrote:

quoted

The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.

Give this a try:

--- a/sequencer.c
+++ b/sequencer.c

libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'

This is a good example: yes, I'm convinced that the code does need to
be reorganized.  Please resend your {sequencer.c ->
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.

Why should sequencer.c move into builtin/ to solve this?  Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?

Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.

How is it only useful for builtin commands?  By that logic everything
belongs in builtin/ because it's all only used by builtin commands (I
realise that is what you're arguing towards).

But we make a distinction between things that are specific to one
command (especially argument parsing and user interaction) and more
generally useful features.  Copying notes around in the notes tree is
generally useful so why shouldn't it be in notes.c with the other note
manipulation functions?  The current API may not be completely suitable
but that doesn't mean that it cannot be extracted into notes.c.  In
fact, other than the commit message saying "Notes added by 'git notes
copy'" I don't see what's wrong with the current functions being
extracted as-is.

On Sun, Jun 9, 2013 at 11:02 AM, John Keeping [off-list ref] wrote:

On Sun, Jun 09, 2013 at 10:40:32AM -0500, Felipe Contreras wrote:

quoted

On Sun, Jun 9, 2013 at 10:12 AM, John Keeping [off-list ref] wrote:

quoted

On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:

quoted

Felipe Contreras wrote:

quoted

The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.

Give this a try:

--- a/sequencer.c
+++ b/sequencer.c

libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'

This is a good example: yes, I'm convinced that the code does need to
be reorganized.  Please resend your {sequencer.c ->
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.

Why should sequencer.c move into builtin/ to solve this?  Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?

Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.

How is it only useful for builtin commands?  By that logic everything
belongs in builtin/ because it's all only used by builtin commands (I
realise that is what you're arguing towards).

Which is precisely the point of this patch. If everything is for
builtin commands, then we don't have a git library, and git.a should
contain everything under builtin/*.o.

But we make a distinction between things that are specific to one
command (especially argument parsing and user interaction) and more
generally useful features.

No, we don't. Everything under ./*.o goes to libgit.a, and everything
under ./builtin/*.o goes to 'git'. So builtin/commit.o can access code
from builtin/notes.o, but sequencer.o can't.

-- 
Felipe Contreras

John Keeping wrote:

How is it only useful for builtin commands?  By that logic everything
belongs in builtin/ because it's all only used by builtin commands (I
realise that is what you're arguing towards).

sequencer.c is merely a common API for builtins to invoke
"continuations": i.e. stop the program persisting enough state to let
to user continue, allow the user to do whatever conflict resolutions
using whatever tools, allow the user to continue the original
operation.  sequencer.c provides a uniform UI
(--continue|--skip|--abort), and a uniform way to persist state
(.git/sequencer/todo).  It mainly abstracts out the boring details of
reading/writing the todo lines.

Currently, sequencer.c has no callers other than those in
builtin/revert.c.  In its current shape, it's incapable of being used
by anything else: while an external ruby script (possibly a rebase
implementation) could call into the sequencer to persist state, I
don't think it is going to happen anytime soon.

We might get a proper public api sequencer sometime in the distant
future, but don't confuse that with the current shape of sequencer.c.

But we make a distinction between things that are specific to one
command (especially argument parsing and user interaction) and more
generally useful features.  Copying notes around in the notes tree is
generally useful so why shouldn't it be in notes.c with the other note
manipulation functions?  The current API may not be completely suitable
but that doesn't mean that it cannot be extracted into notes.c.  In
fact, other than the commit message saying "Notes added by 'git notes
copy'" I don't see what's wrong with the current functions being
extracted as-is.

Sure, notes could have a better public api and so could a lot of other
things: worktree operations like reset and checkout come to mind.

The problem is that we seem to be at some frozen in some sort of
stalemate, and some reorganization is definitely required.  What would
you suggest?

On Sun, Jun 09, 2013 at 11:22:06AM -0500, Felipe Contreras wrote:

On Sun, Jun 9, 2013 at 11:02 AM, John Keeping [off-list ref] wrote:

quoted

But we make a distinction between things that are specific to one
command (especially argument parsing and user interaction) and more
generally useful features.

No, we don't. Everything under ./*.o goes to libgit.a, and everything
under ./builtin/*.o goes to 'git'. So builtin/commit.o can access code
from builtin/notes.o, but sequencer.o can't.

I would argue that it was a mistake not to extract these functions from
builtin/notes.c to notes.c when builtin/commit.c started using them.
Calling across from one builtin/*.c file to another is just as wrong as
calling into a builtin/*.c file from a top-level file but the build
system happens not to enforce the former.

John Keeping wrote:

Calling across from one builtin/*.c file to another is just as wrong as
calling into a builtin/*.c file from a top-level file but the build
system happens not to enforce the former.

So libgit.a is a collection of everything that is shared between
builtins?  Does that correspond to reality?

  $ ls *.h | sed 's/.h$/.c/' | xargs file

An example violation: builtin/log.c uses functions defined in
builtin/shortlog.c.

What is the point of all this separation, if no external scripts are
ever going to use libgit.a?

Ramkumar Ramachandra wrote:

So libgit.a is a collection of everything that is shared between
builtins?

That is not to say that we shouldn't share things between builtins.
We can do it in builtin/lib.a, as Felipe has demonstrated here [1].

[1]: http://article.gmane.org/gmane.comp.version-control.git/226975

On Sun, Jun 9, 2013 at 12:03 PM, Ramkumar Ramachandra
[off-list ref] wrote:

John Keeping wrote:

quoted

Calling across from one builtin/*.c file to another is just as wrong as
calling into a builtin/*.c file from a top-level file but the build
system happens not to enforce the former.

So libgit.a is a collection of everything that is shared between
builtins?  Does that correspond to reality?

  $ ls *.h | sed 's/.h$/.c/' | xargs file

An example violation: builtin/log.c uses functions defined in
builtin/shortlog.c.

What is the point of all this separation, if no external scripts are
ever going to use libgit.a?

And all the functions should be static, which doesn't seem to be the case:

00000000000003c0 T add_files_to_cache
0000000000000530 T interactive_add
0000000000000410 T run_add_interactive
0000000000001920 T textconv_object
00000000000005b0 T fmt_merge_msg
0000000000000090 T fmt_merge_msg_config
0000000000000c00 T init_db
0000000000000b40 T set_git_dir_init
0000000000000360 T overlay_tree_on_cache
0000000000000500 T report_path_error
00000000000011a0 T copy_note_for_rewrite
0000000000001210 T finish_copy_notes_for_rewrite
0000000000001060 T init_copy_notes_for_rewrite
0000000000000000 T prune_packed_objects
0000000000000510 T shortlog_add_commit
00000000000006b0 T shortlog_init
0000000000000780 T shortlog_output
0000000000000000 T stripspace

-- 
Felipe Contreras

Op 9-6-2013 17:40, Felipe Contreras schreef:

On Sun, Jun 9, 2013 at 10:12 AM, John Keeping [off-list ref] wrote:

quoted

On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:

quoted

Felipe Contreras wrote:

quoted

The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.

Give this a try:

--- a/sequencer.c
+++ b/sequencer.c

libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'

This is a good example: yes, I'm convinced that the code does need to
be reorganized.  Please resend your {sequencer.c ->
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.

Why should sequencer.c move into builtin/ to solve this?  Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?

Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.

I think that libgit.a should contain all code to be able to carry out 
all functions of git. The stuff in builtin/ is just a command-line user 
interface. Whether or not sequencer should be in builtin depends on 
whether the sequencer is only part of this command-line user interface.

I think that the sequencer code is at the moment unusable if you do not 
use the code in builtin/ so that would advocate to move it into 
builtin/. If sequencer is in libgit, and I write my own (graphical) user 
interface, I expect to be able to use it.

Vincent

On Sun, Jun 09, 2013 at 12:13:41PM -0500, Felipe Contreras wrote:

On Sun, Jun 9, 2013 at 12:03 PM, Ramkumar Ramachandra
[off-list ref] wrote:

quoted

John Keeping wrote:

quoted

Calling across from one builtin/*.c file to another is just as wrong as
calling into a builtin/*.c file from a top-level file but the build
system happens not to enforce the former.

So libgit.a is a collection of everything that is shared between
builtins?  Does that correspond to reality?

I think that's *precisely* what libgit.a is.  It doesn't currently
correspond exactly to reality, but that's mostly for historic reasons
(see below).

quoted

  $ ls *.h | sed 's/.h$/.c/' | xargs file

An example violation: builtin/log.c uses functions defined in
builtin/shortlog.c.

What is the point of all this separation, if no external scripts are
ever going to use libgit.a?

Why do we structure code in a certain way at all?  The reason libgit.a
was introduced (according to commit 0a02ce7) is:

    This introduces the concept of git "library" objects that
    the real programs use, and makes it easier to add such
    things to a "libgit.a".

And all the functions should be static, which doesn't seem to be the case:

00000000000003c0 T add_files_to_cache
0000000000000530 T interactive_add
0000000000000410 T run_add_interactive
0000000000001920 T textconv_object
00000000000005b0 T fmt_merge_msg
0000000000000090 T fmt_merge_msg_config
0000000000000c00 T init_db
0000000000000b40 T set_git_dir_init
0000000000000360 T overlay_tree_on_cache
0000000000000500 T report_path_error
00000000000011a0 T copy_note_for_rewrite
0000000000001210 T finish_copy_notes_for_rewrite
0000000000001060 T init_copy_notes_for_rewrite
0000000000000000 T prune_packed_objects
0000000000000510 T shortlog_add_commit
00000000000006b0 T shortlog_init
0000000000000780 T shortlog_output
0000000000000000 T stripspace

A quick check with "git log -S..." shows that most of these have barely
been touched since the builtin/ directory was created.  So the reason
they're not static is most likely because no one has tidied them up
since the division between builtins was introduced.

It is a fact of life that as we live and work with a system we realise
that there may be a better way of doing something.  This doesn't mean
that someone needs to immediately convert everything to the new way,
it is often sufficient to do new things in the new way and slowly move
existing things across as and when they are touched for other reasons.

On Sun, Jun 9, 2013 at 12:30 PM, Vincent van Ravesteijn [off-list ref] wrote:

Op 9-6-2013 17:40, Felipe Contreras schreef:

quoted

On Sun, Jun 9, 2013 at 10:12 AM, John Keeping [off-list ref] wrote:

quoted

On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:

quoted

Felipe Contreras wrote:

quoted

The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.

Give this a try:

--- a/sequencer.c
+++ b/sequencer.c

libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'

This is a good example: yes, I'm convinced that the code does need to
be reorganized.  Please resend your {sequencer.c ->
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.

Why should sequencer.c move into builtin/ to solve this?  Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?

Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.

I think that libgit.a should contain all code to be able to carry out all
functions of git. The stuff in builtin/ is just a command-line user
interface. Whether or not sequencer should be in builtin depends on whether
the sequencer is only part of this command-line user interface.

The sequencer is only part of the command-line user interface.

I think that the sequencer code is at the moment unusable if you do not use
the code in builtin/ so that would advocate to move it into builtin/. If
sequencer is in libgit, and I write my own (graphical) user interface, I
expect to be able to use it.

As do I, but it appears all other Git developers disagree; libgit.a is
not really a library, and will never be used by anything other than
Git's core.

Hence this patch.

-- 
Felipe Contreras

On Sun, Jun 9, 2013 at 12:32 PM, John Keeping [off-list ref] wrote:

On Sun, Jun 09, 2013 at 12:13:41PM -0500, Felipe Contreras wrote:

quoted

On Sun, Jun 9, 2013 at 12:03 PM, Ramkumar Ramachandra
[off-list ref] wrote:

quoted

John Keeping wrote:

quoted

Calling across from one builtin/*.c file to another is just as wrong as
calling into a builtin/*.c file from a top-level file but the build
system happens not to enforce the former.

So libgit.a is a collection of everything that is shared between
builtins?  Does that correspond to reality?

I think that's *precisely* what libgit.a is.

We don't care what libgit.a is; the important thing is what it *should* be.

A quick check with "git log -S..." shows that most of these have barely
been touched since the builtin/ directory was created.  So the reason
they're not static is most likely because no one has tidied them up
since the division between builtins was introduced.

It is a fact of life that as we live and work with a system we realise
that there may be a better way of doing something.  This doesn't mean
that someone needs to immediately convert everything to the new way,
it is often sufficient to do new things in the new way and slowly move
existing things across as and when they are touched for other reasons.

Really?

builtin/ls-files.c:307:13: error: static declaration of
‘overlay_tree_on_cache’ follows non-static declaration
 static void overlay_tree_on_cache(const char *tree_name, const char *prefix)
             ^
In file included from builtin/ls-files.c:8:0:
./cache.h:1318:6: note: previous declaration of ‘overlay_tree_on_cache’ was here
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
      ^
builtin/ls-files.c:361:12: error: static declaration of
‘report_path_error’ follows non-static declaration
 static int report_path_error(const char *ps_matched, const char
**pathspec, const char *prefix)
            ^
In file included from builtin/ls-files.c:8:0:
./cache.h:1317:5: note: previous declaration of ‘report_path_error’ was here
 int report_path_error(const char *ps_matched, const char **pathspec,
const char *prefix);
     ^
make: *** [builtin/ls-files.o] Error 1
make: *** Waiting for unfinished jobs....
builtin/add.c:184:12: error: static declaration of
‘add_files_to_cache’ follows non-static declaration
 static int add_files_to_cache(const char *prefix, const char
**pathspec, int flags)
            ^
In file included from builtin/add.c:6:0:
./cache.h:1283:5: note: previous declaration of ‘add_files_to_cache’ was here
 int add_files_to_cache(const char *prefix, const char **pathspec, int flags);
     ^
builtin/add.c:280:12: error: static declaration of
‘run_add_interactive’ follows non-static declaration
 static int run_add_interactive(const char *revision, const char *patch_mode,
            ^
In file included from ./builtin.h:7:0,
                 from builtin/add.c:7:
./commit.h:187:12: note: previous declaration of ‘run_add_interactive’ was here
 extern int run_add_interactive(const char *revision, const char *patch_mode,
            ^
builtin/add.c:309:12: error: static declaration of ‘interactive_add’
follows non-static declaration
 static int interactive_add(int argc, const char **argv, const char
*prefix, int patch)
            ^
In file included from ./builtin.h:7:0,
                 from builtin/add.c:7:
./commit.h:186:12: note: previous declaration of ‘interactive_add’ was here
 extern int interactive_add(int argc, const char **argv, const char
*prefix, int patch);
            ^
builtin/add.c:184:12: warning: ‘add_files_to_cache’ defined but not
used [-Wunused-function]
 static int add_files_to_cache(const char *prefix, const char
**pathspec, int flags)
            ^
make: *** [builtin/add.o] Error 1
builtin/fmt-merge-msg.c:19:12: error: static declaration of
‘fmt_merge_msg_config’ follows non-static declaration
 static int fmt_merge_msg_config(const char *key, const char *value, void *cb)
            ^
In file included from builtin/fmt-merge-msg.c:9:0:
./fmt-merge-msg.h:5:12: note: previous declaration of
‘fmt_merge_msg_config’ was here
 extern int fmt_merge_msg_config(const char *key, const char *value, void *cb);
            ^
builtin/fmt-merge-msg.c:592:12: error: static declaration of
‘fmt_merge_msg’ follows non-static declaration
 static int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
            ^
In file included from builtin/fmt-merge-msg.c:1:0:
./builtin.h:26:12: note: previous declaration of ‘fmt_merge_msg’ was here
 extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
            ^
make: *** [builtin/fmt-merge-msg.o] Error 1
builtin/init-db.c:316:12: error: static declaration of
‘set_git_dir_init’ follows non-static declaration
 static int set_git_dir_init(const char *git_dir, const char *real_git_dir,
            ^
In file included from builtin/init-db.c:6:0:
./cache.h:421:12: note: previous declaration of ‘set_git_dir_init’ was here
 extern int set_git_dir_init(const char *git_dir, const char
*real_git_dir, int);
            ^
builtin/init-db.c:368:12: error: static declaration of ‘init_db’
follows non-static declaration
 static int init_db(const char *template_dir, unsigned int flags)
            ^
In file included from builtin/init-db.c:6:0:
./cache.h:422:12: note: previous declaration of ‘init_db’ was here
 extern int init_db(const char *template_dir, unsigned int flags);
            ^
make: *** [builtin/init-db.o] Error 1
builtin/shortlog.c:110:13: error: static declaration of
‘shortlog_add_commit’ follows non-static declaration
 static void shortlog_add_commit(struct shortlog *log, struct commit *commit)
             ^
In file included from builtin/shortlog.c:9:0:
./shortlog.h:24:6: note: previous declaration of ‘shortlog_add_commit’ was here
 void shortlog_add_commit(struct shortlog *log, struct commit *commit);
      ^
builtin/shortlog.c:207:13: error: static declaration of
‘shortlog_init’ follows non-static declaration
 static void shortlog_init(struct shortlog *log)
             ^
In file included from builtin/shortlog.c:9:0:
./shortlog.h:22:6: note: previous declaration of ‘shortlog_init’ was here
 void shortlog_init(struct shortlog *log);
      ^
builtin/shortlog.c:287:13: error: static declaration of
‘shortlog_output’ follows non-static declaration
 static void shortlog_output(struct shortlog *log)
             ^
In file included from builtin/shortlog.c:9:0:
./shortlog.h:26:6: note: previous declaration of ‘shortlog_output’ was here
 void shortlog_output(struct shortlog *log);
      ^
builtin/shortlog.c:287:13: warning: ‘shortlog_output’ defined but not
used [-Wunused-function]
 static void shortlog_output(struct shortlog *log)
             ^
make: *** [builtin/shortlog.o] Error 1
builtin/stripspace.c:36:13: error: static declaration of ‘stripspace’
follows non-static declaration
 static void stripspace(struct strbuf *sb, int skip_comments)
             ^
In file included from ./builtin.h:5:0,
                 from builtin/stripspace.c:1:
./strbuf.h:165:13: note: previous declaration of ‘stripspace’ was here
 extern void stripspace(struct strbuf *buf, int skip_comments);
             ^
make: *** [builtin/stripspace.o] Error 1
make[2]: `GIT-VERSION-FILE' is up to date.
builtin/prune-packed.c:37:13: error: static declaration of
‘prune_packed_objects’ follows non-static declaration
 static void prune_packed_objects(int opts)
             ^
In file included from builtin/prune-packed.c:1:0:
./builtin.h:18:13: note: previous declaration of ‘prune_packed_objects’ was here
 extern void prune_packed_objects(int);
             ^
make: *** [builtin/prune-packed.o] Error 1
builtin/notes.c:358:34: error: static declaration of
‘init_copy_notes_for_rewrite’ follows non-static declaration
 static struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
                                  ^
In file included from builtin/notes.c:11:0:
./builtin.h:39:27: note: previous declaration of
‘init_copy_notes_for_rewrite’ was here
 struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd);
                           ^
builtin/notes.c:396:12: error: static declaration of
‘copy_note_for_rewrite’ follows non-static declaration
 static int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
            ^
In file included from builtin/notes.c:11:0:
./builtin.h:40:5: note: previous declaration of ‘copy_note_for_rewrite’ was here
 int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
     ^
builtin/notes.c:406:13: error: static declaration of
‘finish_copy_notes_for_rewrite’ follows non-static declaration
 static void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c)
             ^
In file included from builtin/notes.c:11:0:
./builtin.h:42:6: note: previous declaration of
‘finish_copy_notes_for_rewrite’ was here
 void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c);
      ^
make: *** [builtin/notes.o] Error 1
builtin/blame.c:112:12: error: static declaration of ‘textconv_object’
follows non-static declaration
 static int textconv_object(const char *path,
            ^
In file included from builtin/blame.c:8:0:
./builtin.h:44:12: note: previous declaration of ‘textconv_object’ was here
 extern int textconv_object(const char *path, unsigned mode, const
unsigned char *sha1, int sha1_valid, char **buf, unsigned long
*buf_size);
            ^
make: *** [builtin/blame.o] Error 1

-- 
Felipe Contreras

On Sun, Jun 09, 2013 at 07:30:31PM +0200, Vincent van Ravesteijn wrote:

I think that libgit.a should contain all code to be able to carry out
all functions of git. The stuff in builtin/ is just a command-line
user interface. Whether or not sequencer should be in builtin depends
on whether the sequencer is only part of this command-line user
interface.

One code organization issue I have not seen mentioned is that there is
more CLI than what is in builtin, and libgit.a does more than simply
provide code for the sources in builtin/. There are also external
commands shipped in git.git that are not linked against git.c or the
other builtins.

Once upon a time, all commands were that way, and that was the origin of
libgit.a: the set of common code used by all of the C commands in
git.git. Over time, those commands became builtins (mostly to keep the
size of the libexec dir down). These days there are only a handful of
external commands left, mostly ones that have startup time overhead from
the dynamic loader (e.g., remote-curl, http-push, imap-send).

That is what libgit.a _is_ now.  I do not mean to imply any additional
judgement on what it could be. But if the goal is to make libgit.a
"functions that programs outside git.git would want, and nothing else",
we may want to additionally split out a "libgit-internal.a" consisting
of code that is used by multiple externals in git, but which would not
be appropriate for clients to use.

For example, I think most of "http.c" is in that boat, as it is full of
wrappers for curl code that are of enough quality to reuse within git,
but a little too half-baked to be part of a stable API.

We can always link directly against http.o, too, of course. The point of
putting the files into a static library is that it makes the link
faster, and if there are only a handful of such links, it may not be
worth the effort.

-Peff

On Mon, Jun 10, 2013 at 4:45 PM, Jeff King [off-list ref] wrote:

That is what libgit.a _is_ now.  I do not mean to imply any additional
judgement on what it could be. But if the goal is to make libgit.a
"functions that programs outside git.git would want, and nothing else",
we may want to additionally split out a "libgit-internal.a" consisting
of code that is used by multiple externals in git, but which would not
be appropriate for clients to use.

That might make sense, but that still doesn't clarify what belongs in
./*.o, and what belongs in ./builtin/*.o. And right now that creates a
mess where you have code shared between ./builtin/*.o that is defined
in cache.h (overlay_tree_on_cache), and some in builtin.h
(init_copy_notes_for_rewrite). And it's not clear what should be done
when code in ./*.o needs to access functionality in ./builtin/*.o,
specially if that code is only useful for git builtins, and nothing
else.

-- 
Felipe Contreras

On Mon, Jun 10, 2013 at 04:52:57PM -0500, Felipe Contreras wrote:

On Mon, Jun 10, 2013 at 4:45 PM, Jeff King [off-list ref] wrote:

quoted

That is what libgit.a _is_ now.  I do not mean to imply any additional
judgement on what it could be. But if the goal is to make libgit.a
"functions that programs outside git.git would want, and nothing else",
we may want to additionally split out a "libgit-internal.a" consisting
of code that is used by multiple externals in git, but which would not
be appropriate for clients to use.

That might make sense, but that still doesn't clarify what belongs in
./*.o, and what belongs in ./builtin/*.o. And right now that creates a
mess where you have code shared between ./builtin/*.o that is defined
in cache.h (overlay_tree_on_cache), and some in builtin.h
(init_copy_notes_for_rewrite). And it's not clear what should be done
when code in ./*.o needs to access functionality in ./builtin/*.o,
specially if that code is only useful for git builtins, and nothing
else.

My general impression of the goal of our current code organization is:

  1. builtin/*.c should each contain a single builtin command and its
     supporting static functions. Each file gets linked into git.o to
     make the "main" git executable.

  2. ./*.c is one of:

       a. Shared code usable by externals and builtins, which gets
          linked into libgit.a

       b. An external command itself, with its own main(). It gets
          linked against libgit.a.

  3. Functions in libgit.a should be defined in a header file specific
     to their module (e.g., refs.h). cache.h picks up the slack for
     things that are general, or too small to get their own header file,
     or otherwise don't group well.

I said it was a "goal", because I know that we do not follow that in
many places, so it is certainly easy to find counter-examples (and nor
do I think it cannot be changed; I am just trying to describe the
current rationale). Under that organization, there is no place for "code
that does not go into libgit.a, but is not a builtin command in itself".
There was never a need in the past, because libgit.a was a bit of a
dumping ground for linkable functions, and nobody cared that it had
everything and the kitchen sink.

If we want to start caring, then we probably need to create a separate
"kitchen sink"-like library, with the rule that things in libgit.a
cannot depend on it. In other words, a support library for Git's
commands, for the parts that are not appropriate to expose as part of a
library API.

-Peff

On Mon, Jun 10, 2013 at 5:06 PM, Jeff King [off-list ref] wrote:

On Mon, Jun 10, 2013 at 04:52:57PM -0500, Felipe Contreras wrote:

quoted

On Mon, Jun 10, 2013 at 4:45 PM, Jeff King [off-list ref] wrote:

quoted

That is what libgit.a _is_ now.  I do not mean to imply any additional
judgement on what it could be. But if the goal is to make libgit.a
"functions that programs outside git.git would want, and nothing else",
we may want to additionally split out a "libgit-internal.a" consisting
of code that is used by multiple externals in git, but which would not
be appropriate for clients to use.

That might make sense, but that still doesn't clarify what belongs in
./*.o, and what belongs in ./builtin/*.o. And right now that creates a
mess where you have code shared between ./builtin/*.o that is defined
in cache.h (overlay_tree_on_cache), and some in builtin.h
(init_copy_notes_for_rewrite). And it's not clear what should be done
when code in ./*.o needs to access functionality in ./builtin/*.o,
specially if that code is only useful for git builtins, and nothing
else.

My general impression of the goal of our current code organization is:

  1. builtin/*.c should each contain a single builtin command and its
     supporting static functions. Each file gets linked into git.o to
     make the "main" git executable.

We already know this is not the case. Maybe this should be fixed by
moving all the shared code between builtins to libgit.a, but maybe we
already know at some level this is not wise, and that's why we haven't
done so.

If we want to start caring, then we probably need to create a separate
"kitchen sink"-like library, with the rule that things in libgit.a
cannot depend on it. In other words, a support library for Git's
commands, for the parts that are not appropriate to expose as part of a
library API.

Yes, that's clearly what we should be doing, which is precisely what
my patch that creates builtin/lib.a does.

So we have two options:

a) Do what we clearly should do; create builtin/lib.a, and move code
there that is specific to builtin commands.

b) Do what we think we have been doing; and move _all_ shared code to
libgit.a (which shouldn't be called libgit, because it's not really a
library), and cleanup builtin/*.c so they don't share anything among
themselves directly.

I vote for a), not only because we already know that's what we
_should_ do, but because we are basically already there.

Leaving things as they are is not really an option; that's a mess.

-- 
Felipe Contreras