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

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

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
---
 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

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

[Ramkumar Ramachandra]

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.

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

[Felipe Contreras]

On Sat, Jun 8, 2013 at 1:02 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> 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 ;)

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

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

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> 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.

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

[John Keeping]

On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
> Felipe Contreras wrote:
> > 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?

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

[Felipe Contreras]

On Sun, Jun 9, 2013 at 10:12 AM, John Keeping <john@keeping.me.uk> wrote:
> On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
>> Felipe Contreras wrote:
>> > 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

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

[Felipe Contreras]

On Sun, Jun 9, 2013 at 9:56 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> 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

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

[John Keeping]

On Sun, Jun 09, 2013 at 10:40:32AM -0500, Felipe Contreras wrote:
> On Sun, Jun 9, 2013 at 10:12 AM, John Keeping <john@keeping.me.uk> wrote:
> > On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
> >> Felipe Contreras wrote:
> >> > 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.

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

[Felipe Contreras]

On Sun, Jun 9, 2013 at 11:02 AM, John Keeping <john@keeping.me.uk> wrote:
> On Sun, Jun 09, 2013 at 10:40:32AM -0500, Felipe Contreras wrote:
>> On Sun, Jun 9, 2013 at 10:12 AM, John Keeping <john@keeping.me.uk> wrote:
>> > On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
>> >> Felipe Contreras wrote:
>> >> > 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

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

[Ramkumar Ramachandra]

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?

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

[John Keeping]

On Sun, Jun 09, 2013 at 11:22:06AM -0500, Felipe Contreras wrote:
> On Sun, Jun 9, 2013 at 11:02 AM, John Keeping <john@keeping.me.uk> wrote:
> > 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.

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

[Ramkumar Ramachandra]

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?

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

[Ramkumar Ramachandra]

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

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

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:03 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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?

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

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

[Vincent van Ravesteijn]

Op 9-6-2013 17:40, Felipe Contreras schreef:
> On Sun, Jun 9, 2013 at 10:12 AM, John Keeping <john@keeping.me.uk> wrote:
>> On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
>>> Felipe Contreras wrote:
>>>> 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

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

[John Keeping]

On Sun, Jun 09, 2013 at 12:13:41PM -0500, Felipe Contreras wrote:
> On Sun, Jun 9, 2013 at 12:03 PM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
> > 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?

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).

> >   $ 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.

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

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:30 PM, Vincent van Ravesteijn <vfr@lyx.org> wrote:
> Op 9-6-2013 17:40, Felipe Contreras schreef:
>
>> On Sun, Jun 9, 2013 at 10:12 AM, John Keeping <john@keeping.me.uk> wrote:
>>>
>>> On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
>>>>
>>>> Felipe Contreras wrote:
>>>>>
>>>>> 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

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

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:32 PM, John Keeping <john@keeping.me.uk> wrote:
> On Sun, Jun 09, 2013 at 12:13:41PM -0500, Felipe Contreras wrote:
>> On Sun, Jun 9, 2013 at 12:03 PM, Ramkumar Ramachandra
>> <artagnon@gmail.com> wrote:
>> > 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?
>
> 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

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

[Jeff King]

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

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

[Felipe Contreras]

On Mon, Jun 10, 2013 at 4:45 PM, Jeff King <peff@peff.net> 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

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

[Jeff King]

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

> On Mon, Jun 10, 2013 at 4:45 PM, Jeff King <peff@peff.net> 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.

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

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

[Felipe Contreras]

