[PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git'

[PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git'

[Ævar Arnfjörð Bjarmason]

Change the compilation of the main 'git' binary to not have the CC
clobber 'git' in-place. This means that e.g. running the test suite
in-place and recompiling won't fail whatever tests happen to be
running for the duration of the binary being regenerated.

This is not a complete solution, in particular I'm not doing this for
any of the git-* binaries, so it's effectively only for
SKIP_DASHED_BUILT_INS=Y.

I think that's fine for a small best-effort solution to this, even
those who rely on dashed binaries for something are likely mostly
using 'git' for whatever scripts they have running out of their
git.git checkout.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index dfb0f1000fa..c5c754a9a12 100644
--- a/Makefile
+++ b/Makefile
@@ -2159,8 +2159,9 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
 git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
-		$(filter %.o,$^) $(LIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) \
+		$(filter %.o,$^) $(LIBS) && \
+	mv $@+ $@
 
 help.sp help.s help.o: command-list.h
 
-- 
2.31.0.rc0.126.g04f22c5b82

Re: [PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git'

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the compilation of the main 'git' binary to not have the CC
> clobber 'git' in-place. This means that e.g. running the test suite
> in-place and recompiling won't fail whatever tests happen to be
> running for the duration of the binary being regenerated.

I am not sure why I would want to support the workflow this is
trying to help.

I do not want to see a patch on this list claiming that "While the
whole test suite is running, I tweaked the source three times and
recompiled, so some tests may have used my second iteration while
others may have used my third and the final version of the code---in
any case, all tests passed".  And when a patch that was developed
that way came in, I do not want to hear "Yes, the test suite used
mixed binaries, but I KNOW the difference between my second and
third iteration should not matter the parts of the earlier tests
that used the older iteration".

Re: [PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git'

[Ævar Arnfjörð Bjarmason]

On Sun, Mar 07 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the compilation of the main 'git' binary to not have the CC
>> clobber 'git' in-place. This means that e.g. running the test suite
>> in-place and recompiling won't fail whatever tests happen to be
>> running for the duration of the binary being regenerated.
>
> I am not sure why I would want to support the workflow this is
> trying to help.

Because it also allows me and others to do more testing on patches to
git.git.

If I'm working on a patch to e.g. git-fsck I might be doing
edit->save->some-tests, where "some-tests" are a subset of the test
suite I think is relevant to fsck.

But after doing N commits with passing tests I might notice that some
other part of the test suite I didn't expect to have anything to do with
fsck broke because I wasn't running that test.

I wasn't running that test because I'm not going to wait 10-15 minutes
for it to run while actively editing, but will wait 30s-1m for 10-50
test files to run.

So I can also have the full test suite running in a loop in some side
window that'll give me a headsup if the "while do-full-tests; [...]"
loop breaks, at which point I'm likely to investigate it sooner than
otherwise, and not waste time going down the wrong path.

You can of course do that now, but it requires having a worktree on the
side or whatever, which isn't always ideal (sometimes I'd like to have
these tests on uncommitted changes).

This change makes it mostly just work.


> I do not want to see a patch on this list claiming that "While the
> whole test suite is running, I tweaked the source three times and
> recompiled, so some tests may have used my second iteration while
> others may have used my third and the final version of the code---in
> any case, all tests passed".  And when a patch that was developed
> that way came in, I do not want to hear "Yes, the test suite used
> mixed binaries, but I KNOW the difference between my second and
> third iteration should not matter the parts of the earlier tests
> that used the older iteration".

We have a lot of existing rules in the Makefile that are of the form:

    make thing >thing+ &&
    mv thing+ thing

Where we're not doing the rename dance to avoid clobbering the file
we're reading.

As far as I can see all/most of those rules can just be rewritten like
e.g. this if this is a use-case we not only don't care about, but would
like to go out of our way to break:

    diff --git a/Makefile b/Makefile
    index dfb0f1000fa..8b57c3a5e63 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -2190,2 +2190 @@ config-list.h:
    -       $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
    -               >$@+ && mv $@+ $@
    +       $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh >$@

Re: [PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git'

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> We have a lot of existing rules in the Makefile that are of the form:
>
>     make thing >thing+ &&
>     mv thing+ thing
>
> Where we're not doing the rename dance to avoid clobbering the file
> we're reading.

I am afraid that you are totally misreading the intent of that age
old convention, which is spelled:

	thing:
		rm -f thing thing+
		prepare contents for thing >thing+
		mv thing+ thing

It protects us from a failure mode where "prepare contents for
thing" step is broken and leaves a "thing" that does not work, but
confuses make that make does not need to rebuild it, if you wrote it
as such:

	thing:
		prepare contents for thing >thing

I think more recent make actually has some knob you can tweak to
tell "if the rule failed, remove it", but the convention predates
it.

In any case, it is not "we are trying to make thing available while
it is being rewritten" at all.

Re: [PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git'

[Jeff King]

On Mon, Mar 08, 2021 at 01:38:32PM +0100, Ævar Arnfjörð Bjarmason wrote:

> >> Change the compilation of the main 'git' binary to not have the CC
> >> clobber 'git' in-place. This means that e.g. running the test suite
> >> in-place and recompiling won't fail whatever tests happen to be
> >> running for the duration of the binary being regenerated.
> >
> > I am not sure why I would want to support the workflow this is
> > trying to help.
> 
> Because it also allows me and others to do more testing on patches to
> git.git.
> 
> If I'm working on a patch to e.g. git-fsck I might be doing
> edit->save->some-tests, where "some-tests" are a subset of the test
> suite I think is relevant to fsck.
> 
> But after doing N commits with passing tests I might notice that some
> other part of the test suite I didn't expect to have anything to do with
> fsck broke because I wasn't running that test.
> 
> I wasn't running that test because I'm not going to wait 10-15 minutes
> for it to run while actively editing, but will wait 30s-1m for 10-50
> test files to run.
> 
> So I can also have the full test suite running in a loop in some side
> window that'll give me a headsup if the "while do-full-tests; [...]"
> loop breaks, at which point I'm likely to investigate it sooner than
> otherwise, and not waste time going down the wrong path.
> 
> You can of course do that now, but it requires having a worktree on the
> side or whatever, which isn't always ideal (sometimes I'd like to have
> these tests on uncommitted changes).

I don't always use it, but I have a "ci" script[1] that just runs the
test on each individual commit in a loop. The interesting things about
it (beyond a simple loop) are:

  - it operates in a worktree (that copies the config.mak from the main
    worktree if necessary).

  - it uses Michael Haggerty's git-test[2] to memoize results for a given
    tree. That makes it reasonable to leave running in the background,
    where it will only use CPU when there's something useful to do.
    I also use git-test for "git rebase -x", so a final "is each commit
    OK" check usually runs instantly, because the results are cached.

  - it uses inotifywait to decide when HEAD has been updated. This is
    mostly a fun hack. It could also just poll every 10 seconds or
    whatever.

  - it triggers a custom command when the tests fail. I can share my
    sad-trombone.wav with you if you need. ;)

Your mention of 10-15 minutes makes me wonder why your system is so
slow, though. I generally run the whole suite (minus cvs/svn/p4 bits) in
under a minute. I know it's _much_ slower on Windows, but I didn't think
that was your platform.

(In general, I'm mildly negative on your patch here. I have definitely
run into this myself, but I think having the test suite loudly complain
is a good way to remind you that you have not in fact run the whole
suite on a given build).

[1] https://github.com/peff/git/blob/meta/ci
[2] https://github.com/mhagger/git-test

-Peff

[PATCH v2 0/5] Makefile: don't die on AIX with open ./git

[Ævar Arnfjörð Bjarmason]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8n, Size: 534 bytes --]

A v2, much the same, but moreso. But now with entirely different
reasons for existing.

Ævar Arnfjörð Bjarmason (5):
  Makefile: rename objects in-place, don't clobber
  Makefile: rename scripts in-place, don't clobber
  Makefile: don't needlessly "rm $@ $@+" before "mv $@+ $@"
  Makefile: add the ".DELETE_ON_ERROR" flag
  Makefile: don't "rm configure" before generating it

 Makefile | 102 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 41 deletions(-)

-- 
2.31.1.461.gd47399f6574

[PATCH v2 1/5] Makefile: rename objects in-place, don't clobber

[Ævar Arnfjörð Bjarmason]

Use the established "[...] -o $@+ && mv $@+ $@" pattern in the
Makefile for those rules that don't use it already.

This improves portability on OS such as AIX that complain if object
files in-use are clobbered (but not if they're rm'd, or renamed
away). On e.g. AIX 7.2 running a "./git log" in one terminal and
trying to clobber the "git" object in another will yield:

    $ >git
    bash: git: Cannot open or remove a file containing a running program.

And trying to recompile "git" will likewise fail:

    LINK git
    ld: 0711-851 SEVERE ERROR: Output file: git
        The file is in use and cannot be overwritten.

It's perfectly happy to have the file renamed or removed from under it
though:

    $ mv git git2
    $ >git2
    bash: git2: Cannot open or remove a file containing a running program.
    $ rm git2
    rm: Remove git2? y
    $ file git2
    file: 0653-900 cannot open git2.

On AIX the test suite is still a dumpster fire. I'm running into this
because an orphaned "git-daemon" is sometimes left running, causing
subsequent compilation to fail without manual intervention. It's also
annoying not to be able to run some ad-hoc command using the built git
without holding up subsequent compilation until any such programs are
stopped.

This pattern of not clobbering, but rather generating a "$@+" object
to rename in-place to "$@" has been present in the Makefile since
6a2e50f9dfd (git --version tells which version of git you have.,
2005-09-07), it just hasn't been consistently used for all the rules
that generated objects.

Per the log of changes to the Makfile and Junio's recent comment about
[1] why that pattern got introduced it was for a different reason
entirely, i.e. ("[]" edits are mine, for brevity):

    [T]hat age old convention [...] is spelled [as]:

    	thing:
    		rm -f thing thing+
    		prepare contents for thing >thing+
    		mv thing+ thing

    It protects us from a failure mode where "prepare contents for
    thing" step is broken and leaves a "thing" that does not work, but
    confuses make that make does not need to rebuild it, if you wrote it
    as such:

    	thing:
    		prepare contents for thing >thing

    [It might leave behind a corrupt 'thing'.] In any case, it is not
    "we are trying to make thing available while it is being
    rewritten" at all.

That makes perfect sense for shellscripts, but as this change shows
there's other good reasons to use this age old convention that weren't
considered at the time.

1. http://lore.kernel.org/git/xmqqpn097e9o.fsf@gitster.c.googlers.com

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/Makefile b/Makefile
index 55c8035fa80..b0dbf5888b7 100644
--- a/Makefile
+++ b/Makefile
@@ -2166,8 +2166,9 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
 git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
-		$(filter %.o,$^) $(LIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) \
+		$(filter %.o,$^) $(LIBS) && \
+	mv $@+ $@
 
 help.sp help.s help.o: command-list.h
 
@@ -2526,18 +2527,22 @@ compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null
 endif
 
 git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) && \
+	mv $@+ $@
 
 git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-		$(IMAP_SEND_LDFLAGS) $(LIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(IMAP_SEND_LDFLAGS) $(LIBS) && \
+	mv $@+ $@
 
 git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-		$(CURL_LIBCURL) $(LIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(CURL_LIBCURL) $(LIBS) && \
+	mv $@+ $@
 git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) && \
+	mv $@+ $@
 
 $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
 	$(QUIET_LNCP)$(RM) $@ && \
@@ -2546,14 +2551,17 @@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
 	cp $< $@
 
 $(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)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) && \
+	mv $@+ $@
 
 $(LIB_FILE): $(LIB_OBJS)
-	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
+	$(QUIET_AR)$(AR) $(ARFLAGS) $@+ $^ && \
+	mv $@+ $@
 
 $(XDIFF_LIB): $(XDIFF_OBJS)
-	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
+	$(QUIET_AR)$(AR) $(ARFLAGS) $@+ $^&& \
+	mv $@+ $@
 
 export DEFAULT_EDITOR DEFAULT_PAGER
 
@@ -2834,7 +2842,8 @@ perf: all
 t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS) && \
