Compile Error v2.13.2 on Solaris SPARC

Compile Error v2.13.2 on Solaris SPARC

[Michael Kebe]

When compiling 2.13.2 on Solaris SPARC I get this error:

    CC sha1dc/sha1.o
sha1dc/sha1.c:41:58: error: operator '==' has no right operand
 #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
                                                          ^
gmake: *** [sha1dc/sha1.o] Error 1

The define _BIG_ENDIAN is set by Solaris on SPARC systems. So the
check in line 41 gives this error.

The _BIG_ENDIAN define is used few line below for defining
SHA1DC_BIGENDIAN. This is needed for Solaris SPARC systems.
See
https://github.com/cr-marcstevens/sha1collisiondetection/commit/33a694a9ee1b79c24be45f9eab5ac0e1aeeaf271

Greetings
Michael

Re: Compile Error v2.13.2 on Solaris SPARC

[Michael Kebe]

No luck with the patch.

Still got:

    CC sha1dc/sha1.o
sha1dc/sha1.c:43:58: error: operator '==' has no right operand
      (defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) ||   \
                                                          ^
gmake: *** [sha1dc/sha1.o] Error 1

Greetings
Michael

2017-06-26 10:42 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>
> On Mon, Jun 26 2017, Michael Kebe jotted:
>
>> When compiling 2.13.2 on Solaris SPARC I get this error:
>>
>>     CC sha1dc/sha1.o
>> sha1dc/sha1.c:41:58: error: operator '==' has no right operand
>>  #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
>>                                                           ^
>> gmake: *** [sha1dc/sha1.o] Error 1
>>
>> The define _BIG_ENDIAN is set by Solaris on SPARC systems. So the
>> check in line 41 gives this error.
>>
>> The _BIG_ENDIAN define is used few line below for defining
>> SHA1DC_BIGENDIAN. This is needed for Solaris SPARC systems.
>> See
>> https://github.com/cr-marcstevens/sha1collisiondetection/commit/33a694a9ee1b79c24be45f9eab5ac0e1aeeaf271
>
> I can see why this would error out. In sys/isa_defs.h on SPARC there's
> just `#define _BIG_ENDIAN`
> (http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/sys/isa_defs.h),
> and (on Linux):
>
>     $ cat /tmp/test.c
>     #define _FOO
>     #define _BAR 1
>     #if (_BAR == _FOO)
>     #endif
>     $ gcc -E /tmp/test.c
>     # 1 "/tmp/test.c"
>     # 1 "<built-in>"
>     # 1 "<command-line>"
>     # 31 "<command-line>"
>     # 1 "/usr/include/stdc-predef.h" 1 3 4
>     # 32 "<command-line>" 2
>     # 1 "/tmp/test.c"
>     /tmp/test.c:3:18: error: operator '==' has no right operand
>      #if (_BAR == _FOO)
>
> What I don't get is how this would have worked for Liam then (see
> 20170613020939.gemh3m5z6czgwmzp@oracle.com). Differences in Solaris
> versions and how their headers look like?
>
> Does this patch fix it for you?
>
> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
> index facea1bb56..0b75b31b67 100644
> --- a/sha1dc/sha1.c
> +++ b/sha1dc/sha1.c
> @@ -36,9 +36,11 @@
>  #undef SHA1DC_BIGENDIAN
>  #endif
>
> -#if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
> +#if (defined(BYTE_ORDER) || defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || \
> +     defined(__BYTE_ORDER__))
>
> -#if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
> +#if ((defined(BYTE_ORDER) && (BYTE_ORDER == BIG_ENDIAN)) || \
> +     (defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) ||   \
>       (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
>       (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
>  #define SHA1DC_BIGENDIAN
>
> I thought maybe BYTE_ORDER would work after searching the Illumos
> sources a bit more:
> http://src.illumos.org/source/search?q=BYTE_ORDER&project=illumos-gate

Re: Compile Error v2.13.2 on Solaris SPARC

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 26 2017, Michael Kebe jotted:

> No luck with the patch.
>
> Still got:
>
>     CC sha1dc/sha1.o
> sha1dc/sha1.c:43:58: error: operator '==' has no right operand
>       (defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) ||   \
>                                                           ^
> gmake: *** [sha1dc/sha1.o] Error 1

Does this patch change anything, with or without the previous patch:

diff --git a/git-compat-util.h b/git-compat-util.h
index 047172d173..1327aea229 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -131,6 +131,14 @@
 # else
 # define _XOPEN_SOURCE 500
 # endif
+
+/*
+ * Bring in macros defining _BIG_ENDIAN etc. Should be brought in by
+ * the likes of stdio.h, but include it here in case it hasn't been
+ * included already.
+ */
+#include <sys/isa_defs.h>
+
 #elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && \
       !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \
       !defined(__TANDEM) && !defined(__QNX__) && !defined(__MirBSD__) && \

>
> Greetings
> Michael
>
> 2017-06-26 10:42 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>>
>> On Mon, Jun 26 2017, Michael Kebe jotted:
>>
>>> When compiling 2.13.2 on Solaris SPARC I get this error:
>>>
>>>     CC sha1dc/sha1.o
>>> sha1dc/sha1.c:41:58: error: operator '==' has no right operand
>>>  #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
>>>                                                           ^
>>> gmake: *** [sha1dc/sha1.o] Error 1
>>>
>>> The define _BIG_ENDIAN is set by Solaris on SPARC systems. So the
>>> check in line 41 gives this error.
>>>
>>> The _BIG_ENDIAN define is used few line below for defining
>>> SHA1DC_BIGENDIAN. This is needed for Solaris SPARC systems.
>>> See
>>> https://github.com/cr-marcstevens/sha1collisiondetection/commit/33a694a9ee1b79c24be45f9eab5ac0e1aeeaf271
>>
>> I can see why this would error out. In sys/isa_defs.h on SPARC there's
>> just `#define _BIG_ENDIAN`
>> (http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/sys/isa_defs.h),
>> and (on Linux):
>>
>>     $ cat /tmp/test.c
>>     #define _FOO
>>     #define _BAR 1
>>     #if (_BAR == _FOO)
>>     #endif
>>     $ gcc -E /tmp/test.c
>>     # 1 "/tmp/test.c"
>>     # 1 "<built-in>"
>>     # 1 "<command-line>"
>>     # 31 "<command-line>"
>>     # 1 "/usr/include/stdc-predef.h" 1 3 4
>>     # 32 "<command-line>" 2
>>     # 1 "/tmp/test.c"
>>     /tmp/test.c:3:18: error: operator '==' has no right operand
>>      #if (_BAR == _FOO)
>>
>> What I don't get is how this would have worked for Liam then (see
>> 20170613020939.gemh3m5z6czgwmzp@oracle.com). Differences in Solaris
>> versions and how their headers look like?
>>
>> Does this patch fix it for you?
>>
>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
>> index facea1bb56..0b75b31b67 100644
>> --- a/sha1dc/sha1.c
>> +++ b/sha1dc/sha1.c
>> @@ -36,9 +36,11 @@
>>  #undef SHA1DC_BIGENDIAN
>>  #endif
>>
>> -#if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
>> +#if (defined(BYTE_ORDER) || defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || \
>> +     defined(__BYTE_ORDER__))
>>
>> -#if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
>> +#if ((defined(BYTE_ORDER) && (BYTE_ORDER == BIG_ENDIAN)) || \
>> +     (defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) ||   \
>>       (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
>>       (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
>>  #define SHA1DC_BIGENDIAN
>>
>> I thought maybe BYTE_ORDER would work after searching the Illumos
>> sources a bit more:
>> http://src.illumos.org/source/search?q=BYTE_ORDER&project=illumos-gate

Re: Compile Error v2.13.2 on Solaris SPARC

[Michael Kebe]

Still no luck, with one or both patches.

Greetings
Michael

2017-06-26 14:47 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>
> On Mon, Jun 26 2017, Michael Kebe jotted:
>
>> No luck with the patch.
>>
>> Still got:
>>
>>     CC sha1dc/sha1.o
>> sha1dc/sha1.c:43:58: error: operator '==' has no right operand
>>       (defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) ||   \
>>                                                           ^
>> gmake: *** [sha1dc/sha1.o] Error 1
>
> Does this patch change anything, with or without the previous patch:
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 047172d173..1327aea229 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -131,6 +131,14 @@
>  # else
>  # define _XOPEN_SOURCE 500
>  # endif
> +
> +/*
> + * Bring in macros defining _BIG_ENDIAN etc. Should be brought in by
> + * the likes of stdio.h, but include it here in case it hasn't been
> + * included already.
> + */
> +#include <sys/isa_defs.h>
> +
>  #elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && \
>        !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \
>        !defined(__TANDEM) && !defined(__QNX__) && !defined(__MirBSD__) && \
>
>>
>> Greetings
>> Michael
>>
>> 2017-06-26 10:42 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>>>
>>> On Mon, Jun 26 2017, Michael Kebe jotted:
>>>
>>>> When compiling 2.13.2 on Solaris SPARC I get this error:
>>>>
>>>>     CC sha1dc/sha1.o
>>>> sha1dc/sha1.c:41:58: error: operator '==' has no right operand
>>>>  #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
>>>>                                                           ^
>>>> gmake: *** [sha1dc/sha1.o] Error 1
>>>>
>>>> The define _BIG_ENDIAN is set by Solaris on SPARC systems. So the
>>>> check in line 41 gives this error.
>>>>
>>>> The _BIG_ENDIAN define is used few line below for defining
>>>> SHA1DC_BIGENDIAN. This is needed for Solaris SPARC systems.
>>>> See
>>>> https://github.com/cr-marcstevens/sha1collisiondetection/commit/33a694a9ee1b79c24be45f9eab5ac0e1aeeaf271
>>>
>>> I can see why this would error out. In sys/isa_defs.h on SPARC there's
>>> just `#define _BIG_ENDIAN`
>>> (http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/sys/isa_defs.h),
>>> and (on Linux):
>>>
>>>     $ cat /tmp/test.c
>>>     #define _FOO
>>>     #define _BAR 1
>>>     #if (_BAR == _FOO)
>>>     #endif
>>>     $ gcc -E /tmp/test.c
>>>     # 1 "/tmp/test.c"
>>>     # 1 "<built-in>"
>>>     # 1 "<command-line>"
>>>     # 31 "<command-line>"
>>>     # 1 "/usr/include/stdc-predef.h" 1 3 4
>>>     # 32 "<command-line>" 2
>>>     # 1 "/tmp/test.c"
>>>     /tmp/test.c:3:18: error: operator '==' has no right operand
>>>      #if (_BAR == _FOO)
>>>
>>> What I don't get is how this would have worked for Liam then (see
>>> 20170613020939.gemh3m5z6czgwmzp@oracle.com). Differences in Solaris
>>> versions and how their headers look like?
>>>
>>> Does this patch fix it for you?
>>>
>>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
>>> index facea1bb56..0b75b31b67 100644
>>> --- a/sha1dc/sha1.c
>>> +++ b/sha1dc/sha1.c
>>> @@ -36,9 +36,11 @@
>>>  #undef SHA1DC_BIGENDIAN
>>>  #endif
>>>
>>> -#if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
>>> +#if (defined(BYTE_ORDER) || defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || \
>>> +     defined(__BYTE_ORDER__))
>>>
>>> -#if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
>>> +#if ((defined(BYTE_ORDER) && (BYTE_ORDER == BIG_ENDIAN)) || \
>>> +     (defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) ||   \
>>>       (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
>>>       (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
>>>  #define SHA1DC_BIGENDIAN
>>>
>>> I thought maybe BYTE_ORDER would work after searching the Illumos
>>> sources a bit more:
>>> http://src.illumos.org/source/search?q=BYTE_ORDER&project=illumos-gate
>

Re: Compile Error v2.13.2 on Solaris SPARC

[Liam R. Howlett]

* ?var Arnfj?r? Bjarmason <avarab@gmail.com> [170626 08:47]:
> 
> On Mon, Jun 26 2017, Michael Kebe jotted:
> 
> > No luck with the patch.
> >
> > Still got:
> >
> >     CC sha1dc/sha1.o
> > sha1dc/sha1.c:43:58: error: operator '==' has no right operand
> >       (defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) ||   \
> >                                                           ^
> > gmake: *** [sha1dc/sha1.o] Error 1
> 
> Does this patch change anything, with or without the previous patch:
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 047172d173..1327aea229 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -131,6 +131,14 @@
>  # else
>  # define _XOPEN_SOURCE 500
>  # endif
> +
> +/*
> + * Bring in macros defining _BIG_ENDIAN etc. Should be brought in by
> + * the likes of stdio.h, but include it here in case it hasn't been
> + * included already.
> + */
> +#include <sys/isa_defs.h>
> +
>  #elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && \
>        !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \
>        !defined(__TANDEM) && !defined(__QNX__) && !defined(__MirBSD__) && \
> 

This addition still fails on Solaris for me.  It appears that _BIG_ENDIAN is
defined but with no value on this platform.


> >
> > Greetings
> > Michael
> >
> > 2017-06-26 10:42 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
> >>
> >> On Mon, Jun 26 2017, Michael Kebe jotted:
> >>
> >>> When compiling 2.13.2 on Solaris SPARC I get this error:
> >>>
> >>>     CC sha1dc/sha1.o
> >>> sha1dc/sha1.c:41:58: error: operator '==' has no right operand
> >>>  #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
> >>>                                                           ^
> >>> gmake: *** [sha1dc/sha1.o] Error 1
> >>>
> >>> The define _BIG_ENDIAN is set by Solaris on SPARC systems. So the
> >>> check in line 41 gives this error.
> >>>
> >>> The _BIG_ENDIAN define is used few line below for defining
> >>> SHA1DC_BIGENDIAN. This is needed for Solaris SPARC systems.
> >>> See
> >>> https://github.com/cr-marcstevens/sha1collisiondetection/commit/33a694a9ee1b79c24be45f9eab5ac0e1aeeaf271
> >>
> >> I can see why this would error out. In sys/isa_defs.h on SPARC there's
> >> just `#define _BIG_ENDIAN`
> >> (http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/sys/isa_defs.h),
> >> and (on Linux):
> >>
> >>     $ cat /tmp/test.c
> >>     #define _FOO
> >>     #define _BAR 1
> >>     #if (_BAR == _FOO)
> >>     #endif
> >>     $ gcc -E /tmp/test.c
> >>     # 1 "/tmp/test.c"
> >>     # 1 "<built-in>"
> >>     # 1 "<command-line>"
> >>     # 31 "<command-line>"
> >>     # 1 "/usr/include/stdc-predef.h" 1 3 4
> >>     # 32 "<command-line>" 2
> >>     # 1 "/tmp/test.c"
> >>     /tmp/test.c:3:18: error: operator '==' has no right operand
> >>      #if (_BAR == _FOO)
> >>
> >> What I don't get is how this would have worked for Liam then (see
> >> 20170613020939.gemh3m5z6czgwmzp@oracle.com). Differences in Solaris
> >> versions and how their headers look like?

I am running Linux and 2.13.2 compiles and works fine for me on SPARC.


If you want to keep the compact layout you have in the #if defined()
portion, you can get away with reversing the logic as follows:

--------- >8 -------------
diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index facea1bb5..808b520cd 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -36,20 +36,20 @@
 #undef SHA1DC_BIGENDIAN
 #endif

-#if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
+#if !(defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))

-#if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
+#if (defined(_BIG_ENDIAN) || defined(__BIG_ENDIAN) || defined(__BIG_ENDIAN__) || \
+     defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
+     defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
+     defined(__sparc))
 #define SHA1DC_BIGENDIAN
 #endif
 
 #else
+#if ((defined(_BYTE_ORDER) && defined(_BIG_ENDIAN)) || \
+     (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
+     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )

-#if (defined(_BIG_ENDIAN) || defined(__BIG_ENDIAN) || defined(__BIG_ENDIAN__) || \
-     defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
-     defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
-     defined(__sparc))
 #define SHA1DC_BIGENDIAN
 #endif

Re: Compile Error v2.13.2 on Solaris SPARC

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 26 2017, Michael Kebe jotted:

> Still no luck, with one or both patches.

Could you please attach (or pastebin or whatever) your copy of
/usr/include/sys/isa_defs.h? And what Solaris/Illumos/Whatever version
is this?

Maybe this patch works for you:

diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index facea1bb56..4f747c3aea 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -36,17 +36,19 @@
 #undef SHA1DC_BIGENDIAN
 #endif
 
-#if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
+#if (defined(BYTE_ORDER) || defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || \
+     defined(__BYTE_ORDER__))
 
-#if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
+#if ((defined(BYTE_ORDER) && defined(BIG_ENDIAN) && (BYTE_ORDER == BIG_ENDIAN)) || \
+     (defined(_BYTE_ORDER) && defined(_BIG_ENDIAN) && (_BYTE_ORDER == _BIG_ENDIAN)) ||   \
+     (defined(__BYTE_ORDER) && defined(__BIG_ENDIAN) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
+     (defined(__BYTE_ORDER__) && defined(__BIG_ENDIAN__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
 #define SHA1DC_BIGENDIAN
 #endif
 
 #else
 
-#if (defined(_BIG_ENDIAN) || defined(__BIG_ENDIAN) || defined(__BIG_ENDIAN__) || \
+#if (defined(BIG_ENDIAN) || defined(_BIG_ENDIAN) || defined(__BIG_ENDIAN) || defined(__BIG_ENDIAN__) || \
      defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
      defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
      defined(__sparc))


Make sure to run the test suite after that, because it may compile but
not diagnose you as Big Endian if it doesn't work, thus failing horribly
on runtime.

> Greetings
> Michael
>
> 2017-06-26 14:47 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>>
>> On Mon, Jun 26 2017, Michael Kebe jotted:
>>
>>> No luck with the patch.
>>>
>>> Still got:
>>>
>>>     CC sha1dc/sha1.o
>>> sha1dc/sha1.c:43:58: error: operator '==' has no right operand
>>>       (defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) ||   \
>>>                                                           ^
>>> gmake: *** [sha1dc/sha1.o] Error 1
>>
>> Does this patch change anything, with or without the previous patch:
>>
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index 047172d173..1327aea229 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -131,6 +131,14 @@
>>  # else
>>  # define _XOPEN_SOURCE 500
>>  # endif
>> +
>> +/*
>> + * Bring in macros defining _BIG_ENDIAN etc. Should be brought in by
>> + * the likes of stdio.h, but include it here in case it hasn't been
>> + * included already.
>> + */
>> +#include <sys/isa_defs.h>
>> +
>>  #elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && \
>>        !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \
>>        !defined(__TANDEM) && !defined(__QNX__) && !defined(__MirBSD__) && \
>>
>>>
>>> Greetings
>>> Michael
>>>
>>> 2017-06-26 10:42 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>>>>
>>>> On Mon, Jun 26 2017, Michael Kebe jotted:
>>>>
>>>>> When compiling 2.13.2 on Solaris SPARC I get this error:
>>>>>
>>>>>     CC sha1dc/sha1.o
>>>>> sha1dc/sha1.c:41:58: error: operator '==' has no right operand
>>>>>  #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
>>>>>                                                           ^
>>>>> gmake: *** [sha1dc/sha1.o] Error 1
>>>>>
>>>>> The define _BIG_ENDIAN is set by Solaris on SPARC systems. So the
>>>>> check in line 41 gives this error.
>>>>>
>>>>> The _BIG_ENDIAN define is used few line below for defining
>>>>> SHA1DC_BIGENDIAN. This is needed for Solaris SPARC systems.
>>>>> See
>>>>> https://github.com/cr-marcstevens/sha1collisiondetection/commit/33a694a9ee1b79c24be45f9eab5ac0e1aeeaf271
>>>>
>>>> I can see why this would error out. In sys/isa_defs.h on SPARC there's
>>>> just `#define _BIG_ENDIAN`
>>>> (http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/sys/isa_defs.h),
>>>> and (on Linux):
>>>>
>>>>     $ cat /tmp/test.c
>>>>     #define _FOO
>>>>     #define _BAR 1
>>>>     #if (_BAR == _FOO)
>>>>     #endif
>>>>     $ gcc -E /tmp/test.c
>>>>     # 1 "/tmp/test.c"
>>>>     # 1 "<built-in>"
>>>>     # 1 "<command-line>"
>>>>     # 31 "<command-line>"
>>>>     # 1 "/usr/include/stdc-predef.h" 1 3 4
>>>>     # 32 "<command-line>" 2
>>>>     # 1 "/tmp/test.c"
>>>>     /tmp/test.c:3:18: error: operator '==' has no right operand
>>>>      #if (_BAR == _FOO)
>>>>
>>>> What I don't get is how this would have worked for Liam then (see
>>>> 20170613020939.gemh3m5z6czgwmzp@oracle.com). Differences in Solaris
>>>> versions and how their headers look like?
>>>>
>>>> Does this patch fix it for you?
>>>>
>>>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
>>>> index facea1bb56..0b75b31b67 100644
>>>> --- a/sha1dc/sha1.c
>>>> +++ b/sha1dc/sha1.c
>>>> @@ -36,9 +36,11 @@
>>>>  #undef SHA1DC_BIGENDIAN
>>>>  #endif
>>>>
>>>> -#if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
>>>> +#if (defined(BYTE_ORDER) || defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || \
>>>> +     defined(__BYTE_ORDER__))
>>>>
>>>> -#if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
>>>> +#if ((defined(BYTE_ORDER) && (BYTE_ORDER == BIG_ENDIAN)) || \
>>>> +     (defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) ||   \
>>>>       (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
>>>>       (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
>>>>  #define SHA1DC_BIGENDIAN
>>>>
>>>> I thought maybe BYTE_ORDER would work after searching the Illumos
>>>> sources a bit more:
>>>> http://src.illumos.org/source/search?q=BYTE_ORDER&project=illumos-gate
>>

Re: Compile Error v2.13.2 on Solaris SPARC

[Michael Kebe]

2017-06-26 22:27 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
> Could you (or anyone else for that matter) please test it with:
>
>     git clone --branch bigend-detect-solaris-again https://github.com/avar/sha1collisiondetection.git &&
>     cd sha1collisiondetection &&
>     make test

Still no luck.

~/sha1collisiondetection (bigend-detect-solaris-again *)$ CC=gcc gmake test
mkdir -p dep_lib && gcc -O2 -Wall -Werror -Wextra -pedantic -std=c90
-Ilib  -M -MF dep_lib/sha1.d lib/sha1.c
lib/sha1.c:63:58: error: operator '==' has no right operand
 #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
                                                          ^

Running Solaris on sparc:
$ uname -a
SunOS er202 5.11 11.3 sun4v sparc sun4v


The isa_defs.h is available here:
https://gist.github.com/michaelkebe/472720cd684b5b2a504b8eeb24049870


Greetings
Michael

Re: Compile Error v2.13.2 on Solaris SPARC

[Michael Kebe]

On the Solaris system here __BYTE_ORDER__ set to 4321 and _BIG_ENDIAN
is defined, but has no value.

The problem is the not short circuiting macro...

-------------------------8<------------------------------
#undef _FOO1
#undef _FOO2
#undef _FOO2

#undef _BAR1
#undef _BAR2
#undef _BAR3

#define _FOO3 42

//comment out this line or give it a value to make the preprocesser happy
#define _BAR1

#if (defined(_FOO1) || defined(_FOO2) || defined(_FOO3))

// not short circuiting... preprocesser tries to evaluate (_FOO1 &&
_BAR1) but _BAR1 has no value...
#if ((defined(_FOO1) && (_FOO1 == _BAR1)) || \
          (defined(_FOO2) && (_FOO2 == _BAR2)) || \
          (defined(_FOO3) && (_FOO3 == BAR3)) )
#define SHA1DC_BIGENDIAN
#endif

#endif
-------------------------8<------------------------------
https://gist.github.com/michaelkebe/c963c7478b7b55ad197f0665986870d4

What do you think?

2017-06-27 7:41 GMT+02:00 Michael Kebe <michael.kebe@gmail.com>:
> 2017-06-26 22:27 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>> Could you (or anyone else for that matter) please test it with:
>>
>>     git clone --branch bigend-detect-solaris-again https://github.com/avar/sha1collisiondetection.git &&
>>     cd sha1collisiondetection &&
>>     make test
>
> Still no luck.
>
> ~/sha1collisiondetection (bigend-detect-solaris-again *)$ CC=gcc gmake test
> mkdir -p dep_lib && gcc -O2 -Wall -Werror -Wextra -pedantic -std=c90
> -Ilib  -M -MF dep_lib/sha1.d lib/sha1.c
> lib/sha1.c:63:58: error: operator '==' has no right operand
>  #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
>                                                           ^
>
> Running Solaris on sparc:
> $ uname -a
> SunOS er202 5.11 11.3 sun4v sparc sun4v
>
>
> The isa_defs.h is available here:
> https://gist.github.com/michaelkebe/472720cd684b5b2a504b8eeb24049870
>
>
> Greetings
> Michael

[PATCH 0/3] update sha1dc from PR #36

[Ævar Arnfjörð Bjarmason]

This hopefully fixes the Solaris SPARC issue & doesn't cause
regressions elsewhere, e.g. on Cygwin. Adam, it would be great if you
could test that platform.

I've already confirmed with Michael Kebe + another SPARC user
(CosmicDJ on Freenode #Solaris) that it works on Solaris SPARC. The
question is whether it breaks anything else.

Per the upstream pull request:
https://github.com/cr-marcstevens/sha1collisiondetection/pull/36

Marc would (understandably) like some wider testing of this before
merging it into the upstream project.

WRT the submodule URL & branch changing: I have no idea how
git-submodule handles this in the general case, but it Just Works with
GitHub because it allows fetching arbitrary SHA1s that any ref
(including pull req refs) point to. So on top of pu just doing "git
submodule update" works to update to the new copy.

Junio C Hamano (1):
  sha1collisiondetection: automatically enable when submodule is
    populated

Ævar Arnfjörð Bjarmason (2):
  sha1dc: update from my PR #36
  sha1dc: optionally use sha1collisiondetection as a submodule

 .gitmodules            |  4 +++
 Makefile               | 16 ++++++++++
 hash.h                 |  4 +++
 sha1collisiondetection |  1 +
 sha1dc/sha1.c          | 80 ++++++++++++++++++++++++++++++++++++++------------
 5 files changed, 86 insertions(+), 19 deletions(-)
 create mode 100644 .gitmodules
 create mode 160000 sha1collisiondetection

-- 
2.13.1.611.g7e3b11ae1

[PATCH 1/3] sha1dc: update from my PR #36

[Ævar Arnfjörð Bjarmason]

Update sha1dc from my PR #36[1] which'll hopefully be integrated by
upstream soon.

This solves the Big Endian detection on Solaris reported against
v2.13.2[2], hopefully without any regressions.

See commit a0103914c2 ("sha1dc: update from upstream", 2017-05-20) and
6b851e536b ("sha1dc: update from upstream", 2017-06-06) for previous
attempts in the 2.13 series to address various compile-time feature
detection in this library.

1. https://github.com/cr-marcstevens/sha1collisiondetection/pull/36
   https://github.com/avar/sha1collisiondetection/commit/56ab30c4c998e1e7f3075705087a2f0c4c4202d7
2. <CAKKM46tHq13XiW5C8sux3=PZ1VHSu_npG8ExfWwcPD7rkZkyRQ@mail.gmail.com>
   (https://public-inbox.org/git/CAKKM46tHq13XiW5C8sux3=PZ1VHSu_npG8ExfWwcPD7rkZkyRQ@mail.gmail.com/)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sha1dc/sha1.c | 80 +++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 61 insertions(+), 19 deletions(-)

diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index facea1bb56..1ff325b37a 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -23,6 +23,13 @@
 #include "sha1.h"
 #include "ubc_check.h"
 
+#if (defined(__amd64__) || defined(__amd64) || defined(__x86_64__) || defined(__x86_64) || \
+     defined(i386) || defined(__i386) || defined(__i386__) || defined(__i486__)  || \
+     defined(__i586__) || defined(__i686__) || defined(_M_IX86) || defined(__X86__) || \
+     defined(_X86_) || defined(__THW_INTEL__) || defined(__I86__) || defined(__INTEL__) || \
+     defined(__386) || defined(_M_X64) || defined(_M_AMD64))
+#define SHA1DC_ON_INTEL_LIKE_PROCESSOR
+#endif
 
 /*
    Because Little-Endian architectures are most common,
@@ -32,28 +39,70 @@
    If you are compiling on a big endian platform and your compiler does not define one of these,
    you will have to add whatever macros your tool chain defines to indicate Big-Endianness.
  */
-#ifdef SHA1DC_BIGENDIAN
-#undef SHA1DC_BIGENDIAN
+
+#if defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__)
+/*
+ * Should detect Big Endian under GCC since at least 4.6.0 (gcc svn
+ * rev #165881). See
+ * https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
+ *
+ * This also works under clang since 3.2, it copied the GCC-ism. See
+ * clang.git's 3b198a97d2 ("Preprocessor: add __BYTE_ORDER__
+ * predefined macro", 2012-07-27)
+ */
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+#define SHA1DC_BIGENDIAN
 #endif
 
-#if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
+#else /* Not under GCC-alike */
 
-#if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
+#if defined(__BYTE_ORDER) && defined(__BIG_ENDIAN)
+/*
+ * Should detect Big Endian under glibc.git since 14245eb70e ("entered
+ * into RCS", 1992-11-25). Defined in <endian.h> which will have been
+ * brought in by standard headers. See glibc.git and
+ * https://sourceforge.net/p/predef/wiki/Endianness/
+ */
+#if __BYTE_ORDER == __BIG_ENDIAN
 #define SHA1DC_BIGENDIAN
 #endif
 
-#else
+#else /* Not under GCC-alike or glibc */
 
-#if (defined(_BIG_ENDIAN) || defined(__BIG_ENDIAN) || defined(__BIG_ENDIAN__) || \
-     defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
+#if (defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
      defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
      defined(__sparc))
+/*
+ * Should define Big Endian for a whitelist of known processors. See
+ * https://sourceforge.net/p/predef/wiki/Endianness/ and
+ * http://www.oracle.com/technetwork/server-storage/solaris/portingtosolaris-138514.html
+ */
 #define SHA1DC_BIGENDIAN
-#endif
 
-#endif
+#else /* Not under GCC-alike or glibc or <processor whitelist> */
+
+#if defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
+/*
+ * As a last resort before we fall back on _BIG_ENDIAN or whatever
+ * else we're not 100% sure about below, we blacklist specific
+ * processors here. We could add more, see
+ * e.g. https://wiki.debian.org/ArchitectureSpecificsMemo
+ */
+#else /* Not under GCC-alike or glibc or <processor whitelist>  or <processor blacklist> */
+
+#ifdef _BIG_ENDIAN
+/*
+ * Solaris / illumos defines either _LITTLE_ENDIAN or _BIG_ENDIAN in
+ * <sys/isa_defs.h>.
+ */
+#define SHA1DC_BIGENDIAN
+#else
+/*#error "Uncomment this to see if you fall through all the detection"*/
+#endif /* Big Endian because of _BIG_ENDIAN (Solaris)*/
+#endif /* !SHA1DC_ON_INTEL_LIKE_PROCESSOR */
+#endif /* Big Endian under whitelist of processors */
+#endif /* Big Endian under glibc */
+#endif /* Big Endian under GCC-alike */
 
 #if (defined(SHA1DC_FORCE_LITTLEENDIAN) && defined(SHA1DC_BIGENDIAN))
 #undef SHA1DC_BIGENDIAN