On Mon, Jun 10, 2013 at 5:06 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Jun 10, 2013 at 04:52:57PM -0500, Felipe Contreras wrote:
>
>> On Mon, Jun 10, 2013 at 4:45 PM, Jeff King <peff@peff.net> 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.
>
> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Correct; that is what we aimed for when we made builtin-*.c (later
moved to builtin/*.c).  Some builtin/*.c files can contain more than
one cmd_foo implementations, so "single" is not a solid rule, and it
does not have to be, because all of them are expected to be linked
into the main binary together with git.c to be called from main().

And as you hinted, if some global data or functions in it turns out
to be useful for standalone binaries, their definitions must migrate
out of buitlin/*.c to ./*.c files, because standalone binaries with
their own main() definition cannot be linked with builtin/*.o, the
latter of which requires to be linked with git.o with its own main().

>   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.

The rationale behind libgit.a was so that make targets for the
standalone binaries (note: all of them were standalone in the
beginning) do not have to list *.o files that each of them needs to
be linked with.  It was primary done as a convenient way to have the
linker figure out the dependency and link only what was needed.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> 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.
>
> Correct; that is what we aimed for when we made builtin-*.c (later
> moved to builtin/*.c).  Some builtin/*.c files can contain more than
> one cmd_foo implementations, so "single" is not a solid rule, and it
> does not have to be, because all of them are expected to be linked
> into the main binary together with git.c to be called from main().
>
> And as you hinted, if some global data or functions in it turns out
> to be useful for standalone binaries, their definitions must migrate
> out of buitlin/*.c to ./*.c files, because standalone binaries with
> their own main() definition cannot be linked with builtin/*.o, the
> latter of which requires to be linked with git.o with its own main().
> ...
> The rationale behind libgit.a was so that make targets for the
> standalone binaries (note: all of them were standalone in the
> beginning) do not have to list *.o files that each of them needs to
> be linked with.  It was primary done as a convenient way to have the
> linker figure out the dependency and link only what was needed.

For the particular case of trying to make sequencer.o, which does
not currently have dependencies on builtin/*.o, depend on something
that is in builtin/notes.o, the link phase of standalone that wants
anything from revision.o (which is pretty much everything ;-) goes
like this:

        upload-pack.c   wants handle_revision_opt etc.
        revision.c      provides handle_revision_opt
                        wants name_decoration etc.
        log-tree.c      provides name_decoration
                        wants append_signoff
        sequencer.c     provides append_signoff

So sequencer.o _is_ meant to be usable from standalone and belongs
to libgit.a

If sequencer.o wants to call init_copy_notes_for_rewrite() and its
friends [*1*] that are currently in builtin/notes.o, first the
called function(s) should be moved outside builtin/notes.o to
notes.o or somewhere more library-ish place to be included in
libgit.a, which is meant to be usable from standalone.


[Footnote]

*1* ... which is a very reasonable thing to do.  But moving
    sequencer.o to builtin/sequencer.o is *not* the way to do this.

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

[Felipe Contreras]

On Mon, Jun 10, 2013 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:

> For the particular case of trying to make sequencer.o, which does
> not currently have dependencies on builtin/*.o, depend on something
> that is in builtin/notes.o, the link phase of standalone that wants
> anything from revision.o (which is pretty much everything ;-) goes
> like this:
>
>         upload-pack.c   wants handle_revision_opt etc.
>         revision.c      provides handle_revision_opt
>                         wants name_decoration etc.
>         log-tree.c      provides name_decoration
>                         wants append_signoff
>         sequencer.c     provides append_signoff
>
> So sequencer.o _is_ meant to be usable from standalone and belongs
> to libgit.a

Not after my patch. It moves append_signoff *out* of sequencer, which
in fact has nothing to do with the sequencer in the first place.

> If sequencer.o wants to call init_copy_notes_for_rewrite() and its
> friends [*1*] that are currently in builtin/notes.o, first the
> called function(s) should be moved outside builtin/notes.o to
> notes.o or somewhere more library-ish place to be included in
> libgit.a, which is meant to be usable from standalone.
>
>
> [Footnote]
>
> *1* ... which is a very reasonable thing to do.  But moving
>     sequencer.o to builtin/sequencer.o is *not* the way to do this.

By now we all know what is the *CURRENT* way to do this; in other
words, the status quo, which is BTW all messed up, because builtin/*.o
objects depend on each other already.

We are discussing the way it *SHOULD* be. Why aren't you leaning on that?

-- 
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> *1* ... which is a very reasonable thing to do.  But moving
>>     sequencer.o to builtin/sequencer.o is *not* the way to do this.
>
> By now we all know what is the *CURRENT* way to do this; in other
> words, the status quo, which is BTW all messed up, because builtin/*.o
> objects depend on each other already.

builtin/*.o are allowed to depend on each other.  They are by
definition builtins, meant to be linked into a single binary.

> We are discussing the way it *SHOULD* be. Why aren't you leaning on that?

And I do not see the reason why builtin/*.o should not depend on
each other.  It is not messed up at all.  They are meant to be
linked into a single binary---that is what being "built-in" is.

A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o
by moving parts that do not have to be in the single "git" binary
but are also usable in standalone binaries out of them.

And that is what I just suggested.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> And I do not see the reason why builtin/*.o should not depend on
> each other.  It is not messed up at all.  They are meant to be
> linked into a single binary---that is what being "built-in" is.
>
> A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o
> by moving parts that do not have to be in the single "git" binary
> but are also usable in standalone binaries out of them.

Actually, as long as these pieces are currently used by builtins,
moving them (e.g. init_copy_notes_for_rewrite()) out of builtin/*.o 
will not make these parts not to be in the single "git" binary at
all, so the above is grossly misstated.

 - There may be pieces of usefully reusable code buried in
   builtin/*.o;

 - By definition, any code (piece of data or function definition) in
   builtin/*.o cannot be used in standalone binaries, because all of
   builtin/*.o expect to link with git.o and expect their cmd_foo()
   getting called from main in it;

 - By moving the useful reusable pieces ont of builtin/*.o and
   adding them to libgit.a, these pieces become usable from
   standalone binaries as well.

And that is the reason why slimming builtin/*.o is the way it
*SHOULD* be.

Another thing to think about is looking at pieces of data and
functions defined in each *.o files and moving things around within
them.  For example, looking at the dependency chain I quoted earlier
for sequencer.o to build upload-pack, which is about responding to
"git fetch" on the sending side:

        upload-pack.c   wants handle_revision_opt etc.
        revision.c      provides handle_revision_opt
                        wants name_decoration etc.
        log-tree.c      provides name_decoration
                        wants append_signoff
        sequencer.c     provides append_signoff

It is already crazy. There is no reason for the pack sender to be
linking with the sequencer interpreter machinery. If the function
definition (and possibly other ones) are split into separate source
files (still in libgit.a), git-upload-pack binary does not have to
pull in the whole sequencer.c at all.

Coming back to the categorization Peff earlier made in the thread, I
think I am OK with adding new two subdirectories to the root level,
i.e.

    builtin/	- the ones that define cmd_foo()
    commands/   - the ones that has main() for standalone commands
    libsrc/     - the ones that go in libgit.a

We may also want to add another subdirectory to hold scripted
Porcelains, but the primary topic of this thread is what to do about
the C library, so it is orthogonal in that sense, but if we were to
go in the "group things in subdirectories to slim the root level"
direction, it may be worth considering doing so at the same time.

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

[Felipe Contreras]

On Mon, Jun 10, 2013 at 7:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> *1* ... which is a very reasonable thing to do.  But moving
>>>     sequencer.o to builtin/sequencer.o is *not* the way to do this.
>>
>> By now we all know what is the *CURRENT* way to do this; in other
>> words, the status quo, which is BTW all messed up, because builtin/*.o
>> objects depend on each other already.
>
> builtin/*.o are allowed to depend on each other.  They are by
> definition builtins, meant to be linked into a single binary.

That's not what John Keeping said[1]. I'm going to assume he was
wrong, but I don't think that's relevant for my point.

Either way, the meaning of builtin/ should probably be explained somewhere.

>> We are discussing the way it *SHOULD* be. Why aren't you leaning on that?
>
> And I do not see the reason why builtin/*.o should not depend on
> each other.  It is not messed up at all.  They are meant to be
> linked into a single binary---that is what being "built-in" is.
>
> A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o
> by moving parts that do not have to be in the single "git" binary
> but are also usable in standalone binaries out of them.
>
> And that is what I just suggested.

But init_copy_notes_for_rewrite() can *not* be used by anything other
than git builtins. Standalone binaries will never use such a function,
therefore it doesn't belong in libgit.a. Another example is
alias_lookup(). They belong in builtin/lib.a.

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

-- 
Felipe Contreras

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

[Felipe Contreras]

On Mon, Jun 10, 2013 at 8:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> And I do not see the reason why builtin/*.o should not depend on
>> each other.  It is not messed up at all.  They are meant to be
>> linked into a single binary---that is what being "built-in" is.
>>
>> A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o
>> by moving parts that do not have to be in the single "git" binary
>> but are also usable in standalone binaries out of them.
>
> Actually, as long as these pieces are currently used by builtins,
> moving them (e.g. init_copy_notes_for_rewrite()) out of builtin/*.o
> will not make these parts not to be in the single "git" binary at
> all, so the above is grossly misstated.
>
>  - There may be pieces of usefully reusable code buried in
>    builtin/*.o;
>
>  - By definition, any code (piece of data or function definition) in
>    builtin/*.o cannot be used in standalone binaries, because all of
>    builtin/*.o expect to link with git.o and expect their cmd_foo()
>    getting called from main in it;
>
>  - By moving the useful reusable pieces ont of builtin/*.o and
>    adding them to libgit.a, these pieces become usable from
>    standalone binaries as well.

What if these reusable pieces should not be used by standalone binaries?

> And that is the reason why slimming builtin/*.o is the way it
> *SHOULD* be.
>
> Another thing to think about is looking at pieces of data and
> functions defined in each *.o files and moving things around within
> them.  For example, looking at the dependency chain I quoted earlier
> for sequencer.o to build upload-pack, which is about responding to
> "git fetch" on the sending side:
>
>         upload-pack.c   wants handle_revision_opt etc.
>         revision.c      provides handle_revision_opt
>                         wants name_decoration etc.
>         log-tree.c      provides name_decoration
>                         wants append_signoff
>         sequencer.c     provides append_signoff
>
> It is already crazy. There is no reason for the pack sender to be
> linking with the sequencer interpreter machinery. If the function
> definition (and possibly other ones) are split into separate source
> files (still in libgit.a), git-upload-pack binary does not have to
> pull in the whole sequencer.c at all.

Agreed, which is precisely why my patches move that code out of
sequencer.c. Maybe log-tree.c is not the right destination, but it is
a step in the right direction.

> Coming back to the categorization Peff earlier made in the thread, I
> think I am OK with adding new two subdirectories to the root level,
> i.e.
>
>     builtin/    - the ones that define cmd_foo()

As is the case right now.

>     commands/   - the ones that has main() for standalone commands

Good.

>     libsrc/     - the ones that go in libgit.a

lib/ is probably descriptive enough.

But this doesn't answer the question; what about code that is shared
between builtins, but cannot be used by standalone programs?

I'd wager it belongs to builtin/ and should be linked to
builtin/lib.a. Maybe you would like to have a separate builtin/lib/
directory, but I think that's overkill.

> We may also want to add another subdirectory to hold scripted
> Porcelains, but the primary topic of this thread is what to do about
> the C library, so it is orthogonal in that sense, but if we were to
> go in the "group things in subdirectories to slim the root level"
> direction, it may be worth considering doing so at the same time.

Agreed. Plus there's completions, shell prompt, and other script-like
tools that shouldn't really belong in contrib/, and probably installed
by default.

-- 
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>>  - There may be pieces of usefully reusable code buried in
>>    builtin/*.o;
>>
>>  - By definition, any code (piece of data or function definition) in
>>    builtin/*.o cannot be used in standalone binaries, because all of
>>    builtin/*.o expect to link with git.o and expect their cmd_foo()
>>    getting called from main in it;
>>
>>  - By moving the useful reusable pieces ont of builtin/*.o and
>>    adding them to libgit.a, these pieces become usable from
>>    standalone binaries as well.
>
> What if these reusable pieces should not be used by standalone binaries?

I am not sure what you mean.  A piece is either reusable or not.
When would one piece _be_ reusable and should *not* be used in one
context but not in another?

There are distinctions between being "useful" and "usable", but I
think the zeroth order of approximation when thinking about this is
to think builtin/*.o as set of subroutines called by git.c::main().

These set of subroutines may call out to more generic helper
functions that are usable from anywhere both within builtins and
also from standalone.  They may also call to their own helper
functions that were originally designed to support only their use
by the original caller from somewhere in builtin/*.o (most commonly
in the same file, marked as static).

The general direction, if we want to have an improve libgit.a,
should be to see if the functions and their data that are private
to builtin/*.o can be used from standalone, either as they are or
with more generalization, and turn them from helpers specific to one
cmd_foo() into more generally useful library-ish functions.

There may be pieces in the callchain from that entry point to
cmd_foo() that are implementation details of git.c::main(); for
example the loop that does command dispatching to check with
builtins, external commands that begin with git-, and aliases, is
one of them, and would not be usable (nor it is useful) outside the
context of "git" aggregate binary.  But there are things that ought
to be usable that are currently in builtin/*.o, which prevents them
from being used by standalone binaries.  If a remote helper binary
that is standalone wants to call "create_note()", it is not
sufficient to make it non-static in builtin/notes.o, for example.

But if it is moved outside builtin/notes.o, it becomes usable.

I think the "git notes", being one of the most recent additions,
haven't gone through enough round of refactoring to come to the best
separation between library-ish part (i.e. could be in notes.o, even
though it is mostly about the underlying data structure manipulation
and contains no higher-level operations like actually creating and
copying, which might want to be in a separate source notes-lib.o)
and its CLI implementation "builtin/notes.o".

> But this doesn't answer the question; what about code that is shared
> between builtins, but cannot be used by standalone programs?

Again, I do not know what you mean by "cannot" here.  My tentative
answer to that question is "the eventual goal should be not to have
any code in that class, and that is a reasonable goal we can achieve
once we refactor what ought to be reusable out of builtin/*.o".

What are the examples you have in mind, code that we want to forbid
standalone from using?

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

[Felipe Contreras]

On Tue, Jun 11, 2013 at 12:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>>  - There may be pieces of usefully reusable code buried in
>>>    builtin/*.o;
>>>
>>>  - By definition, any code (piece of data or function definition) in
>>>    builtin/*.o cannot be used in standalone binaries, because all of
>>>    builtin/*.o expect to link with git.o and expect their cmd_foo()
>>>    getting called from main in it;
>>>
>>>  - By moving the useful reusable pieces ont of builtin/*.o and
>>>    adding them to libgit.a, these pieces become usable from
>>>    standalone binaries as well.
>>
>> What if these reusable pieces should not be used by standalone binaries?
>
> I am not sure what you mean.  A piece is either reusable or not.

It can be reusable for A, but not for B. A being the 'git' binary, B
being other standalone binaries.

>> But this doesn't answer the question; what about code that is shared
>> between builtins, but cannot be used by standalone programs?
>
> Again, I do not know what you mean by "cannot" here.  My tentative
> answer to that question is "the eventual goal should be not to have
> any code in that class, and that is a reasonable goal we can achieve
> once we refactor what ought to be reusable out of builtin/*.o".
>
> What are the examples you have in mind, code that we want to forbid
> standalone from using?

init_copy_notes_for_rewrite(). Nothing outside the 'git' binary would
need that. If you disagree, show me an example.

-- 
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> What are the examples you have in mind, code that we want to forbid
>> standalone from using?
>
> init_copy_notes_for_rewrite(). Nothing outside the 'git' binary would
> need that. If you disagree, show me an example.

"Nothing would need that", if you are talking about the current
codebase, I would agree that nothing would link to it.

But that is not a good justification for closing door to others that
come later who may want to have a standalone that would want to use
it.  Think about rewriting filter-branch.sh in C but not as a
built-in, for example.

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

[Felipe Contreras]

On Tue, Jun 11, 2013 at 12:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> What are the examples you have in mind, code that we want to forbid
>>> standalone from using?
>>
>> init_copy_notes_for_rewrite(). Nothing outside the 'git' binary would
>> need that. If you disagree, show me an example.
>
> "Nothing would need that", if you are talking about the current
> codebase, I would agree that nothing would link to it.
>
> But that is not a good justification for closing door to others that
> come later who may want to have a standalone that would want to use
> it.  Think about rewriting filter-branch.sh in C but not as a
> built-in, for example.

Why would anybody rewrite filter-branch, and not make it a builtin? It
should be a builtin. That's the whole point of builtins.

Moreover, if you are going to argue that we shouldn't be closing the
door, then why not link ./builtin/*.o to libgit.a? If you are
seriously considering the highly unlikely hypothetical standalone
git-filter-branch scenario, you should consider the even more likely
scenario where somebody needs to access code from ./builtin/*.o; that
scenario is not even hypothetical, we know it's happened multiple
times, and we know it's going to happen again.

-- 
Felipe Contreras

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

[Linus Torvalds]

On Tue, Jun 11, 2013 at 11:06 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Moreover, if you are going to argue that we shouldn't be closing the
> door [...]

Felipe, you saying "if you are going to argue ..." to anybody else is
kind of ironic.

Why is it every thread I see you in, you're being a dick and arguing
for some theoretical thing that nobody else cares about?

This whole thread has been one long argument about totally pointless
things that wouldn't improve anything one way or the other. It's
bikeshedding of the worst kind. Just let it go.

              Linus

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Moreover, if you are going to argue that we shouldn't be closing the
> door, then why not link ./builtin/*.o to libgit.a?

Huh?  It does not make any sense.  builtin/*.o files have cmd_foo()
that are expected to be called from git.c::main(), but libgit.a
files are linked with no constraints whose main() they are linking
with.

> If you are
> seriously considering the highly unlikely hypothetical standalone
> git-filter-branch scenario, you should consider the even more likely
> scenario where somebody needs to access code from ./builtin/*.o; that
> scenario is not even hypothetical, we know it's happened multiple
> times, and we know it's going to happen again.

That is exactly why I said that builtin/*.o should be refactored to
pick "does not have to be in builtin" bits, which will result in a
better division of labor.  Reusable bits should live in the library,
while a particular implementation of command remain in builtin/*
that utilize the reusable bits.

You still haven't justified why we have to _forbid_ any outside
callers from calling copy_notes_for_rewrite().

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

[Felipe Contreras]

On Tue, Jun 11, 2013 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Moreover, if you are going to argue that we shouldn't be closing the
>> door, then why not link ./builtin/*.o to libgit.a?
>
> Huh?  It does not make any sense.  builtin/*.o files have cmd_foo()
> that are expected to be called from git.c::main(), but libgit.a
> files are linked with no constraints whose main() they are linking
> with.


>> If you are
>> seriously considering the highly unlikely hypothetical standalone
>> git-filter-branch scenario, you should consider the even more likely
>> scenario where somebody needs to access code from ./builtin/*.o; that
>> scenario is not even hypothetical, we know it's happened multiple
>> times, and we know it's going to happen again.
>
> That is exactly why I said that builtin/*.o should be refactored to
> pick "does not have to be in builtin" bits, which will result in a
> better division of labor.  Reusable bits should live in the library,
> while a particular implementation of command remain in builtin/*
> that utilize the reusable bits.
>
> You still haven't justified why we have to _forbid_ any outside
> callers from calling copy_notes_for_rewrite().

Because only builtins _should_ use it. I asked you for an example, and
you said a hypothetical standalone 'git-filter-branch' might use it,
but you have not explained why it should be standalone, when it's
clear it should be a builtin.

If we assume your argument is valid for the hypothetical
'git-filter-branch', if that's the case, then it would be even more
reasonable to assume that there will be other standalone binaries that
would want to use all sort of functions from ./builtin/*.o. Let's put
an example: reset_index(). Some standalone program wants to use that
function. What do you we do?

The shortest route is to make it non-static, and add it to builtin.h.
But that would not be enough, we need the infrastructure prepared for
that; link libgit.a with ./builtin/*.o.

I don't think that's the way to go, but your line of argumentation
leads directly there; if we are worrying about anything that any
potential standalone program could want, then we should worry about
reset_index() not being easily accessible to them.

IMO we should be clear and say no; standalone programs should not
access copy_notes_for_rewrite(), that's for builtins. If we move all
the code that potential standalone programs could want to libgit.a, it
wouldn't be a library at all, and it would basically contain
everything.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Tue, Jun 11, 2013 at 1:14 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jun 11, 2013 at 11:06 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>>
>> Moreover, if you are going to argue that we shouldn't be closing the
>> door [...]
>
> Felipe, you saying "if you are going to argue ..." to anybody else is
> kind of ironic.

Supposing the other side's argument is correct is a standard
discussing technique.

> Why is it every thread I see you in, you're being a dick and arguing
> for some theoretical thing that nobody else cares about?

I don't know. I've sent 800 patches in the last three months (patches,
not email comments), and you pick this one to reply to. Maybe because
you enjoy insulting people?

> This whole thread has been one long argument about totally pointless
> things that wouldn't improve anything one way or the other. It's
> bikeshedding of the worst kind. Just let it go.

Why don't you ask Junio to let it go? If it's irrelevant, than it
doesn't matter if this patch is applied or not. You say it's
bike-shedding, that implies that Junio likes red, and I like blue, and
both are equally useless. So let's go for blue then.

Presumably Junio doesn't agree with you, he does truly think it should
be red, in fact, he doesn't think it's just a color, it's something
important, and I agree, and apparently other people in the mailing
list also agree, as most of them have voiced their opinion that red is
the color.

Now, do you have something of value to say which of the two options
should be, or are you just going to engage in double standards and
personal attacks?

If you truly think this is bikeshedding, at least be fair and complain
about that to the people that argue for red, not just the ones that
argue for blue.

-- 
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Tue, Jun 11, 2013 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Moreover, if you are going to argue that we shouldn't be closing the
>>> door, then why not link ./builtin/*.o to libgit.a?
>>
>> Huh?  It does not make any sense.  builtin/*.o files have cmd_foo()
>> that are expected to be called from git.c::main(), but libgit.a
>> files are linked with no constraints whose main() they are linking
>> with.
> ...
>> That is exactly why I said that builtin/*.o should be refactored to
>> pick "does not have to be in builtin" bits, which will result in a
>> better division of labor.  Reusable bits should live in the library,
>> while a particular implementation of command remain in builtin/*
>> that utilize the reusable bits.
>>
>> You still haven't justified why we have to _forbid_ any outside
>> callers from calling copy_notes_for_rewrite().
>
> Because only builtins _should_ use it.

And there is no justification behind that "_should_" claim; you are
not making any technical argument to explain it.

> I asked you for an example, and
> you said a hypothetical standalone 'git-filter-branch' might use it,

Of course it has to be hypothetical; I already said with the current
code no standalone does use it---it is not arranged to be doable so
there is no user.  If you want to have examples of future possible
callers, they have to be hypothetical---the future by definition
hasn't happened.  But that does not mean hypothetical is impractical
nor useless.

There are out-of-tree programs like cgit that will not be built-in
but already link with libgit.a.  Moving things that can be used by
outside people out of builtin/*.o to libgit.a would allow uses that
you and I cannot imagine offhand.  I do not see a reason for us to
forbid a filter-branch replacement out of tree as a standalone.

I do not see a point in continuing to discuss this (or any design
level issues) with you.  You seem to go into a wrong direction to
break the design of the overall system, not in a direction to
improve anything.  I do not know, and at this point I do not care,
if you are doing so deliberately to sabotage Git.  Just stop.

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

[Felipe Contreras]

On Tue, Jun 11, 2013 at 2:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Tue, Jun 11, 2013 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> Moreover, if you are going to argue that we shouldn't be closing the
>>>> door, then why not link ./builtin/*.o to libgit.a?
>>>
>>> Huh?  It does not make any sense.  builtin/*.o files have cmd_foo()
>>> that are expected to be called from git.c::main(), but libgit.a
>>> files are linked with no constraints whose main() they are linking
>>> with.
>> ...
>>> That is exactly why I said that builtin/*.o should be refactored to
>>> pick "does not have to be in builtin" bits, which will result in a
>>> better division of labor.  Reusable bits should live in the library,
>>> while a particular implementation of command remain in builtin/*
>>> that utilize the reusable bits.
>>>
>>> You still haven't justified why we have to _forbid_ any outside
>>> callers from calling copy_notes_for_rewrite().
>>
>> Because only builtins _should_ use it.
>
> And there is no justification behind that "_should_" claim; you are
> not making any technical argument to explain it.

The first argument of init_copy_notes_for_rewrite() is the name of the
builtin command. There hardly could be any more justification.

>> I asked you for an example, and
>> you said a hypothetical standalone 'git-filter-branch' might use it,
>
> Of course it has to be hypothetical; I already said with the current
> code no standalone does use it---it is not arranged to be doable so
> there is no user.  If you want to have examples of future possible
> callers, they have to be hypothetical---the future by definition
> hasn't happened.  But that does not mean hypothetical is impractical
> nor useless.

So? It's still hypothetical, which is what I said. What are you
complaining about? About the fact that I made a correct assessment?

> There are out-of-tree programs like cgit that will not be built-in
> but already link with libgit.a.  Moving things that can be used by
> outside people out of builtin/*.o to libgit.a would allow uses that
> you and I cannot imagine offhand.  I do not see a reason for us to
> forbid a filter-branch replacement out of tree as a standalone.

Yeah, I already ran that argument, and you conveniently chose to
escape the next logical conclusion that I already put forward:

--- a/Makefile
+++ b/Makefile
@@ -990,6 +990,8 @@ BUILTIN_OBJS += builtin/verify-pack.o
 BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/write-tree.o

+LIB_OBJS += $(BUILTIN_OBJS)
+
 GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
 EXTLIBS =

I don't think that's the right direction.

> I do not see a point in continuing to discuss this (or any design
> level issues) with you.  You seem to go into a wrong direction to
> break the design of the overall system, not in a direction to
> improve anything.  I do not know, and at this point I do not care,
> if you are doing so deliberately to sabotage Git.  Just stop.

That's your opinion, and it's not shared by others (outside of the Git
project). If you were right and Git was moving in the right direction,
there would be no need for libgit2.

-- 
Felipe Contreras

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> This whole thread has been one long argument about totally pointless
> things that wouldn't improve anything one way or the other. It's
> bikeshedding of the worst kind. Just let it go.

The proposal to move sequencer.c to builtins/sequencer.c and then
adding a filter in Makefile to exclude so that "git-sequencer" is
not built is "it wouldn't improve anything one way or the other".
It is to throw in something into a set to which it does not belong,
and then working around that mistake with another kludge.

The problem that triggered the wrong solution actually is real,
however.

A function that sequencer.c (in libgit.a so that it could be used by
standalone) may want to use in the future currently lives in
builtin/notes.c.  If you add a call to that function to sequencer.c
without doing anything else, standalones like git-upload-pack will
stop linking correctly.  The git-upload-pack wants the revision
traversal machinery in revision.o, which in turn wants to be able to
see log-tree.o, which in turn wants to link with sequencer.o to see
one global variable (there may be other dependencies).  All of these
objects are currently in libgit.a so that both builtins and standalones
can use them.

Moving sequencer.c to builtin/ is not even a solution.  Linking
git-upload-pack will still pull in builtin/notes.o along with
cmd_notes(), which is not called from main(); as you remember,
cmd_foo() in all builtin/*.o are designed to be called from
git.c::main().

There is only one right solution.  If a useful function is buried in
builtin/*.o as a historical accident (i.e. it started its life as a
helper for that particular command, and nobody else used it from
outside so far) and that makes it impossible to use the function
from outside builtin/*.o, refactor the function and its callers and
move it to libgit.a.

So I do not think this is not even a bikeshedding.  Just one side
being right, and the other side continuing to repeat nonsense
without listening.

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

[Felipe Contreras]

On Tue, Jun 11, 2013 at 2:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> This whole thread has been one long argument about totally pointless
>> things that wouldn't improve anything one way or the other. It's
>> bikeshedding of the worst kind. Just let it go.
>
> The proposal to move sequencer.c to builtins/sequencer.c and then
> adding a filter in Makefile to exclude so that "git-sequencer" is
> not built is "it wouldn't improve anything one way or the other".
> It is to throw in something into a set to which it does not belong,

In your opinion.

> and then working around that mistake with another kludge.

In your opinion.

You continually use absolutist rhetoric to try to convince yourself
and others that what you say is absolute 100% fact. But it's not, it's
your opinion.

> So I do not think this is not even a bikeshedding.  Just one side
> being right, and the other side continuing to repeat nonsense
> without listening.

George W. Bush said history would prove him right, but saying so
doesn't make it so. At least he had the decency to acknowledge that
other people had different valid opinions.

builtin/lib.a makes perfect sense, and it's the first logical step in
moving libgit.a towards libgit2.

-- 
Felipe Contreras

[PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]

[Johan Herland]

> There is only one right solution.  If a useful function is buried in
> builtin/*.o as a historical accident (i.e. it started its life as a
> helper for that particular command, and nobody else used it from
> outside so far) and that makes it impossible to use the function
> from outside builtin/*.o, refactor the function and its callers and
> move it to libgit.a.

Here goes...

...Johan

Johan Herland (3):
  finish_copy_notes_for_rewrite(): Let caller provide commit message
  Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c
  Move create_notes_commit() from notes-merge.c into notes-utils.c

 Makefile         |   2 +
 builtin.h        |  16 ------
 builtin/commit.c |   3 +-
 builtin/notes.c  | 136 ++---------------------------------------------
 notes-merge.c    |  27 +---------
 notes-merge.h    |  14 -----
 notes-utils.c    | 157 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 notes-utils.h    |  37 +++++++++++++
 8 files changed, 203 insertions(+), 189 deletions(-)
 create mode 100644 notes-utils.c
 create mode 100644 notes-utils.h

-- 
1.8.1.3.704.g33f7d4f

[PATCH 1/3] finish_copy_notes_for_rewrite(): Let caller provide commit message

[Johan Herland]

When copying notes for a rewritten object, the resulting notes commit
would have the following hardcoded commit message:

  Notes added by 'git notes copy'

This is obviously bogus when the notes rewriting is performed by
'git commit --amend'.

Therefore, let the caller specify an appropriate notes commit message
instead of hardcoding it. The above message is used for 'git notes copy',
but when calling finish_copy_notes_for_rewrite() from builtin/commit.c,
we use the following message instead:

  Notes added by 'git commit --amend'

Cc: Thomas Rast <trast@inf.ethz.ch>
Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin.h        | 2 +-
 builtin/commit.c | 2 +-
 builtin/notes.c  | 9 +++++----
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin.h b/builtin.h
index faef559..78fb14d 100644
--- a/builtin.h
+++ b/builtin.h
@@ -36,7 +36,7 @@ struct notes_rewrite_cfg {
 struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd);
 int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
 			  const unsigned char *from_obj, const unsigned char *to_obj);
-void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c);
+void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c, const char *msg);
 
 extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, int sha1_valid, char **buf, unsigned long *buf_size);
 
diff --git a/builtin/commit.c b/builtin/commit.c
index d2f30d9..f8df8ca 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1591,7 +1591,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		if (cfg) {
 			/* we are amending, so current_head is not NULL */
 			copy_note_for_rewrite(cfg, current_head->object.sha1, sha1);
-			finish_copy_notes_for_rewrite(cfg);
+			finish_copy_notes_for_rewrite(cfg, "Notes added by 'git commit --amend'");
 		}
 		run_rewrite_hook(current_head->object.sha1, sha1);
 	}
