[PATCH] Makefile: allow NO_DC_SHA1_SUBMODULE to be set in config.mak

[PATCH] Makefile: allow NO_DC_SHA1_SUBMODULE to be set in config.mak

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Junio,

Could you please add (or squash) this on top of the 'ab/sha1dc-build'
branch, so that I can build with NO_DC_SHA1_SUBMODULE=NoThanks in my
config.mak.

[If I were to get a vote, I would vote no to the submodule. ;-) ]

Thanks!

ATB,
Ramsay Jones

 Makefile | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 929b49b04..91bbb0ed8 100644
--- a/Makefile
+++ b/Makefile
@@ -1042,6 +1042,10 @@ EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)
 
+include config.mak.uname
+-include config.mak.autogen
+-include config.mak
+
 ifndef NO_DC_SHA1_SUBMODULE
 	ifndef DC_SHA1_EXTERNAL
 		ifneq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
@@ -1053,10 +1057,6 @@ whatever reason define NO_DC_SHA1_SUBMODULE=NoThanks)
 	endif
 endif
 
-include config.mak.uname
--include config.mak.autogen
--include config.mak
-
 ifdef DEVELOPER
 CFLAGS += $(DEVELOPER_CFLAGS)
 endif
-- 
2.15.0

Re: [PATCH] Makefile: allow NO_DC_SHA1_SUBMODULE to be set in config.mak

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Junio,
>
> Could you please add (or squash) this on top of the 'ab/sha1dc-build'
> branch, so that I can build with NO_DC_SHA1_SUBMODULE=NoThanks in my
> config.mak.

Makes sense.  The patch looks scary by appearing to move the
includes far to the front of the Makefile, but it in fact is moving
the NO_DC_SHA1_SUBMODULE block slightly down and it is a sensible
and safe move.

I actually think that the block can go even further down, perhaps
close to the run of choices "what variant are we building?" we make
at around we have "ifdef NO_CURL".

Ævar?

> diff --git a/Makefile b/Makefile
> index 929b49b04..91bbb0ed8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1042,6 +1042,10 @@ EXTLIBS =
>  
>  GIT_USER_AGENT = git/$(GIT_VERSION)
>  
> +include config.mak.uname
> +-include config.mak.autogen
> +-include config.mak
> +
>  ifndef NO_DC_SHA1_SUBMODULE
>  	ifndef DC_SHA1_EXTERNAL
>  		ifneq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
> @@ -1053,10 +1057,6 @@ whatever reason define NO_DC_SHA1_SUBMODULE=NoThanks)
>  	endif
>  endif
>  
> -include config.mak.uname
> --include config.mak.autogen
> --include config.mak
> -
>  ifdef DEVELOPER
>  CFLAGS += $(DEVELOPER_CFLAGS)
>  endif

Re: [PATCH] Makefile: allow NO_DC_SHA1_SUBMODULE to be set in config.mak

[Junio C Hamano]

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

> Makes sense.  The patch looks scary by appearing to move the
> includes far to the front of the Makefile, but it in fact is moving
> the NO_DC_SHA1_SUBMODULE block slightly down and it is a sensible
> and safe move.

A completely unrelated tangent.  This was a good change to try your
anchored diff on.  Viewing this change with

   $ git show --anchored='include config.mak'

looks a lot less scary than the way it is shown by default (which
matches what was posted on the list).

Good job.

Re: [PATCH] Makefile: allow NO_DC_SHA1_SUBMODULE to be set in config.mak

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 12, 2017 at 8:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> ---
>>
>> Hi Junio,
>>
>> Could you please add (or squash) this on top of the 'ab/sha1dc-build'
>> branch, so that I can build with NO_DC_SHA1_SUBMODULE=NoThanks in my
>> config.mak.
>
> Makes sense.  The patch looks scary by appearing to move the
> includes far to the front of the Makefile, but it in fact is moving
> the NO_DC_SHA1_SUBMODULE block slightly down and it is a sensible
> and safe move.
>
> I actually think that the block can go even further down, perhaps
> close to the run of choices "what variant are we building?" we make
> at around we have "ifdef NO_CURL".
>
> Ævar?

Makes sense to me, do you want to squash this + your proposed edit &
I'll pick it up if there's another version, or I can re-submit.

>> diff --git a/Makefile b/Makefile
>> index 929b49b04..91bbb0ed8 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1042,6 +1042,10 @@ EXTLIBS =
>>
>>  GIT_USER_AGENT = git/$(GIT_VERSION)
>>
>> +include config.mak.uname
>> +-include config.mak.autogen
>> +-include config.mak
>> +
>>  ifndef NO_DC_SHA1_SUBMODULE
>>       ifndef DC_SHA1_EXTERNAL
>>               ifneq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
>> @@ -1053,10 +1057,6 @@ whatever reason define NO_DC_SHA1_SUBMODULE=NoThanks)
>>       endif
>>  endif
>>
>> -include config.mak.uname
>> --include config.mak.autogen
>> --include config.mak
>> -
>>  ifdef DEVELOPER
>>  CFLAGS += $(DEVELOPER_CFLAGS)
>>  endif