@@ -63,15 +112,8 @@
 #endif
 /*ENDIANNESS SELECTION*/
 
-#if (defined SHA1DC_FORCE_UNALIGNED_ACCESS || \
-     defined(__amd64__) || defined(__amd64) || defined(__x86_64__) || defined(__x86_64) || \
-     defined(i386) || defined(__i386) || defined(__i386__) || defined(__i486__)  || \
-     defined(__i586__) || defined(__i686__) || defined(_M_IX86) || defined(__X86__) || \
-     defined(_X86_) || defined(__THW_INTEL__) || defined(__I86__) || defined(__INTEL__) || \
-     defined(__386) || defined(_M_X64) || defined(_M_AMD64))
-
+#if defined(SHA1DC_FORCE_UNALIGNED_ACCESS) || defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
 #define SHA1DC_ALLOW_UNALIGNED_ACCESS
-
 #endif /*UNALIGNMENT DETECTION*/
 
 
-- 
2.13.1.611.g7e3b11ae1

[PATCH 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Ævar Arnfjörð Bjarmason]

Add an option to use the sha1collisiondetection library from the
submodule in sha1collisiondetection/ instead of in the copy in the
sha1dc/ directory.

This allows us to try out the submodule in sha1collisiondetection
without breaking the build for anyone who's not expecting them as we
work out any kinks.

This uses my own fork which integrates PR #36. See the preceding
commit ("sha1dc: update from my PR #36", 2017-06-27) for details.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .gitmodules            |  4 ++++
 Makefile               | 12 ++++++++++++
 hash.h                 |  4 ++++
 sha1collisiondetection |  1 +
 4 files changed, 21 insertions(+)
 create mode 100644 .gitmodules
 create mode 160000 sha1collisiondetection

diff --git a/.gitmodules b/.gitmodules
new file mode 100644
index 0000000000..2fea9996e9
--- /dev/null
+++ b/.gitmodules
@@ -0,0 +1,4 @@
+[submodule "sha1collisiondetection"]
+	path = sha1collisiondetection
+	url = https://github.com/avar/sha1collisiondetection.git
+	branch = bigend-detect-solaris-again
diff --git a/Makefile b/Makefile
index b94cd5633c..f0cac1f246 100644
--- a/Makefile
+++ b/Makefile
@@ -162,6 +162,12 @@ all::
 # algorithm. This is slower, but may detect attempted collision attacks.
 # Takes priority over other *_SHA1 knobs.
 #
+# Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
+# sha1collisiondetection shipped as a submodule instead of the
+# non-submodule copy in sha1dc/. This is an experimental option used
+# by the git project to migrate to using sha1collisiondetection as a
+# submodule.
+#
 # Define OPENSSL_SHA1 environment variable when running make to link
 # with the SHA1 routine from openssl library.
 #
@@ -1448,8 +1454,14 @@ ifdef APPLE_COMMON_CRYPTO
 	BASIC_CFLAGS += -DSHA1_APPLE
 else
 	DC_SHA1 := YesPlease
+ifdef DC_SHA1_SUBMODULE
+	LIB_OBJS += sha1collisiondetection/lib/sha1.o
+	LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
+	BASIC_CFLAGS += -DDC_SHA1_SUBMODULE
+else
 	LIB_OBJS += sha1dc/sha1.o
 	LIB_OBJS += sha1dc/ubc_check.o
+endif
 	BASIC_CFLAGS += \
 		-DSHA1_DC \
 		-DSHA1DC_NO_STANDARD_INCLUDES \
diff --git a/hash.h b/hash.h
index a11fc9233f..bef3e630a0 100644
--- a/hash.h
+++ b/hash.h
@@ -8,7 +8,11 @@
 #elif defined(SHA1_OPENSSL)
 #include <openssl/sha.h>
 #elif defined(SHA1_DC)
+#ifdef DC_SHA1_SUBMODULE
+#include "sha1collisiondetection/lib/sha1.h"
+#else
 #include "sha1dc/sha1.h"
+#endif
 #else /* SHA1_BLK */
 #include "block-sha1/sha1.h"
 #endif
diff --git a/sha1collisiondetection b/sha1collisiondetection
new file mode 160000
index 0000000000..56ab30c4c9
--- /dev/null
+++ b/sha1collisiondetection
@@ -0,0 +1 @@
+Subproject commit 56ab30c4c998e1e7f3075705087a2f0c4c4202d7
-- 
2.13.1.611.g7e3b11ae1

[PATCH 3/3] sha1collisiondetection: automatically enable when submodule is populated

[Ævar Arnfjörð Bjarmason]

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

If a user wants to experiment with the version of collision
detecting sha1 from the submodule, the user needed to not just
populate the submodule but also needed to turn the knob.

A Makefile trick is easy enough to do so, so let's do this.  When
somebody with a copy of the submodule populated wants not to use it,
that can be done by overriding it in config.mak or from the command
line.

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

diff --git a/Makefile b/Makefile
index f0cac1f246..3baa1eac0b 100644
--- a/Makefile
+++ b/Makefile
@@ -1009,6 +1009,10 @@ EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)
 
+ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
+DC_SHA1_SUBMODULE = auto
+endif
+
 include config.mak.uname
 -include config.mak.autogen
 -include config.mak
-- 
2.13.1.611.g7e3b11ae1

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Junio C Hamano]

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

> Update sha1dc from my PR #36[1] which'll hopefully be integrated by
> upstream soon.

Please be careful about the title of the patch.  "log --oneline"
does not even let you tell your readers who calls this as "my"
change, and readers would be clueless what PR #36 is.  Something
like

    sha1dc: correct endian detection for Solaris

may give us more relevant information in the oneline output.

> @@ -23,6 +23,13 @@
>  #include "sha1.h"
>  #include "ubc_check.h"
>  
> +#if (defined(__amd64__) || defined(__amd64) || defined(__x86_64__) || defined(__x86_64) || \
> +     defined(i386) || defined(__i386) || defined(__i386__) || defined(__i486__)  || \
> +     defined(__i586__) || defined(__i686__) || defined(_M_IX86) || defined(__X86__) || \
> +     defined(_X86_) || defined(__THW_INTEL__) || defined(__I86__) || defined(__INTEL__) || \
> +     defined(__386) || defined(_M_X64) || defined(_M_AMD64))
> +#define SHA1DC_ON_INTEL_LIKE_PROCESSOR
> +#endif

It is good that you made this orthogonal to the rest.

> +#else /* Not under GCC-alike */
>  
> +#if defined(__BYTE_ORDER) && defined(__BIG_ENDIAN)
> +/*
> + * Should detect Big Endian under glibc.git since 14245eb70e ("entered
> + * into RCS", 1992-11-25). Defined in <endian.h> which will have been
> + * brought in by standard headers. See glibc.git and
> + * https://sourceforge.net/p/predef/wiki/Endianness/
> + */
> +#if __BYTE_ORDER == __BIG_ENDIAN
>  #define SHA1DC_BIGENDIAN
>  #endif

Note that this part of the file considers it a valid way for a
platform to define a constant BIG_ENDIAN that can be compared to
BYTE_ORDER to determine the endianness, implying that such a scheme
would also define LITTLE_ENDIAN and a port of such a platform to a
little endian box will still _define_ the constant BIG_ENDIAN; it
aill have BYTE_ORDER defined to the same value as LITTLE_ENDIAN,
though.

> +#if (defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
>       defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
>       defined(__sparc))
> +/*
> + * Should define Big Endian for a whitelist of known processors. See
> + * https://sourceforge.net/p/predef/wiki/Endianness/ and
> + * http://www.oracle.com/technetwork/server-storage/solaris/portingtosolaris-138514.html
> + */
>  #define SHA1DC_BIGENDIAN

These look sensible.

> +#else /* Not under GCC-alike or glibc or <processor whitelist> */
> +
> +#if defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
> +/*
> + * As a last resort before we fall back on _BIG_ENDIAN or whatever
> + * else we're not 100% sure about below, we blacklist specific
> + * processors here. We could add more, see
> + * e.g. https://wiki.debian.org/ArchitectureSpecificsMemo
> + */
> +#else /* Not under GCC-alike or glibc or <processor whitelist>  or <processor blacklist> */
> +
> +#ifdef _BIG_ENDIAN
> +/*
> + * Solaris / illumos defines either _LITTLE_ENDIAN or _BIG_ENDIAN in
> + * <sys/isa_defs.h>.
> + */
> +#define SHA1DC_BIGENDIAN

This makes readers of this patch wonder why we assume platforms
won't define _LITTLE_ENDIAN and _BIG_ENDIAN at the same time, just
like we saw in the section with __BIG_ENDIAN above.

Thanks, but this is starting to feel like watching a whack-a-mole
played while blindfolded.  At some point, somebody upstream should
declare that enough is enough and introduce the "SHA1DC_FORCE_ENDIAN" 
macro.

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Junio C Hamano]

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

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> +#else /* Not under GCC-alike or glibc or <processor whitelist>  or <processor blacklist> */
>> +
>> +#ifdef _BIG_ENDIAN
>> +/*
>> + * Solaris / illumos defines either _LITTLE_ENDIAN or _BIG_ENDIAN in
>> + * <sys/isa_defs.h>.
>> + */
>> +#define SHA1DC_BIGENDIAN
>
> This makes readers of this patch wonder why we assume platforms
> won't define _LITTLE_ENDIAN and _BIG_ENDIAN at the same time, just
> like we saw in the section with __BIG_ENDIAN above.

To be a bit more constructive, I'd feel it MUCH safer, if this "If
_BIG_ENDIAN is defined, set SHA1DC_BIGENDIAN" is done _ONLY_ when
we definitively KNOW that we are on Solaris, something like:

	#if defined(__sun) && defined(_BIG_ENDIAN)
	/*
	 * Solaris ...
	 */
	#define SHA1DC_BIGENDIAN
	#endif

> Thanks, but this is starting to feel like watching a whack-a-mole
> played while blindfolded.  At some point, somebody upstream should
> declare that enough is enough and introduce the "SHA1DC_FORCE_ENDIAN" 
> macro.

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Junio C Hamano]

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

>> +#if (defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
>>       defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
>>       defined(__sparc))
>> +/*
>> + * Should define Big Endian for a whitelist of known processors. See
>> + * https://sourceforge.net/p/predef/wiki/Endianness/ and
>> + * http://www.oracle.com/technetwork/server-storage/solaris/portingtosolaris-138514.html
>> + */
>>  #define SHA1DC_BIGENDIAN
>
> These look sensible.

By the way, I wonder why this didn't catch the sparc running
Solaris.  What does Michael's system use to let the software know
that it is targetted for a Sparc, if not __sparc?

Re: Compile Error v2.13.2 on Solaris SPARC

[Liam R. Howlett]

How about:

---- 8< ----
diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index facea1bb5..ed8c63f2d 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -38,9 +38,18 @@
 
 #if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
 
+#define EMPTY_VAL(x) x ## 1
+#define EMPTY(x) EMPTY_VAL(x)
+
+#if (defined(_BIG_ENDIAN) && (EMPTY(_BIG_ENDIAN) == 1))
+#undef _BIG_ENDIAN
+#define _BIG_ENDIAN 4321
+#endif
+
 #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
      (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
+     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) || \
+     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == _BIG_ENDIAN)) )
 #define SHA1DC_BIGENDIAN
 #endif


* Michael Kebe <michael.kebe@gmail.com> [170627 02:28]:
> On the Solaris system here __BYTE_ORDER__ set to 4321 and _BIG_ENDIAN
> is defined, but has no value.
> 
> The problem is the not short circuiting macro...
> 
> -------------------------8<------------------------------
> #undef _FOO1
> #undef _FOO2
> #undef _FOO2
> 
> #undef _BAR1
> #undef _BAR2
> #undef _BAR3
> 
> #define _FOO3 42
> 
> //comment out this line or give it a value to make the preprocesser happy
> #define _BAR1
> 
> #if (defined(_FOO1) || defined(_FOO2) || defined(_FOO3))
> 
> // not short circuiting... preprocesser tries to evaluate (_FOO1 &&
> _BAR1) but _BAR1 has no value...
> #if ((defined(_FOO1) && (_FOO1 == _BAR1)) || \
>           (defined(_FOO2) && (_FOO2 == _BAR2)) || \
>           (defined(_FOO3) && (_FOO3 == BAR3)) )
> #define SHA1DC_BIGENDIAN
> #endif
> 
> #endif
> -------------------------8<------------------------------
> https://gist.github.com/michaelkebe/c963c7478b7b55ad197f0665986870d4
> 
> What do you think?
> 
> 2017-06-27 7:41 GMT+02:00 Michael Kebe <michael.kebe@gmail.com>:
> > 2017-06-26 22:27 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
> >> Could you (or anyone else for that matter) please test it with:
> >>
> >>     git clone --branch bigend-detect-solaris-again https://github.com/avar/sha1collisiondetection.git &&
> >>     cd sha1collisiondetection &&
> >>     make test
> >
> > Still no luck.
> >
> > ~/sha1collisiondetection (bigend-detect-solaris-again *)$ CC=gcc gmake test
> > mkdir -p dep_lib && gcc -O2 -Wall -Werror -Wextra -pedantic -std=c90
> > -Ilib  -M -MF dep_lib/sha1.d lib/sha1.c
> > lib/sha1.c:63:58: error: operator '==' has no right operand
> >  #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
> >                                                           ^
> >
> > Running Solaris on sparc:
> > $ uname -a
> > SunOS er202 5.11 11.3 sun4v sparc sun4v
> >
> >
> > The isa_defs.h is available here:
> > https://gist.github.com/michaelkebe/472720cd684b5b2a504b8eeb24049870
> >
> >
> > Greetings
> > Michael

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Liam R. Howlett]

* Junio C Hamano <gitster@pobox.com> [170627 11:55]:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> +#if (defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
> >>       defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
> >>       defined(__sparc))
> >> +/*
> >> + * Should define Big Endian for a whitelist of known processors. See
> >> + * https://sourceforge.net/p/predef/wiki/Endianness/ and
> >> + * http://www.oracle.com/technetwork/server-storage/solaris/portingtosolaris-138514.html
> >> + */
> >>  #define SHA1DC_BIGENDIAN
> >
> > These look sensible.
> 
> By the way, I wonder why this didn't catch the sparc running
> Solaris.  What does Michael's system use to let the software know
> that it is targetted for a Sparc, if not __sparc?
> 

I have issues running the test on my solaris system.  Could you send him
the changes you made on sha1dc in git?  I suspect his platform does
define __sparc.

Cheers,
Liam

Re: Compile Error v2.13.2 on Solaris SPARC

[Junio C Hamano]

"Liam R. Howlett" <Liam.Howlett@Oracle.com> writes:

> How about:
>
> ---- 8< ----
> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
> index facea1bb5..ed8c63f2d 100644
> --- a/sha1dc/sha1.c
> +++ b/sha1dc/sha1.c
> @@ -38,9 +38,18 @@
>  
>  #if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
>  
> +#define EMPTY_VAL(x) x ## 1
> +#define EMPTY(x) EMPTY_VAL(x)
> +
> +#if (defined(_BIG_ENDIAN) && (EMPTY(_BIG_ENDIAN) == 1))
> +#undef _BIG_ENDIAN
> +#define _BIG_ENDIAN 4321
> +#endif

I'd say it is a bad idea to define a symbol that you _know_ a
platform header file defines.  Any header you may include from the
platform after these lines still expects the symbol to be defined in
a way it defines without getting molested and will misbehave.

Re: Compile Error v2.13.2 on Solaris SPARC

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27 2017, Liam R. Howlett jotted:

> How about:
>
> ---- 8< ----
> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
> index facea1bb5..ed8c63f2d 100644
> --- a/sha1dc/sha1.c
> +++ b/sha1dc/sha1.c
> @@ -38,9 +38,18 @@
>
>  #if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
>
> +#define EMPTY_VAL(x) x ## 1
> +#define EMPTY(x) EMPTY_VAL(x)
> +
> +#if (defined(_BIG_ENDIAN) && (EMPTY(_BIG_ENDIAN) == 1))
> +#undef _BIG_ENDIAN
> +#define _BIG_ENDIAN 4321
> +#endif
> +
>  #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
>       (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
> -     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
> +     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) || \
> +     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == _BIG_ENDIAN)) )
>  #define SHA1DC_BIGENDIAN
>  #endif

Might be workable, but have you seen my
20170627121718.12078-2-avarab@gmail.com ([PATCH 1/3] sha1dc: update from
my PR #36). It should avoid this problem entirely.

> * Michael Kebe <michael.kebe@gmail.com> [170627 02:28]:
>> On the Solaris system here __BYTE_ORDER__ set to 4321 and _BIG_ENDIAN
>> is defined, but has no value.
>>
>> The problem is the not short circuiting macro...
>>
>> -------------------------8<------------------------------
>> #undef _FOO1
>> #undef _FOO2
>> #undef _FOO2
>>
>> #undef _BAR1
>> #undef _BAR2
>> #undef _BAR3
>>
>> #define _FOO3 42
>>
>> //comment out this line or give it a value to make the preprocesser happy
>> #define _BAR1
>>
>> #if (defined(_FOO1) || defined(_FOO2) || defined(_FOO3))
>>
>> // not short circuiting... preprocesser tries to evaluate (_FOO1 &&
>> _BAR1) but _BAR1 has no value...
>> #if ((defined(_FOO1) && (_FOO1 == _BAR1)) || \
>>           (defined(_FOO2) && (_FOO2 == _BAR2)) || \
>>           (defined(_FOO3) && (_FOO3 == BAR3)) )
>> #define SHA1DC_BIGENDIAN
>> #endif
>>
>> #endif
>> -------------------------8<------------------------------
>> https://gist.github.com/michaelkebe/c963c7478b7b55ad197f0665986870d4
>>
>> What do you think?
>>
>> 2017-06-27 7:41 GMT+02:00 Michael Kebe <michael.kebe@gmail.com>:
>> > 2017-06-26 22:27 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>> >> Could you (or anyone else for that matter) please test it with:
>> >>
>> >>     git clone --branch bigend-detect-solaris-again https://github.com/avar/sha1collisiondetection.git &&
>> >>     cd sha1collisiondetection &&
>> >>     make test
>> >
>> > Still no luck.
>> >
>> > ~/sha1collisiondetection (bigend-detect-solaris-again *)$ CC=gcc gmake test
>> > mkdir -p dep_lib && gcc -O2 -Wall -Werror -Wextra -pedantic -std=c90
>> > -Ilib  -M -MF dep_lib/sha1.d lib/sha1.c
>> > lib/sha1.c:63:58: error: operator '==' has no right operand
>> >  #if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
>> >                                                           ^
>> >
>> > Running Solaris on sparc:
>> > $ uname -a
>> > SunOS er202 5.11 11.3 sun4v sparc sun4v
>> >
>> >
>> > The isa_defs.h is available here:
>> > https://gist.github.com/michaelkebe/472720cd684b5b2a504b8eeb24049870
>> >
>> >
>> > Greetings
>> > Michael

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Update sha1dc from my PR #36[1] which'll hopefully be integrated by
>> upstream soon.
>
> Please be careful about the title of the patch.  "log --oneline"
> does not even let you tell your readers who calls this as "my"
> change, and readers would be clueless what PR #36 is.  Something
> like
>
>     sha1dc: correct endian detection for Solaris
>
> may give us more relevant information in the oneline output.

Will fix. Can you integrate it as-is into pu anyway? I'm going to need
to re-submit it regardless once we get some testing on it & upstream
merges the PR, but having it in Travis et al in the meantime would be
great.

>> @@ -23,6 +23,13 @@
>>  #include "sha1.h"
>>  #include "ubc_check.h"
>>
>> +#if (defined(__amd64__) || defined(__amd64) || defined(__x86_64__) || defined(__x86_64) || \
>> +     defined(i386) || defined(__i386) || defined(__i386__) || defined(__i486__)  || \
>> +     defined(__i586__) || defined(__i686__) || defined(_M_IX86) || defined(__X86__) || \
>> +     defined(_X86_) || defined(__THW_INTEL__) || defined(__I86__) || defined(__INTEL__) || \
>> +     defined(__386) || defined(_M_X64) || defined(_M_AMD64))
>> +#define SHA1DC_ON_INTEL_LIKE_PROCESSOR
>> +#endif
>
> It is good that you made this orthogonal to the rest.
>
>> +#else /* Not under GCC-alike */
>>
>> +#if defined(__BYTE_ORDER) && defined(__BIG_ENDIAN)
>> +/*
>> + * Should detect Big Endian under glibc.git since 14245eb70e ("entered
>> + * into RCS", 1992-11-25). Defined in <endian.h> which will have been
>> + * brought in by standard headers. See glibc.git and
>> + * https://sourceforge.net/p/predef/wiki/Endianness/
>> + */
>> +#if __BYTE_ORDER == __BIG_ENDIAN
>>  #define SHA1DC_BIGENDIAN
>>  #endif
>
> Note that this part of the file considers it a valid way for a
> platform to define a constant BIG_ENDIAN that can be compared to
> BYTE_ORDER to determine the endianness, implying that such a scheme
> would also define LITTLE_ENDIAN and a port of such a platform to a
> little endian box will still _define_ the constant BIG_ENDIAN; it
> aill have BYTE_ORDER defined to the same value as LITTLE_ENDIAN,
> though.

This may fail if we have some non-glibc platform that's defining
__BYTE_ORDER and __BIG_ENDIAN, but if it's glibc then __BIG_ENDIAN will
always be defined, even on little-endian platforms.

>> +#if (defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
>>       defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
>>       defined(__sparc))
>> +/*
>> + * Should define Big Endian for a whitelist of known processors. See
>> + * https://sourceforge.net/p/predef/wiki/Endianness/ and
>> + * http://www.oracle.com/technetwork/server-storage/solaris/portingtosolaris-138514.html
>> + */
>>  #define SHA1DC_BIGENDIAN
>
> These look sensible.
>
>> +#else /* Not under GCC-alike or glibc or <processor whitelist> */
>> +
>> +#if defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
>> +/*
>> + * As a last resort before we fall back on _BIG_ENDIAN or whatever
>> + * else we're not 100% sure about below, we blacklist specific
>> + * processors here. We could add more, see
>> + * e.g. https://wiki.debian.org/ArchitectureSpecificsMemo
>> + */
>> +#else /* Not under GCC-alike or glibc or <processor whitelist>  or <processor blacklist> */
>> +
>> +#ifdef _BIG_ENDIAN
>> +/*
>> + * Solaris / illumos defines either _LITTLE_ENDIAN or _BIG_ENDIAN in
>> + * <sys/isa_defs.h>.
>> + */
>> +#define SHA1DC_BIGENDIAN
>
> This makes readers of this patch wonder why we assume platforms
> won't define _LITTLE_ENDIAN and _BIG_ENDIAN at the same time, just
> like we saw in the section with __BIG_ENDIAN above.

At least on Solaris if we get this far that won't be the case.

> Thanks, but this is starting to feel like watching a whack-a-mole
> played while blindfolded.

Yeah for sure, I don't have access to solaris/cygwin or other obscure
platforms myself.

I do think this is fundimentally a better approach though since we first
check exactly what we know GCC / clang have, then glibc etc.

But yeah, by the time we're detecting Solaris I'm somewhat shooting
blind.

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27 2017, Junio C. Hamano jotted:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> +#else /* Not under GCC-alike or glibc or <processor whitelist>  or <processor blacklist> */
>>> +
>>> +#ifdef _BIG_ENDIAN
>>> +/*
>>> + * Solaris / illumos defines either _LITTLE_ENDIAN or _BIG_ENDIAN in
>>> + * <sys/isa_defs.h>.
>>> + */
>>> +#define SHA1DC_BIGENDIAN
>>
>> This makes readers of this patch wonder why we assume platforms
>> won't define _LITTLE_ENDIAN and _BIG_ENDIAN at the same time, just
>> like we saw in the section with __BIG_ENDIAN above.
>
> To be a bit more constructive, I'd feel it MUCH safer, if this "If
> _BIG_ENDIAN is defined, set SHA1DC_BIGENDIAN" is done _ONLY_ when
> we definitively KNOW that we are on Solaris, something like:
>
> 	#if defined(__sun) && defined(_BIG_ENDIAN)
> 	/*
> 	 * Solaris ...
> 	 */
> 	#define SHA1DC_BIGENDIAN
> 	#endif

Yes, this would be better, but I don't know what macro test to use to
test for Solaris. Oracle documents defined(sun), but that doesn't work
on the Solaris versions we tested, and looking/searching illumos headers
I didn't find anything obvious.

The __sun define is ours, and would work for us, but upstream couldn't
take it.

>> Thanks, but this is starting to feel like watching a whack-a-mole
>> played while blindfolded.  At some point, somebody upstream should
>> declare that enough is enough and introduce the "SHA1DC_FORCE_ENDIAN"
>> macro.

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27 2017, Junio C. Hamano jotted:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> +#if (defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
>>>       defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
>>>       defined(__sparc))
>>> +/*
>>> + * Should define Big Endian for a whitelist of known processors. See
>>> + * https://sourceforge.net/p/predef/wiki/Endianness/ and
>>> + * http://www.oracle.com/technetwork/server-storage/solaris/portingtosolaris-138514.html
>>> + */
>>>  #define SHA1DC_BIGENDIAN
>>
>> These look sensible.
>
> By the way, I wonder why this didn't catch the sparc running
> Solaris.  What does Michael's system use to let the software know
> that it is targetted for a Sparc, if not __sparc?

Because in the current code is, abbreviated:

    #if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
    #if /* byte order is bendian */
    #define SHA1DC_BIGENDIAN
    #endif
    #else
    #if /*some processor tests/* || defined(__sparc))
    #define SHA1DC_BIGENDIAN
    #endif

And since Solaris defines _BYTE_ORDER we never get to checking __sparc,
and in fact the "/* byte order is bendian */" test errors out.

This is fixed by my patch, where we first check gcc settings, then
glibc, then processors, and finally _BYTE_ORDER (but as discussed that
last bit could be adjusted to sun && _BYTE_ORDER, if we can find what
"sun" is.

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Junio C Hamano]

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

> On Tue, Jun 27 2017, Junio C. Hamano jotted:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> Update sha1dc from my PR #36[1] which'll hopefully be integrated by
>>> upstream soon.
>>
>> Please be careful about the title of the patch.  "log --oneline"
>> does not even let you tell your readers who calls this as "my"
>> change, and readers would be clueless what PR #36 is.  Something
>> like
>>
>>     sha1dc: correct endian detection for Solaris
>>
>> may give us more relevant information in the oneline output.
>
> Will fix. Can you integrate it as-is into pu anyway? I'm going to need
> to re-submit it regardless once we get some testing on it & upstream
> merges the PR, but having it in Travis et al in the meantime would be
> great.

I somehow thought that your throwing a pull request at git.git would 
automatically trigger a Travis test, without you having to wait for
me to do anything?  Am I misinformed?

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Tue, Jun 27 2017, Junio C. Hamano jotted:
>>
>>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>>
>>>> Update sha1dc from my PR #36[1] which'll hopefully be integrated by
>>>> upstream soon.
>>>
>>> Please be careful about the title of the patch.  "log --oneline"
>>> does not even let you tell your readers who calls this as "my"
>>> change, and readers would be clueless what PR #36 is.  Something
>>> like
>>>
>>>     sha1dc: correct endian detection for Solaris
>>>
>>> may give us more relevant information in the oneline output.
>>
>> Will fix. Can you integrate it as-is into pu anyway? I'm going to need
>> to re-submit it regardless once we get some testing on it & upstream
>> merges the PR, but having it in Travis et al in the meantime would be
>> great.
>
> I somehow thought that your throwing a pull request at git.git would
> automatically trigger a Travis test, without you having to wait for
> me to do anything?  Am I misinformed?

It's a PR against sha1collisiondetection, not git.git, but yeah, good
point, so that runs the same tests?

Still, the set of people testing pu > number of things running the
Travis tests, so it would be good to have it in pu, so we can just make
2.13.3 and hopefully not a .4 for the same issue :)

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Junio C Hamano]

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

>>> +#if defined(__BYTE_ORDER) && defined(__BIG_ENDIAN)
>>> +/*
>>> + * Should detect Big Endian under glibc.git since 14245eb70e ("entered
>>> + * into RCS", 1992-11-25). Defined in <endian.h> which will have been
>>> + * brought in by standard headers. See glibc.git and
>>> + * https://sourceforge.net/p/predef/wiki/Endianness/
>>> + */
>>> +#if __BYTE_ORDER == __BIG_ENDIAN
>>>  #define SHA1DC_BIGENDIAN
>>>  #endif
>>
>> Note that this part of the file considers it a valid way for a
>> platform to define a constant BIG_ENDIAN that can be compared to
>> BYTE_ORDER to determine the endianness, implying that such a scheme
>> would also define LITTLE_ENDIAN and a port of such a platform to a
>> little endian box will still _define_ the constant BIG_ENDIAN; it
>> aill have BYTE_ORDER defined to the same value as LITTLE_ENDIAN,
>> though.
>
> This may fail if we have some non-glibc platform that's defining
> __BYTE_ORDER and __BIG_ENDIAN, but if it's glibc then __BIG_ENDIAN will
> always be defined, even on little-endian platforms.

Yes, that is exactly the point of my comment.  We want to be
prepared to see a platform that is not big endian to define
BIG_ENDIAN (with some underscore).

>>> +#else /* Not under GCC-alike or glibc or <processor whitelist>  or <processor blacklist> */
>>> +
>>> +#ifdef _BIG_ENDIAN
>>> +/*
>>> + * Solaris / illumos defines either _LITTLE_ENDIAN or _BIG_ENDIAN in
>>> + * <sys/isa_defs.h>.
>>> + */
>>> +#define SHA1DC_BIGENDIAN
>>
>> This makes readers of this patch wonder why we assume platforms
>> won't define _LITTLE_ENDIAN and _BIG_ENDIAN at the same time, just
>> like we saw in the section with __BIG_ENDIAN above.
>
> At least on Solaris if we get this far that won't be the case.

Yes, but the remainder of world is not all Solaris.

Re: Compile Error v2.13.2 on Solaris SPARC

[Liam R. Howlett]

* Junio C Hamano <gitster@pobox.com> [170627 13:38]:
> "Liam R. Howlett" <Liam.Howlett@Oracle.com> writes:
> 
> > How about:
> >
> > ---- 8< ----
> > diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
> > index facea1bb5..ed8c63f2d 100644
> > --- a/sha1dc/sha1.c
> > +++ b/sha1dc/sha1.c
> > @@ -38,9 +38,18 @@
> >  
> >  #if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
> >  
> > +#define EMPTY_VAL(x) x ## 1
> > +#define EMPTY(x) EMPTY_VAL(x)
> > +
> > +#if (defined(_BIG_ENDIAN) && (EMPTY(_BIG_ENDIAN) == 1))
> > +#undef _BIG_ENDIAN
> > +#define _BIG_ENDIAN 4321
> > +#endif
> 
> I'd say it is a bad idea to define a symbol that you _know_ a
> platform header file defines.  Any header you may include from the
> platform after these lines still expects the symbol to be defined in
> a way it defines without getting molested and will misbehave.