diff --git a/builtin/notes.c b/builtin/notes.c
index 57748a6..6a80714 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -403,11 +403,11 @@ int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
 	return ret;
 }
 
-void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c)
+void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c, const char *msg)
 {
 	int i;
 	for (i = 0; c->trees[i]; i++) {
-		commit_notes(c->trees[i], "Notes added by 'git notes copy'");
+		commit_notes(c->trees[i], msg);
 		free_notes(c->trees[i]);
 	}
 	free(c->trees);
@@ -420,6 +420,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 	struct notes_rewrite_cfg *c = NULL;
 	struct notes_tree *t = NULL;
 	int ret = 0;
+	const char *msg = "Notes added by 'git notes copy'";
 
 	if (rewrite_cmd) {
 		c = init_copy_notes_for_rewrite(rewrite_cmd);
@@ -461,10 +462,10 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 	}
 
 	if (!rewrite_cmd) {
-		commit_notes(t, "Notes added by 'git notes copy'");
+		commit_notes(t, msg);
 		free_notes(t);
 	} else {
-		finish_copy_notes_for_rewrite(c);
+		finish_copy_notes_for_rewrite(c, msg);
 	}
 	return ret;
 }
-- 
1.8.1.3.704.g33f7d4f

[PATCH 2/3] Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c

