Re: [PATCH 01/14] Extend index to save more flags
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Sep 20, 2008, 21:59 UTC
- Message-ID
- <m3skrulbrd.fsf@localhost.localdomain>
- In-Reply-To
- <1221904913-25887-2-git-send-email-pclouds@gmail.com>
Comments below are just nitpicking. Feel free to diregard them...
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 12 quoted lines
> 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@gmail.com>
It would be nice if at least this part of series got accepted...
Show 18 quoted lines
> ---
> 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...
Show 12 quoted lines
> +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.
Show 15 quoted lines
> + 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-diskSemicolon 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.
Show 14 quoted lines
> + *
> + * 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 0x80000000Perhaps CE_RESERVED then?
Show 12 quoted lines
> + > +#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))...
Show 11 quoted lines
> 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