Okay.  Thanks.  I thought a c file would be safe, especially after the
includes but there is indeed a possible include (ifdef'ed) later.

This compressed logic is causing a lot of issues.  Could we just rewrite
it as a whole lot of #if/#else, statements to avoid running across the
issue where the precompiler does not short-circuit the checks?  Would
this cause any other issues?

Alternatively, we can replace the undef/define with the define of
SHA1DC_BIGENDIAN and make this an #if/#else..

A third option is to compile a small test and just -DSHA1DC_BIGENDIAN in
the Makefile.

Cheers,
Liam

Re: [PATCH 0/3] update sha1dc from PR #36

[Stefan Beller]

On Tue, Jun 27, 2017 at 5:17 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> This hopefully fixes the Solaris SPARC issue & doesn't cause
> regressions elsewhere, e.g. on Cygwin. Adam, it would be great if you
> could test that platform.
>
> I've already confirmed with Michael Kebe + another SPARC user
> (CosmicDJ on Freenode #Solaris) that it works on Solaris SPARC. The
> question is whether it breaks anything else.
>
> Per the upstream pull request:
> https://github.com/cr-marcstevens/sha1collisiondetection/pull/36
>
> Marc would (understandably) like some wider testing of this before
> merging it into the upstream project.
>
> WRT the submodule URL & branch changing: I have no idea how
> git-submodule handles this in the general case,

It is trying a couple of things, see cmd_update in git-submodule.sh
look for the call sites of fetch_in_submodule which tries to
fetch the default branch, by-sha1 if that doesn't help.

> but it Just Works with
> GitHub because it allows fetching arbitrary SHA1s that any ref
> (including pull req refs) point to.

Which is only half the story (but it works most of the time).
They may have uploadpack.allowTipSHA1InWant but what is
really interesting is uploadpack.allowReachableSHA1InWant.

Re: [PATCH 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Stefan Beller]

On Tue, Jun 27, 2017 at 5:17 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Add an option to use the sha1collisiondetection library from the
> submodule in sha1collisiondetection/ instead of in the copy in the
> sha1dc/ directory.
>
> This allows us to try out the submodule in sha1collisiondetection
> without breaking the build for anyone who's not expecting them as we
> work out any kinks.
>
> This uses my own fork which integrates PR #36. See the preceding
> commit ("sha1dc: update from my PR #36", 2017-06-27) for details.
>

> +++ b/.gitmodules
> @@ -0,0 +1,4 @@
> +[submodule "sha1collisiondetection"]
> +       path = sha1collisiondetection
> +       url = https://github.com/avar/sha1collisiondetection.git
> +       branch = bigend-detect-solaris-again

What is the motivation for the branch field here?
While this series fixes a hot issue, in the long run we'd rather
want to plug in the original upstream with the master branch?

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>>> +#if defined(__BYTE_ORDER) && defined(__BIG_ENDIAN)
>>>> +/*
>>>> + * Should detect Big Endian under glibc.git since 14245eb70e ("entered
>>>> + * into RCS", 1992-11-25). Defined in <endian.h> which will have been
>>>> + * brought in by standard headers. See glibc.git and
>>>> + * https://sourceforge.net/p/predef/wiki/Endianness/
>>>> + */
>>>> +#if __BYTE_ORDER == __BIG_ENDIAN
>>>>  #define SHA1DC_BIGENDIAN
>>>>  #endif
>>>
>>> Note that this part of the file considers it a valid way for a
>>> platform to define a constant BIG_ENDIAN that can be compared to
>>> BYTE_ORDER to determine the endianness, implying that such a scheme
>>> would also define LITTLE_ENDIAN and a port of such a platform to a
>>> little endian box will still _define_ the constant BIG_ENDIAN; it
>>> aill have BYTE_ORDER defined to the same value as LITTLE_ENDIAN,
>>> though.
>>
>> This may fail if we have some non-glibc platform that's defining
>> __BYTE_ORDER and __BIG_ENDIAN, but if it's glibc then __BIG_ENDIAN will
>> always be defined, even on little-endian platforms.
>
> Yes, that is exactly the point of my comment.  We want to be
> prepared to see a platform that is not big endian to define
> BIG_ENDIAN (with some underscore).

Indeed, but FWIW this is the very first test in v2.13.0, so this
specific logic has already seen quite a bit of testing/porting on
numerous systems without issues (except Solaris obviously), but we'll
hopefully fix that this time around.

>>>> +#else /* Not under GCC-alike or glibc or <processor whitelist>  or <processor blacklist> */
>>>> +
>>>> +#ifdef _BIG_ENDIAN
>>>> +/*
>>>> + * Solaris / illumos defines either _LITTLE_ENDIAN or _BIG_ENDIAN in
>>>> + * <sys/isa_defs.h>.
>>>> + */
>>>> +#define SHA1DC_BIGENDIAN
>>>
>>> This makes readers of this patch wonder why we assume platforms
>>> won't define _LITTLE_ENDIAN and _BIG_ENDIAN at the same time, just
>>> like we saw in the section with __BIG_ENDIAN above.
>>
>> At least on Solaris if we get this far that won't be the case.

It actually turns out this isn't needed, since we can just check __sparc
and skip checking _BIG_ENDIAN, so no Solaris-specific code needed.

> Yes, but the remainder of world is not all Solaris.

Indeed, but either once we're this far in the checks we're past anything
that runs gcc/clang/glibc or <list of known bigendian processors>, which
is going to be a small list regardless.

Anyway, will fix.

Re: Compile Error v2.13.2 on Solaris SPARC

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27 2017, Liam R. Howlett jotted:

> This compressed logic is causing a lot of issues.  Could we just rewrite
> it as a whole lot of #if/#else, statements to avoid running across the
> issue where the precompiler does not short-circuit the checks?  Would
> this cause any other issues?

Again, this is hopefully addressed by my
20170627121718.12078-2-avarab@gmail.com ([PATCH 1/3] sha1dc: update from
my PR #36).

> A third option is to compile a small test and just -DSHA1DC_BIGENDIAN in
> the Makefile.

This would be ideal, but so far the only facility we have for that is
the configure script, which there have been objections to making a hard
dep in the past, thus we have various bits done via macros that would be
better done via built-time compiling & testing a C program.

My memory of such discussions is hazy though, did people fundimentally
object to the idea, or was it just an objection to autoconf in
particular, I don't know.

If it was just autoconf maybe someone more clever at Makefile magic than
me could come up with a way to compile a test program that would then
define a flag that would be passed to the rest of the programs, and set
up dependencies in such a way that it was done before anything else, I
don't know if that's easy/possible with make.

Re: [PATCH 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27, 2017 at 8:46 PM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Jun 27, 2017 at 5:17 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Add an option to use the sha1collisiondetection library from the
>> submodule in sha1collisiondetection/ instead of in the copy in the
>> sha1dc/ directory.
>>
>> This allows us to try out the submodule in sha1collisiondetection
>> without breaking the build for anyone who's not expecting them as we
>> work out any kinks.
>>
>> This uses my own fork which integrates PR #36. See the preceding
>> commit ("sha1dc: update from my PR #36", 2017-06-27) for details.
>>
>
>> +++ b/.gitmodules
>> @@ -0,0 +1,4 @@
>> +[submodule "sha1collisiondetection"]
>> +       path = sha1collisiondetection
>> +       url = https://github.com/avar/sha1collisiondetection.git
>> +       branch = bigend-detect-solaris-again
>
> What is the motivation for the branch field here?
> While this series fixes a hot issue, in the long run we'd rather
> want to plug in the original upstream with the master branch?

Just so we can cook this in pu (including for those cloning with
--recursive) until upstream merges my patches.

We're now in a chicken & egg scenario where the patch I have (minus
nits mentioned on list, will fix) should work, but needs testing, and
once it has testing upstream is willing to merge it, and getting it
into pu (with my clone/branch) will give it more testing).

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Junio C Hamano]

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

> Because in the current code is, abbreviated:
>
>     #if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
>     #if /* byte order is bendian */
>     #define SHA1DC_BIGENDIAN
>     #endif
>     #else
>     #if /*some processor tests/* || defined(__sparc))
>     #define SHA1DC_BIGENDIAN
>     #endif
>
> And since Solaris defines _BYTE_ORDER we never get to checking __sparc,
> and in fact the "/* byte order is bendian */" test errors out.
>
> This is fixed by my patch, where we first check gcc settings, then
> glibc, then processors, and finally _BYTE_ORDER (but as discussed that
> last bit could be adjusted to sun && _BYTE_ORDER, if we can find what
> "sun" is.

Well, if Solaris defines _BYTE_ORDER, doesn't that mean they define
two constants _BIG_ENDIAN and _LITTLE_ENDIAN to compare it with?  If
that is the case, I suspect that the change to make "comparison
between __BYTE_ORDER and __BIG_ENDIAN for GCC only" is going in a
wrong direction, as it wants to take the same approach as GCC, but
just uses a slightly different symbol.

I wonder if the approach like the following might be cleaner to
extend as we find other oddball platforms.

    #undef __SHA1DC_BYTE_ORDER
    #if defined(_BYTE_ORDER)
    #define __SHA1DC_BYTE_ORDER _BYTE_ORDER
    #elif defined(__BYTE_ORDER)
    #define __SHA1DC_BYTE_ORDER __BYTE_ORDER
    #elif defined(__BYTE_ORDER__))
    #define __SHA1DC_BYTE_ORDER __BYTE_ORDER__
    #endif

    #ifdef __SHA1DC_BYTE_ORDER
     #undef __SHA1DC_BIG_ENDIAN
     /* do the same for variations of BIG_ENDIAN constant */
     #if defined(_BIG_ENDIAN)
	...
     #endif

     #if __SHA1DC_BYTE_ORDER == __SHA1DC_BIG_ENDIAN
     #define SHA1DC_BIGENDIAN
     #endif
    #else
     /* 
      * as the platform does not use "compare BYTE-ORDER with
      * BIG_ENDIAN macro" strategy, defined-ness of BIG_ENDIAN
      * may be usable as a sign that it is a big-endian box.
      */
    #endif

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Junio C Hamano]

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

> I wonder if the approach like the following might be cleaner to
> extend as we find other oddball platforms.
>
>     #undef __SHA1DC_BYTE_ORDER
>     #if defined(_BYTE_ORDER)
>     #define __SHA1DC_BYTE_ORDER _BYTE_ORDER
>     #elif defined(__BYTE_ORDER)
>     #define __SHA1DC_BYTE_ORDER __BYTE_ORDER
>     #elif defined(__BYTE_ORDER__))
>     #define __SHA1DC_BYTE_ORDER __BYTE_ORDER__
>     #endif
>
>     #ifdef __SHA1DC_BYTE_ORDER
>      #undef __SHA1DC_BIG_ENDIAN
>      /* do the same for variations of BIG_ENDIAN constant */
>      #if defined(_BIG_ENDIAN)
> 	...
>      #endif
>
>      #if __SHA1DC_BYTE_ORDER == __SHA1DC_BIG_ENDIAN
>      #define SHA1DC_BIGENDIAN
>      #endif
>     #else
>      /* 
>       * as the platform does not use "compare BYTE-ORDER with
>       * BIG_ENDIAN macro" strategy, defined-ness of BIG_ENDIAN
>       * may be usable as a sign that it is a big-endian box.
>       */
>     #endif

IF the above turns out to be a good approach, it may be better to
determine the __SHA1DC_BIG_ENDIAN outside #ifdef __SHA1DC_BYTE_ORDER
block.  That makes the resulting #if/#endif nest shallower; the last
part for platforms that uses defined-ness of BIG_ENDIAN (with
underscore variants) needs to know __SHA1DC_BYTE_ORDER anyway.

So in short, two preparatory blocks followed by the real thing,
something along the lines of...

        #undef __SHA1DC_BYTE_ORDER
        /* set the above from BYTE_ORDER with underscore */

        #undef __SHA1DC_BIG_ENDIAN
        /* set the above from BIG_ENDIAN with underscore */

        #undef SHA1DC_BIGENDIAN
        #if defined(__SHA1DC_BYTE_ORDER)
        # if defined(__SHA1DC_BIG_ENDIAN)
        #  if __SHA1DC_BYTE_ORDER == __SHA1DC_BIG_ENDIAN
        #   define SHA1DC_BIGENDIAN
        #  endif
        # endif
        #else

          /* other heuristics like processor bits here */

        /* 
         * the platform does not compare BYTE-ORDER with BIG-ENDIAN
         * so take the definedness of BIG-ENDIAN as the sign that
         * the box is big endian.
         */
        # if defined(__SHA1DC_BIG_ENDIAN)
        #  define SHA1DC_BIGENDIAN
        # endif
        #endif

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Because in the current code is, abbreviated:
>>
>>     #if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
>>     #if /* byte order is bendian */
>>     #define SHA1DC_BIGENDIAN
>>     #endif
>>     #else
>>     #if /*some processor tests/* || defined(__sparc))
>>     #define SHA1DC_BIGENDIAN
>>     #endif
>>
>> And since Solaris defines _BYTE_ORDER we never get to checking __sparc,
>> and in fact the "/* byte order is bendian */" test errors out.
>>
>> This is fixed by my patch, where we first check gcc settings, then
>> glibc, then processors, and finally _BYTE_ORDER (but as discussed that
>> last bit could be adjusted to sun && _BYTE_ORDER, if we can find what
>> "sun" is.
>
> Well, if Solaris defines _BYTE_ORDER, doesn't that mean they define
> two constants _BIG_ENDIAN and _LITTLE_ENDIAN to compare it with?

No, under gcc/clang & glibc you're expected to compare them. Under
Solaris it's just defined(_BIG_ENDIAN), but as explained in another
comment this whole thing actually turns out to be not needed, on Solaris
it's sufficient that we fall through and check __sparc.

> that is the case, I suspect that the change to make "comparison
> between __BYTE_ORDER and __BIG_ENDIAN for GCC only" is going in a
> wrong direction, as it wants to take the same approach as GCC, but
> just uses a slightly different symbol.
>
> I wonder if the approach like the following might be cleaner to
> extend as we find other oddball platforms.
>
>     #undef __SHA1DC_BYTE_ORDER
>     #if defined(_BYTE_ORDER)
>     #define __SHA1DC_BYTE_ORDER _BYTE_ORDER
>     #elif defined(__BYTE_ORDER)
>     #define __SHA1DC_BYTE_ORDER __BYTE_ORDER
>     #elif defined(__BYTE_ORDER__))
>     #define __SHA1DC_BYTE_ORDER __BYTE_ORDER__
>     #endif
>
>     #ifdef __SHA1DC_BYTE_ORDER
>      #undef __SHA1DC_BIG_ENDIAN
>      /* do the same for variations of BIG_ENDIAN constant */
>      #if defined(_BIG_ENDIAN)
> 	...
>      #endif
>
>      #if __SHA1DC_BYTE_ORDER == __SHA1DC_BIG_ENDIAN
>      #define SHA1DC_BIGENDIAN
>      #endif

As explained above no, because some e.g. _BIG_ENDIAN don't have a value
at all.

But more generally we can't assume that we can just exhaustively search
for some ^_*BIG_ENDIAN_*$ macro and compare it with some
^_*BYTE_ORDER_*$ value and get an expected result.

>     #else
>      /*
>       * as the platform does not use "compare BYTE-ORDER with
>       * BIG_ENDIAN macro" strategy, defined-ness of BIG_ENDIAN
>       * may be usable as a sign that it is a big-endian box.
>       */
>     #endif

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Junio C Hamano]

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

>>> And since Solaris defines _BYTE_ORDER we never get to checking __sparc,
>>> and in fact the "/* byte order is bendian */" test errors out.
>>> ...
>> Well, if Solaris defines _BYTE_ORDER, doesn't that mean they define
>> two constants _BIG_ENDIAN and _LITTLE_ENDIAN to compare it with?
>
> No, under gcc/clang & glibc you're expected to compare them. Under
> Solaris it's just defined(_BIG_ENDIAN), but as explained in another
> comment this whole thing actually turns out to be not needed, on Solaris
> it's sufficient that we fall through and check __sparc.

Huh.  It makes me wonder what we are expected to use _BYTE_ORDER on
Solaris for, then, if it defines _BIG_ENDIAN and _BYTE_ORDER, and
still wants us to use the definedness of _BIG_ENDIAN.  It does not
make any sense.

But if we can use __sparc and fold it into various arm/mipts bits,
that is much simpler ;-)

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Liam R. Howlett]