[Johan Herland]

This is a pure code movement of the machinery for copying notes to
rewritten objects. This code was located in builtin/notes.c for
historical reasons. In order to make it available to builtin/commit.c
it was declared in builtin.h. This was more of an accident of history
than a concious design, and we now want to make this machinery more
widely available.

Hence, this patch moves the code into the new notes-utils.[hc] files
which are included into libgit.a. Except for adjusting #includes
accordingly, this patch merely moves the relevant functions verbatim
into the new files.

Cc: Thomas Rast <trast@inf.ethz.ch>
Signed-off-by: Johan Herland <johan@herland.net>
---
 Makefile         |   2 +
 builtin.h        |  16 -------
 builtin/commit.c |   1 +
 builtin/notes.c  | 131 +-----------------------------------------------------
 notes-utils.c    | 132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 notes-utils.h    |  23 ++++++++++
 6 files changed, 159 insertions(+), 146 deletions(-)
 create mode 100644 notes-utils.c
 create mode 100644 notes-utils.h

diff --git a/Makefile b/Makefile
index 0f931a2..22deee1 100644
--- a/Makefile
+++ b/Makefile
@@ -682,6 +682,7 @@ LIB_H += merge-recursive.h
 LIB_H += mergesort.h
 LIB_H += notes-cache.h
 LIB_H += notes-merge.h
+LIB_H += notes-utils.h
 LIB_H += notes.h
 LIB_H += object.h
 LIB_H += pack-refs.h
@@ -815,6 +816,7 @@ LIB_OBJS += name-hash.o
 LIB_OBJS += notes.o
 LIB_OBJS += notes-cache.o
 LIB_OBJS += notes-merge.o
+LIB_OBJS += notes-utils.o
 LIB_OBJS += object.o
 LIB_OBJS += pack-check.o
 LIB_OBJS += pack-refs.o
diff --git a/builtin.h b/builtin.h
index 78fb14d..72bb2a8 100644
--- a/builtin.h
+++ b/builtin.h
@@ -5,7 +5,6 @@
 #include "strbuf.h"
 #include "cache.h"
 #include "commit.h"
-#include "notes.h"
 
 #define DEFAULT_MERGE_LOG_LEN 20
 
