Re: [PATCH 01/14] Extend index to save more flags
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:45:23
Comments below are just nitpicking. Feel free to diregard them... Nguyễn Thái Ngọc Duy [off-list ref] writes:
The on-disk format of index only saves 16 bit flags, nearly all have been used. The last bit (CE_EXTENDED) is used to for future extension. This patch extends index entry format to save more flags in future. The new entry format will be used when CE_EXTENDED bit is 1. Because older implementation may not understand CE_EXTENDED bit and misread the new format, if there is any extended entry in index, index header version will turn 3, which makes it incompatible for older git. If there is none, header version will return to 2 again. Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
It would be nice if at least this part of series got accepted...
quoted hunk ↗ jump to hunk
--- cache.h | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---- read-cache.c | 51 +++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 95 insertions(+), 14 deletions(-)diff --git a/cache.h b/cache.h index f4b8ddf..77b6eb3 100644 --- a/cache.h +++ b/cache.h@@ -109,6 +109,26 @@ struct ondisk_cache_entry { char name[FLEX_ARRAY]; /* more */ }; +/* + * This struct is used when CE_EXTENDED bit is 1 + * The struct must match ondisk_cache_entry exactly from + * ctime till flags + */
Errr... "must match"? Wouldn't "does match" be better? This type is defined below, not is to be defined...
+struct ondisk_cache_entry_extended {
+ struct cache_time ctime;
+ struct cache_time mtime;
+ unsigned int dev;
+ unsigned int ino;
+ unsigned int mode;
+ unsigned int uid;
+ unsigned int gid;
+ unsigned int size;
+ unsigned char sha1[20];
+ unsigned short flags;
+ unsigned short flags2;flags and flags2? Why not flags1 and flags2, or flags[2], or flags and flags_ext/flags_extended? Just nitpicking.
quoted hunk ↗ jump to hunk
+ char name[FLEX_ARRAY]; /* more */ +}; + struct cache_entry { unsigned int ce_ctime; unsigned int ce_mtime;@@ -130,7 +150,15 @@ struct cache_entry { #define CE_VALID (0x8000) #define CE_STAGESHIFT 12 -/* In-memory only */ +/* + * Range 0xFFFF0000 in ce_flags is divided into + * two parts: in-memory flags and on-disk ones. + * Flags in CE_EXTENDED_FLAGS will get saved on-disk
Semicolon at the end of below text to separate, I think. Or at least comma.
+ * if you want to save a new flag, add it in + * CE_EXTENDED_FLAGS
Nice comment.
quoted hunk ↗ jump to hunk
+ * + * In-memory only flags + */ #define CE_UPDATE (0x10000) #define CE_REMOVE (0x20000) #define CE_UPTODATE (0x40000)@@ -140,6 +168,24 @@ struct cache_entry { #define CE_UNHASHED (0x200000) /* + * Extended on-disk flags + */ +/* CE_EXTENDED2 is for future extension */ +#define CE_EXTENDED2 0x80000000
Perhaps CE_RESERVED then?
+ +#define CE_EXTENDED_FLAGS (0) + +/* + * Safeguard to avoid saving wrong flags: + * - CE_EXTENDED2 won't get saved until its semantic is known + * - Bits in 0x0000FFFF have been saved in ce_flags already + * - Bits in 0x003F0000 are currently in-memory flags + */ +#if CE_EXTENDED_FLAGS & 0x80CFFFFF +#error "CE_EXTENDED_FLAGS out of range" +#endif
I don't quite understand the above fragment (especially with the fact that CE_EXTENDED_FLAGS is defined as (0))...
quoted hunk ↗ jump to hunk
diff --git a/read-cache.c b/read-cache.c index c5a8659..667c36b 100644 --- a/read-cache.c +++ b/read-cache.c@@ -1096,7 +1096,7 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size) if (hdr->hdr_signature != htonl(CACHE_SIGNATURE)) return error("bad signature"); - if (hdr->hdr_version != htonl(2)) + if (hdr->hdr_version != htonl(2) && hdr->hdr_version != htonl(3)) return error("bad index version");
By the way: what was index version 1? [...] -- Jakub Narebski Poland ShadeHawk on #git