* Junio C Hamano <gitster@pobox.com> [170627 15:10]:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > Because in the current code is, abbreviated:
> >
> >     #if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
> >     #if /* byte order is bendian */
> >     #define SHA1DC_BIGENDIAN
> >     #endif
> >     #else
> >     #if /*some processor tests/* || defined(__sparc))
> >     #define SHA1DC_BIGENDIAN
> >     #endif
> >
> > And since Solaris defines _BYTE_ORDER we never get to checking __sparc,
> > and in fact the "/* byte order is bendian */" test errors out.
> >
> > This is fixed by my patch, where we first check gcc settings, then
> > glibc, then processors, and finally _BYTE_ORDER (but as discussed that
> > last bit could be adjusted to sun && _BYTE_ORDER, if we can find what
> > "sun" is.
> 
> Well, if Solaris defines _BYTE_ORDER, doesn't that mean they define
> two constants _BIG_ENDIAN and _LITTLE_ENDIAN to compare it with?  If
> that is the case, I suspect that the change to make "comparison
> between __BYTE_ORDER and __BIG_ENDIAN for GCC only" is going in a
> wrong direction, as it wants to take the same approach as GCC, but
> just uses a slightly different symbol.

That's not the case.  _BIG_ENDIAN is defined but not to a value.  I
believe _BYTE_ORDER is defined as 4321, but _BIG_ENDIAN is just defined.
This is why I proposed detecting the empty state of _BIG_ENDIAN.  If
_BIG_ENDIAN is defined but not to a value, then we already know it's big
endian.  I believe your approach below can be altered slightly to
account for this issue.

> 
> I wonder if the approach like the following might be cleaner to
> extend as we find other oddball platforms.
> 
>     #undef __SHA1DC_BYTE_ORDER
>     #if defined(_BYTE_ORDER)
>     #define __SHA1DC_BYTE_ORDER _BYTE_ORDER
>     #elif defined(__BYTE_ORDER)
>     #define __SHA1DC_BYTE_ORDER __BYTE_ORDER
>     #elif defined(__BYTE_ORDER__))
>     #define __SHA1DC_BYTE_ORDER __BYTE_ORDER__
>     #endif
> 
>     #ifdef __SHA1DC_BYTE_ORDER
>      #undef __SHA1DC_BIG_ENDIAN
>      /* do the same for variations of BIG_ENDIAN constant */
>      #if defined(_BIG_ENDIAN)
> 	...
>      #endif
> 
>      #if __SHA1DC_BYTE_ORDER == __SHA1DC_BIG_ENDIAN
>      #define SHA1DC_BIGENDIAN
>      #endif
>     #else
>      /* 
>       * as the platform does not use "compare BYTE-ORDER with
>       * BIG_ENDIAN macro" strategy, defined-ness of BIG_ENDIAN
>       * may be usable as a sign that it is a big-endian box.
>       */
>     #endif
> 

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Junio C Hamano]

On Tue, Jun 27, 2017 at 12:38 PM, Liam R. Howlett
<Liam.Howlett@oracle.com> wrote:
> That's not the case.  _BIG_ENDIAN is defined but not to a value.  I
> believe _BYTE_ORDER is defined as 4321, but _BIG_ENDIAN is just defined.

OK. Thanks.

That sounds pretty much useless in the 21st century, where _BYTE_ORDER might
want to be defined as 87654321 ;-)

Re: [PATCH 1/3] sha1dc: update from my PR #36

[Junio C Hamano]

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

> On Tue, Jun 27 2017, Junio C. Hamano jotted:
>
>> I somehow thought that your throwing a pull request at git.git would
>> automatically trigger a Travis test, without you having to wait for
>> me to do anything?  Am I misinformed?
>
> It's a PR against sha1collisiondetection, not git.git, but yeah, good
> point, so that runs the same tests?
>
> Still, the set of people testing pu > number of things running the
> Travis tests, so it would be good to have it in pu, so we can just make
> 2.13.3 and hopefully not a .4 for the same issue :)

This has happened.

Until the upstream takes it and .gitmodules can point at them, I'd
hesitate to merge it to 'next', but I think your plan is the same as
mine.

Thanks.

[PATCH v2 0/3] update sha1dc from PR #36

[Ævar Arnfjörð Bjarmason]

A v2 addressing comments to the v1, tbdiff below. Just fixes the
subject line on 1/3 & gets rid of the redundant _BIG_ENDIAN detection
for Solaris, we can just use __sparc (and indeed we did before this).

Junio C Hamano (1):
  sha1collisiondetection: automatically enable when submodule is
    populated

Ævar Arnfjörð Bjarmason (2):
  sha1dc: correct endian detection for Solaris (and others?)
  sha1dc: optionally use sha1collisiondetection as a submodule

 .gitmodules            |  4 +++
 Makefile               | 16 +++++++++++
 hash.h                 |  4 +++
 sha1collisiondetection |  1 +
 sha1dc/sha1.c          | 73 +++++++++++++++++++++++++++++++++++++-------------
 5 files changed, 79 insertions(+), 19 deletions(-)
 create mode 100644 .gitmodules
 create mode 160000 sha1collisiondetection

    @@ -1,6 +1,6 @@
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

    -    sha1dc: update from my PR #36
    +    sha1dc: correct endian detection for Solaris (and others?)

         Update sha1dc from my PR #36[1] which'll hopefully be integrated by
         upstream soon.
    @@ -96,22 +96,15 @@
     +
     +#if defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
     +/*
    -+ * As a last resort before we fall back on _BIG_ENDIAN or whatever
    -+ * else we're not 100% sure about below, we blacklist specific
    -+ * processors here. We could add more, see
    -+ * e.g. https://wiki.debian.org/ArchitectureSpecificsMemo
    ++ * As a last resort before we do anything else we're not 100% sure
    ++ * about below, we blacklist specific processors here. We could add
    ++ * more, see e.g. https://wiki.debian.org/ArchitectureSpecificsMemo
     + */
     +#else /* Not under GCC-alike or glibc or <processor whitelist>  or <processor blacklist> */
     +
    -+#ifdef _BIG_ENDIAN
    -+/*
    -+ * Solaris / illumos defines either _LITTLE_ENDIAN or _BIG_ENDIAN in
    -+ * <sys/isa_defs.h>.
    -+ */
    -+#define SHA1DC_BIGENDIAN
    -+#else
    ++/* We do nothing more here for now */
     +/*#error "Uncomment this to see if you fall through all the detection"*/
    -+#endif /* Big Endian because of _BIG_ENDIAN (Solaris)*/
    ++
     +#endif /* !SHA1DC_ON_INTEL_LIKE_PROCESSOR */
     +#endif /* Big Endian under whitelist of processors */
     +#endif /* Big Endian under glibc */
2: 63a5d0cb2a ! 2: 28750d3c24 sha1dc: optionally use sha1collisiondetection as a submodule
    @@ -79,4 +79,4 @@
     --- /dev/null
     +++ b/sha1collisiondetection
     @@
    -+Subproject commit 56ab30c4c998e1e7f3075705087a2f0c4c4202d7
    ++Subproject commit 9d3a0b3783afab335a1819543d039cf2980577bb


-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 1/3] sha1dc: correct endian detection for Solaris (and others?)

[Ævar Arnfjörð Bjarmason]

Update sha1dc from my PR #36[1] which'll hopefully be integrated by
upstream soon.

This solves the Big Endian detection on Solaris reported against
v2.13.2[2], hopefully without any regressions.

See commit a0103914c2 ("sha1dc: update from upstream", 2017-05-20) and
6b851e536b ("sha1dc: update from upstream", 2017-06-06) for previous
attempts in the 2.13 series to address various compile-time feature
detection in this library.

1. https://github.com/cr-marcstevens/sha1collisiondetection/pull/36
   https://github.com/avar/sha1collisiondetection/commit/56ab30c4c998e1e7f3075705087a2f0c4c4202d7
2. <CAKKM46tHq13XiW5C8sux3=PZ1VHSu_npG8ExfWwcPD7rkZkyRQ@mail.gmail.com>
   (https://public-inbox.org/git/CAKKM46tHq13XiW5C8sux3=PZ1VHSu_npG8ExfWwcPD7rkZkyRQ@mail.gmail.com/)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sha1dc/sha1.c | 73 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 54 insertions(+), 19 deletions(-)

diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index facea1bb56..1e7e30763e 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -23,6 +23,13 @@
 #include "sha1.h"
 #include "ubc_check.h"
 
+#if (defined(__amd64__) || defined(__amd64) || defined(__x86_64__) || defined(__x86_64) || \
+     defined(i386) || defined(__i386) || defined(__i386__) || defined(__i486__)  || \
+     defined(__i586__) || defined(__i686__) || defined(_M_IX86) || defined(__X86__) || \
+     defined(_X86_) || defined(__THW_INTEL__) || defined(__I86__) || defined(__INTEL__) || \
+     defined(__386) || defined(_M_X64) || defined(_M_AMD64))
+#define SHA1DC_ON_INTEL_LIKE_PROCESSOR
+#endif
 
 /*
    Because Little-Endian architectures are most common,
@@ -32,28 +39,63 @@
    If you are compiling on a big endian platform and your compiler does not define one of these,
    you will have to add whatever macros your tool chain defines to indicate Big-Endianness.
  */
-#ifdef SHA1DC_BIGENDIAN
-#undef SHA1DC_BIGENDIAN
+
+#if defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__)
+/*
+ * Should detect Big Endian under GCC since at least 4.6.0 (gcc svn
+ * rev #165881). See
+ * https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
+ *
+ * This also works under clang since 3.2, it copied the GCC-ism. See
+ * clang.git's 3b198a97d2 ("Preprocessor: add __BYTE_ORDER__
+ * predefined macro", 2012-07-27)
+ */
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+#define SHA1DC_BIGENDIAN
 #endif
 
-#if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
+#else /* Not under GCC-alike */
 
-#if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
+#if defined(__BYTE_ORDER) && defined(__BIG_ENDIAN)
+/*
+ * Should detect Big Endian under glibc.git since 14245eb70e ("entered
+ * into RCS", 1992-11-25). Defined in <endian.h> which will have been
+ * brought in by standard headers. See glibc.git and
+ * https://sourceforge.net/p/predef/wiki/Endianness/
+ */
+#if __BYTE_ORDER == __BIG_ENDIAN
 #define SHA1DC_BIGENDIAN
 #endif
 
-#else
+#else /* Not under GCC-alike or glibc */
 
-#if (defined(_BIG_ENDIAN) || defined(__BIG_ENDIAN) || defined(__BIG_ENDIAN__) || \
-     defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
+#if (defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
      defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
      defined(__sparc))
+/*
+ * Should define Big Endian for a whitelist of known processors. See
+ * https://sourceforge.net/p/predef/wiki/Endianness/ and
+ * http://www.oracle.com/technetwork/server-storage/solaris/portingtosolaris-138514.html
+ */
 #define SHA1DC_BIGENDIAN
-#endif
 
-#endif
+#else /* Not under GCC-alike or glibc or <processor whitelist> */
+
+#if defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
+/*
+ * As a last resort before we do anything else we're not 100% sure
+ * about below, we blacklist specific processors here. We could add
+ * more, see e.g. https://wiki.debian.org/ArchitectureSpecificsMemo
+ */
+#else /* Not under GCC-alike or glibc or <processor whitelist>  or <processor blacklist> */
+
+/* We do nothing more here for now */
+/*#error "Uncomment this to see if you fall through all the detection"*/
+
+#endif /* !SHA1DC_ON_INTEL_LIKE_PROCESSOR */
+#endif /* Big Endian under whitelist of processors */
+#endif /* Big Endian under glibc */
+#endif /* Big Endian under GCC-alike */
 
 #if (defined(SHA1DC_FORCE_LITTLEENDIAN) && defined(SHA1DC_BIGENDIAN))
 #undef SHA1DC_BIGENDIAN
@@ -63,15 +105,8 @@
 #endif
 /*ENDIANNESS SELECTION*/
 
-#if (defined SHA1DC_FORCE_UNALIGNED_ACCESS || \
-     defined(__amd64__) || defined(__amd64) || defined(__x86_64__) || defined(__x86_64) || \
-     defined(i386) || defined(__i386) || defined(__i386__) || defined(__i486__)  || \
-     defined(__i586__) || defined(__i686__) || defined(_M_IX86) || defined(__X86__) || \
-     defined(_X86_) || defined(__THW_INTEL__) || defined(__I86__) || defined(__INTEL__) || \
-     defined(__386) || defined(_M_X64) || defined(_M_AMD64))
-
+#if defined(SHA1DC_FORCE_UNALIGNED_ACCESS) || defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
 #define SHA1DC_ALLOW_UNALIGNED_ACCESS
-
 #endif /*UNALIGNMENT DETECTION*/
 
 
-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Ævar Arnfjörð Bjarmason]

Add an option to use the sha1collisiondetection library from the
submodule in sha1collisiondetection/ instead of in the copy in the
sha1dc/ directory.

This allows us to try out the submodule in sha1collisiondetection
without breaking the build for anyone who's not expecting them as we
work out any kinks.

This uses my own fork which integrates PR #36. See the preceding
commit ("sha1dc: update from my PR #36", 2017-06-27) for details.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .gitmodules            |  4 ++++
 Makefile               | 12 ++++++++++++
 hash.h                 |  4 ++++
 sha1collisiondetection |  1 +
 4 files changed, 21 insertions(+)
 create mode 100644 .gitmodules
 create mode 160000 sha1collisiondetection

diff --git a/.gitmodules b/.gitmodules
new file mode 100644
index 0000000000..2fea9996e9
--- /dev/null
+++ b/.gitmodules
@@ -0,0 +1,4 @@
+[submodule "sha1collisiondetection"]
+	path = sha1collisiondetection
+	url = https://github.com/avar/sha1collisiondetection.git
+	branch = bigend-detect-solaris-again
diff --git a/Makefile b/Makefile
index b94cd5633c..f0cac1f246 100644
--- a/Makefile
+++ b/Makefile
@@ -162,6 +162,12 @@ all::
 # algorithm. This is slower, but may detect attempted collision attacks.
 # Takes priority over other *_SHA1 knobs.
 #
+# Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
+# sha1collisiondetection shipped as a submodule instead of the
+# non-submodule copy in sha1dc/. This is an experimental option used
+# by the git project to migrate to using sha1collisiondetection as a
+# submodule.
+#
 # Define OPENSSL_SHA1 environment variable when running make to link
 # with the SHA1 routine from openssl library.
 #
@@ -1448,8 +1454,14 @@ ifdef APPLE_COMMON_CRYPTO
 	BASIC_CFLAGS += -DSHA1_APPLE
 else
 	DC_SHA1 := YesPlease
+ifdef DC_SHA1_SUBMODULE
+	LIB_OBJS += sha1collisiondetection/lib/sha1.o
+	LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
+	BASIC_CFLAGS += -DDC_SHA1_SUBMODULE
+else
 	LIB_OBJS += sha1dc/sha1.o
 	LIB_OBJS += sha1dc/ubc_check.o
