Re: [PATCH 1/5] diff_filespec: reorder dirty_submodule macro definitions

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2014, 18:46 UTC
Message-ID: <xmqqha92xx7g.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140117011945.GA7249@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> diff_filespec has a 2-bit "dirty_submodule" field and
> defines two flags as macros. Originally these were right
> next to each other, but a new field was accidentally added
> in between in commit 4682d85.

Interesting.

 - 4682d852 (diff-index.c: "git diff" has no need to read blob from
   the standard input, 2012-06-27) wants to use this rule: all the
   bitfield definitions first, and then whatever macro constants
   next.

 - 25e5e2bf (combine-diff: support format_callback, 2011-08-19),
   wants to use a different rule: a run of (one bitfield definition
   and zero-or-more macro constants to be used in that bitfield).

When they were merged together at d7afe648 (Merge branch 'jc/refactor-diff-stdin', 2012-07-13), these two conflicting philosophies crashed.

That is the commit to be blamed for this mess ;-)

I am of course fine with the end result this patch gives us.

Thanks.

Show 37 quoted lines

> This patch puts the field and
> its flags back together.
>
> Using an enum like:
>
>   enum {
> 	  DIRTY_SUBMODULE_UNTRACKED = 1,
> 	  DIRTY_SUBMODULE_MODIFIED = 2
>   } dirty_submodule;
>
> would be more obvious, but it bloats the structure. Limiting
> the enum size like:
>
>   } dirty_submodule : 2;
>
> might work, but it is not portable.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  diffcore.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/diffcore.h b/diffcore.h
> index 1c16c85..f822f9e 100644
> --- a/diffcore.h
> +++ b/diffcore.h
> @@ -43,9 +43,9 @@ struct diff_filespec {
>  	unsigned should_free : 1; /* data should be free()'ed */
>  	unsigned should_munmap : 1; /* data should be munmap()'ed */
>  	unsigned dirty_submodule : 2;  /* For submodules: its work tree is dirty */
> -	unsigned is_stdin : 1;
>  #define DIRTY_SUBMODULE_UNTRACKED 1
>  #define DIRTY_SUBMODULE_MODIFIED  2
> +	unsigned is_stdin : 1;
>  	unsigned has_more_entries : 1; /* only appear in combined diff */
>  	struct userdiff_driver *driver;
>  	/* data should be considered "binary"; -1 means "don't know yet" */