Re: [PATCH 2/5] add get_sha1_with_mode

From: Junio C Hamano <junkio@cox.net>
Date: Apr 23, 2007, 02:33 UTC
Message-ID: <7vmz0zkeff.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11772602402479-git-send-email-mkoegler@auto.tuwien.ac.at>

Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:

Show 34 quoted lines

> get_sha1_with_mode basically behaves as get_sha1. It has an additional
> parameter for storing the mode of the object. This parameter may be NULL.
> If the mode can not be determinded, it stores S_IFINVALID.
>
> Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> ---
>  cache.h     |    1 +
>  sha1_name.c |   11 ++++++++++-
>  2 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index d425c26..a9ae3f8 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -320,6 +320,7 @@ static inline unsigned int hexval(unsigned int c)
>  #define DEFAULT_ABBREV 7
>  
>  extern int get_sha1(const char *str, unsigned char *sha1);
> +extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
>  extern int get_sha1_hex(const char *hex, unsigned char *sha1);
>  extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
>  extern int read_ref(const char *filename, unsigned char *sha1);
> diff --git a/sha1_name.c b/sha1_name.c
> index 267ea3f..1349c0a 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -643,11 +643,18 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
>   */
>  int get_sha1(const char *name, unsigned char *sha1)
>  {
> +	return get_sha1_with_mode(name, sha1, NULL);
> +}
> +
> +int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned* mode)

Style. Asterisk "*" is next to variable, not type in our code, like "unsigned *mode".

Show 29 quoted lines

> +{
>  	int ret, bracket_depth;
>  	unsigned unused;
>  	int namelen = strlen(name);
>  	const char *cp;
>  
> +	if (mode)
> +		*mode = S_IFINVALID;
>  	prepare_alt_odb();
>  	ret = get_sha1_1(name, namelen, sha1);
>  	if (!ret)
> @@ -685,6 +692,8 @@ int get_sha1(const char *name, unsigned char *sha1)
>  				break;
>  			if (ce_stage(ce) == stage) {
>  				hashcpy(sha1, ce->sha1);
> +				if (mode)
> +					*mode = ntohl(ce->ce_mode);
>  				return 0;
>  			}
>  			pos++;
> @@ -703,7 +712,7 @@ int get_sha1(const char *name, unsigned char *sha1)
>  		unsigned char tree_sha1[20];
>  		if (!get_sha1_1(name, cp-name, tree_sha1))
>  			return get_tree_entry(tree_sha1, cp+1, sha1,
> -					      &unused);
> +					      mode ? mode : &unused);
>  	}
>  	return ret;
>  }

Hmmmm. I'm not sure if it is worth to have "store only of mode pointer is not NULL" check in many places. Wouldn't it make more sense to require callers of _with_mode() variant to always send in a valid pointer (after all, it is the caller who chose to call the _with_mode() variant), and make get_sha1() like this?

	int get_sha1(const char *name, unsigned char *sha1)
	{
        	unsigned discard;
                return get_sha1_with_mode(name, sha1, &discard);
	}