Re: [PATCH 01/14] Extend index to save more flags

[Jakub Narebski <jnareb@gmail.com>]

Comments below are just nitpicking. Feel free to diregard them...

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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@gmail.com>

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