@@ -23,21 +22,6 @@ struct fmt_merge_msg_opts {
 extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 			 struct fmt_merge_msg_opts *);
 
-struct notes_rewrite_cfg {
-	struct notes_tree **trees;
-	const char *cmd;
-	int enabled;
-	combine_notes_fn combine;
-	struct string_list *refs;
-	int refs_from_env;
-	int mode_from_env;
-};
-
-struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd);
-int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
-			  const unsigned char *from_obj, const unsigned char *to_obj);
-void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c, const char *msg);
-
 extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, int sha1_valid, char **buf, unsigned long *buf_size);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
diff --git a/builtin/commit.c b/builtin/commit.c
index f8df8ca..ce40176 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -29,6 +29,7 @@
 #include "gpg-interface.h"
 #include "column.h"
 #include "sequencer.h"
+#include "notes-utils.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [options] [--] <pathspec>..."),
diff --git a/builtin/notes.c b/builtin/notes.c
index 6a80714..9ed2508 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -18,9 +18,7 @@
 #include "parse-options.h"
 #include "string-list.h"
 #include "notes-merge.h"
-
-static void commit_notes(struct notes_tree *t, const char *msg);
-static combine_notes_fn parse_combine_notes_fn(const char *v);
+#include "notes-utils.h"
 
 static const char * const git_notes_usage[] = {
 	N_("git notes [--ref <notes_ref>] [list [<object>]]"),
@@ -287,133 +285,6 @@ static int parse_reedit_arg(const struct option *opt, const char *arg, int unset
 	return parse_reuse_arg(opt, arg, unset);
 }
 
-static void commit_notes(struct notes_tree *t, const char *msg)
-{
-	struct strbuf buf = STRBUF_INIT;
-	unsigned char commit_sha1[20];
-
-	if (!t)
-		t = &default_notes_tree;
-	if (!t->initialized || !t->ref || !*t->ref)
-		die(_("Cannot commit uninitialized/unreferenced notes tree"));
-	if (!t->dirty)
-		return; /* don't have to commit an unchanged tree */
-
-	/* Prepare commit message and reflog message */
-	strbuf_addstr(&buf, msg);
-	if (buf.buf[buf.len - 1] != '\n')
-		strbuf_addch(&buf, '\n'); /* Make sure msg ends with newline */
-
-	create_notes_commit(t, NULL, &buf, commit_sha1);
-	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
-	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR);
-
-	strbuf_release(&buf);
-}
-
-static combine_notes_fn parse_combine_notes_fn(const char *v)
-{
-	if (!strcasecmp(v, "overwrite"))
-		return combine_notes_overwrite;
-	else if (!strcasecmp(v, "ignore"))
-		return combine_notes_ignore;
-	else if (!strcasecmp(v, "concatenate"))
-		return combine_notes_concatenate;
-	else if (!strcasecmp(v, "cat_sort_uniq"))
-		return combine_notes_cat_sort_uniq;
-	else
-		return NULL;
-}
-
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
-{
-	struct notes_rewrite_cfg *c = cb;
-	if (!prefixcmp(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
-		c->enabled = git_config_bool(k, v);
-		return 0;
-	} else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
-		if (!v)
-			config_error_nonbool(k);
-		c->combine = parse_combine_notes_fn(v);
-		if (!c->combine) {
-			error(_("Bad notes.rewriteMode value: '%s'"), v);
-			return 1;
-		}
-		return 0;
-	} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
-		/* note that a refs/ prefix is implied in the
-		 * underlying for_each_glob_ref */
-		if (!prefixcmp(v, "refs/notes/"))
-			string_list_add_refs_by_glob(c->refs, v);
-		else
-			warning(_("Refusing to rewrite notes in %s"
-				" (outside of refs/notes/)"), v);
-		return 0;
-	}
-
-	return 0;
-}
-
-
-struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
-{
-	struct notes_rewrite_cfg *c = xmalloc(sizeof(struct notes_rewrite_cfg));
-	const char *rewrite_mode_env = getenv(GIT_NOTES_REWRITE_MODE_ENVIRONMENT);
-	const char *rewrite_refs_env = getenv(GIT_NOTES_REWRITE_REF_ENVIRONMENT);
-	c->cmd = cmd;
-	c->enabled = 1;
-	c->combine = combine_notes_concatenate;
-	c->refs = xcalloc(1, sizeof(struct string_list));
-	c->refs->strdup_strings = 1;
-	c->refs_from_env = 0;
-	c->mode_from_env = 0;
-	if (rewrite_mode_env) {
-		c->mode_from_env = 1;
-		c->combine = parse_combine_notes_fn(rewrite_mode_env);
-		if (!c->combine)
-			/* TRANSLATORS: The first %s is the name of the
-			   environment variable, the second %s is its value */
-			error(_("Bad %s value: '%s'"), GIT_NOTES_REWRITE_MODE_ENVIRONMENT,
-					rewrite_mode_env);
-	}
-	if (rewrite_refs_env) {
-		c->refs_from_env = 1;
-		string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
-	}
-	git_config(notes_rewrite_config, c);
-	if (!c->enabled || !c->refs->nr) {
-		string_list_clear(c->refs, 0);
-		free(c->refs);
-		free(c);
-		return NULL;
-	}
-	c->trees = load_notes_trees(c->refs);
-	string_list_clear(c->refs, 0);
-	free(c->refs);
-	return c;
-}
-
-int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
-			  const unsigned char *from_obj, const unsigned char *to_obj)
-{
-	int ret = 0;
-	int i;
-	for (i = 0; c->trees[i]; i++)
-		ret = copy_note(c->trees[i], from_obj, to_obj, 1, c->combine) || ret;
-	return ret;
-}
-
-void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c, const char *msg)
-{
-	int i;
-	for (i = 0; c->trees[i]; i++) {
-		commit_notes(c->trees[i], msg);
-		free_notes(c->trees[i]);
-	}
-	free(c->trees);
-	free(c);
-}
-
 static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/notes-utils.c b/notes-utils.c
new file mode 100644
index 0000000..2ae5cb2
--- /dev/null
+++ b/notes-utils.c
@@ -0,0 +1,132 @@
+#include "cache.h"
+#include "commit.h"
+#include "refs.h"
+#include "notes-utils.h"
+#include "notes-merge.h" // need create_notes_commit()
+
+void commit_notes(struct notes_tree *t, const char *msg)
+{
+	struct strbuf buf = STRBUF_INIT;
+	unsigned char commit_sha1[20];
+
+	if (!t)
+		t = &default_notes_tree;
+	if (!t->initialized || !t->ref || !*t->ref)
+		die(_("Cannot commit uninitialized/unreferenced notes tree"));
+	if (!t->dirty)
+		return; /* don't have to commit an unchanged tree */
+
+	/* Prepare commit message and reflog message */
+	strbuf_addstr(&buf, msg);
+	if (buf.buf[buf.len - 1] != '\n')
+		strbuf_addch(&buf, '\n'); /* Make sure msg ends with newline */
+
+	create_notes_commit(t, NULL, &buf, commit_sha1);
+	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
+	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR);
+
+	strbuf_release(&buf);
+}
+
+static combine_notes_fn parse_combine_notes_fn(const char *v)
+{
+	if (!strcasecmp(v, "overwrite"))
+		return combine_notes_overwrite;
+	else if (!strcasecmp(v, "ignore"))
+		return combine_notes_ignore;
+	else if (!strcasecmp(v, "concatenate"))
+		return combine_notes_concatenate;
+	else if (!strcasecmp(v, "cat_sort_uniq"))
+		return combine_notes_cat_sort_uniq;
+	else
+		return NULL;
+}
+
+static int notes_rewrite_config(const char *k, const char *v, void *cb)
+{
+	struct notes_rewrite_cfg *c = cb;
+	if (!prefixcmp(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
+		c->enabled = git_config_bool(k, v);
+		return 0;
+	} else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
+		if (!v)
+			config_error_nonbool(k);
+		c->combine = parse_combine_notes_fn(v);
+		if (!c->combine) {
+			error(_("Bad notes.rewriteMode value: '%s'"), v);
+			return 1;
+		}
+		return 0;
+	} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
+		/* note that a refs/ prefix is implied in the
+		 * underlying for_each_glob_ref */
+		if (!prefixcmp(v, "refs/notes/"))
+			string_list_add_refs_by_glob(c->refs, v);
+		else
+			warning(_("Refusing to rewrite notes in %s"
+				" (outside of refs/notes/)"), v);
+		return 0;
+	}
+
+	return 0;
+}
+
+
+struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
+{
+	struct notes_rewrite_cfg *c = xmalloc(sizeof(struct notes_rewrite_cfg));
+	const char *rewrite_mode_env = getenv(GIT_NOTES_REWRITE_MODE_ENVIRONMENT);
+	const char *rewrite_refs_env = getenv(GIT_NOTES_REWRITE_REF_ENVIRONMENT);
+	c->cmd = cmd;
+	c->enabled = 1;
+	c->combine = combine_notes_concatenate;
+	c->refs = xcalloc(1, sizeof(struct string_list));
+	c->refs->strdup_strings = 1;
+	c->refs_from_env = 0;
+	c->mode_from_env = 0;
+	if (rewrite_mode_env) {
+		c->mode_from_env = 1;
+		c->combine = parse_combine_notes_fn(rewrite_mode_env);
+		if (!c->combine)
+			/* TRANSLATORS: The first %s is the name of the
+			   environment variable, the second %s is its value */
+			error(_("Bad %s value: '%s'"), GIT_NOTES_REWRITE_MODE_ENVIRONMENT,
+					rewrite_mode_env);
+	}
+	if (rewrite_refs_env) {
+		c->refs_from_env = 1;
+		string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
+	}
+	git_config(notes_rewrite_config, c);
+	if (!c->enabled || !c->refs->nr) {
+		string_list_clear(c->refs, 0);
+		free(c->refs);
+		free(c);
+		return NULL;
+	}
+	c->trees = load_notes_trees(c->refs);
+	string_list_clear(c->refs, 0);
+	free(c->refs);
+	return c;
+}
+
+int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
+			  const unsigned char *from_obj, const unsigned char *to_obj)
+{
+	int ret = 0;
+	int i;
+	for (i = 0; c->trees[i]; i++)
+		ret = copy_note(c->trees[i], from_obj, to_obj, 1, c->combine) || ret;
+	return ret;
+}
+
+void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c, const char *msg)
+{
+	int i;
+	for (i = 0; c->trees[i]; i++) {
+		commit_notes(c->trees[i], msg);
+		free_notes(c->trees[i]);
+	}
+	free(c->trees);
+	free(c);
+}
diff --git a/notes-utils.h b/notes-utils.h
new file mode 100644
index 0000000..0661e99
--- /dev/null
+++ b/notes-utils.h
@@ -0,0 +1,23 @@
+#ifndef NOTES_UTILS_H
+#define NOTES_UTILS_H
+
+#include "notes.h"
+
+void commit_notes(struct notes_tree *t, const char *msg);
+
+struct notes_rewrite_cfg {
+	struct notes_tree **trees;
+	const char *cmd;
+	int enabled;
+	combine_notes_fn combine;
+	struct string_list *refs;
+	int refs_from_env;
+	int mode_from_env;
+};
+
+struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd);
+int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
+			  const unsigned char *from_obj, const unsigned char *to_obj);
+void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c, const char *msg);
+
+#endif
-- 
1.8.1.3.704.g33f7d4f