+	mv $@+ $@
 
 check-sha1:: t/helper/test-tool$X
 	t/helper/test-sha1.sh
@@ -3333,6 +3342,7 @@ FUZZ_CXXFLAGS ?= $(CFLAGS)
 
 $(FUZZ_PROGRAMS): all
 	$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) $(LIB_OBJS) $(BUILTIN_OBJS) \
-		$(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@
+		$(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@+ && \
+	mv $@+ $@
 
 fuzz-all: $(FUZZ_PROGRAMS)
-- 
2.31.1.461.gd47399f6574

Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Per the log of changes to the Makfile and Junio's recent comment about
> [1] why that pattern got introduced it was for a different reason
> entirely, i.e. ("[]" edits are mine, for brevity):
>
>     [T]hat age old convention [...] is spelled [as]:
>
>     	thing:
>     		rm -f thing thing+
>     		prepare contents for thing >thing+

Did I say that?  I recall I specifically avoided the "redirection"
because this is *NOT* shell-script only principle.  If a command is
so broken that "cmd -o thing" that fails to work correctly leaves a
broken output in thing, then the pattern should be applied and made
to "cmd -o thing+ && mv thing+ thing".

On the other hand, if "cmd" refrains from leaving a half-baked
result in "-o thing" (and I believe $(CC) is well-behaved even on
AIX), I do not think it is a good idea to use that pattern.  One
version of AIX may refuse to overwrite a file in use because
creat("thing") that is necessary for "-o thing" may fail while
"thing" is in use), but another system may refuse to rename over a
file in use (i.e. "-o thing+" into a brand new "thing+" may be OK
but the final "mv thing+ thing" may fail).  So pretending that it is
a cure for broken use case is cluttering Makefile for no real
benefit and leading readers into confused thinking.

>     		mv thing+ thing
>
>     It protects us from a failure mode where "prepare contents for
>     thing" step is broken and leaves a "thing" that does not work, but
>     confuses make that make does not need to rebuild it, if you wrote it
>     as such:
>
>     	thing:
>     		prepare contents for thing >thing
>
>     [It might leave behind a corrupt 'thing'.] In any case, it is not
>     "we are trying to make thing available while it is being
>     rewritten" at all.
>
> That makes perfect sense for shellscripts, but as this change shows
> there's other good reasons to use this age old convention that weren't
> considered at the time.

So, no, the age old convention may have considered and does not
apply to such a use case.

>  git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
> -		$(filter %.o,$^) $(LIBS)
> +	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) \
> +		$(filter %.o,$^) $(LIBS) && \
> +	mv $@+ $@

Really, does anybody else use "$(CC) -o $@" in such a way in their
Makefile?  Having to do this smells simply crazy (I am not saying
you are crazy---the platform that forces you to write such a thing
is crazy).

So, while I do not think the end result would break the build (other
than it probably would leave crufts "make clean" would not notice
behind when interrupted in the middle), I am moderately negative on
this change.

Thanks.

Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber

[Junio C Hamano]

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

>>     	thing:
>>     		rm -f thing thing+
>>     		prepare contents for thing >thing+
>
> Did I say that?  I recall I specifically avoided the "redirection"
> because this is *NOT* shell-script only principle.

Ah, I did say that in the response to the previous iteration.  But
the same principle applied to your other patch for [ce]tags which
took "-o output", and I was recalling my response to that thread.

In any case, whether "cmd >thing" or "cmd -o thing", if cmd leaves a
broken output in thing when it fails, it needs the "into thing+ and
rename to thing" dance.  Redirecion always need it, but a well behaved
command like $(CC) should not need it.

Thanks.

Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 29 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Per the log of changes to the Makfile and Junio's recent comment about
>> [1] why that pattern got introduced it was for a different reason
>> entirely, i.e. ("[]" edits are mine, for brevity):
>>
>>     [T]hat age old convention [...] is spelled [as]:
>>
>>     	thing:
>>     		rm -f thing thing+
>>     		prepare contents for thing >thing+
>
> Did I say that?  I recall I specifically avoided the "redirection"
> because this is *NOT* shell-script only principle.  If a command is
> so broken that "cmd -o thing" that fails to work correctly leaves a
> broken output in thing, then the pattern should be applied and made
> to "cmd -o thing+ && mv thing+ thing".

[I see this was already noted downthread...]

> On the other hand, if "cmd" refrains from leaving a half-baked
> result in "-o thing" (and I believe $(CC) is well-behaved even on
> AIX), I do not think it is a good idea to use that pattern.  One
> version of AIX may refuse to overwrite a file in use because
> creat("thing") that is necessary for "-o thing" may fail while
> "thing" is in use), but another system may refuse to rename over a
> file in use (i.e. "-o thing+" into a brand new "thing+" may be OK
> but the final "mv thing+ thing" may fail).  So pretending that it is
> a cure for broken use case is cluttering Makefile for no real
> benefit and leading readers into confused thinking.

It does fix this issue entirely on AIX. So it's a cure for the broken
case.

I can assure you that not having to regularly log in to the GCC farm AIX
box and remember how to deal with IBM's ps/kill etc. just to do another
build/test is a real benefit :)

Whereas maybe there's another system we're not fixing with this patch,
but does it matter? I don't see how it would make things worse for that
OS, if it exists. But it sounds like it's just a hypothetical.

>>     		mv thing+ thing
>>
>>     It protects us from a failure mode where "prepare contents for
>>     thing" step is broken and leaves a "thing" that does not work, but
>>     confuses make that make does not need to rebuild it, if you wrote it
>>     as such:
>>
>>     	thing:
>>     		prepare contents for thing >thing
>>
>>     [It might leave behind a corrupt 'thing'.] In any case, it is not
>>     "we are trying to make thing available while it is being
>>     rewritten" at all.
>>
>> That makes perfect sense for shellscripts, but as this change shows
>> there's other good reasons to use this age old convention that weren't
>> considered at the time.
>
> So, no, the age old convention may have considered and does not
> apply to such a use case.

The reason I mentioned this was to specifically address the implied "why
would we need this?" part of your E-Mail that you're quoting.

I think that started out as us talking past one another, so let me try
to summarize.

I was basically saying "here's a well-known command idiom" that can be
used for XYZ.

I think you're basically saying "in git.git, I introduced this idiom to
deal with problem ABC".

ABC and XYZ aren't incompatible. I'm just saying this can and does solve
both problems[continued below].

>>  git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
>> -		$(filter %.o,$^) $(LIBS)
>> +	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) \
>> +		$(filter %.o,$^) $(LIBS) && \
>> +	mv $@+ $@
>
> Really, does anybody else use "$(CC) -o $@" in such a way in their
> Makefile?  Having to do this smells simply crazy (I am not saying
> you are crazy---the platform that forces you to write such a thing
> is crazy).

Yes, if you do say a Google search for "Cannot open or remove a file
containing a running program" you'll find that there's 15k results of
people basically (re)discovering this problem in porting their software
to AIX, and the solutions being some variant of "yes AIX sucks, just use
this 'cmd >x+ && mv x+ x' trick".

You can also invoke a "slibclean" program to reticulate AIX's splines,
but I thought this sucked less.

Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> Really, does anybody else use "$(CC) -o $@" in such a way in their
>> Makefile?  Having to do this smells simply crazy (I am not saying
>> you are crazy---the platform that forces you to write such a thing
>> is crazy).
>
> Yes, if you do say a Google search for "Cannot open or remove a file
> containing a running program" you'll find that there's 15k results of
> people basically (re)discovering this problem in porting their software
> to AIX, and the solutions being some variant of "yes AIX sucks, just use
> this 'cmd >x+ && mv x+ x' trick".

What I meant was if there are well known upstream projects whose
Makefile actually use

	$(CC) -o $@+ ...
	mv $@+ $@

I wouldn't be surprised if AIX community maintained collections of
patches to many projects to turn

	$(CC) -o $@ ...

in the Makefiles taken from upstream projects into

	$(CC) -o $@+ ...
	mv $@+ $@

to work AIX around.  As an upstream, however, I am not interested in
forcing that pattern on users of other platforms.

In any case, I do not care too much about the "I am building a new
binary while running, without installing, the one I built" use case
and do not agree with the idea of making the Makefile ugly only to
support such a use case.  That is where my comments are coming from
on this topic.  FWIW, AIX developers who do not do the "build, run
without installing, and rebuild while the old one is still running"
will not need the "$(CC) -o $@+ && mv $@+ $@" either, right?

Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 30 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> Really, does anybody else use "$(CC) -o $@" in such a way in their
>>> Makefile?  Having to do this smells simply crazy (I am not saying
>>> you are crazy---the platform that forces you to write such a thing
>>> is crazy).
>>
>> Yes, if you do say a Google search for "Cannot open or remove a file
>> containing a running program" you'll find that there's 15k results of
>> people basically (re)discovering this problem in porting their software
>> to AIX, and the solutions being some variant of "yes AIX sucks, just use
>> this 'cmd >x+ && mv x+ x' trick".
>
> What I meant was if there are well known upstream projects whose
> Makefile actually use
>
> 	$(CC) -o $@+ ...
> 	mv $@+ $@
>
> I wouldn't be surprised if AIX community maintained collections of
> patches to many projects to turn
>
> 	$(CC) -o $@ ...
>
> in the Makefiles taken from upstream projects into
>
> 	$(CC) -o $@+ ...
> 	mv $@+ $@
>
> to work AIX around.  As an upstream, however, I am not interested in
> forcing that pattern on users of other platforms.

Who's going to notice or care? We have some mixture of clobbering, "mv
$@+ $@" etc. now in our Makefile for various rules and I think unless
you're debugging those specific rules you won't notice.

The case of the $@+ being left behind is quite obscure, and with *+ in
our .gitignore won't be noticed (e.g. by a "git add ." or something).

> In any case, I do not care too much about the "I am building a new
> binary while running, without installing, the one I built" use case
> and do not agree with the idea of making the Makefile ugly only to
> support such a use case.  That is where my comments are coming from
> on this topic.  FWIW, AIX developers who do not do the "build, run
> without installing, and rebuild while the old one is still running"
> will not need the "$(CC) -o $@+ && mv $@+ $@" either, right?

