Re: [PATCH 1/5] diff_filespec: reorder dirty_submodule macro definitions
Jeff King p...@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.
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 p...@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 */
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] diff_filespec: reorder dirty_submodule macro definitions
On Fri, Jan 17, 2014 at 10:46:59AM -0800, Junio C Hamano wrote: Jeff King p...@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 ;-) That makes sense. I had assumed it was a mis-merge initially, so was surprised to find 4682d85. It just looked like an error to me, but the rule you gave above does at least make sense. I'm happy with it either way. I almost just pulled the macro definitions, including DIFF_FILE_VALID, out of the struct definition completely. I see the value in having the flags near their bitfield, but it makes the definition a bit harder to read. I am of course fine with the end result this patch gives us. Me too, though if you feel like doing it the other way, I'm fine with that, too. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] diff_filespec: reorder dirty_submodule macro definitions
Jeff King p...@peff.net writes: I'm happy with it either way. I almost just pulled the macro definitions, including DIFF_FILE_VALID, out of the struct definition completely. I see the value in having the flags near their bitfield, but it makes the definition a bit harder to read. Yeah, my thoughts exactly when I did those two conflicting changes. I have a slight preference Constants go with the fields they are used in over fields and macros mixed together is harder to read, so let's use your patch as-is. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/5] diff_filespec: reorder dirty_submodule macro definitions
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. 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 p...@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 */
--
1.8.5.2.500.g8060133
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
