Re: [PATCH] git-count-objects: Fix a disk-space under-estimate on Cygwin

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2009, 07:00 UTC
Message-ID: <7vd43d8yva.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4B059280.40902@ramsay1.demon.co.uk>

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---

Could you write a concise summary in the commit log message to explain why it is a correct fix? Your detailed analysis after three dash lines was an amusing read, but people who will be reading "git log" output 6 months down the road won't have an access to it. Something like...

    Cygwin has st_blocks in struct stat, but at least on NTFS, the field
    counts in blocks of st_blksize bytes, not in 512-byte blocks.

The Makefile already explains what NO_ST_BLOCKS_IN_STRUCT_STAT is about:

    # Define NO_ST_BLOCKS_IN_STRUCT_STAT if your platform does not have st_blocks
    # field that counts the on-disk footprint in 512-byte blocks.

so the above explanation should be enough.

Note that this is not any standard compliance. POSIX cops out like this in the <sys/stat.h> description:

    The unit for the st_blocks member of the stat structure is not defined
    within POSIX.1-2008. In some implementations it is 512 bytes. It may
    differ on a file system basis. There is no correlation between values
    of the st_blocks and st_blksize, and the f_bsize (from
    <sys/statvfs.h>) structure members.

IOW, a system like Cygwin that does not use 512-byte is not in violation.

Show 12 quoted lines

> diff --git a/Makefile b/Makefile
> index 5d5976f..5624563 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -783,6 +783,10 @@ ifeq ($(uname_O),Cygwin)
>  	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
>  	NO_TRUSTABLE_FILEMODE = UnfortunatelyYes
>  	OLD_ICONV = UnfortunatelyYes
> +	# The st_blocks field is not in units of 512 bytes, as the code
> +	# expects, which leads to an under-estimate of the disk space used.
> +	# In order to use an alternate algorithm, we claim to lack st_blocks.
> +	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease

This comment is redundant, as the explanation of the Makefile variable already said the same thing.

Also it is somewhat wrong to say "claim to lack". The Makefile variable merely means "do not use this field". The reason may be that the platform lacks it, or it may have it but it does not count in 512-byte blocks. It does not matter---either way the field is not usable.