+endif
 	BASIC_CFLAGS += \
 		-DSHA1_DC \
 		-DSHA1DC_NO_STANDARD_INCLUDES \
diff --git a/hash.h b/hash.h
index a11fc9233f..bef3e630a0 100644
--- a/hash.h
+++ b/hash.h
@@ -8,7 +8,11 @@
 #elif defined(SHA1_OPENSSL)
 #include <openssl/sha.h>
 #elif defined(SHA1_DC)
+#ifdef DC_SHA1_SUBMODULE
+#include "sha1collisiondetection/lib/sha1.h"
+#else
 #include "sha1dc/sha1.h"
+#endif
 #else /* SHA1_BLK */
 #include "block-sha1/sha1.h"
 #endif
diff --git a/sha1collisiondetection b/sha1collisiondetection
new file mode 160000
index 0000000000..9d3a0b3783
--- /dev/null
+++ b/sha1collisiondetection
@@ -0,0 +1 @@
+Subproject commit 9d3a0b3783afab335a1819543d039cf2980577bb
-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 3/3] sha1collisiondetection: automatically enable when submodule is populated

[Ævar Arnfjörð Bjarmason]

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

If a user wants to experiment with the version of collision
detecting sha1 from the submodule, the user needed to not just
populate the submodule but also needed to turn the knob.

A Makefile trick is easy enough to do so, so let's do this.  When
somebody with a copy of the submodule populated wants not to use it,
that can be done by overriding it in config.mak or from the command
line.

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

diff --git a/Makefile b/Makefile
index f0cac1f246..3baa1eac0b 100644
--- a/Makefile
+++ b/Makefile
@@ -1009,6 +1009,10 @@ EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)
 
+ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
+DC_SHA1_SUBMODULE = auto
+endif
+
 include config.mak.uname
 -include config.mak.autogen
 -include config.mak
-- 
2.13.1.611.g7e3b11ae1

Re: [PATCH v2 0/3] update sha1dc from PR #36

[Junio C Hamano]

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

> A v2 addressing comments to the v1, tbdiff below. Just fixes the
> subject line on 1/3 & gets rid of the redundant _BIG_ENDIAN detection
> for Solaris, we can just use __sparc (and indeed we did before this).

Thanks.  Let's have this tested by minority platform folks to help
the upstream digest your pull request ;-)

Re: [PATCH v2 0/3] update sha1dc from PR #36

[Junio C Hamano]

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

> A v2 addressing comments to the v1, tbdiff below. Just fixes the
> subject line on 1/3 & gets rid of the redundant _BIG_ENDIAN detection
> for Solaris, we can just use __sparc (and indeed we did before this).

Thanks.  Let's have this tested by minority platform folks to help
the upstream digest your pull request.  The change since the last
round makes perfect sense to me.

Thanks.

Re: [PATCH v2 0/3] update sha1dc from PR #36

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> A v2 addressing comments to the v1, tbdiff below. Just fixes the
>> subject line on 1/3 & gets rid of the redundant _BIG_ENDIAN detection
>> for Solaris, we can just use __sparc (and indeed we did before this).
>
> Thanks.  Let's have this tested by minority platform folks to help
> the upstream digest your pull request.  The change since the last
> round makes perfect sense to me.

I got someone on #cygwin on Freenode to test this for me (compile git +
t0013-sha1dc.sh), it passed, ditto for 2x people testing on Solaris
SPARC.

Updated a comment on the PR with that:
https://github.com/cr-marcstevens/sha1collisiondetection/pull/36#issuecomment-311502713

I'm pretty sure this un-breaks everything & doesn't regress on cases
where we fixed things earlier.

Re: [PATCH v2 0/3] update sha1dc from PR #36

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 27 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> A v2 addressing comments to the v1, tbdiff below. Just fixes the
>> subject line on 1/3 & gets rid of the redundant _BIG_ENDIAN detection
>> for Solaris, we can just use __sparc (and indeed we did before this).
>
> Thanks.  Let's have this tested by minority platform folks to help
> the upstream digest your pull request.  The change since the last
> round makes perfect sense to me.

Just a brief update. This has been found not to work on some BSDs (with
older GCC), but there's a patch being discussed at
https://github.com/cr-marcstevens/sha1collisiondetection/pull/34

The discussion is sorted out, just waiting on n-soda to submit a patch
(or to tell me he'd like me to), then the known issues will be fixed &
it should be merged quickly, then I'll submit a v3.

Re: [PATCH v2 0/3] update sha1dc from PR #36

[Junio C Hamano]

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

> On Tue, Jun 27 2017, Junio C. Hamano jotted:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> A v2 addressing comments to the v1, tbdiff below. Just fixes the
>>> subject line on 1/3 & gets rid of the redundant _BIG_ENDIAN detection
>>> for Solaris, we can just use __sparc (and indeed we did before this).
>>
>> Thanks.  Let's have this tested by minority platform folks to help
>> the upstream digest your pull request.  The change since the last
>> round makes perfect sense to me.
>
> Just a brief update. This has been found not to work on some BSDs (with
> older GCC), but there's a patch being discussed at
> https://github.com/cr-marcstevens/sha1collisiondetection/pull/34
>
> The discussion is sorted out, just waiting on n-soda to submit a patch
> (or to tell me he'd like me to), then the known issues will be fixed &
> it should be merged quickly, then I'll submit a v3.

Thanks.  

It's good that I learned to pretend that I didn't hear anything like

    I'm pretty sure this un-breaks everything & doesn't regress on cases
    where we fixed things earlier.

from anybody, and instead wait a bit longer ;-)

[PATCH v3 0/3] Update sha1dc from upstream

[Ævar Arnfjörð Bjarmason]

The upstream discussion about solving Big Endian detection concluded
with something that hopefully works on all our platforms, see
https://github.com/cr-marcstevens/sha1collisiondetection/pull/34

This updates us to the latest upstream commit.

Junio C Hamano (1):
  sha1collisiondetection: automatically enable when submodule is
    populated

Ævar Arnfjörð Bjarmason (2):
  sha1dc: update from upstream
  sha1dc: optionally use sha1collisiondetection as a submodule

 .gitmodules            |  4 +++
 Makefile               | 16 +++++++++
 hash.h                 |  4 +++
 sha1collisiondetection |  1 +
 sha1dc/sha1.c          | 90 +++++++++++++++++++++++++++++++++++++-------------
 5 files changed, 92 insertions(+), 23 deletions(-)
 create mode 100644 .gitmodules
 create mode 160000 sha1collisiondetection

-- 
2.13.1.611.g7e3b11ae1

[PATCH v3 1/3] sha1dc: update from upstream

[Ævar Arnfjörð Bjarmason]

Update sha1dc from the latest version by the upstream maintainer[1].

See commit 6b851e536b ("sha1dc: update from upstream", 2017-06-06) for
the last update.

This solves the Big Endian detection on Solaris reported against
v2.13.2[2], hopefully without any regressions. A version of this has
been tested on two Solaris SPARC installations, Cygwin (by jturney on
cygwin@Freenode), and on numerous more boring systems (mainly
linux/x86_64). See [3] for a discussion of the implementation and
platform-specific issues.

See commit a0103914c2 ("sha1dc: update from upstream", 2017-05-20) and
6b851e536b ("sha1dc: update from upstream", 2017-06-06) for previous
attempts in the 2.13 series to address various compile-time feature
detection in this library.

1. https://github.com/cr-marcstevens/sha1collisiondetection/commit/19d97bf5af05312267c2e874ee6bcf584d9e9681
2. <CAKKM46tHq13XiW5C8sux3=PZ1VHSu_npG8ExfWwcPD7rkZkyRQ@mail.gmail.com>
   (https://public-inbox.org/git/CAKKM46tHq13XiW5C8sux3=PZ1VHSu_npG8ExfWwcPD7rkZkyRQ@mail.gmail.com/)
3. https://github.com/cr-marcstevens/sha1collisiondetection/pull/34

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sha1dc/sha1.c | 90 ++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 67 insertions(+), 23 deletions(-)

diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index 3a1735e5ca..d5948826c8 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -10,6 +10,9 @@
 #include <memory.h>
 #include <stdio.h>
 #include <stdlib.h>
+#ifdef __unix__
+#include <sys/types.h> /* make sure macros like _BIG_ENDIAN visible */
+#endif
 #endif
 
 #ifdef SHA1DC_CUSTOM_INCLUDE_SHA1_C
@@ -23,6 +26,13 @@
 #include "sha1.h"
 #include "ubc_check.h"
 
+#if (defined(__amd64__) || defined(__amd64) || defined(__x86_64__) || defined(__x86_64) || \
+     defined(i386) || defined(__i386) || defined(__i386__) || defined(__i486__)  || \
+     defined(__i586__) || defined(__i686__) || defined(_M_IX86) || defined(__X86__) || \
+     defined(_X86_) || defined(__THW_INTEL__) || defined(__I86__) || defined(__INTEL__) || \
+     defined(__386) || defined(_M_X64) || defined(_M_AMD64))
+#define SHA1DC_ON_INTEL_LIKE_PROCESSOR
+#endif
 
 /*
    Because Little-Endian architectures are most common,
@@ -32,29 +42,70 @@
    If you are compiling on a big endian platform and your compiler does not define one of these,
    you will have to add whatever macros your tool chain defines to indicate Big-Endianness.
  */
-#ifdef SHA1DC_BIGENDIAN
-#undef SHA1DC_BIGENDIAN
-#endif
 
-#if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
-
-#if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
-     (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
+#if defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__)
+/*
+ * Should detect Big Endian under GCC since at least 4.6.0 (gcc svn
+ * rev #165881). See
+ * https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
+ *
+ * This also works under clang since 3.2, it copied the GCC-ism. See
+ * clang.git's 3b198a97d2 ("Preprocessor: add __BYTE_ORDER__
+ * predefined macro", 2012-07-27)
+ */
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
 #define SHA1DC_BIGENDIAN
 #endif
 
-#else
-
-#if (defined(_BIG_ENDIAN) || defined(__BIG_ENDIAN) || defined(__BIG_ENDIAN__) || \
-     defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
-     defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
-     defined(__sparc))
+/* Not under GCC-alike */
+#elif defined(__BYTE_ORDER) && defined(__BIG_ENDIAN)
+/*
+ * Should detect Big Endian under glibc.git since 14245eb70e ("entered
+ * into RCS", 1992-11-25). Defined in <endian.h> which will have been
+ * brought in by standard headers. See glibc.git and
+ * https://sourceforge.net/p/predef/wiki/Endianness/
+ */
+#if __BYTE_ORDER == __BIG_ENDIAN
 #define SHA1DC_BIGENDIAN
 #endif
 
+/* Not under GCC-alike or glibc */
+#elif defined(_BYTE_ORDER) && defined(_BIG_ENDIAN) && defined(_LITTLE_ENDIAN)
+/*
+ * *BSD and newlib (embeded linux, cygwin, etc).
+ * the defined(_BIG_ENDIAN) && defined(_LITTLE_ENDIAN) part prevents
+ * this condition from matching with Solaris/sparc.
+ * (Solaris defines only one endian macro)
+ */
+#if _BYTE_ORDER == _BIG_ENDIAN
+#define SHA1DC_BIGENDIAN
 #endif
 
+/* Not under GCC-alike or glibc or *BSD or newlib */
+#elif (defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
+       defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
+       defined(__sparc))
+/*
+ * Should define Big Endian for a whitelist of known processors. See
+ * https://sourceforge.net/p/predef/wiki/Endianness/ and
+ * http://www.oracle.com/technetwork/server-storage/solaris/portingtosolaris-138514.html
+ */
+#define SHA1DC_BIGENDIAN
+
+/* Not under GCC-alike or glibc or *BSD or newlib or <processor whitelist> */
+#elif defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
+/*
+ * As a last resort before we do anything else we're not 100% sure
+ * about below, we blacklist specific processors here. We could add
+ * more, see e.g. https://wiki.debian.org/ArchitectureSpecificsMemo
+ */
+#else /* Not under GCC-alike or glibc or *BSD or newlib or <processor whitelist>  or <processor blacklist> */
+
+/* We do nothing more here for now */
+/*#error "Uncomment this to see if you fall through all the detection"*/
+
+#endif /* Big Endian detection */
+
 #if (defined(SHA1DC_FORCE_LITTLEENDIAN) && defined(SHA1DC_BIGENDIAN))
 #undef SHA1DC_BIGENDIAN
 #endif
@@ -63,15 +114,8 @@
 #endif
 /*ENDIANNESS SELECTION*/
 
-#if (defined SHA1DC_FORCE_UNALIGNED_ACCESS || \
-     defined(__amd64__) || defined(__amd64) || defined(__x86_64__) || defined(__x86_64) || \
-     defined(i386) || defined(__i386) || defined(__i386__) || defined(__i486__)  || \
-     defined(__i586__) || defined(__i686__) || defined(_M_IX86) || defined(__X86__) || \
-     defined(_X86_) || defined(__THW_INTEL__) || defined(__I86__) || defined(__INTEL__) || \
-     defined(__386) || defined(_M_X64) || defined(_M_AMD64))
-
+#if defined(SHA1DC_FORCE_UNALIGNED_ACCESS) || defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
 #define SHA1DC_ALLOW_UNALIGNED_ACCESS
-
 #endif /*UNALIGNMENT DETECTION*/
 
 
@@ -918,7 +962,7 @@ static void sha1recompress_fast_ ## t (uint32_t ihvin[5], uint32_t ihvout[5], co
 
 #ifdef _MSC_VER
 #pragma warning(push)
-#pragma warning(disable: 4127)  /* Compiler complains about the checks in the above macro being constant. */
+#pragma warning(disable: 4127)  /* Complier complains about the checks in the above macro being constant. */
 #endif
 
 #ifdef DOSTORESTATE0
-- 
2.13.1.611.g7e3b11ae1

[PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Ævar Arnfjörð Bjarmason]

Add an option to use the sha1collisiondetection library from the
submodule in sha1collisiondetection/ instead of in the copy in the
sha1dc/ directory.

This allows us to try out the submodule in sha1collisiondetection
without breaking the build for anyone who's not expecting them as we
work out any kinks.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitmodules            |  4 ++++
 Makefile               | 12 ++++++++++++
 hash.h                 |  4 ++++
 sha1collisiondetection |  1 +
 4 files changed, 21 insertions(+)
 create mode 100644 .gitmodules
 create mode 160000 sha1collisiondetection

diff --git a/.gitmodules b/.gitmodules
new file mode 100644
index 0000000000..cbeebdab7a
--- /dev/null
+++ b/.gitmodules
@@ -0,0 +1,4 @@
+[submodule "sha1collisiondetection"]
+	path = sha1collisiondetection
+	url = https://github.com/cr-marcstevens/sha1collisiondetection.git
+	branch = master
diff --git a/Makefile b/Makefile
index b94cd5633c..f0cac1f246 100644
--- a/Makefile
+++ b/Makefile
@@ -162,6 +162,12 @@ all::
 # algorithm. This is slower, but may detect attempted collision attacks.
 # Takes priority over other *_SHA1 knobs.
 #
+# Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
+# sha1collisiondetection shipped as a submodule instead of the
+# non-submodule copy in sha1dc/. This is an experimental option used
+# by the git project to migrate to using sha1collisiondetection as a
+# submodule.
+#
 # Define OPENSSL_SHA1 environment variable when running make to link
 # with the SHA1 routine from openssl library.
 #
@@ -1448,8 +1454,14 @@ ifdef APPLE_COMMON_CRYPTO
 	BASIC_CFLAGS += -DSHA1_APPLE
 else
 	DC_SHA1 := YesPlease
+ifdef DC_SHA1_SUBMODULE
+	LIB_OBJS += sha1collisiondetection/lib/sha1.o
+	LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
+	BASIC_CFLAGS += -DDC_SHA1_SUBMODULE
+else
 	LIB_OBJS += sha1dc/sha1.o
 	LIB_OBJS += sha1dc/ubc_check.o