[PATCH 3/3] Move create_notes_commit() from notes-merge.c into notes-utils.c

[Johan Herland]

create_notes_commit() is needed by both the notes-merge code, and by
commit_notes() in notes-utils. Since it is generally useful, and not
bound to the notes-merge machinery, we move it from (the more specific)
notes-merge to (the more general) notes-utils.

Signed-off-by: Johan Herland <johan@herland.net>
---
 notes-merge.c | 27 +--------------------------
 notes-merge.h | 14 --------------
 notes-utils.c | 27 ++++++++++++++++++++++++++-
 notes-utils.h | 14 ++++++++++++++
 4 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index 0f67bd3..ab18857 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -9,6 +9,7 @@
 #include "notes.h"
 #include "notes-merge.h"
 #include "strbuf.h"
+#include "notes-utils.h"
 
 struct notes_merge_pair {
 	unsigned char obj[20], base[20], local[20], remote[20];
@@ -530,32 +531,6 @@ static int merge_from_diffs(struct notes_merge_options *o,
 	return conflicts ? -1 : 1;
 }
 
-void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
-			 const struct strbuf *msg, unsigned char *result_sha1)
-{
-	unsigned char tree_sha1[20];
-
-	assert(t->initialized);
-
-	if (write_notes_tree(t, tree_sha1))
-		die("Failed to write notes tree to database");
-
-	if (!parents) {
-		/* Deduce parent commit from t->ref */
-		unsigned char parent_sha1[20];
-		if (!read_ref(t->ref, parent_sha1)) {
-			struct commit *parent = lookup_commit(parent_sha1);
-			if (!parent || parse_commit(parent))
-				die("Failed to find/parse commit %s", t->ref);
-			commit_list_insert(parent, &parents);
-		}
-		/* else: t->ref points to nothing, assume root/orphan commit */
-	}
-
-	if (commit_tree(msg, tree_sha1, parents, result_sha1, NULL, NULL))
-		die("Failed to commit notes tree to database");
-}
-
 int notes_merge(struct notes_merge_options *o,
 		struct notes_tree *local_tree,
 		unsigned char *result_sha1)
diff --git a/notes-merge.h b/notes-merge.h
index 0c11b17..1d01f6a 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -26,20 +26,6 @@ struct notes_merge_options {
 void init_notes_merge_options(struct notes_merge_options *o);
 
 /*
- * Create new notes commit from the given notes tree
- *
- * Properties of the created commit:
- * - tree: the result of converting t to a tree object with write_notes_tree().
- * - parents: the given parents OR (if NULL) the commit referenced by t->ref.
- * - author/committer: the default determined by commmit_tree().
- * - commit message: msg
- *
- * The resulting commit SHA1 is stored in result_sha1.
- */
-void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
-			 const struct strbuf *msg, unsigned char *result_sha1);
-
-/*
  * Merge notes from o->remote_ref into o->local_ref
  *
  * The given notes_tree 'local_tree' must be the notes_tree referenced by the
diff --git a/notes-utils.c b/notes-utils.c
index 2ae5cb2..9107c37 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -2,7 +2,32 @@
 #include "commit.h"
 #include "refs.h"
 #include "notes-utils.h"
-#include "notes-merge.h" // need create_notes_commit()
+
+void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
+			 const struct strbuf *msg, unsigned char *result_sha1)
+{
+	unsigned char tree_sha1[20];
+
+	assert(t->initialized);
+
+	if (write_notes_tree(t, tree_sha1))
+		die("Failed to write notes tree to database");
+
+	if (!parents) {
+		/* Deduce parent commit from t->ref */
+		unsigned char parent_sha1[20];
+		if (!read_ref(t->ref, parent_sha1)) {
+			struct commit *parent = lookup_commit(parent_sha1);
+			if (!parent || parse_commit(parent))
+				die("Failed to find/parse commit %s", t->ref);
+			commit_list_insert(parent, &parents);
+		}
+		/* else: t->ref points to nothing, assume root/orphan commit */
+	}
+
+	if (commit_tree(msg, tree_sha1, parents, result_sha1, NULL, NULL))
+		die("Failed to commit notes tree to database");
+}
 
 void commit_notes(struct notes_tree *t, const char *msg)
 {
diff --git a/notes-utils.h b/notes-utils.h
index 0661e99..b4cb1bf 100644
--- a/notes-utils.h
+++ b/notes-utils.h
@@ -3,6 +3,20 @@
 
 #include "notes.h"
 
+/*
+ * Create new notes commit from the given notes tree
+ *
+ * Properties of the created commit:
+ * - tree: the result of converting t to a tree object with write_notes_tree().
+ * - parents: the given parents OR (if NULL) the commit referenced by t->ref.
+ * - author/committer: the default determined by commmit_tree().
+ * - commit message: msg
+ *
+ * The resulting commit SHA1 is stored in result_sha1.
+ */
+void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
+			 const struct strbuf *msg, unsigned char *result_sha1);
+
 void commit_notes(struct notes_tree *t, const char *msg);
 
 struct notes_rewrite_cfg {
-- 
1.8.1.3.704.g33f7d4f

Re: [PATCH 2/3] Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c

[Felipe Contreras]

On Tue, Jun 11, 2013 at 7:13 PM, Johan Herland <johan@herland.net> wrote:
> This is a pure code movement of the machinery for copying notes to
> rewritten objects. This code was located in builtin/notes.c for
> historical reasons. In order to make it available to builtin/commit.c
> it was declared in builtin.h. This was more of an accident of history
> than a concious design, and we now want to make this machinery more
> widely available.
>
> Hence, this patch moves the code into the new notes-utils.[hc] files
> which are included into libgit.a. Except for adjusting #includes
> accordingly, this patch merely moves the relevant functions verbatim
> into the new files.
>
> Cc: Thomas Rast <trast@inf.ethz.ch>
> Signed-off-by: Johan Herland <johan@herland.net>

I wonder where you got that idea from. Did you come up with that out thin air?

And here goes my bet; nobody will ever use these notes-utils outside
of the git binary. Ever.

-- 
Felipe Contreras

Re: [PATCH 2/3] Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c

[Johan Herland]

On Wed, Jun 12, 2013 at 2:32 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Tue, Jun 11, 2013 at 7:13 PM, Johan Herland <johan@herland.net> wrote:
>> This is a pure code movement of the machinery for copying notes to
>> rewritten objects. This code was located in builtin/notes.c for
>> historical reasons. In order to make it available to builtin/commit.c
>> it was declared in builtin.h. This was more of an accident of history
>> than a concious design, and we now want to make this machinery more
>> widely available.
>>
>> Hence, this patch moves the code into the new notes-utils.[hc] files
>> which are included into libgit.a. Except for adjusting #includes
>> accordingly, this patch merely moves the relevant functions verbatim
>> into the new files.
>>
>> Cc: Thomas Rast <trast@inf.ethz.ch>
>> Signed-off-by: Johan Herland <johan@herland.net>
>
> I wonder where you got that idea from. Did you come up with that out thin air?

Obviously not. I should add

Suggested-by: Junio C Hamano <gitster@pobox.com>


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 1/3] finish_copy_notes_for_rewrite(): Let caller provide commit message

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> When copying notes for a rewritten object, the resulting notes commit
> would have the following hardcoded commit message:
>
>   Notes added by 'git notes copy'
>
> This is obviously bogus when the notes rewriting is performed by
> 'git commit --amend'.
>
> Therefore, let the caller specify an appropriate notes commit message
> instead of hardcoding it. The above message is used for 'git notes copy',
> but when calling finish_copy_notes_for_rewrite() from builtin/commit.c,
> we use the following message instead:
>
>   Notes added by 'git commit --amend'
>
> Cc: Thomas Rast <trast@inf.ethz.ch>
> Signed-off-by: Johan Herland <johan@herland.net>

Makes sense.  Thanks.

Re: [PATCH 2/3] Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c

[Felipe Contreras]