Re: [PATCH] Makefile: allow NO_DC_SHA1_SUBMODULE to be set in config.mak

[Jonathan Tan]

On Tue, 12 Dec 2017 11:57:28 -0800
Junio C Hamano <gitster@pobox.com> wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Makes sense.  The patch looks scary by appearing to move the
> > includes far to the front of the Makefile, but it in fact is moving
> > the NO_DC_SHA1_SUBMODULE block slightly down and it is a sensible
> > and safe move.
> 
> A completely unrelated tangent.  This was a good change to try your
> anchored diff on.  Viewing this change with
> 
>    $ git show --anchored='include config.mak'
> 
> looks a lot less scary than the way it is shown by default (which
> matches what was posted on the list).
> 
> Good job.

Thanks, glad that it helped.

Re: [PATCH] Makefile: allow NO_DC_SHA1_SUBMODULE to be set in config.mak

[Junio C Hamano]

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

>> I actually think that the block can go even further down, perhaps
>> close to the run of choices "what variant are we building?" we make
>> at around we have "ifdef NO_CURL".
>>
>> Ævar?
>
> Makes sense to me, do you want to squash this + your proposed edit &
> I'll pick it up if there's another version, or I can re-submit.

OK.  I'll squash in the 'move it further down' to the original
commit that removed DC_SHA1_SUBMODULE and added NO_DC_SHA1_SUBMODULE
when rebuilding 'pu' branch.

Thanks.

Re: [PATCH] Makefile: allow NO_DC_SHA1_SUBMODULE to be set in config.mak

[Junio C Hamano]

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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> I actually think that the block can go even further down, perhaps
>>> close to the run of choices "what variant are we building?" we make
>>> at around we have "ifdef NO_CURL".
>>>
>>> Ævar?
>>
>> Makes sense to me, do you want to squash this + your proposed edit &
>> I'll pick it up if there's another version, or I can re-submit.
>
> OK.  I'll squash in the 'move it further down' to the original
> commit that removed DC_SHA1_SUBMODULE and added NO_DC_SHA1_SUBMODULE
> when rebuilding 'pu' branch.
>
> Thanks.

Another minor thing I noticed (which I do not have any squashable
fix for) is that "make distclean" does not even work without
submodule or this environment, which feels a bit too excessive.  I
haven't tried to figure out how involved a fix for that would be yet
and I do not mind leaving it broken if it would be too much work.

Re: [PATCH] Makefile: allow NO_DC_SHA1_SUBMODULE to be set in config.mak

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 12 2017, Junio C. Hamano jotted:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>>> I actually think that the block can go even further down, perhaps
>>>> close to the run of choices "what variant are we building?" we make
>>>> at around we have "ifdef NO_CURL".
>>>>
>>>> Ævar?
>>>
>>> Makes sense to me, do you want to squash this + your proposed edit &
>>> I'll pick it up if there's another version, or I can re-submit.
>>
>> OK.  I'll squash in the 'move it further down' to the original
>> commit that removed DC_SHA1_SUBMODULE and added NO_DC_SHA1_SUBMODULE
>> when rebuilding 'pu' branch.
>>
>> Thanks.
>
> Another minor thing I noticed (which I do not have any squashable
> fix for) is that "make distclean" does not even work without
> submodule or this environment, which feels a bit too excessive.  I
> haven't tried to figure out how involved a fix for that would be yet
> and I do not mind leaving it broken if it would be too much work.

Yes, that sucks. I can't think of a better fix for it than this massive
hack of outsourcing printing the error to the header itself, which'll
only be executed if we actually compile:

    diff --git a/Makefile b/Makefile
    index ba3e061edd..881cf55159 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -1022,9 +1022,7 @@ GIT_USER_AGENT = git/$(GIT_VERSION)

     ifndef DC_SHA1_EXTERNAL
            ifneq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
    -$(error The sha1collisiondetection submodule is not checked out. \
    -Please make it available, either by cloning with --recurse-submodules, \
    -or by running "git submodule update --init".)
    +               BASIC_CFLAGS += -DERROR_SHA1COLLISIONDETECTION_SUBMODULE
            endif
     else
            ifdef NO_DC_SHA1_SUBMODULE
    diff --git a/sha1dc_git.h b/sha1dc_git.h
    index be1d48abbe..93a9be976a 100644
    --- a/sha1dc_git.h
    +++ b/sha1dc_git.h
    @@ -1,5 +1,11 @@
     /* Plumbing with collition-detecting SHA1 code */

    +#ifdef ERROR_SHA1COLLISIONDETECTION_SUBMODULE
    +#error "The sha1collisiondetection submodule is not checked out." \
    +       "Please make it available, either by cloning with --recurse-submodules," \
    +       "or by running \"git submodule update --init\"."
    +#endif
    +
     #ifdef DC_SHA1_EXTERNAL
     #include <sha1dc/sha1.h>
     #else

But maybe I missed a way to make the $(error) path work only if certain
targets were about to be compiled.