On 2020-06-23 at 08:59:28, René Scharfe wrote:
> The following patch helps by avoiding a commit flag collision between
> http-push.c and commit-reach.c.  I don't know if it causes other
> collisions, though.

Nice catch.

I don't think it collides with other things because the other things
that overlap are either builtin code (which isn't used in git http-push)
or upload-pack.c, which would already be having problems before this
because it overlaps both before and after (and doesn't appear to be used
in http-push either).  Therefore, I feel confident in saying this
probably doesn't make anything worse.

> How could we possibly check that?  Perhaps by having a commit flag
> register (a global unsigned int) and having functions announce their
> bits in it.  Colliding announcements would BUG().

By my count, we have only 88 individual bits used.  If we moved all of
the builtin functions plus upload-pack (which shouldn't overlap) to a
single range, that would account for 53 bits, which would leave us 35
bits for all the rest of the code.  Since we need at most 27 bits for a
builtin command, if we used a 64-bit integer, we'd have space for all
the remaining bits not to overlap, plus bits for the type and flags
bits.

Since we'd be doing only bit operations on the flags variable, the
performance impact on 32-bit systems would be very minimal, although
we'd allocate an extra 4 bytes for struct object.  I don't know if
that's a problem.

Assuming we don't want to do that right now, may I have your sign-off
for the following code, René, so I can add it to a patch along with my
test?

> diff --git a/http-push.c b/http-push.c
> index 822f326599..99adbebdcf 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -70,10 +70,10 @@ enum XML_Status {
>  #define LOCK_REFRESH 30
> 
>  /* Remember to update object flag allocation in object.h */
> -#define LOCAL    (1u<<16)
> -#define REMOTE   (1u<<17)
> -#define FETCHING (1u<<18)
> -#define PUSHING  (1u<<19)
> +#define LOCAL    (1u<<11)
> +#define REMOTE   (1u<<12)
> +#define FETCHING (1u<<13)
> +#define PUSHING  (1u<<14)
> 
>  /* We allow "recursive" symbolic refs. Only within reason, though */
>  #define MAXDEPTH 5
> diff --git a/object.h b/object.h
> index b22328b838..a496d2e4e1 100644
> --- a/object.h
> +++ b/object.h
> @@ -67,7 +67,7 @@ struct object_array {
>   * builtin/blame.c:                        12-13
>   * bisect.c:                                        16
>   * bundle.c:                                        16
> - * http-push.c:                                     16-----19
> + * http-push.c:                          11-----14
>   * commit-graph.c:                                15
>   * commit-reach.c:                                  16-----19
>   * sha1-name.c:                                              20
> 

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204