On Wed, Jun 12, 2013 at 2:10 AM, Johan Herland <johan@herland.net> wrote:
> On Wed, Jun 12, 2013 at 2:32 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Tue, Jun 11, 2013 at 7:13 PM, Johan Herland <johan@herland.net> wrote:
>>> This is a pure code movement of the machinery for copying notes to
>>> rewritten objects. This code was located in builtin/notes.c for
>>> historical reasons. In order to make it available to builtin/commit.c
>>> it was declared in builtin.h. This was more of an accident of history
>>> than a concious design, and we now want to make this machinery more
>>> widely available.
>>>
>>> Hence, this patch moves the code into the new notes-utils.[hc] files
>>> which are included into libgit.a. Except for adjusting #includes
>>> accordingly, this patch merely moves the relevant functions verbatim
>>> into the new files.
>>>
>>> Cc: Thomas Rast <trast@inf.ethz.ch>
>>> Signed-off-by: Johan Herland <johan@herland.net>
>>
>> I wonder where you got that idea from. Did you come up with that out thin air?
>
> Obviously not. I should add
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>

You are still not explaining where the idea came from. And you are
doing that with the express purpose of annoying.

Where did the idea come from?

-- 
Felipe Contreras

Re: [PATCH 2/3] Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c

[Johan Herland]

On Wed, Jun 12, 2013 at 8:28 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Wed, Jun 12, 2013 at 2:10 AM, Johan Herland <johan@herland.net> wrote:
>> On Wed, Jun 12, 2013 at 2:32 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>>> On Tue, Jun 11, 2013 at 7:13 PM, Johan Herland <johan@herland.net> wrote:
>>>> This is a pure code movement of the machinery for copying notes to
>>>> rewritten objects. This code was located in builtin/notes.c for
>>>> historical reasons. In order to make it available to builtin/commit.c
>>>> it was declared in builtin.h. This was more of an accident of history
>>>> than a concious design, and we now want to make this machinery more
>>>> widely available.
>>>>
>>>> Hence, this patch moves the code into the new notes-utils.[hc] files
>>>> which are included into libgit.a. Except for adjusting #includes
>>>> accordingly, this patch merely moves the relevant functions verbatim
>>>> into the new files.
>>>>
>>>> Cc: Thomas Rast <trast@inf.ethz.ch>
>>>> Signed-off-by: Johan Herland <johan@herland.net>
>>>
>>> I wonder where you got that idea from. Did you come up with that out thin air?
>>
>> Obviously not. I should add
>>
>> Suggested-by: Junio C Hamano <gitster@pobox.com>
>
> You are still not explaining where the idea came from. And you are
> doing that with the express purpose of annoying.

Truly, I am not trying to annoy anyone. I have not followed the
preceding discussion closely, and I wrote the patch based solely on
one paragraph from Junio's email[1].

> Where did the idea come from?

I got it from Junio. I do not know if I might have accidentally
plagiarized something you already submitted to the mailing list,
although I would be surprised if that was the case, since - as far as
I understand - you are opposed to this solution. Furthermore, I
thought you might not like having your name mentioned in a patch you
do not agree with, but if you think differently I have no problem
adding your name. I don't know what kind of attribution you would
prefer though:

Originally-envisioned-by: Felipe Contreras <felipe.contreras@gmail.com>?
NAKed-by: Felipe Contreras <felipe.contreras@gmail.com>?
Something else?


...Johan