+endif
 	BASIC_CFLAGS += \
 		-DSHA1_DC \
 		-DSHA1DC_NO_STANDARD_INCLUDES \
diff --git a/hash.h b/hash.h
index a11fc9233f..bef3e630a0 100644
--- a/hash.h
+++ b/hash.h
@@ -8,7 +8,11 @@
 #elif defined(SHA1_OPENSSL)
 #include <openssl/sha.h>
 #elif defined(SHA1_DC)
+#ifdef DC_SHA1_SUBMODULE
+#include "sha1collisiondetection/lib/sha1.h"
+#else
 #include "sha1dc/sha1.h"
+#endif
 #else /* SHA1_BLK */
 #include "block-sha1/sha1.h"
 #endif
diff --git a/sha1collisiondetection b/sha1collisiondetection
new file mode 160000
index 0000000000..19d97bf5af
--- /dev/null
+++ b/sha1collisiondetection
@@ -0,0 +1 @@
+Subproject commit 19d97bf5af05312267c2e874ee6bcf584d9e9681
-- 
2.13.1.611.g7e3b11ae1

[PATCH v3 3/3] sha1collisiondetection: automatically enable when submodule is populated

[Ævar Arnfjörð Bjarmason]

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

If a user wants to experiment with the version of collision
detecting sha1 from the submodule, the user needed to not just
populate the submodule but also needed to turn the knob.

A Makefile trick is easy enough to do so, so let's do this.  When
somebody with a copy of the submodule populated wants not to use it,
that can be done by overriding it in config.mak or from the command
line.

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

diff --git a/Makefile b/Makefile
index f0cac1f246..3baa1eac0b 100644
--- a/Makefile
+++ b/Makefile
@@ -1009,6 +1009,10 @@ EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)
 
+ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
+DC_SHA1_SUBMODULE = auto
+endif
+
 include config.mak.uname
 -include config.mak.autogen
 -include config.mak
-- 
2.13.1.611.g7e3b11ae1

Re: [PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Junio C Hamano]

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

> Add an option to use the sha1collisiondetection library from the
> submodule in sha1collisiondetection/ instead of in the copy in the
> sha1dc/ directory.
>
> This allows us to try out the submodule in sha1collisiondetection
> without breaking the build for anyone who's not expecting them as we
> work out any kinks.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  .gitmodules            |  4 ++++
>  Makefile               | 12 ++++++++++++
>  hash.h                 |  4 ++++
>  sha1collisiondetection |  1 +
>  4 files changed, 21 insertions(+)
>  create mode 100644 .gitmodules
>  create mode 160000 sha1collisiondetection
>
> diff --git a/.gitmodules b/.gitmodules
> new file mode 100644
> index 0000000000..cbeebdab7a
> --- /dev/null
> +++ b/.gitmodules
> @@ -0,0 +1,4 @@
> +[submodule "sha1collisiondetection"]
> +	path = sha1collisiondetection
> +	url = https://github.com/cr-marcstevens/sha1collisiondetection.git
> +	branch = master

Do we need to say this "branch" bit?

Other than that looks good to me.

Thanks.

Re: [PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 03 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Add an option to use the sha1collisiondetection library from the
>> submodule in sha1collisiondetection/ instead of in the copy in the
>> sha1dc/ directory.
>>
>> This allows us to try out the submodule in sha1collisiondetection
>> without breaking the build for anyone who's not expecting them as we
>> work out any kinks.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  .gitmodules            |  4 ++++
>>  Makefile               | 12 ++++++++++++
>>  hash.h                 |  4 ++++
>>  sha1collisiondetection |  1 +
>>  4 files changed, 21 insertions(+)
>>  create mode 100644 .gitmodules
>>  create mode 160000 sha1collisiondetection
>>
>> diff --git a/.gitmodules b/.gitmodules
>> new file mode 100644
>> index 0000000000..cbeebdab7a
>> --- /dev/null
>> +++ b/.gitmodules
>> @@ -0,0 +1,4 @@
>> +[submodule "sha1collisiondetection"]
>> +	path = sha1collisiondetection
>> +	url = https://github.com/cr-marcstevens/sha1collisiondetection.git
>> +	branch = master
>
> Do we need to say this "branch" bit?

Yes, it's to make future updates easier, see b928922727 ("submodule add:
If --branch is given, record it in .gitmodules", 2012-12-19).

> Other than that looks good to me.
>
> Thanks.

Re: [PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Junio C Hamano]

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

>>> diff --git a/.gitmodules b/.gitmodules
>>> new file mode 100644
>>> index 0000000000..cbeebdab7a
>>> --- /dev/null
>>> +++ b/.gitmodules
>>> @@ -0,0 +1,4 @@
>>> +[submodule "sha1collisiondetection"]
>>> +	path = sha1collisiondetection
>>> +	url = https://github.com/cr-marcstevens/sha1collisiondetection.git
>>> +	branch = master
>>
>> Do we need to say this "branch" bit?
>
> Yes, it's to make future updates easier, see b928922727 ("submodule add:
> If --branch is given, record it in .gitmodules", 2012-12-19).

Why?  It's not like we want to _follow_ the 'master' branch of that
sha1collisiondetection repository.  We declare that a specific
commit from the (sub)module is suited for our project, and do not
really care to automatically update from whatever happens to be at
the tip of 'master' there.

>
>> Other than that looks good to me.
>>
>> Thanks.

Re: [PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 04 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>>> diff --git a/.gitmodules b/.gitmodules
>>>> new file mode 100644
>>>> index 0000000000..cbeebdab7a
>>>> --- /dev/null
>>>> +++ b/.gitmodules
>>>> @@ -0,0 +1,4 @@
>>>> +[submodule "sha1collisiondetection"]
>>>> +	path = sha1collisiondetection
>>>> +	url = https://github.com/cr-marcstevens/sha1collisiondetection.git
>>>> +	branch = master
>>>
>>> Do we need to say this "branch" bit?
>>
>> Yes, it's to make future updates easier, see b928922727 ("submodule add:
>> If --branch is given, record it in .gitmodules", 2012-12-19).
>
> Why?  It's not like we want to _follow_ the 'master' branch of that
> sha1collisiondetection repository.  We declare that a specific
> commit from the (sub)module is suited for our project, and do not
> really care to automatically update from whatever happens to be at
> the tip of 'master' there.

I'm honestly at a bit of a loss as to to what the confusion is here. So
to try to unravel that let's start from square one, explaining some
things you surely know already, but hopefully clearing this up.

> It's not like we want to _follow_ the 'master' branch of that
> sha1collisiondetection repository.

Git has no support at all for submodules that somehow follow an upstream
branch in the SVN sense of remotes, so no, that's not what putting
"branch" into the .gitmodules config means at all.

> We declare that a specific commit from the (sub)module is suited for
> our project, and do not really care to automatically update from
> whatever happens to be at the tip of 'master' there.

There is no automatic updating involved. The "master" branch here is
just metadata. If and when we bump the sha1collisiondetection submodule
that's going to be from the master branch, so by recording it we save
ourselves one step (in theory) by issuing some "pull updates from the
branch we always update from" command, rather than being at a loss as to
where we should go from the currently detached ref to N potential
upstream branches to update from.

Now, it seems git-submodule's tooling for doing this is still rather
crappy, but I think that's the idea, maybe I'm just holding it wrong.

Before git ever got this "branch" key in .gitmodules I'd added it to my
own aliases (and they're happily compatible with git-submodule). It
still (for me) works better than what git-submodule does:

    $ git config alias.sm-mainbranch
    !git config --file ../.gitmodules submodule.$NAME.branch || git describe --all --always | sed 's!^heads/!!'
    $ git config alias.sm-pull-all
    !git submodule foreach 'git checkout $(NAME=$name git sm-mainbranch) && git pull'

So in a repo with submodules I can simply run "git sm-pull-all" and
it'll update them all to the branch they're tracking, and at this point
I can "git add" them and review the updates.

I think some invocation of "git submodule update ???" will do the same,
but I can't from the docs see what that is right now.

In any case, if and when I/others figure that out the metadata will be
there, saving us one step in updating this in the future.

>>
>>> Other than that looks good to me.
>>>
>>> Thanks.

Re: [PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Stefan Beller]

On Tue, Jul 4, 2017 at 3:50 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:

>
> I think some invocation of "git submodule update ???" will do the same,
> but I can't from the docs see what that is right now.

'--remote' is what you are looking for.

When we have the branch field configured, the workflow for *creating*
the patch sent
to Junio might be different than it currently is. Currently, you would
send a patch that is
produced as:

  git -C sha1collisiondetection pull origin master
  git add sha1collisiondetection
  git commit -m "serious reasoning in the commit message"

This could be 'simplified' to

  git submodule update --remote
  git add sha1collisiondetection
  git commit -m "serious reasoning in the commit message"

but as we had different branches in the submodule field,
I wonder how much of an ease this is.

For Junio the workflow stays the same, as he would just
apply the patch, so I understand why he might see the
branch field as pollution.

Re: [PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Jul 4, 2017 at 3:50 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>
>>
>> I think some invocation of "git submodule update ???" will do the same,
>> but I can't from the docs see what that is right now.
>
> '--remote' is what you are looking for.
>
> When we have the branch field configured, the workflow for *creating*
> the patch sent
> to Junio might be different than it currently is. Currently, you would
> send a patch that is
> produced as:
>
>   git -C sha1collisiondetection pull origin master
>   git add sha1collisiondetection
>   git commit -m "serious reasoning in the commit message"
>
> This could be 'simplified' to
>
>   git submodule update --remote
>   git add sha1collisiondetection
>   git commit -m "serious reasoning in the commit message"
>
> but as we had different branches in the submodule field,
> I wonder how much of an ease this is.
>
> For Junio the workflow stays the same, as he would just
> apply the patch, so I understand why he might see the
> branch field as pollution.

My reaction was more about "the rest of us", not me or those who
choose to bind a new/different commit in the submodule to the
superproject.

I was recalling a wish by submodule users in a distant past that
lets "submodule update" to detach to the tip of the named branch in
the submodule, regardless of what commit the superproject points at
with its gitlink.  

When those merely following along with this project did "pull &&
submodule update", I do not want the submodule directory to check
out the commit that happens to be at the tip of their 'master'
branch.  If "submodule update" requires an explicit "--remote"
option to work in that way, then my worry is allayed, greatly.

Thanks.

Re: [PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Stefan Beller]

On Tue, Jul 4, 2017 at 6:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Tue, Jul 4, 2017 at 3:50 PM, Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>
>>>
>>> I think some invocation of "git submodule update ???" will do the same,
>>> but I can't from the docs see what that is right now.
>>
>> '--remote' is what you are looking for.
>>
>> When we have the branch field configured, the workflow for *creating*
>> the patch sent
>> to Junio might be different than it currently is. Currently, you would
>> send a patch that is
>> produced as:
>>
>>   git -C sha1collisiondetection pull origin master
>>   git add sha1collisiondetection
>>   git commit -m "serious reasoning in the commit message"
>>
>> This could be 'simplified' to
>>
>>   git submodule update --remote
>>   git add sha1collisiondetection
>>   git commit -m "serious reasoning in the commit message"
>>
>> but as we had different branches in the submodule field,
>> I wonder how much of an ease this is.
>>
>> For Junio the workflow stays the same, as he would just
>> apply the patch, so I understand why he might see the
>> branch field as pollution.
>
> My reaction was more about "the rest of us", not me or those who
> choose to bind a new/different commit in the submodule to the
> superproject.

Currently the rest of us would not care IMHO, as there is no
difference with the field recorded or not. (I just checked if it
would provide slight benefits for shallow clones, but not so)

> I was recalling a wish by submodule users in a distant past that
> lets "submodule update" to detach to the tip of the named branch in
> the submodule, regardless of what commit the superproject points at
> with its gitlink.

Yes, I heard that a couple times when poking around for opinions
and I think the issue has 2 facets:
* They actually want to be on a branch, such that the workflow
  is 'normal'. (Whatever that means, "detached HEAD" sounds
  scary, but workflow-wise it is not. It just requires an additional
  "checkout -b" once the work done seems worth preserving)
* None of the people I talked to wanted to get rid of exact-tracking,
  solely. But they always came in trade off with the first point
  outweighing the benefits of exact tracking.
  Although for this purpose the "update --remote" also doesn't
  quite fit as it does not re-attach the HEAD to a branch at
  the same commit as the remote tracking branch.

> When those merely following along with this project did "pull &&
> submodule update", I do not want the submodule directory to check
> out the commit that happens to be at the tip of their 'master'
> branch.  If "submodule update" requires an explicit "--remote"
> option to work in that way, then my worry is allayed, greatly.
>
> Thanks.

Re: [PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 05 2017, Stefan Beller jotted:

> On Tue, Jul 4, 2017 at 6:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Tue, Jul 4, 2017 at 3:50 PM, Ævar Arnfjörð Bjarmason
>>> <avarab@gmail.com> wrote:
>>>
>>>>
>>>> I think some invocation of "git submodule update ???" will do the same,
>>>> but I can't from the docs see what that is right now.
>>>
>>> '--remote' is what you are looking for.
>>>
>>> When we have the branch field configured, the workflow for *creating*
>>> the patch sent
>>> to Junio might be different than it currently is. Currently, you would
>>> send a patch that is
>>> produced as:
>>>
>>>   git -C sha1collisiondetection pull origin master
>>>   git add sha1collisiondetection
>>>   git commit -m "serious reasoning in the commit message"
>>>
>>> This could be 'simplified' to
>>>
>>>   git submodule update --remote
>>>   git add sha1collisiondetection
>>>   git commit -m "serious reasoning in the commit message"
>>>
>>> but as we had different branches in the submodule field,
>>> I wonder how much of an ease this is.
>>>
>>> For Junio the workflow stays the same, as he would just
>>> apply the patch, so I understand why he might see the
>>> branch field as pollution.
>>
>> My reaction was more about "the rest of us", not me or those who
>> choose to bind a new/different commit in the submodule to the
>> superproject.
>
> Currently the rest of us would not care IMHO, as there is no
> difference with the field recorded or not. (I just checked if it
> would provide slight benefits for shallow clones, but not so)
>
>> I was recalling a wish by submodule users in a distant past that
>> lets "submodule update" to detach to the tip of the named branch in
>> the submodule, regardless of what commit the superproject points at
>> with its gitlink.
>
> Yes, I heard that a couple times when poking around for opinions
> and I think the issue has 2 facets:
> * They actually want to be on a branch, such that the workflow
>   is 'normal'. (Whatever that means, "detached HEAD" sounds
>   scary, but workflow-wise it is not. It just requires an additional
>   "checkout -b" once the work done seems worth preserving)
> * None of the people I talked to wanted to get rid of exact-tracking,
>   solely. But they always came in trade off with the first point
>   outweighing the benefits of exact tracking.
>   Although for this purpose the "update --remote" also doesn't
>   quite fit as it does not re-attach the HEAD to a branch at
>   the same commit as the remote tracking branch.

For my submodule use, I'd like to have it checkout the branchname
recorded in the .gitmodules with tracking info set to upstream, and then
"git reset --hard" to the commit that's recorded in the commit info.

This would exactly mirror the state the repo was in when I initally ran
"git submodule add" on it, and make it easy to start making new commits
on my local branch, submit a PR of those to upstream, and have things
like "git fetch && git log @{u}.." work.

>> When those merely following along with this project did "pull &&
>> submodule update", I do not want the submodule directory to check
>> out the commit that happens to be at the tip of their 'master'
>> branch.  If "submodule update" requires an explicit "--remote"
>> option to work in that way, then my worry is allayed, greatly.
>>
>> Thanks.