Comments below are just nitpicking. Feel free to diregard them...
Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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 <pclouds@xxxxxxxxx>
It would be nice if at least this part of series got accepted...
> ---
> 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.
> + 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.
> + *
> + * 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))...
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