[1]: Quoted from <7vehc8a05n.fsf@alter.siamese.dyndns.org>:
On Tue, Jun 11, 2013 at 9:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> There is only one right solution.  If a useful function is buried in
> builtin/*.o as a historical accident (i.e. it started its life as a
> helper for that particular command, and nobody else used it from
> outside so far) and that makes it impossible to use the function
> from outside builtin/*.o, refactor the function and its callers and
> move it to libgit.a.

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 2/3] Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c

[Felipe Contreras]

Johan Herland wrote:
> On Wed, Jun 12, 2013 at 8:28 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > On Wed, Jun 12, 2013 at 2:10 AM, Johan Herland <johan@herland.net> wrote:
> >> On Wed, Jun 12, 2013 at 2:32 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> >>> On Tue, Jun 11, 2013 at 7:13 PM, Johan Herland <johan@herland.net> wrote:
> >>>> This is a pure code movement of the machinery for copying notes to
> >>>> rewritten objects. This code was located in builtin/notes.c for
> >>>> historical reasons. In order to make it available to builtin/commit.c
> >>>> it was declared in builtin.h. This was more of an accident of history
> >>>> than a concious design, and we now want to make this machinery more
> >>>> widely available.
> >>>>
> >>>> Hence, this patch moves the code into the new notes-utils.[hc] files
> >>>> which are included into libgit.a. Except for adjusting #includes
> >>>> accordingly, this patch merely moves the relevant functions verbatim
> >>>> into the new files.
> >>>>
> >>>> Cc: Thomas Rast <trast@inf.ethz.ch>
> >>>> Signed-off-by: Johan Herland <johan@herland.net>
> >>>
> >>> I wonder where you got that idea from. Did you come up with that out thin air?
> >>
> >> Obviously not. I should add
> >>
> >> Suggested-by: Junio C Hamano <gitster@pobox.com>
> >
> > You are still not explaining where the idea came from. And you are
> > doing that with the express purpose of annoying.
> 
> Truly, I am not trying to annoy anyone. I have not followed the
> preceding discussion closely, and I wrote the patch based solely on
> one paragraph from Junio's email[1].

Here is another pagraph:

> Moving sequencer.c to builtin/ is not even a solution.  Linking
> git-upload-pack will still pull in builtin/notes.o along with cmd_notes(),
> which is not called from main(); as you remember, cmd_foo() in all
> builtin/*.o are designed to be called from git.c::main().

Which clearly refers to:
http://article.gmane.org/gmane.comp.version-control.git/226752

> > Where did the idea come from?
> 
> I got it from Junio. I do not know if I might have accidentally
> plagiarized something you already submitted to the mailing list,
> although I would be surprised if that was the case, since - as far as
> I understand - you are opposed to this solution.

You are aware I opposed this *solution*, yet were not aware that I sent the
first patch in this thread, which clearly states the *problem*?

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

http://article.gmane.org/gmane.comp.version-control.git/226845

> Originally-envisioned-by: Felipe Contreras <felipe.contreras@gmail.com>?

Do I have to do it for you? Your commit message is all wrong, because nowhere
are you pointing out *why* you are making the change.

---
Move copy_note_for_rewrite + friends to notes-utils.c

In order to make these functionas available to top-level objects (e.g.
sequencer.o), we need to move them out of the builtin/ subdirectory.

Reported-by: Felipe Contreras <felipe.contreras@gmail.com>
---

-- 
Felipe Contreras

Re: [PATCH 2/3] Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> This is a pure code movement of the machinery for copying notes to
> rewritten objects. This code was located in builtin/notes.c for
> historical reasons. In order to make it available to builtin/commit.c
> it was declared in builtin.h. This was more of an accident of history
> than a concious design, and we now want to make this machinery more
> widely available.
>
> Hence, this patch moves the code into the new notes-utils.[hc] files
> which are included into libgit.a. Except for adjusting #includes
> accordingly, this patch merely moves the relevant functions verbatim
> into the new files.
>
> Cc: Thomas Rast <trast@inf.ethz.ch>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  Makefile         |   2 +
>  builtin.h        |  16 -------
>  builtin/commit.c |   1 +
>  builtin/notes.c  | 131 +-----------------------------------------------------
>  notes-utils.c    | 132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  notes-utils.h    |  23 ++++++++++
>  6 files changed, 159 insertions(+), 146 deletions(-)
>  create mode 100644 notes-utils.c
>  create mode 100644 notes-utils.h

Output from "git show -C1 --stat" after applying this patch shows
mostly removals (i.e. builtin/notes.c loses what was lifted from it,
notes-utils.c starts its life as a copy of the former and the patch
shows removal of what should not move to notes-utils.c).  After
inspecting "added" lines to these two files, I did not spot anything
suspicious, except for one C++/C99 comment (will locally touch-up).

Thanks.

Re: [PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> There is only one right solution.  If a useful function is buried in
>> builtin/*.o as a historical accident (i.e. it started its life as a
>> helper for that particular command, and nobody else used it from
>> outside so far) and that makes it impossible to use the function
>> from outside builtin/*.o, refactor the function and its callers and
>> move it to libgit.a.
>
> Here goes...
>
> ...Johan

With these three patches, if you apply the following skeleton patch
(lifted from $gmane/226851 and adjusted minimally to the change
these patches introduce), we can see that the link breakage Felipe
observed in the message:

    Felipe Contreras <felipe.contreras@gmail.com> writes in $gmane/226851:
    > 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'

is gone.

    > It is not the first time, nor the last that top-level code needs
    > builtin code, and the solution is easy; organize the code.

And as I already said, the above is correct.  The problem and the
general approach to solve it correctly were identified in the
message.

But what followed was a nonsense, which ended up wastign everybody's
time:

> ... Alas, this
> simple solution reject on the basis that we shouldn't organize the
> code, because the code is not meant to be organized.

The proposed patch was rejected on the basis that it was organized
the code in a wrong way.  And your patch shows how it should be
done.

Thanks for doing it right.

-- skeleton patch --

 sequencer.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index ab6f8a7..4281466 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -14,6 +14,7 @@
 #include "merge-recursive.h"
 #include "refs.h"
 #include "argv-array.h"
+#include "notes-utils.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -979,6 +980,17 @@ static void save_opts(struct replay_opts *opts)
 	}
 }
 
+static void copy_notes(const char *name, const char *msg)
+{
+       struct notes_rewrite_cfg *cfg;
+
+       cfg = init_copy_notes_for_rewrite(name);
+       if (!cfg)
+               return;
+
+       finish_copy_notes_for_rewrite(cfg, msg);
+}
+
 static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 {
 	struct commit_list *cur;
@@ -997,6 +1009,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 			return res;
 	}
 
+	copy_notes("cherry-pick", "notes copied by cherry-pick");
+
 	/*
 	 * Sequence of picks finished successfully; cleanup by
 	 * removing the .git/sequencer directory

Re: [PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]

[Felipe Contreras]

On Wed, Jun 12, 2013 at 3:02 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> ... Alas, this
>> simple solution reject on the basis that we shouldn't organize the
>> code, because the code is not meant to be organized.
>
> The proposed patch was rejected on the basis that it was organized
> the code in a wrong way.  And your patch shows how it should be
> done.

In your opinion.

The fact that nobody outside of 'git' will ever use
init_copy_notes_for_rewrite() still remains. Therefore this
"organization" is wrong.

-- 
Felipe Contreras

Re: [PATCH 2/3] Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c

[Andreas Krey]

On Wed, 12 Jun 2013 13:28:05 +0000, Felipe Contreras wrote:
...
> And you are
> doing that with the express purpose of annoying.

Where did 'assume good faith' go to today?

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: [PATCH 2/3] Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c

[Felipe Contreras]

On Thu, Jun 13, 2013 at 1:45 AM, Andreas Krey <a.krey@gmx.de> wrote:
> On Wed, 12 Jun 2013 13:28:05 +0000, Felipe Contreras wrote:
> ...
>> And you are
>> doing that with the express purpose of annoying.
>
> Where did 'assume good faith' go to today?

Did you read the last part?

"This does not mean that one should continue to assume good faith when
there's evidence to the contrary."

That being said, my evidence was not solid, and while there is still
the possibility that he was indeed acting in good faith, I've received
no response from him, and Junio has committed the change without any
mentioning of where the idea come from.

Either way, I bet you my good faith suggestion will *not* end up in
the official guidelines, nor will any suggestion of mine.

-- 
Felipe Contreras

Re: [PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Wed, Jun 12, 2013 at 3:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> The proposed patch was rejected on the basis that it was organized
>> the code in a wrong way.  And your patch shows how it should be
>> done.
>
> In your opinion.
>
> The fact that nobody outside of 'git' will ever use
> init_copy_notes_for_rewrite() still remains. Therefore this
> "organization" is wrong.

That is a fact?

It is your opinion on what might happen in the future.  

And you ignored external projects that may want to link with
libgit.a, and closed the door for future improvements.  Johan's
implementation has the same effect of allowing sequencer.c to call
these functions without doing so.

Anyway, I have a more important thing to say.

You sometimes identify the right problem to tackle, but often the
discussions on your patches go in a wrong direction that does not
help solving the original problem at all.  The two examples I can
immediately recall offhand are:

 (1) a possible "blame" enhancement, where gitk, that currently runs
     two passes of it to identify where each line ultimately came
     from and to identify where each line was moved to the current
     place, could ask it to learn both with a single run.

 (2) refactoring builtin/notes.c to make it possible for sequencer
     machinery can also call useful helper functions buried in it.

but I am sure other reviewers can recall other instances in the
recent past.

Your patches were wrong in both cases, but that is not an issue.  If
you are not familiar with the area you are trying to improve, it is
understandable that initial attempts may try to solve the right
problem in a wrong way.  That is perfectly normal.

That is what the patch review process is there to help.

Reviewers who are more familiar with the area (either the code flow
and data structure used in blame, or how the object files are laid
out in the source tree and the build procedure is designed to link
them to which binary) can point the contributor in a direction that
would take us to a better result in the end.  During the discussion,
it may turn out that reviewers have overlooked issues that also need
to be addressed, or there may be further adjustments needed that are
initially overlooked by everyone.  The solution to these problems is
for contributors and reviewers to _collaborate_ to come up with a
better end result, which is often different from both the original
patch and the suggestions in the initial review.

When it is your patch, however, we repeatedly saw that the review
process got derailed in the middle.

The reviewers tried to reach a good end result in the same way as
they interact with other contributors, i.e. by showing a way they
think is better, trying to make the contributor realize why it is
better by rephrasing and coming up with other examples.

This iteration takes a lot of resources, but the reviewers are
hoping that we will see a good result at the end of the review and
everybody wins. They are trying to collaborate.

If there is no will to collaborate on the contributor's end,
however, and the primary thing the contributor wants to do is to
endlessly argue, the efforts by reviewers are all wasted. We do not
get anywhere.

That is how I perceive what happens to many of your patches.  I am
sure you will say "that is your opinion", but I do not think I am
alone.  And I am also sure you will then say "majority is not always
right".

But the thing is, that majority is what writes the majority of the
code and does the majority of the reviews, so as maintainer I *do*
have to give their opinion a lot of weight, not to mention my own
opinion about how to help keep the community the most productive.

And I have to conclude that the cost of having to deal with you
outweighs the benefit the project gets out of having you around.
Therefore I have ask you to leave and not bother us anymore.

Goodbye.

Re: [PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]

[Felipe Contreras]

On Thu, Jun 13, 2013 at 12:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Wed, Jun 12, 2013 at 3:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> The proposed patch was rejected on the basis that it was organized
>>> the code in a wrong way.  And your patch shows how it should be
>>> done.
>>
>> In your opinion.
>>
>> The fact that nobody outside of 'git' will ever use
>> init_copy_notes_for_rewrite() still remains. Therefore this
>> "organization" is wrong.
>
> That is a fact?

No, it's not.

> It is your opinion on what might happen in the future.

That's right, an informed opinion.

> And you ignored external projects that may want to link with
> libgit.a,

Like which project?

Moreover:

% find /opt/git -name '*.a'

Returns nothing. The cannot link to libgit.a, and besides, we don't
provide a public API at all.

> and closed the door for future improvements.  Johan's
> implementation has the same effect of allowing sequencer.c to call
> these functions without doing so.

That would be closing the door to ghosts.

Do you want to bet? Five years from now nobody will be using
init_copy_notes_for_rewrite().

You loose, we move it to builtin/lib.a, you win, we don't.

> Anyway, I have a more important thing to say.
>
> You sometimes identify the right problem to tackle, but often the
> discussions on your patches go in a wrong direction that does not
> help solving the original problem at all.

So what? I'm a human, am I not allowed to make mistakes?

You make mistakes too.

> The two examples I can immediately recall offhand are:
>
>  (1) a possible "blame" enhancement, where gitk, that currently runs
>      two passes of it to identify where each line ultimately came
>      from and to identify where each line was moved to the current
>      place, could ask it to learn both with a single run.

Yes, *I ACKNOWLEDGED* the direction was not the right one, and I
didn't have the time nor the patience to go into such a tedious
direction.

>From my recommended guideline:

* Accept comments on your reviews gracefully. If the original patch
submitter doesn't agree with your review, don't take offense. Don't
assume the submitter has to automatically modify the patches according
to your comments, or even necessarily seek a compromise. The submitter
is entitled to his opinion, and so are you. Also, remember that each
person has their own priorities in life, and it might take time before
the submitter has time to implement the changes, if ever. The changes
you request might be beyond the time the submitter is willing to
spend, and it's OK for him to decide to drop the patches as a result.
You can help by picking the patches yourself in those situations.

>  (2) refactoring builtin/notes.c to make it possible for sequencer
>      machinery can also call useful helper functions buried in it.

You are wrong. My patches did solve the original problem, I know
because I was the one that found the original problem.

> The solution to these problems is
> for contributors and reviewers to _collaborate_ to come up with a
> better end result, which is often different from both the original
> patch and the suggestions in the initial review.

Collaboration requires both sides to work on the problem. Not one side
pointing fingers and the other side doing all the work.

> When it is your patch, however, we repeatedly saw that the review
> process got derailed in the middle.

When working collaboratively it's fine to disagree, and it's fine to
have two sides come up with two different patches.

If you disagree with the other side, send a patch that does it properly.

If the other side doesn't do *exactly* what you want, that's not the
review process being derailed.

> If there is no will to collaborate on the contributor's end,
> however, and the primary thing the contributor wants to do is to
> endlessly argue, the efforts by reviewers are all wasted. We do not
> get anywhere.

In order to have and endless argument, *both sides* need to be engaged
in the argument. If you decide that a disagreement has been reached,
the argument ends in a disagreement.

> That is how I perceive what happens to many of your patches.  I am
> sure you will say "that is your opinion", but I do not think I am
> alone.

The opinion of a billion people is still an opinion.

> But the thing is, that majority is what writes the majority of the
> code and does the majority of the reviews, so as maintainer I *do*
> have to give their opinion a lot of weight, not to mention my own
> opinion about how to help keep the community the most productive.

Indeed, but that doesn't make it a fact. It remains an opinion.

> And I have to conclude that the cost of having to deal with you
> outweighs the benefit the project gets out of having you around.
> Therefore I have ask you to leave and not bother us anymore.

We shall see.

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

-- 
Felipe Contreras

Re: [PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]

[Felipe Contreras]

On Thu, Jun 13, 2013 at 1:16 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Thu, Jun 13, 2013 at 12:24 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> But the thing is, that majority is what writes the majority of the
>> code and does the majority of the reviews, so as maintainer I *do*
>> have to give their opinion a lot of weight, not to mention my own
>> opinion about how to help keep the community the most productive.
>
> Indeed, but that doesn't make it a fact. It remains an opinion.

And just to make it clear, I didn't deny you are the only one with
commit access, and therefore you make all the shots. You made a
decision, fine, I never said you can't do that.

What I said is that you should not use words to imply that your
*opinion* is a fact. The fact that you make a decision doesn't make
your opinion a fact, and the fact that many people share your opinion
doesn't make it a fact either.

So, instead of saying:

"Just one side being right, and the other side continuing to repeat
nonsense without listening."

You should say:

"Simply a matter of disagreement where the code belongs."

-- 
Felipe Contreras