Re: [PATCH v3] Replace memcpy with hashcpy when dealing hash copy globally

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2014, 21:48 UTC
Message-ID: <xmqqfvmwfhaq.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1393839599-6955-1-git-send-email-sunheehnus@gmail.com>

Sun He <sunheehnus@gmail.com> writes:

> Replacing memcpy with hashcpy is more directly and elegant.

Can we justify the change without being subjective?

> Leave ppc/sha1.c alone, as it is an isolated component.
> Pull cache.h(actually ../cache.h) in just for one memcpy
> there is not proper.

That is not the reason why that memcpy of 20-byte must stay as it is. If for whatever reason we later choose to switch to using another hash function, say MD5, to come up with the object names, the majority of memcpy(..., 20) must change to copy 16 bytes, and it makes sense to contain that implementation-specific knowledge of twenty behind the hashcpy() abstraction. The 20-byte memcpy() call in ppc/sha1.c, however, is an implementation of *THE* SHA-1 algorithm, whose output is and will always be 20-byte. It will not change when we decide to replace what hash function is used to name our objects (which would result in updating the implementation of hashcpy()). That is the reason why you shouldn't touch that one. It has to be explicitly 20 byte, without ever getting affected by what length our hashcpy() may choose to copy.

Perhaps...

	We invented hashcpy() to keep the abstraction of "object
	name" behind it.  Use it instead of calling memcpy() with
	hard-coded 20-byte length when moving object names between
        pieces of memory.

	Leave ppc/sha1.c as-is, because the function *is* about
	*the* SHA-1 hash algorithm whose output is and will always
	be 20-byte.

or something.

> Find the potential places with memcpy by the bash command:
>  $ find . | xargs grep "memcpy.*\(.*20.*\)"

If you are planning to hack on git, learn how to use it first ;-)

    $ git grep 'memcpy.*, 20)'