On Wed, Oct 06 2021, Johannes Schindelin wrote: > On Tue, 14 Sep 2021, Junio C Hamano wrote: > >> An alternative would be to bypass the contrib/ phase and start as a >> new subcommand that is first-class citizen from day one and let it >> spend as much time as it needs to mature. > > I don't think that there is a lot of sense in that. The main benefits of > `scalar` are in the `register` and the `clone` part, and the most natural > end game would hence be for `git init` and `git clone` to sprout new > options to support Scalar's features, in a Git-native way. > > As I have explained earlier, the `scalar` command has existing users, > and therefore its command-line interface is not up for discussion (for > example, turning `scalar` into `git scalar` would be a usability > disaster). Scalar's _functionality_, however, should make it into Git > proper. Into existing built-ins, that is. Given the sub-thread this seems like it's trying to be an indirect reply to me, but I haven't been advocating changing the UX of "scalar" in any way, or that we should have a "git scalar". I have in fact been advocating exactly what you're advocating here. So we're in violent agreement. Yes the CLI UI shouldn't change, that's the whole point of having a "scalar" in git.git, not a "git scalar" or whatever. I've been suggesting we can simplify the *build system* git.git uses, something no user will ever see. > So I don't think that the contrib/ phase can be by-passed. It would not > make sense to port Scalar to a new builtin. To the contrary, > contrib/scalar/ should be the final destination for the `scalar` command. > And you can't bypass a final destination. That simply makes no sense. I'm right now using a "scalar" installed on my system based on the diff at the end of this E-Mail that changes the *build system* without any user-facing changes (see https://lore.kernel.org/git/87k0jhn0p9.fsf@xxxxxxxxxxxxxxxxxxx/) for a previous reference. $ which scalar /home/avar/local/bin/scalar $ scalar -h 2>&1 | head -n 1 usage: scalar [-C <directory>] [-c <key>=<value>] <command> [<options>] $ man scalar [...] NAME scalar - an opinionated repository management tool $ git help scalar No manual entry for gitscalar Except that I think your patches as they stand (correct me if I'm wrong) don't have any way to install it or its documentation, just to build it in-place. > So why bother with contrib/ at all? you may ask. The reason is that it > makes it substantially easier for me to move the features into core Git, > as I can incrementally implement those new options for Git's built-ins, > use them in `contrib/scalar/` instead of duplicating the functionality, > and then make use of Scalar's Functional Test suite for a much more > comprehensive testing (which has served us already really well in the > past). It also doesn't hurt that this way, my day job will be very happy > because Scalar users directly benefit from that work. I think all of that summarizes "why have this live in git.git", which I 100% agree with. It's not a counter-argument to a working solution for simplifying your proposed build system integration. > Of course, these suggestions to integrate Scalar more into the core part > of Git (missing the point that the final destination for the functionality > is not a new built-in, but rather new options for existing built-ins) made > everything much more cumbersome for me instead, for no gain that would be > apparent to me, impeding on aforementioned ease to move the features into > core Git (which has not happened yet, as a consequence), but hopefully > this will soon be a thing of the past. The diff below doesn't make scalar a built-in. > So I would like to request that we close the discussion about the question > whether to integrate Scalar more into the top-level Makefile or into > git.c, and instead go ahead with keeping the `scalar` command in > contrib/scalar/. The freed-up time can then be used to focus on the much > more rewarding project of upstreaming Scalar's functionality such as > teaching `git clone` a short-and-sweet option that Just Makes Sense for > large monorepos (i.e. that imitates at least a large part of what `scalar > clone` does right now). The reason I care about this is because duplicating this as a one-off may be easier for you now, but it creates a lot of maintenance burden for others down the line. For example, I've been fixing memory leaks recently and have a linux-leaks CI job that's now running on GitHub. Scalar would benefit from having a t/t9099-scalar.sh run as part of that. Yes you can special-case it somehow and teach ci/, or even "make test" to do the same thing. But there's a lot of little things both current and future that are going to be like that. Here's another one: $ make -C contrib/scalar scalar $ echo bad >cache.h $ make -C contrib/scalar scalar make: 'scalar' is up to date. I.e. because it's using its own Makefile it's not getting the very basics of the dependency graph right. My version? $ make scalar $ touch cache.h $ make scalar [... Works exactly as well as doing the same with "git"...] Yes we could fix that and other things etc, but why not just get all that for free in around 1/4 lines of Makefile boilerplate (and less than that in complexity)? .gitignore | 1 + Documentation/Makefile | 3 ++ {contrib/scalar => Documentation}/scalar.txt | 4 +- Makefile | 44 +++++++++++----- contrib/scalar/.gitignore | 5 -- contrib/scalar/Makefile | 57 -------------------- contrib/scalar/t/Makefile | 78 ---------------------------- contrib/scalar/scalar.c => scalar.c | 0 {contrib/scalar/t => t}/t9099-scalar.sh | 8 +-- 9 files changed, 37 insertions(+), 163 deletions(-) [Note that this probably doesn't fully apply to your series/master, not because of any great textual/semantic conflict, but just because I find it convenient to stack my WIP/unsubmitted serieses in related areas on top of one another. This is on top of other Makefile changes I've got unsubmitted, but it would be easy/trivial to re-apply on master+your series]. diff --git a/.gitignore b/.gitignore index 68ecb7f7e9c..ec9771e3532 100644 --- a/.gitignore +++ b/.gitignore @@ -217,6 +217,7 @@ /configure /.vscode/ /tags +/scalar /TAGS /cscope* /compile_commands.json diff --git a/Documentation/Makefile b/Documentation/Makefile index f5605b7767f..57b72e1999a 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -18,6 +18,9 @@ MAN1_TXT += $(filter-out \ MAN1_TXT += git.txt MAN1_TXT += gitk.txt MAN1_TXT += gitweb.txt +ifndef NO_INSTALL_SCALAR_DOC +MAN1_TXT += scalar.txt +endif # man5 / man7 guides (note: new guides should also be added to command-list.txt) MAN5_TXT += gitattributes.txt diff --git a/contrib/scalar/scalar.txt b/Documentation/scalar.txt similarity index 99% rename from contrib/scalar/scalar.txt rename to Documentation/scalar.txt index 3a80f829edc..f287ab18c31 100644 --- a/contrib/scalar/scalar.txt +++ b/Documentation/scalar.txt @@ -149,6 +149,6 @@ SEE ALSO -------- linkgit:git-clone[1], linkgit:git-maintenance[1]. -Scalar +GIT --- -Associated with the linkgit:git[1] suite +Part of the linkgit:git[1] suite diff --git a/Makefile b/Makefile index fd617f8c775..536028922e1 100644 --- a/Makefile +++ b/Makefile @@ -602,6 +602,7 @@ LIB_OBJS = LIB_OBJS_NO_COMPAT_OBJS = OBJECTS = PROGRAM_OBJS = +SCALAR_OBJS = SCRIPT_LIB = SCRIPT_PERL = SCRIPT_PROGRAMS = @@ -807,6 +808,7 @@ BUILT_INS += $(BUILT_INS_EXTRA) # what 'all' will build but not install in gitexecdir OTHER_PROGRAMS = git$X +OTHER_PROGRAMS += scalar$X ARTIFACTS_TAR += $(OTHER_PROGRAMS) # what test wrappers are needed and 'install' will install, in bindir @@ -818,12 +820,18 @@ BINDIR_PROGRAMS_NEED_X += git-upload-pack BINDIR_PROGRAMS_NO_X += git-cvsserver +ifdef INSTALL_SCALAR +BINDIR_PROGRAMS_NEED_X += scalar +endif INSTALL_BINDIR_XPROGRAMS += $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) INSTALL_BINDIR_PROGRAMS += $(INSTALL_BINDIR_XPROGRAMS) $(BINDIR_PROGRAMS_NO_X) # We have bin-wrappers for programs that we don't install TEST_BINDIR_PROGRAMS_NEED_X += $(BINDIR_PROGRAMS_NEED_X) TEST_BINDIR_PROGRAMS_NEED_X += $(TEST_PROGRAMS_NEED_X) +ifndef INSTALL_SCALAR +TEST_BINDIR_PROGRAMS_NEED_X += scalar$X +endif TEST_BINDIR_PROGRAMS += $(TEST_BINDIR_PROGRAMS_NEED_X) TEST_BINDIR_PROGRAMS += $(BINDIR_PROGRAMS_NO_X) @@ -2230,7 +2238,7 @@ please_set_SHELL_PATH_to_a_more_modern_shell: shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell -strip: $(BIN_PROGRAMS) git$X +strip: $(BIN_PROGRAMS) git$X scalar$X $(STRIP) $(STRIP_OPTS) $^ ### Flags affecting all rules @@ -2286,6 +2294,10 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \ $(filter %.o,$^) $(LIBS) +scalar$X: scalar.o GIT-LDFLAGS $(GITLIBS) + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \ + $(filter %.o,$^) $(LIBS) + help.sp help.s help.o: command-list.h builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX @@ -2557,7 +2569,12 @@ GIT_OBJS += git.o .PHONY: git-objs git-objs: $(GIT_OBJS) +SCALAR_OBJS += scalar.o +.PHONY: scalar-objs +scalar-objs: $(SCALAR_OBJS) + OBJECTS += $(GIT_OBJS) +OBJECTS += $(SCALAR_OBJS) OBJECTS += $(PROGRAM_OBJS) OBJECTS += $(TEST_OBJS) OBJECTS += $(XDIFF_OBJS) @@ -2565,13 +2582,10 @@ OBJECTS += $(FUZZ_OBJS) ifndef NO_CURL OBJECTS += http.o http-walker.o remote-curl.o endif +OBJECTS += $(SCALAR_OBJECTS) .PHONY: objects objects: $(OBJECTS) -SCALAR_SOURCES := contrib/scalar/scalar.c -SCALAR_OBJECTS := $(SCALAR_SOURCES:c=o) -OBJECTS += $(SCALAR_OBJECTS) - dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d) dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS)))) @@ -2710,10 +2724,6 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) -contrib/scalar/scalar$X: $(SCALAR_OBJECTS) GIT-LDFLAGS $(GITLIBS) - $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \ - $(filter %.o,$^) $(LIBS) - $(LIB_FILE): $(LIB_OBJS) $(LIB_COMPAT_OBJS) $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^ @@ -3260,11 +3270,17 @@ endif install-gitweb: $(MAKE) -C gitweb install +ifdef INSTALL_SCALAR +NO_INSTALL_SCALAR_DOC = +else +NO_INSTALL_SCALAR_DOC = NoScalarPlease +endif + install-doc: install-man-perl - $(MAKE) -C Documentation install + $(MAKE) -C Documentation install NO_INSTALL_SCALAR_DOC=$(NO_INSTALL_SCALAR_DOC) install-man: install-man-perl - $(MAKE) -C Documentation install-man + $(MAKE) -C Documentation install-man NO_INSTALL_SCALAR_DOC=$(NO_INSTALL_SCALAR_DOC) install-man-perl: man-perl $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mandir_SQ)/man3' @@ -3272,13 +3288,13 @@ install-man-perl: man-perl (cd '$(DESTDIR_SQ)$(mandir_SQ)/man3' && umask 022 && $(TAR) xof -) install-html: - $(MAKE) -C Documentation install-html + $(MAKE) -C Documentation install-html NO_INSTALL_SCALAR_DOC=$(NO_INSTALL_SCALAR_DOC) install-info: - $(MAKE) -C Documentation install-info + $(MAKE) -C Documentation install-info NO_INSTALL_SCALAR_DOC=$(NO_INSTALL_SCALAR_DOC) install-pdf: - $(MAKE) -C Documentation install-pdf + $(MAKE) -C Documentation install-pdf NO_INSTALL_SCALAR_DOC=$(NO_INSTALL_SCALAR_DOC) quick-install-doc: $(MAKE) -C Documentation quick-install diff --git a/contrib/scalar/.gitignore b/contrib/scalar/.gitignore deleted file mode 100644 index 00441073f59..00000000000 --- a/contrib/scalar/.gitignore +++ /dev/null @@ -1,5 +0,0 @@ -/*.xml -/*.1 -/*.html -/*.exe -/scalar diff --git a/contrib/scalar/Makefile b/contrib/scalar/Makefile deleted file mode 100644 index 8620042f281..00000000000 --- a/contrib/scalar/Makefile +++ /dev/null @@ -1,57 +0,0 @@ -QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir -QUIET_SUBDIR1 = - -ifneq ($(findstring s,$(MAKEFLAGS)),s) -ifndef V - QUIET_GEN = @echo ' ' GEN $@; - QUIET_SUBDIR0 = +@subdir= - QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \ - $(MAKE) $(PRINT_DIR) -C $$subdir - QUIET = @ -else - export V -endif -endif - -all: - -include ../../config.mak.uname --include ../../config.mak.autogen --include ../../config.mak - -TARGETS = scalar$(X) scalar.o -GITLIBS = ../../common-main.o ../../libgit.a ../../xdiff/lib.a - -all: scalar$X ../../bin-wrappers/scalar - -$(GITLIBS): - $(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(subst ../../,,$@) - -$(TARGETS): $(GITLIBS) scalar.c - $(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(patsubst %,contrib/scalar/%,$@) - -clean: - $(RM) $(TARGETS) ../../bin-wrappers/scalar - $(RM) scalar.1 scalar.html scalar.xml - -../../bin-wrappers/scalar: ../../wrap-for-bin.sh Makefile - @mkdir -p ../../bin-wrappers - $(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ - -e 's|@@BUILD_DIR@@|$(shell cd ../.. && pwd)|' \ - -e 's|@@PROG@@|contrib/scalar/scalar$(X)|' < $< > $@ && \ - chmod +x $@ - -test: all - $(MAKE) -C t - -docs: scalar.html scalar.1 - -scalar.html: | scalar.1 # prevent them from trying to build `doc.dep` in parallel - -scalar.html scalar.1: scalar.txt - $(QUIET_SUBDIR0)../../Documentation$(QUIET_SUBDIR1) \ - MAN_TXT=../contrib/scalar/scalar.txt \ - ../contrib/scalar/$@ - $(QUIET)test scalar.1 != "$@" || mv ../../Documentation/$@ . - -.PHONY: all clean docs test FORCE diff --git a/contrib/scalar/t/Makefile b/contrib/scalar/t/Makefile deleted file mode 100644 index 6170672bb37..00000000000 --- a/contrib/scalar/t/Makefile +++ /dev/null @@ -1,78 +0,0 @@ -# Run scalar tests -# -# Copyright (c) 2005,2021 Junio C Hamano, Johannes Schindelin -# - --include ../../../config.mak.autogen --include ../../../config.mak - -SHELL_PATH ?= $(SHELL) -PERL_PATH ?= /usr/bin/perl -RM ?= rm -f -PROVE ?= prove -DEFAULT_TEST_TARGET ?= test -TEST_LINT ?= test-lint - -ifdef TEST_OUTPUT_DIRECTORY -TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results -else -TEST_RESULTS_DIRECTORY = ../../../t/test-results -endif - -# Shell quote; -SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH)) -PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH)) -TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY)) - -T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)) - -all: $(DEFAULT_TEST_TARGET) - -test: $(TEST_LINT) - $(MAKE) aggregate-results-and-cleanup - -prove: $(TEST_LINT) - @echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS) - $(MAKE) clean-except-prove-cache - -$(T): - @echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS) - -clean-except-prove-cache: - $(RM) -r 'trash directory'.* '$(TEST_RESULTS_DIRECTORY_SQ)' - $(RM) -r valgrind/bin - -clean: clean-except-prove-cache - $(RM) .prove - -test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax - -test-lint-duplicates: - @dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \ - test -z "$$dups" || { \ - echo >&2 "duplicate test numbers:" $$dups; exit 1; } - -test-lint-executable: - @bad=`for i in $(T); do test -x "$$i" || echo $$i; done` && \ - test -z "$$bad" || { \ - echo >&2 "non-executable tests:" $$bad; exit 1; } - -test-lint-shell-syntax: - @'$(PERL_PATH_SQ)' ../../../t/check-non-portable-shell.pl $(T) - -aggregate-results-and-cleanup: $(T) - $(MAKE) aggregate-results - $(MAKE) clean - -aggregate-results: - for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \ - echo "$$f"; \ - done | '$(SHELL_PATH_SQ)' ../../../t/aggregate-results.sh - -valgrind: - $(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind" - -test-results: - mkdir -p test-results - -.PHONY: $(T) aggregate-results clean valgrind diff --git a/contrib/scalar/scalar.c b/scalar.c similarity index 100% rename from contrib/scalar/scalar.c rename to scalar.c diff --git a/contrib/scalar/t/t9099-scalar.sh b/t/t9099-scalar.sh similarity index 94% rename from contrib/scalar/t/t9099-scalar.sh rename to t/t9099-scalar.sh index 7e8771d0eff..f686ba5d84c 100755 --- a/contrib/scalar/t/t9099-scalar.sh +++ b/t/t9099-scalar.sh @@ -2,13 +2,7 @@ test_description='test the `scalar` command' -TEST_DIRECTORY=$PWD/../../../t -export TEST_DIRECTORY - -# Make it work with --no-bin-wrappers -PATH=$PWD/..:$PATH - -. ../../../t/test-lib.sh +. ./test-lib.sh GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt" export GIT_TEST_MAINT_SCHEDULER
