Subject: Re: [PATCH v5 00/15] Upstreaming the Scalar command

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 07 Oct 2021 13:28:07 +0200

On Thu, Oct 07 2021, Johannes Schindelin via GitGitGadget wrote:

>  * The OBJECTS list in the Makefile will now include Scalar.

So that looks like a partial fix for what I brought up in [1] [...]

> Range-diff vs v4:
>
>   1:  852ec003109 !  1:  7119a8efc21 scalar: create a rudimentary executable
>      @@ Commit message
>           Signed-off-by: Johannes Schindelin <[email-masked]>
>       
>        ## Makefile ##
>      -@@ Makefile: endif
>      - .PHONY: objects
>      - objects: $(OBJECTS)
>      - 
>      +@@ Makefile: OBJECTS += $(FUZZ_OBJS)
>      + ifndef NO_CURL
>      + 	OBJECTS += http.o http-walker.o remote-curl.o
>      + endif
>      ++
>       +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))))
>      + .PHONY: objects
>      + objects: $(OBJECTS)

Except that this & contrib/scalar/Makefile is still broken in multiple
ways. We now have two Makefiles that can build contrib/scalar/scalar:

    touch advice.h; make -j8 contrib/scalar/scalar

But try:

    $ touch advice.h; (cd contrib/scalar 2>/dev/null && make scalar)
    make: 'scalar' is up to date.

I.e. (I'm presuming in response to what I brought up in [1]) the
depenency graph in the top-level Makefile is correct in this specific
area. But it understands the ".depends" files (depending on
COMPUTE_HEADER_DEPENDENCIES), your sub-Makefile doesn't.

There's similar whack-a-mole issues in other areas, e.g.:

    make -C contrib/scalar/ test

Will break or not depending on whether you've built the top-level
git.

I noticed at least one other subtle breakage (first thing I checked
after those two).

I'm happy to send you a working patch to integrate that fixes all these
issues, it also integrates with "make install", this series leaves us
with a "scalar" binary, but no way to install it, if we just piggy-back
on the existing installation procedure.

The side-thread on the v3[3] that you most recently replied to is
conflating some suggestion of shipping this as a built-in, with the
purely build-system implementation details I'm suggesting here.

I did mention using it as a built-in in [4], but for the semi-related
issue of scalar.c copy/pasting less code from git.c. But that was in the
context of such a thing being purely a non-visible implementation
detail. I.e. it would still be "scalar", not "git scalar".

*That* suggestion is just a side-musing about whether it would be easier
to teach git.c to inspects its argv and have a special-case for
dispatching to cmd_scalar(), a user would never know the difference. I
think that might also be worthwhile, but I care *way* less about that
than making maintaining the Makefile a hassle, and it's an entirely
orthogonal suggestion.

1. https://lore.kernel.org/git/[email-masked]/
2. https://lore.kernel.org/git/[email-masked]/
3. https://lore.kernel.org/git/[email-masked]
4. https://lore.kernel.org/git/[email-masked]/