I daresay that uses cases of:

 * The tests break, you login to the CI to run gdb, fix a small bug,
   compile (doesn't work), but being forced to close that gdb session
   would be annoying (e.g. maybe I'm just looking at a data in a struct
   I didn't change).

 * Ditto, but maybe debugging two things at the same time, having an
   open "cat-file --batch" session etc.

Aren't something obscure to someone wanting to work on a codebase. I'm
submitting these because this is an active impediment to me submitting
portability patches on AIX, of which I already have some:

    git log --grep=AIX --author=Ævar origin/master

Anything that makes that less painful is a win, and the tiny amount of
Makefile complexity seems worth it to me.

Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> What I meant was if there are well known upstream projects whose
>> Makefile actually use
>>
>> 	$(CC) -o $@+ ...
>> 	mv $@+ $@

... or can you find a popular build generator or two that write such
rules for the final linkage stage (or individual object file
generation step) and argue that all the projects that use the tool
do benefit from such a rule because they can run without installing
and rebuild while the old one is running?

Then I may have found a good reason to believe that some projects
thought hard and long and came to the same conclusion as you did.

I started from the place where I didn't find your rationale for
wanting such a construct sensible, and such a finding may tempt me
to think again.  But otherwise...

[PATCH v2 2/5] Makefile: rename scripts in-place, don't clobber

[Ævar Arnfjörð Bjarmason]

Change the rules that generate auxiliary files such as
GIT-BUILD-OPTIONS, *.s and other non-object files to use the same
in-place move pattern as we use for object files as of the preceding
commit.

Unlike the change in the preceding commit, this one isn't isn't needed
for AIX portability. I think it makes sense to do this for
consistency, we'll now use the same pattern for object and non-object
generation.

It'll also guard against any weird issues with e.g. a command that
wants to read GIT-PERL-DEFINES to generate one of the %.perl scripts
racing with either a concurrent instance of "make" that has partially
updated the file, or test-lib.sh dying with some particularly weird
error because GIT-BUILD-OPTIONS was partway through being updated when
it ran.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index b0dbf5888b7..ef2d4a9973b 100644
--- a/Makefile
+++ b/Makefile
@@ -2246,7 +2246,8 @@ git.res: git.rc GIT-VERSION-FILE GIT-PREFIX
 	$(QUIET_RC)$(RC) \
 	  $(join -DMAJOR= -DMINOR= -DMICRO= -DPATCHLEVEL=, $(wordlist 1, 4, \
 	    $(shell echo $(GIT_VERSION) 0 0 0 0 | tr '.a-zA-Z-' ' '))) \
-	  -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" -i $< -o $@
+	  -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" -i $< -o $@+ && \
+	mv $@+ $@
 
 # This makes sure we depend on the NO_PERL setting itself.
 $(SCRIPT_PERL_GEN): GIT-BUILD-OPTIONS
@@ -2293,7 +2294,8 @@ GIT-PERL-DEFINES: FORCE
 	@FLAGS='$(PERL_DEFINES)'; \
 	    if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
 		echo >&2 "    * new perl-specific parameters"; \
-		echo "$$FLAGS" >$@; \
+		echo "$$FLAGS" >$@+; \
+		mv $@+ $@; \
 	    fi
 
 GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
@@ -2455,7 +2457,8 @@ $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
 
 %.s: %.c GIT-CFLAGS FORCE
-	$(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
+	$(QUIET_CC)$(CC) -o $@+ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< && \
+	mv $@+ $@
 
 ifdef USE_COMPUTED_HEADER_DEPENDENCIES
 # Take advantage of gcc's on-the-fly dependency generation
@@ -2478,9 +2481,8 @@ endif
 ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
 all:: compile_commands.json
 compile_commands.json:
-	@$(RM) $@
-	$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+
-	@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi
+	$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json >$@+ && \
+	mv $@+ $@
 endif
 
 exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX
@@ -2673,11 +2675,13 @@ perl/build/lib/%.pm: perl/%.pm
 	$(QUIET_GEN)mkdir -p $(dir $@) && \
 	sed -e 's|@@LOCALEDIR@@|$(perl_localedir_SQ)|g' \
 	    -e 's|@@NO_PERL_CPAN_FALLBACKS@@|$(NO_PERL_CPAN_FALLBACKS_SQ)|g' \
-	< $< > $@
+	< $< >$@+ && \
+	mv $@+ $@
 
 perl/build/man/man3/Git.3pm: perl/Git.pm
 	$(QUIET_GEN)mkdir -p $(dir $@) && \
-	pod2man $< $@
+	pod2man $< $@+ && \
+	mv $@+ $@
 
 FIND_SOURCE_FILES = ( \
 	git ls-files \
@@ -2805,7 +2809,8 @@ GIT-PYTHON-VARS: FORCE
 	@VARS='$(TRACK_PYTHON)'; \
 	    if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
 		echo >&2 "    * new Python interpreter location"; \
-		echo "$$VARS" >$@; \
+		echo "$$VARS" >$@+; \
+		mv $@+ $@; \
             fi
 endif
 
@@ -2817,8 +2822,9 @@ bin-wrappers/%: wrap-for-bin.sh
 	@mkdir -p bin-wrappers
 	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
-	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
-	chmod +x $@
+	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< >$@+ && \
+	chmod +x $@+ && \
+	mv $@+ $@
 
 # GNU make supports exporting all variables by "export" without parameters.
 # However, the environment gets quite big, and some programs have problems
@@ -2866,8 +2872,9 @@ HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
 HCC = $(HCO:hco=hcc)
 
 %.hcc: %.h
-	@echo '#include "git-compat-util.h"' >$@
-	@echo '#include "$<"' >>$@
+	@echo '#include "git-compat-util.h"' >$@+ && \
+	@echo '#include "$<"' >>$@+ && \
+	mv $@+ $@
 
 $(HCO): %.hco: %.hcc FORCE
 	$(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $<
-- 
2.31.1.461.gd47399f6574

Re: [PATCH v2 2/5] Makefile: rename scripts in-place, don't clobber

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> It'll also guard against any weird issues with e.g. a command that
> wants to read GIT-PERL-DEFINES to generate one of the %.perl scripts
> racing with either a concurrent instance of "make" that has partially
> updated the file, or test-lib.sh dying with some particularly weird
> error because GIT-BUILD-OPTIONS was partway through being updated when
> it ran.

If something like that happens, doesn't that indicate a bug in the
dependency graph in the Makefile or the implementation of "make"?
The generated file is depended on for the consumer to be able to use
a non-stale version---so the consumer should not start before the
generator finishes.

You may be able to hide the breakage coming from "partially written
file is easily recognizable and the consumer would barf".  But I am
afraid that you are introducing a problem harder to diagnose, i.e.
"the consumer reads from a complete file so there is no syntax error
or other things that easily tells you there is a breakage, but what
is used is stale and not up to date".

The same comment applies to this step.  I do not think it would
break (other than adding intermediate crufts) the result if you
generate into temporary and rename to final, but it is not clear
to me what the point is in doing so.

Re: [PATCH v2 2/5] Makefile: rename scripts in-place, don't clobber

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 29 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> It'll also guard against any weird issues with e.g. a command that
>> wants to read GIT-PERL-DEFINES to generate one of the %.perl scripts
>> racing with either a concurrent instance of "make" that has partially
>> updated the file, or test-lib.sh dying with some particularly weird
>> error because GIT-BUILD-OPTIONS was partway through being updated when
>> it ran.
>
> If something like that happens, doesn't that indicate a bug in the
> dependency graph in the Makefile or the implementation of "make"?
> The generated file is depended on for the consumer to be able to use
> a non-stale version---so the consumer should not start before the
> generator finishes.

If everything we were building in the Makefile would be invoked via
other Makefile rules, yes. But if I run say (cd t && ./t0000-basic.sh)
I'm having test-lib.sh pick up one of those (possibly partial) files,
this guarantees they'll all be atomically updated.

> You may be able to hide the breakage coming from "partially written
> file is easily recognizable and the consumer would barf".  But I am
> afraid that you are introducing a problem harder to diagnose, i.e.
> "the consumer reads from a complete file so there is no syntax error
> or other things that easily tells you there is a breakage, but what
> is used is stale and not up to date".
>
> The same comment applies to this step.  I do not think it would
> break (other than adding intermediate crufts) the result if you
> generate into temporary and rename to final, but it is not clear
> to me what the point is in doing so.

I just think it makes sense to do this for consistency. So on "master"
we've got 65 hits for $@+ in the Makefile, at the end of this saga we've
got 100.

I think being consistent across the board makes things easier to read,
and in combination with the later ".DELETE_ON_ERROR" we can also drop
other copy/paste cruft.

[PATCH v2 3/5] Makefile: don't needlessly "rm $@ $@+" before "mv $@+ $@"

[Ævar Arnfjörð Bjarmason]

Now that preceding commits have moved the generation of objects and
scripts in the Makefile to use the "[...] -o $@+ && mv $@+ $@" pattern
we can stop removing "$@" and "$@+" before the rule is run.

I suppose that we could leave this at removing "$@" before we start
out, per the "age old convention" comment in[1]. I.e. instead of:

    rm -f thing thing+
    prepare contents for thing >thing+
    mv thing+ thing

Do:

    rm -f thing
    prepare contents for thing >thing+
    mv thing+ thing

Since the removal of "thing+" is redundant as we're about to clobber
it anyway, but we might be so paranoid as to be guarding against mv(1)
leaving behind a corrupt "thing".

But I think guarding against "mv" failing is a step too far in
paranoia, let's just rely on the "[...] -o $@+ && mv $@+ $@" pattern
working instead.

Especially as we'll see in a follow-up commit, we're just about to use
the GNU make ".DELETE_ON_ERROR" feature, which will reliably do this
for us.

1. http://lore.kernel.org/git/xmqqpn097e9o.fsf@gitster.c.googlers.com

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index ef2d4a9973b..f08635067b3 100644
--- a/Makefile
+++ b/Makefile
@@ -2210,7 +2210,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
 	$(perllibdir_SQ)
 define cmd_munge_script
-$(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
@@ -2278,8 +2277,7 @@ endif
 PERL_DEFINES += $(gitexecdir) $(perllibdir) $(localedir)
 
 $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1{' \
+	$(QUIET_GEN)sed -e '1{' \
 	    -e '	s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 	    -e '	r GIT-PERL-HEADER' \
 	    -e '	G' \
@@ -2299,8 +2297,7 @@ GIT-PERL-DEFINES: FORCE
 	    fi
 
 GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
-	$(QUIET_GEN)$(RM) $@ && \
-	INSTLIBDIR='$(perllibdir_SQ)' && \
+	$(QUIET_GEN)INSTLIBDIR='$(perllibdir_SQ)' && \
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
@@ -2325,8 +2322,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 	mv $@+ $@
 else # NO_PERL
 $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \
@@ -2339,15 +2335,13 @@ $(SCRIPT_PYTHON_GEN): GIT-BUILD-OPTIONS
 ifndef NO_PYTHON
 $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
 $(SCRIPT_PYTHON_GEN): % : %.py
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
+	$(QUIET_GEN)sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
 	    $< >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PYTHON
 $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \
-- 
2.31.1.461.gd47399f6574

Re: [PATCH v2 3/5] Makefile: don't needlessly "rm $@ $@+" before "mv $@+ $@"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>     rm -f thing thing+
>     prepare contents for thing >thing+
>     mv thing+ thing
> ...
> But I think guarding against "mv" failing is a step too far in
> paranoia,...

If mv fails the $(MAKE) rule would fail, so it is OK.  It may leave
thing+ behind, but there is no reason to expect why you would be
able to rm it the next time, so from that point of view, it is OK to
drop the first "rm -f thing+", I would think.  The only case I can
thing of that would help is when you are sharing the working tree
with your team member, the directories are writable to both of you,
but somehow the other person creates thing+ with 0644 or 0755 mode
bits.  You cannot redirect into thing+ the other person left behind,
but you can "rm -f thing+ && cmd >$thing+" (or "cmd -o $thing+") in
such a situation, and that is probably where the pattern comes from
---i.e. simple hygiene.

[PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

Use the GNU make ".DELETE_ON_ERROR" flag. As discussed in preceding
commits we're now less paranoid about "mv" failing, let's bring that
paranoia back in a way that makes more sense, and applies to all rules
in the Makefile.

Now if a command to make X fails X will be removed, the default
behavior of GNU make is to only do so if "make" itself is interrupted
with a signal.

E.g. if we now intentionally break one of the rules with:

    -       mv $@+ $@
    +       mv $@+ $@ && \
    +       false

We'll get output like:

    $ make git
        CC git.o
        LINK git
    make: *** [Makefile:2179: git] Error 1
    make: *** Deleting file 'git'
    $ file git
    git: cannot open `git' (No such file or directory)

Before this change we'd leave the file in place in under this
scenario.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Makefile b/Makefile
index f08635067b3..573bce20357 100644
--- a/Makefile
+++ b/Makefile
@@ -2126,6 +2126,16 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $^
 
+### Flags affecting all rules
+
+# A GNU make extension since gmake 3.72 (released in late 1994) to
+# remove the target of rules if commands in those rules fail. The
+# default is to only do that if make itself receives a signal. Affects
+# all targets, see:
+#
+#    info make --index-search=.DELETE_ON_ERROR
+.DELETE_ON_ERROR:
+
 ### Target-specific flags and dependencies
 
 # The generic compilation pattern rule and automatically
-- 
2.31.1.461.gd47399f6574

Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Use the GNU make ".DELETE_ON_ERROR" flag.

Yay!

With versions of $(MAKE) where this feature is available, it is far
more preferrable to use it than "generate into temporary and rename
to the final" dance.

We do require / depend on GNU but I do not offhand know if this is
available in all versions that still matter.  If we know we can
assume the presence of this feature the I do not mind if we jump
directly to this step without those "do the same for $(CC)" steps
(which I deem crazy) before it.

Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 29 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Use the GNU make ".DELETE_ON_ERROR" flag.
>
> Yay!
>
> With versions of $(MAKE) where this feature is available, it is far
> more preferrable to use it than "generate into temporary and rename
> to the final" dance.
>
> We do require / depend on GNU but I do not offhand know if this is
> available in all versions that still matter.  If we know we can
> assume the presence of this feature the I do not mind if we jump
> directly to this step without those "do the same for $(CC)" steps
> (which I deem crazy) before it.

Even if it's not available in all versions that's OK. You just won't get
the nicer removal behavior on on error on such a jurassic gmake, but
you'll probably have way bigger issues with your late-90s-era software.

It's not a syntax error on a gmake or other make that doesn't know about
it either, i.e. you can also add a target like:

    .THIS_DOES_NOT_EXIST_AS_A_GMAKE_FEATURE:

And gmake willl happily ignore it.

Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Even if it's not available in all versions that's OK. You just won't get
> the nicer removal behavior on on error on such a jurassic gmake, but
> you'll probably have way bigger issues with your late-90s-era software.
>
> It's not a syntax error on a gmake or other make that doesn't know about
> it either, i.e. you can also add a target like:
>
>     .THIS_DOES_NOT_EXIST_AS_A_GMAKE_FEATURE:
>
> And gmake willl happily ignore it.

What I meant was that I would not tolerate "cc -o x+ && mv x+ x",
but I do not mind DELETE_ON_ERROR with "cc -o x" at all.

Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag

[Jeff King]

On Tue, Mar 30, 2021 at 01:31:40AM +0200, Ævar Arnfjörð Bjarmason wrote:

> >> Use the GNU make ".DELETE_ON_ERROR" flag.
> >
> > Yay!
> >
> > With versions of $(MAKE) where this feature is available, it is far
> > more preferrable to use it than "generate into temporary and rename
> > to the final" dance.
> >
> > We do require / depend on GNU but I do not offhand know if this is
> > available in all versions that still matter.  If we know we can
> > assume the presence of this feature the I do not mind if we jump
> > directly to this step without those "do the same for $(CC)" steps
> > (which I deem crazy) before it.
> 
> Even if it's not available in all versions that's OK. You just won't get
> the nicer removal behavior on on error on such a jurassic gmake, but
> you'll probably have way bigger issues with your late-90s-era software.
> 
> It's not a syntax error on a gmake or other make that doesn't know about
> it either, i.e. you can also add a target like:
> 
>     .THIS_DOES_NOT_EXIST_AS_A_GMAKE_FEATURE:
> 
> And gmake willl happily ignore it.

Yes, I think it's fine to treat it as "nice if we have it, but OK
otherwise". But also, .DELETE_ON_ERROR first shows up in the GNU make
git repository in version 3.71.5 from 1994. So I think we can probably
just assume it's everywhere.

-Peff

Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 30, 2021 at 01:31:40AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >> Use the GNU make ".DELETE_ON_ERROR" flag.
>> >
>> > Yay!
>> >
>> > With versions of $(MAKE) where this feature is available, it is far
>> > more preferrable to use it than "generate into temporary and rename
>> > to the final" dance.
>> >
>> > We do require / depend on GNU but I do not offhand know if this is
>> > available in all versions that still matter.  If we know we can
>> > assume the presence of this feature the I do not mind if we jump
>> > directly to this step without those "do the same for $(CC)" steps
>> > (which I deem crazy) before it.
>> 
>> Even if it's not available in all versions that's OK. You just won't get
>> the nicer removal behavior on on error on such a jurassic gmake, but
>> you'll probably have way bigger issues with your late-90s-era software.
>> 
>> It's not a syntax error on a gmake or other make that doesn't know about
>> it either, i.e. you can also add a target like:
>> 
>>     .THIS_DOES_NOT_EXIST_AS_A_GMAKE_FEATURE:
>> 
>> And gmake willl happily ignore it.
>
> Yes, I think it's fine to treat it as "nice if we have it, but OK
> otherwise". But also, .DELETE_ON_ERROR first shows up in the GNU make
> git repository in version 3.71.5 from 1994. So I think we can probably
> just assume it's everywhere.

OK.  That raises my hopes up that we may be able to simplify things
like this

    config-list.h:
            $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
                    >$@+ && mv $@+ $@

into

    config-list.h:
            $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh >$@

Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag

[Jeff King]

On Tue, Mar 30, 2021 at 11:36:06AM -0700, Junio C Hamano wrote:

> > Yes, I think it's fine to treat it as "nice if we have it, but OK
> > otherwise". But also, .DELETE_ON_ERROR first shows up in the GNU make
> > git repository in version 3.71.5 from 1994. So I think we can probably
> > just assume it's everywhere.
> 
> OK.  That raises my hopes up that we may be able to simplify things
> like this
> 
>     config-list.h:
>             $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
>                     >$@+ && mv $@+ $@
> 
> into
> 
>     config-list.h:
>             $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh >$@

Yes, I think .DELETE_ON_ERROR would accomplish roughly the same thing.
Though note that it's essentially a "rollback" strategy. We create the
broken file, realize there was an error, and the roll it back (as
opposed to the "mv" strategy, which confirms everything is good before
committing it into place). This is worse than a "commit" strategy in a
few ways:

  - somebody else may racily see the broken state. I'm not too concerned
    with this, and from the rest of the thread I don't think you are
    either.

  - we may be left in the broken state if the rollback fails. Plausible
    reasons are: somebody kills "make" (I'd hope on the obvious ^C that
    it catches the signal and deletes before exiting), bug in make,
    transient OS error, power failure, etc.

I don't know how paranoid we want to be about this, especially the
latter. My general inclination is to prefer "commit" systems as more
robust, but it is just a Makefile.

-Peff

Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't know how paranoid we want to be about this, especially the
> latter. My general inclination is to prefer "commit" systems as more
> robust, but it is just a Makefile.

;-)

As an old timer, I've written "gen >$@+ && mv $@+ $@" all the time
myself, but it is ugly and felt a bit too conservative.  I do not
think it is wise to overnight remove all the existing "generate in
temporary, move to the final" patterns and delegate $(MAKE) to take
care of failed generator with this mechanism, but I actually would
feel it probably gives us a cleaner Makefile in the longer term.  At
least "bugs in $(MAKE)" won't be our sole problem (i.e. all other
projects that rely on the feature would share the incentives to see
them fixed).

Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag

[Johannes Schindelin]

Hi,

On Wed, 31 Mar 2021, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
> > I don't know how paranoid we want to be about this, especially the
> > latter. My general inclination is to prefer "commit" systems as more
> > robust, but it is just a Makefile.
>
> ;-)
>
> As an old timer, I've written "gen >$@+ && mv $@+ $@" all the time
> myself, but it is ugly and felt a bit too conservative.  I do not
> think it is wise to overnight remove all the existing "generate in
> temporary, move to the final" patterns and delegate $(MAKE) to take
> care of failed generator with this mechanism, but I actually would
> feel it probably gives us a cleaner Makefile in the longer term.  At
> least "bugs in $(MAKE)" won't be our sole problem (i.e. all other
> projects that rely on the feature would share the incentives to see
> them fixed).

Another point in favor of removing that hack is this: `+` is not a valid
character in Windows' filenames. It just so _happens_ that Cygwin (and
therefore the MSYS2 Bash/`make` we use to compile Git for Windows)
_pretends_ that it is a valid filename character, but regular Win32
programs cannot read/write such files, and it would preclude Git from
being compiled with more native versions of a POSIX shell or `make`.

Ciao,
Dscho

[PATCH v2 5/5] Makefile: don't "rm configure" before generating it

[Ævar Arnfjörð Bjarmason]

Remove a "rm configure" before we make a new "configure" file via a
"configure+" temporary file. This was partially made redundant via the
new ".DELETE_ON_ERROR" target.

"Partially" because if we die partway we'll remove the "configure"
now, but as before we'll leave the "configure.ac+" in place. Ideally
we'd have another target for the "configure.ac+" so this would all
work properly, but due to the logic added in 122650457a6 (build: do
not automatically reconfigure unless configure.ac changed, 2013-01-02)
that's non-trivial to untangle.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 573bce20357..ce76c476a3c 100644
--- a/Makefile
+++ b/Makefile
@@ -2358,8 +2358,7 @@ $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
 	mv $@+ $@
 endif # NO_PYTHON
 
-CONFIGURE_RECIPE = $(RM) configure configure.ac+ && \
-		   sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
+CONFIGURE_RECIPE = sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 			configure.ac >configure.ac+ && \
 		   autoconf -o configure configure.ac+ && \
 		   $(RM) configure.ac+
-- 
2.31.1.461.gd47399f6574

[PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane

[Ævar Arnfjörð Bjarmason]

This is on top of my just-submitted "Makefile: don't die on AIX with
open ./git" series: [1]

This series introduces no "real" behavior changes I'd expect anyone to
notice, but refactors a lengthy copy/pasted test/if/else in the
Makefile into a simple helper script.

The "real" behavior change is that we no longer ask the user how
they'd like to install (symlinks, hardlinks, neither?) and then
proceed to ignore what was asked of us and optimistically fallback in
case of errors. I.e. the inability to create symlinks or hardlinks.

Instead we'll just die, the old behavior is still available as
INSTALL_FALLBACK_LN_CP. In practice I think exactly nobody actually
wanted the existing behavior.

It's just something that emerged over almost 2 decades of first
wanting to have the ability to specify such a fallback, and then
adding e.g. support for INSTALL_SYMLINKS along the way.

There's also side-discussion of a bug I discovered along the way in
SKIP_DASHED_BUILT_INS in 4/6. This series doesn't make that bug better
or worse, but it interacts with the flags being changed here.

1. https://lore.kernel.org/git/cover-0.6-00000000000-20210329T161723Z-avarab@gmail.com/
   In practice they apply independently, but since they're touching
   some very adjacent code I'm saying it's "on top" in case a re-roll
   makes it so, and also for ease of local testing.

   I'm trying a new thing of splitting my serieses up a bit, so if
   there's outstanding feedback on the later parts, hopefully the
   former part can proceed independently...

Ævar Arnfjörð Bjarmason (6):
  Makefile: symlink the same way under "symlinks" and "no hardlinks"
  Makefile: begin refactoring out "ln || ln -s || cp" pattern
  Makefile: refactor out "ln || ln -s || cp" pattern
  Makefile: make INSTALL_SYMLINKS affect the build directory
  Makefile: use "ln -f" instead of "rm && ln"
  Makefile: add a INSTALL_FALLBACK_LN_CP mode

 .gitignore  |  1 +
 Makefile    | 91 +++++++++++++++++++++++++++++++----------------------
 ln-or-cp.sh | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+), 38 deletions(-)
 create mode 100755 ln-or-cp.sh

-- 
2.31.1.461.gd47399f6574

[PATCH 1/6] Makefile: symlink the same way under "symlinks" and "no hardlinks"

[Ævar Arnfjörð Bjarmason]

When the INSTALL_SYMLINKS option was added in ad874608d8c (Makefile:
optionally symlink libexec/git-core binaries to bin/git, 2018-03-13)
we retained bug-for-bug compatibility with how the old
NO_INSTALL_HARDLINKS=Y would selectively fall back on symlinks.

In particular INSTALL_SYMLINKS=Y will result in a link tree like:

    bin/git
    libexec/git -> ../bin/git
    libexec/git-add -> ../bin/git

Whereas NO_INSTALL_HARDLINKS=Y in cases where the "ln" would fail would result in:

    bin/git
    libexec/git
    libexec/git-add -> git

I.e. we duplicated the "git" between the bin/ and libexec/
directories (by default they're hardlinked), and potentially had had
e.g. "git-add" pointing at the libexec/git hardlink (or more likely if
"ln" is failing, a copy), instead of the bin/git.

Now we'll instead use the same symlinks to point to the bindir. I
don't see any reason not to do so, and no breakage related to this has
been reported with INSTALL_SYMLINKS in all this time. I just did it
this way to maintain bug-for-bug compatibility at the time.

There is a behavior change here, if the "ln -s" fails we'll no longer
direct it to stderr. Supporting that edge case would be painful, and
as we'll see in subsequent commits that difference is going away
anyway, so let's proceed for now without retaining it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index ce76c476a3c..1e59d90a8d2 100644
--- a/Makefile
+++ b/Makefile
@@ -334,15 +334,16 @@ all::
 # Define INSTALL_SYMLINKS if you prefer to have everything that can be
 # symlinked between bin/ and libexec/ to use relative symlinks between
 # the two. This option overrides NO_CROSS_DIRECTORY_HARDLINKS and
-# NO_INSTALL_HARDLINKS which will also use symlinking by indirection
-# within the same directory in some cases, INSTALL_SYMLINKS will
+# NO_INSTALL_HARDLINKS. This will not produce any indirect symlinks, we will
 # always symlink to the final target directly.
 #
 # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
 # programs as a tar, where bin/ and libexec/ might be on different file systems.
 #
-# Define NO_INSTALL_HARDLINKS if you prefer to use either symbolic links or
-# copies to install built-in git commands e.g. git-cat-file.
+# Define NO_INSTALL_HARDLINKS if you'd like to have programs in bin/
+# and libexec/ either symlinked (we try with INSTALL_SYMLINKS first),
+# or if that fails fall back on a "cp" instead of a "ln". Useful for
+# when you don't want hardlinks at all.
 #
 # Define SKIP_DASHED_BUILT_INS if you do not need the dashed versions of the
 # built-ins to be linked/copied at all.
@@ -3019,33 +3020,30 @@ endif
 	} && \
 	for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
 		$(RM) "$$bindir/$$p" && \
-		test -n "$(INSTALL_SYMLINKS)" && \
+		test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \
 		ln -s "git$X" "$$bindir/$$p" || \
 		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
 		  ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
-		  ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
 		  cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \
 	done && \
 	for p in $(BUILT_INS); do \
 		$(RM) "$$execdir/$$p" && \
 		if test -z "$(SKIP_DASHED_BUILT_INS)"; \
 		then \
-			test -n "$(INSTALL_SYMLINKS)" && \
+			test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \
 			ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \
 			{ test -z "$(NO_INSTALL_HARDLINKS)" && \
 			  ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
-			  ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
 			  cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \
 		fi \
 	done && \
 	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
 	for p in $$remote_curl_aliases; do \
 		$(RM) "$$execdir/$$p" && \
-		test -n "$(INSTALL_SYMLINKS)" && \
+		test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \
 		ln -s "git-remote-http$X" "$$execdir/$$p" || \
 		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
 		  ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
-		  ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
 		  cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
 	done && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
-- 
2.31.1.461.gd47399f6574

Re: [PATCH 1/6] Makefile: symlink the same way under "symlinks" and "no hardlinks"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> In particular INSTALL_SYMLINKS=Y will result in a link tree like:
>
>     bin/git
>     libexec/git -> ../bin/git
>     libexec/git-add -> ../bin/git
>
> Whereas NO_INSTALL_HARDLINKS=Y in cases where the "ln" would fail would result in:
>
>     bin/git
>     libexec/git
>     libexec/git-add -> git
>
> I.e. we duplicated the "git" between the bin/ and libexec/
> directories (by default they're hardlinked), and potentially had had
> e.g. "git-add" pointing at the libexec/git hardlink (or more likely if
> "ln" is failing, a copy), instead of the bin/git.
>
> Now we'll instead use the same symlinks to point to the bindir. I
> don't see any reason not to do so, and no breakage related to this has
> been reported with INSTALL_SYMLINKS in all this time. I just did it
> this way to maintain bug-for-bug compatibility at the time.

Makes sense.  I do not see a reason why libexec/git-add that points
at ../bin/git would cause problems, either.

> +# Define NO_INSTALL_HARDLINKS if you'd like to have programs in bin/
> +# and libexec/ either symlinked (we try with INSTALL_SYMLINKS first),
> +# or if that fails fall back on a "cp" instead of a "ln". Useful for
> +# when you don't want hardlinks at all.

So without INSTALL_SYMLINKS and with NO_INSTALL_HARDLINKS, the only
remaining choice is "cp".  With both set, we favour "ln -s" over
"cp" and do not allow "ln".  With INSTALL_SYMLINKS and without
NO_INSTALL_HARDLINKS, we try "ln -s", "ln" and finally "cp".  With
neither, we try "ln" and fall back to "cp"?

That precedence order does make sense.

> @@ -3019,33 +3020,30 @@ endif
>  	} && \
>  	for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
>  		$(RM) "$$bindir/$$p" && \
> -		test -n "$(INSTALL_SYMLINKS)" && \
> +		test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \

I had an impression that we avoid -o/-a used with "test" (instead
we'd use "test && test" or "test || test")?

In either case, if we are told to favor "ln -s", or told not to use
"ln", we try "ln -s"?  That does not make much sense to me, though.

What do I need to do if I do not ever want symbolic links?  Is it
impossible now?

>  		ln -s "git$X" "$$bindir/$$p" || \
>  		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
>  		  ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
> -		  ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
>  		  cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \
>  	done && \
>  	for p in $(BUILT_INS); do \
>  		$(RM) "$$execdir/$$p" && \
>  		if test -z "$(SKIP_DASHED_BUILT_INS)"; \
>  		then \
> -			test -n "$(INSTALL_SYMLINKS)" && \
> +			test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \

Perhaps the same comment applies here and to the next hunk, too?

>  			ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \
>  			{ test -z "$(NO_INSTALL_HARDLINKS)" && \
>  			  ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
> -			  ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
>  			  cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \
>  		fi \
>  	done && \
>  	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
>  	for p in $$remote_curl_aliases; do \
>  		$(RM) "$$execdir/$$p" && \
> -		test -n "$(INSTALL_SYMLINKS)" && \
> +		test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \
>  		ln -s "git-remote-http$X" "$$execdir/$$p" || \
>  		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
>  		  ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
> -		  ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
>  		  cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
>  	done && \
>  	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"

[PATCH 2/6] Makefile: begin refactoring out "ln || ln -s || cp" pattern

[Ævar Arnfjörð Bjarmason]

Begin refactoring out the "ln || ln -s || cp" pattern in the Makefile
into a script. For now this is trivial, but in subsequent commits
it'll simplify things a lot.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile    | 8 ++------
 ln-or-cp.sh | 8 ++++++++
 2 files changed, 10 insertions(+), 6 deletions(-)
 create mode 100755 ln-or-cp.sh

diff --git a/Makefile b/Makefile
index 1e59d90a8d2..cfc87d7734d 100644
--- a/Makefile
+++ b/Makefile
@@ -2199,9 +2199,7 @@ version.sp version.s version.o: EXTRA_CPPFLAGS = \
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
-	ln $< $@ 2>/dev/null || \
-	ln -s $< $@ 2>/dev/null || \
-	cp $< $@
+	./ln-or-cp.sh $< $@
 
 config-list.h: generate-configlist.sh
 
@@ -2552,9 +2550,7 @@ git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
 
 $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
 	$(QUIET_LNCP)$(RM) $@ && \
-	ln $< $@ 2>/dev/null || \
-	ln -s $< $@ 2>/dev/null || \
-	cp $< $@
+	./ln-or-cp.sh $< $@
 
 $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \
diff --git a/ln-or-cp.sh b/ln-or-cp.sh
new file mode 100755
index 00000000000..de79cd85a81
--- /dev/null
+++ b/ln-or-cp.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+target="$1"
+link="$2"
+
+ln "$target" "$link" 2>/dev/null ||
+ln -s "$target" "$link" 2>/dev/null ||
+cp "$target" "$link"
-- 
2.31.1.461.gd47399f6574

Re: [PATCH 2/6] Makefile: begin refactoring out "ln || ln -s || cp" pattern

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Begin refactoring out the "ln || ln -s || cp" pattern in the Makefile
> into a script. For now this is trivial, but in subsequent commits
> it'll simplify things a lot.

I agree with the approach.  The precedence glitch I commented on in
my review of [1/6] (e.g. how would I say "I never ever want symbolic
links---just use hardlink and fall back to copying") would be much
easier to solve if a single helper is used consistenly---that would
give us a single place to centrally fix.

[PATCH 3/6] Makefile: refactor out "ln || ln -s || cp" pattern

[Ævar Arnfjörð Bjarmason]

Refactor out the hard-to-read and maintain "ln || ln -s || cp"
pattern.

This was initially added in 3e073dc5611 (Makefile: always provide a
fallback when hardlinks fail, 2008-08-25), but since then it's become
a lot more complex as we've added:

 * 3426e34fedd (Add NO_CROSS_DIRECTORY_HARDLINKS support to the
   Makefile, 2009-05-11)

 * NO_INSTALL_HARDLINKS in 70de5e65e8c (Makefile:
   NO_INSTALL_HARDLINKS, 2012-05-02)

 * INSTALL_SYMLINKS in ad874608d8c (Makefile: optionally symlink
   libexec/git-core binaries to bin/git, 2018-03-13)

 * SKIP_DASHED_BUILT_INS 179227d6e21 (Optionally skip linking/copying
   the built-ins, 2020-09-21)

Each of those commits had to add a new special-case to this code,
resulting in quite an unmaintainable mess for adding any sort of new
option.

Let's use the newly introduced ln-or-cp.sh script instead, note that
we only sometimes pass the --no-cross-directory-hardlinks option, per
the previous behavior. The target of the "ln -s" is also another
special snowflake, but we're careful to carry that forward.

As in an earlier commit this also changes the behavior to emit any
errors to stdout. In that earlier case that was done to simplify the
script so that it can use one "ln -s" instead of the two, likewise
we're now unconditionally emitting to stderr if ln (without -s, to
create the hardlink) fails. We always emitted to stderr if "cp"
failed.

As in that earlier commit let's let that pass for now, yes it might be
very verbose in some scenarios, but we're working our way towards a
simpler end-state here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile    | 41 +++++++++++++++++-----------------
 ln-or-cp.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/Makefile b/Makefile
index cfc87d7734d..a4784f28f5b 100644
--- a/Makefile
+++ b/Makefile
@@ -3007,40 +3007,41 @@ endif
 	{ test "$$bindir/" = "$$execdir/" || \
 	  for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
 		$(RM) "$$execdir/$$p" && \
-		test -n "$(INSTALL_SYMLINKS)" && \
-		ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \
-		{ test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
-		  ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
-		  cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \
+		./ln-or-cp.sh \
+			--install-symlinks "$(INSTALL_SYMLINKS)" \
+			--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
+			--no-cross-directory-hardlinks "$(NO_CROSS_DIRECTORY_HARDLINKS)" \
+			--symlink-target "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" \
+			"$$bindir/$$p" "$$execdir/$$p"; \
 	  done; \
 	} && \
 	for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
 		$(RM) "$$bindir/$$p" && \
-		test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \
-		ln -s "git$X" "$$bindir/$$p" || \
-		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
-		  ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
-		  cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \
+		./ln-or-cp.sh \
+			--install-symlinks "$(INSTALL_SYMLINKS)" \
+			--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
+			--symlink-target "git$X" \
+			"$$bindir/git$X" "$$bindir/$$p"; \
 	done && \
 	for p in $(BUILT_INS); do \
 		$(RM) "$$execdir/$$p" && \
 		if test -z "$(SKIP_DASHED_BUILT_INS)"; \
 		then \
-			test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \
-			ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \
-			{ test -z "$(NO_INSTALL_HARDLINKS)" && \
-			  ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
-			  cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \
+			./ln-or-cp.sh \
+				--install-symlinks "$(INSTALL_SYMLINKS)" \
+				--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
+				--symlink-target "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" \
+				"$$execdir/git$X" "$$execdir/$$p"; \
 		fi \
 	done && \
 	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
 	for p in $$remote_curl_aliases; do \
 		$(RM) "$$execdir/$$p" && \
-		test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \
-		ln -s "git-remote-http$X" "$$execdir/$$p" || \
-		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
-		  ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
-		  cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
+		./ln-or-cp.sh \
+			--install-symlinks "$(INSTALL_SYMLINKS)" \
+			--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
+			--symlink-target "git-remote-http$X" \
+			"$$execdir/git-remote-http$X" "$$execdir/$$p"; \
 	done && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 
diff --git a/ln-or-cp.sh b/ln-or-cp.sh
index de79cd85a81..663ffd0489d 100755
--- a/ln-or-cp.sh
+++ b/ln-or-cp.sh
@@ -1,8 +1,65 @@
 #!/bin/sh
 
+install_symlinks=
+no_install_hardlinks=
+no_cross_directory_hardlinks=
+symlink_target=
+while test $# != 0
+do
+	case "$1" in
+	--install-symlinks)
+		install_symlinks="$2"
+		shift
+		;;
+	--no-install-hardlinks)
+		no_install_hardlinks="$2"
+		shift
+		;;
+	--no-cross-directory-hardlinks)
+		no_cross_directory_hardlinks="$2"
+		shift
+		;;
+	--symlink-target)
+		symlink_target="$2"
+		shift
+		;;
+	*)
+		break
+		;;
+	esac
+	shift
+done
+
 target="$1"
+if test -z "$symlink_target"
+then
+	symlink_target="$target"
+fi
 link="$2"
 
-ln "$target" "$link" 2>/dev/null ||
-ln -s "$target" "$link" 2>/dev/null ||
-cp "$target" "$link"
+hardlink_or_cp () {
+	if test -z "$no_install_hardlinks" -a -z "$no_cross_directory_hardlinks"
+	then
+		if ! ln "$target" "$link"
+		then
+			cp "$target" "$link"
+		fi
+
+	else
+		cp "$target" "$link"
+	fi
+}
+
+main_with_fallbacks () {
+	if test -n "$install_symlinks" -o -n "$no_install_hardlinks"
+	then
+		if ! ln -s "$symlink_target" "$link"
+		then
+			hardlink_or_cp
+		fi
+	else
+		hardlink_or_cp
+	fi
+}
+
+main_with_fallbacks
-- 
2.31.1.461.gd47399f6574

Re: [PATCH 3/6] Makefile: refactor out "ln || ln -s || cp" pattern

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Refactor out the hard-to-read and maintain "ln || ln -s || cp"
> pattern.
>
> This was initially added in 3e073dc5611 (Makefile: always provide a
> fallback when hardlinks fail, 2008-08-25), but since then it's become
> a lot more complex as we've added:
>
>  * 3426e34fedd (Add NO_CROSS_DIRECTORY_HARDLINKS support to the
>    Makefile, 2009-05-11)
>
>  * NO_INSTALL_HARDLINKS in 70de5e65e8c (Makefile:
>    NO_INSTALL_HARDLINKS, 2012-05-02)
>
>  * INSTALL_SYMLINKS in ad874608d8c (Makefile: optionally symlink
>    libexec/git-core binaries to bin/git, 2018-03-13)
>
>  * SKIP_DASHED_BUILT_INS 179227d6e21 (Optionally skip linking/copying
>    the built-ins, 2020-09-21)
>
> Each of those commits had to add a new special-case to this code,
> resulting in quite an unmaintainable mess for adding any sort of new
> option.
>
> Let's use the newly introduced ln-or-cp.sh script instead, note that
> we only sometimes pass the --no-cross-directory-hardlinks option, per
> the previous behavior. The target of the "ln -s" is also another
> special snowflake, but we're careful to carry that forward.

Nice.  These explicit command-line options to the helper may be
harder to initially write and maintain than just exporting the
relevant $(MAKE) macros and using it from the helper, but once
it works correctly, it is much easier to see what is going on.

And obviously, if we want to fix the "I do not ever want to see any
symlinks", it is very clear that main_with_fallbacks is where the
change needs to go.

Raelly nice.

Re: [PATCH 3/6] Makefile: refactor out "ln || ln -s || cp" pattern

[Jeff King]

On Mon, Mar 29, 2021 at 03:24:33PM -0700, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
> > Refactor out the hard-to-read and maintain "ln || ln -s || cp"
> > pattern.
> >
> > This was initially added in 3e073dc5611 (Makefile: always provide a
> > fallback when hardlinks fail, 2008-08-25), but since then it's become
> > a lot more complex as we've added:
> >
> >  * 3426e34fedd (Add NO_CROSS_DIRECTORY_HARDLINKS support to the
> >    Makefile, 2009-05-11)
> >
> >  * NO_INSTALL_HARDLINKS in 70de5e65e8c (Makefile:
> >    NO_INSTALL_HARDLINKS, 2012-05-02)
> >
> >  * INSTALL_SYMLINKS in ad874608d8c (Makefile: optionally symlink
> >    libexec/git-core binaries to bin/git, 2018-03-13)
> >
> >  * SKIP_DASHED_BUILT_INS 179227d6e21 (Optionally skip linking/copying
> >    the built-ins, 2020-09-21)
> >
> > Each of those commits had to add a new special-case to this code,
> > resulting in quite an unmaintainable mess for adding any sort of new
> > option.
> >
> > Let's use the newly introduced ln-or-cp.sh script instead, note that
> > we only sometimes pass the --no-cross-directory-hardlinks option, per
> > the previous behavior. The target of the "ln -s" is also another
> > special snowflake, but we're careful to carry that forward.
> 
> Nice.  These explicit command-line options to the helper may be
> harder to initially write and maintain than just exporting the
> relevant $(MAKE) macros and using it from the helper, but once
> it works correctly, it is much easier to see what is going on.

Another option is to make "ln-or-cp" itself a target that "make" knows
how to build, and bake the values into its "built" version. Besides
making the invocations a bit shorter, it also means that the dependency
graph is more correct. If a rule invokes ln-or-cp, its behavior will
change if $NO_INSTALL_HARDLINKS changes, for example. Telling that to
make requires depending on a sentinel file in each such caller (like
GIT-BUILD-OPTIONS). Whereas we could do that once for the "ln-or-cp"
target, and then everyone who uses it just depends on it.

I had a series a long time ago that moved the whole Makefile in that
direction, but I got nervous that it was too disruptive and too
non-idiomatic to be worth pursuing. So I offer the alternative here
mostly as food for thought. It may not be a good direction (and we
already have good-enough solutions, like depending on
GIT-BUILD-OPTIONS).

-Peff

Re: [PATCH 3/6] Makefile: refactor out "ln || ln -s || cp" pattern

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Nice.  These explicit command-line options to the helper may be
>> harder to initially write and maintain than just exporting the
>> relevant $(MAKE) macros and using it from the helper, but once
>> it works correctly, it is much easier to see what is going on.
>
> Another option is to make "ln-or-cp" itself a target that "make" knows
> how to build, and bake the values into its "built" version.

Aha.  That is even nicer ;-).

[PATCH 4/6] Makefile: make INSTALL_SYMLINKS affect the build directory

[Ævar Arnfjörð Bjarmason]

Change the INSTALL_SYMLINKS flag to also affect whether a built-in
like e.g. git-fetch is a symlink or not in git's build directory.

This doesn't matter for anything other than as an aid to developers
who might be confused about their build not matching the installation,
and who'd like to be reminded that e.g. "git-fetch" is a built-in by
"ls" coloring appropriately it as a symlink.

In order to make this work we need to rebuild the relevant programs
when the INSTALL_SYMLINKS flag changes. This also ensures that we'll
install the right thing, we don't want a different "INSTALL_SYMLINKS"
setting during "make all" and "make install" to result in a broken
installation.

We will do the wrong thing here if both the SKIP_DASHED_BUILT_INS and
INSTALL_SYMLINKS are being flipped. But that's not a new bug:

A build with "INSTALL_SYMLINKS=Y SKIP_DASHED_BUILT_INS=" will result
in e.g. "git-fetch" being a symlink. When building again with
"INSTALL_SYMLINKS= SKIP_DASHED_BUILT_INS=Y", only unconditionally
built programs such as "git-upload-pack" will correctly be flipped to
a hardlink, but e.g. "git-fetch" will be left as a symlink.

That's an existing bug (or unexpected behavior) in the
SKIP_DASHED_BUILT_INS flag, not something new being introduced or made
worse here. It's a bit more noticeable now as we might not expect
these now-stale symlinks to be left behind, and "ls" (in some
configurations) will color them prominently.

But we'll still do the right thing on "make install" since we'll
ignore the likes of "git-fetch" there under "SKIP_DASHED_BUILT_INS=Y".
Under "SKIP_DASHED_BUILT_INS=" we'll correctly flip the symlink to a
hardlink or vice-versa if needed before installation.

Still, we should get around to fixing that SKIP_DASHED_BUILT_INS. You
can't reliably set that flag to "Y" for checking whether the tests
rely on the now-skipped dashed built-ins without first running "make
clean" (or knowing you've always been building with
SKIP_DASHED_BUILT_INS=Y).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore |  1 +
 Makefile   | 19 +++++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index 3dcdb6bb5ab..f90aa21b23b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -5,6 +5,7 @@
 /GIT-BUILD-OPTIONS
 /GIT-CFLAGS
 /GIT-LDFLAGS
+/GIT-LNCPFLAGS
 /GIT-PREFIX
 /GIT-PERL-DEFINES
 /GIT-PERL-HEADER
diff --git a/Makefile b/Makefile
index a4784f28f5b..29d9bade5a8 100644
--- a/Makefile
+++ b/Makefile
@@ -337,6 +337,11 @@ all::
 # NO_INSTALL_HARDLINKS. This will not produce any indirect symlinks, we will
 # always symlink to the final target directly.
 #
+# NO_INSTALL_HARDLINKS which will also use symlinking by indirection
+# within the same directory in some cases, INSTALL_SYMLINKS will
+# always symlink to the final target directly. This option also
+# affects dashed built-ins in the build directory pre-installation.
+#
 # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
 # programs as a tar, where bin/ and libexec/ might be on different file systems.
 #
@@ -2197,9 +2202,12 @@ version.sp version.s version.o: EXTRA_CPPFLAGS = \
 		GIT_CEILING_DIRECTORIES="$(CURDIR)/.." \
 		git rev-parse -q --verify HEAD 2>/dev/null)"'
 
+$(BUILT_INS): GIT-LNCPFLAGS
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
-	./ln-or-cp.sh $< $@
+	./ln-or-cp.sh \
+		--install-symlinks "$(INSTALL_SYMLINKS)" \
+		$< $@
 
 config-list.h: generate-configlist.sh
 
@@ -2548,9 +2556,12 @@ git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
 		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) && \
 	mv $@+ $@
 
+$(REMOTE_CURL_ALIASES): GIT-LNCPFLAGS
 $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
 	$(QUIET_LNCP)$(RM) $@ && \
-	./ln-or-cp.sh $< $@
+	./ln-or-cp.sh \
+		--install-symlinks "$(INSTALL_SYMLINKS)" \
+		$< $@
 
 $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \
@@ -2742,6 +2753,10 @@ GIT-LDFLAGS: FORCE
 		echo "$$FLAGS" >GIT-LDFLAGS; \
             fi
 
+GIT-LNCPFLAGS: FORCE
+	@echo INSTALL_SYMLINKS=\''$(subst ','\'',$(subst ','\'',$(INSTALL_SYMLINKS)))'\' >$@+
+	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+
 # We need to apply sq twice, once to protect from the shell
 # that runs GIT-BUILD-OPTIONS, and then again to protect it
 # and the first level quoting from the shell that runs "echo".
-- 
2.31.1.461.gd47399f6574

Re: [PATCH 4/6] Makefile: make INSTALL_SYMLINKS affect the build directory

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the INSTALL_SYMLINKS flag to also affect whether a built-in
> like e.g. git-fetch is a symlink or not in git's build directory.
>
> This doesn't matter for anything other than as an aid to developers
> who might be confused about their build not matching the installation,
> and who'd like to be reminded that e.g. "git-fetch" is a built-in by
> "ls" coloring appropriately it as a symlink.

I am not with the cause and hence not very interested in this
"feature".

When there are multiple possible reasons why something is made into
a symbolic link, the symlink-ness in the build directory cannot
fundamentally mirror the symlink-ness in the installation, no?
"git" and "git-fetch" may be in the same directory in the build, but
their installation directories are different, so they may be
hardlinked in the former but they may be turned into symlinks in the
latter.

Re: [PATCH 4/6] Makefile: make INSTALL_SYMLINKS affect the build directory

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 30 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the INSTALL_SYMLINKS flag to also affect whether a built-in
>> like e.g. git-fetch is a symlink or not in git's build directory.
>>
>> This doesn't matter for anything other than as an aid to developers
>> who might be confused about their build not matching the installation,
>> and who'd like to be reminded that e.g. "git-fetch" is a built-in by
>> "ls" coloring appropriately it as a symlink.
>
> I am not with the cause and hence not very interested in this
> "feature".
>
> When there are multiple possible reasons why something is made into
> a symbolic link, the symlink-ness in the build directory cannot
> fundamentally mirror the symlink-ness in the installation, no?
> "git" and "git-fetch" may be in the same directory in the build, but
> their installation directories are different, so they may be
> hardlinked in the former but they may be turned into symlinks in the
> latter.

This won't be the case after 6/6 in INSTALL_FALLBACK_LN_CP. I'll make
some note of it here.

In practice I think this fallback mode is useful to almost nobody, so
being able to have the build directory mirror the install for
development purposes makes things much more obvious.

[PATCH 5/6] Makefile: use "ln -f" instead of "rm && ln"

[Ævar Arnfjörð Bjarmason]

Change the invocations and behavior of "ln-or-cp.sh" to not assume
that we're going to "rm" the file in question beforehand.

This reduces the complexity of these rules, and as a bonus means it's
now safe to "make install" on a system that may have running "git"
programs, before this we'd be racing the "rm && ln/cp" and wouldn't
have a working "git" (or one of the built-ins) in the interim.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile    | 10 ++--------
 ln-or-cp.sh |  5 ++---
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index 29d9bade5a8..466df1a8e90 100644
--- a/Makefile
+++ b/Makefile
@@ -2204,8 +2204,7 @@ version.sp version.s version.o: EXTRA_CPPFLAGS = \
 
 $(BUILT_INS): GIT-LNCPFLAGS
 $(BUILT_INS): git$X
-	$(QUIET_BUILT_IN)$(RM) $@ && \
-	./ln-or-cp.sh \
+	$(QUIET_BUILT_IN)./ln-or-cp.sh \
 		--install-symlinks "$(INSTALL_SYMLINKS)" \
 		$< $@
 
@@ -2558,8 +2557,7 @@ git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
 
 $(REMOTE_CURL_ALIASES): GIT-LNCPFLAGS
 $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
-	$(QUIET_LNCP)$(RM) $@ && \
-	./ln-or-cp.sh \
+	$(QUIET_LNCP)./ln-or-cp.sh \
 		--install-symlinks "$(INSTALL_SYMLINKS)" \
 		$< $@
 
@@ -3021,7 +3019,6 @@ endif
 	destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \
 	{ test "$$bindir/" = "$$execdir/" || \
 	  for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
-		$(RM) "$$execdir/$$p" && \
 		./ln-or-cp.sh \
 			--install-symlinks "$(INSTALL_SYMLINKS)" \
 			--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
@@ -3031,7 +3028,6 @@ endif
 	  done; \
 	} && \
 	for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
-		$(RM) "$$bindir/$$p" && \
 		./ln-or-cp.sh \
 			--install-symlinks "$(INSTALL_SYMLINKS)" \
 			--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
@@ -3039,7 +3035,6 @@ endif
 			"$$bindir/git$X" "$$bindir/$$p"; \
 	done && \
 	for p in $(BUILT_INS); do \
-		$(RM) "$$execdir/$$p" && \
 		if test -z "$(SKIP_DASHED_BUILT_INS)"; \
 		then \
 			./ln-or-cp.sh \
@@ -3051,7 +3046,6 @@ endif
 	done && \
 	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
 	for p in $$remote_curl_aliases; do \
-		$(RM) "$$execdir/$$p" && \
 		./ln-or-cp.sh \
 			--install-symlinks "$(INSTALL_SYMLINKS)" \
 			--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
diff --git a/ln-or-cp.sh b/ln-or-cp.sh
index 663ffd0489d..37380993c64 100755
--- a/ln-or-cp.sh
+++ b/ln-or-cp.sh
@@ -40,11 +40,10 @@ link="$2"
 hardlink_or_cp () {
 	if test -z "$no_install_hardlinks" -a -z "$no_cross_directory_hardlinks"
 	then
-		if ! ln "$target" "$link"
+		if ! ln -f "$target" "$link"
 		then
 			cp "$target" "$link"
 		fi
-
 	else
 		cp "$target" "$link"
 	fi
@@ -53,7 +52,7 @@ hardlink_or_cp () {
 main_with_fallbacks () {
 	if test -n "$install_symlinks" -o -n "$no_install_hardlinks"
 	then
-		if ! ln -s "$symlink_target" "$link"
+		if ! ln -f -s "$symlink_target" "$link"
 		then
 			hardlink_or_cp
 		fi
-- 
2.31.1.461.gd47399f6574

Re: [PATCH 5/6] Makefile: use "ln -f" instead of "rm && ln"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the invocations and behavior of "ln-or-cp.sh" to not assume
> that we're going to "rm" the file in question beforehand.
>
> This reduces the complexity of these rules, and as a bonus means it's
> now safe to "make install" on a system that may have running "git"
> programs, before this we'd be racing the "rm && ln/cp" and wouldn't
> have a working "git" (or one of the built-ins) in the interim.

Neither link(2) nor symlink(2) has the equivalent of the -f flag, so
"ln [-s] -f" has to be implemented as an unlink(2) followed by
link(2) or symlink(2) anyway, so you didn't solve the "racing"
problem (if that is a problem in the first place, that is), did you?

The only reason why "rm -f t && ln s t" makes sense over "ln -f s t"
is because there could be a leftover 't' directory from a previous
build or rogue testing process or whatever.  It avoids creating a
hardlink at t/s, unlike "ln -f s t" which would happily do so.  It
would let us notice there is something fishy going on by failing to
remove the stray directory that should not exist.

I do not object to replace "rm then ln" with "ln -f", as the "be
cautious against somebody mistakenly making a directory there" is
not something I find valuable all that much.

But I do not want to be associated with a commit that claims that
"ln -f" avoids race in "rm && ln" ;-).

Thanks.

[PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode

[Ævar Arnfjörð Bjarmason]

Change the default behavior on "make install" where we fallback
through a chain of "ln || ln -s || cp" to instead error out when we
can't symlink or hardlink, and not then fallback on a "cp" (or from a
symlink to hardlink etc.).

The fallback behavior was introduced in 3e073dc5611 (Makefile: always
provide a fallback when hardlinks fail, 2008-08-25), since then we've
gained the ability to specify e.g. that we'd like symlinks via the
INSTALL_SYMLINKS setting.

It doesn't make sense as a default to silently fall back if we can't
satisfy those settings. We should instead error out, at which point
the developer/builder/sysadmin can set one or more of the relevant
hardlink or symlink settings.

This also changes the behavior for the build (not install!) to always
use the new strict mode. This will theoretically break things for
someone who can't make symlinks or hardlinks in their git checkout
when building.

That part of this change would break building on Windows and other
platforms that don't support symlinks if INSTALL_SYMLINKS were to
become the default, but it's not on by default, so we should be fine
here. If and when INSTALL_SYMLINKS ever becomes the default the right
way to deal with this would be to tweak config.mak.uname
appropriately, not to silently fall back on a copy.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile    | 11 +++++++++++
 ln-or-cp.sh | 29 ++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 466df1a8e90..ccbded79093 100644
--- a/Makefile
+++ b/Makefile
@@ -350,6 +350,13 @@ all::
 # or if that fails fall back on a "cp" instead of a "ln". Useful for
 # when you don't want hardlinks at all.
 #
+# Define INSTALL_FALLBACK_LN_CP if you'd like your
+# "INSTALL_SYMLINKS=Y" to fall back on hardlinks if we can't run "ln
+# -s", and for "ln" to fall back on "NO_INSTALL_HARDLINKS=Y" if we
+# can't perform a "ln" and need to fall-back to a "cp". This used to
+# be the default behavior, but we'll now error if we can't satisfy
+# your INSTALL_SYMLINKS, NO_INSTALL_HARDLINKS etc. settings.
+#
 # Define SKIP_DASHED_BUILT_INS if you do not need the dashed versions of the
 # built-ins to be linked/copied at all.
 #
@@ -3020,6 +3027,7 @@ endif
 	{ test "$$bindir/" = "$$execdir/" || \
 	  for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
 		./ln-or-cp.sh \
+			--install-fallback-ln-cp "$(INSTALL_FALLBACK_LN_CP)" \
 			--install-symlinks "$(INSTALL_SYMLINKS)" \
 			--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
 			--no-cross-directory-hardlinks "$(NO_CROSS_DIRECTORY_HARDLINKS)" \
@@ -3029,6 +3037,7 @@ endif
 	} && \
 	for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
 		./ln-or-cp.sh \
+			--install-fallback-ln-cp "$(INSTALL_FALLBACK_LN_CP)" \
 			--install-symlinks "$(INSTALL_SYMLINKS)" \
 			--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
 			--symlink-target "git$X" \
@@ -3038,6 +3047,7 @@ endif
 		if test -z "$(SKIP_DASHED_BUILT_INS)"; \
 		then \
 			./ln-or-cp.sh \
+				--install-fallback-ln-cp "$(INSTALL_FALLBACK_LN_CP)" \
 				--install-symlinks "$(INSTALL_SYMLINKS)" \
 				--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
 				--symlink-target "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" \
@@ -3047,6 +3057,7 @@ endif
 	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
 	for p in $$remote_curl_aliases; do \
 		./ln-or-cp.sh \
+			--install-fallback-ln-cp "$(INSTALL_FALLBACK_LN_CP)" \
 			--install-symlinks "$(INSTALL_SYMLINKS)" \
 			--no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \
 			--symlink-target "git-remote-http$X" \
diff --git a/ln-or-cp.sh b/ln-or-cp.sh
index 37380993c64..f77dad71bdb 100755
--- a/ln-or-cp.sh
+++ b/ln-or-cp.sh
@@ -1,5 +1,6 @@
 #!/bin/sh
 
+install_fallback_ln_cp=
 install_symlinks=
 no_install_hardlinks=
 no_cross_directory_hardlinks=
@@ -7,6 +8,10 @@ symlink_target=
 while test $# != 0
 do
 	case "$1" in
+	--install-fallback-ln-cp)
+		install_fallback_ln_cp="$2"
+		shift
+		;;
 	--install-symlinks)
 		install_symlinks="$2"
 		shift
@@ -61,4 +66,26 @@ main_with_fallbacks () {
 	fi
 }
 
-main_with_fallbacks
+main_no_fallbacks () {
+	if test -n "$no_install_hardlinks" -a -z "$install_symlinks"
+	then
+		cp "$target" "$link"
+	elif test -n "$install_symlinks" -o -n "$no_cross_directory_hardlinks"
+	then
+		ln -f -s "$symlink_target" "$link"
+	elif test -n "$no_install_hardlinks"
+	then
+		cp "$target" "$link"
+	else
+		ln -f "$target" "$link"
+	fi
+}
+
+if test -z "$install_fallback_ln_cp"
+then
+	# The stricter mode, where we know what we want
+	main_no_fallbacks
+else
+	main_with_fallbacks
+
+fi
-- 
2.31.1.461.gd47399f6574

Re: [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the default behavior on "make install" where we fallback
> through a chain of "ln || ln -s || cp" to instead error out when we
> can't symlink or hardlink, and not then fallback on a "cp" (or from a
> symlink to hardlink etc.).
>
> The fallback behavior was introduced in 3e073dc5611 (Makefile: always
> provide a fallback when hardlinks fail, 2008-08-25), since then we've
> gained the ability to specify e.g. that we'd like symlinks via the
> INSTALL_SYMLINKS setting.

Hmph, I am not so sure.  "Use hardlink if we can, as that would not
consume inode, but where hardlinks cannot be used, it is OK to use
symlink, and I do not want to waste disk blocks with cp" is probably
one of the sensible wishes, but at least without "ln || ln -s" fallback,
you cannot do that, no?

So I would understand if there are two orthogonal knobs

 - the order of preference (e.g. hardlink > symlink > copy)
 - which ones are allowed (e.g. "no symlinks please")

but I cannot quite imagine how a system without any fallback would
be useful.

> +main_no_fallbacks () {
> +	if test -n "$no_install_hardlinks" -a -z "$install_symlinks"

As the values of these variables are (presumably) tightly under our
control, the use of -a/-o with test may be safe in these examples,
but to avoid letting clueless shell script newbies to cargo cult
this code, let's use the safer "test -n A && test -z B" form.

Re: [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 30 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the default behavior on "make install" where we fallback
>> through a chain of "ln || ln -s || cp" to instead error out when we
>> can't symlink or hardlink, and not then fallback on a "cp" (or from a
>> symlink to hardlink etc.).
>>
>> The fallback behavior was introduced in 3e073dc5611 (Makefile: always
>> provide a fallback when hardlinks fail, 2008-08-25), since then we've
>> gained the ability to specify e.g. that we'd like symlinks via the
>> INSTALL_SYMLINKS setting.
>
> Hmph, I am not so sure.  "Use hardlink if we can, as that would not
> consume inode, but where hardlinks cannot be used, it is OK to use
> symlink, and I do not want to waste disk blocks with cp" is probably
> one of the sensible wishes, but at least without "ln || ln -s" fallback,
> you cannot do that, no?
>
> So I would understand if there are two orthogonal knobs
>
>  - the order of preference (e.g. hardlink > symlink > copy)
>  - which ones are allowed (e.g. "no symlinks please")
>
> but I cannot quite imagine how a system without any fallback would
> be useful.

Because with explicit knobs I'd like to tell it what to do and not have
it auto-guess. E.g. if I say I want openssl I don't want it to see it's
not there and auto-fallback on gnutls or whatever.

The same for "I want hardlinks/symlinks", usually someone picking one is
building a package, and under a lot of packaging formats that difference
really matters, and either won't be notinced in time or will break
further down the chain.

>> +main_no_fallbacks () {
>> +	if test -n "$no_install_hardlinks" -a -z "$install_symlinks"
>
> As the values of these variables are (presumably) tightly under our
> control, the use of -a/-o with test may be safe in these examples,
> but to avoid letting clueless shell script newbies to cargo cult
> this code, let's use the safer "test -n A && test -z B" form.

*nod*

Re: [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> So I would understand if there are two orthogonal knobs
>>
>>  - the order of preference (e.g. hardlink > symlink > copy)
>>  - which ones are allowed (e.g. "no symlinks please")
>>
>> but I cannot quite imagine how a system without any fallback would
>> be useful.
>
> Because with explicit knobs I'd like to tell it what to do and not have
> it auto-guess.

So how would I explicitly tell "I want hardlinks for everything
else, but use cp when going between /usr/bin and /usr/libexec"
(because /usr/bin and /usr/libexec is not on the same filesystem
on this particular box---I'll tell you to use hardlink everywhere
on another box of mine where they reside on the same filesystem)?

Your argument or analogy with openssl does not make much sense to me
in this case.

Re: [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 31 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> So I would understand if there are two orthogonal knobs
>>>
>>>  - the order of preference (e.g. hardlink > symlink > copy)
>>>  - which ones are allowed (e.g. "no symlinks please")
>>>
>>> but I cannot quite imagine how a system without any fallback would
>>> be useful.
>>
>> Because with explicit knobs I'd like to tell it what to do and not have
>> it auto-guess.
>
> So how would I explicitly tell "I want hardlinks for everything
> else, but use cp when going between /usr/bin and /usr/libexec"
> (because /usr/bin and /usr/libexec is not on the same filesystem
> on this particular box---I'll tell you to use hardlink everywhere
> on another box of mine where they reside on the same filesystem)?

Just as you would now:

    make NO_CROSS_DIRECTORY_HARDLINKS=Y install

That'll get you hardlinks within bin/ and libexec, but copies between
them, and no symlinks.

The behavior change in this series is if your "ln" errors out we'll no
longer silently plow ahead and e.g. not hardlink *within* bin and
libexec in that case.

> Your argument or analogy with openssl does not make much sense to me
> in this case.

The point is the Makefile shouldn't be second-guessing explicit
requests. That's what defaults are for.

Re: [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This is on top of my just-submitted "Makefile: don't die on AIX with
> open ./git" series: [1]
>
> This series introduces no "real" behavior changes I'd expect anyone to
> notice, but refactors a lengthy copy/pasted test/if/else in the
> Makefile into a simple helper script.
>
> The "real" behavior change is that we no longer ask the user how
> they'd like to install (symlinks, hardlinks, neither?) and then
> proceed to ignore what was asked of us and optimistically fallback in
> case of errors. I.e. the inability to create symlinks or hardlinks.
>
> Instead we'll just die, the old behavior is still available as
> INSTALL_FALLBACK_LN_CP. In practice I think exactly nobody actually
> wanted the existing behavior.

I read most of the patches in the series.

While I like the general direction to allow builders/installers more
precisely to specify what methods of installation are preferred, I
do not know if I agree with the exact execution.  It does not help
that the series is built on top of that "generate in temporary and
move to the final, even for $(CC)" series, most of which I do not
think I agree with.

Thanks.