Re: [PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks

[Junio C Hamano <gitster@pobox.com>]

dturner@twopensource.com writes:

> diff --git a/sha1_name.c b/sha1_name.c
> index 6d10f05..23863f7 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1434,6 +1434,12 @@ static int get_sha1_with_context_1(const char *name,
>  			new_filename = resolve_relative_path(filename);
>  			if (new_filename)
>  				filename = new_filename;
> +			if (flags & GET_SHA1_FOLLOW_SYMLINKS) {
> +				ret = get_tree_enty_follow_symlinks(tree_sha1,
> +					filename, sha1, oc->path, &oc->mode);
> +				free(new_filename);
> +				return ret;
> +			}
>  			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
>  			if (ret && only_to_die) {
>  				diagnose_invalid_sha1_path(prefix, filename,

In general, I'd prefer to avoid this kind of "I only need this part
of the existing codeflow and I'll ignore rest by returning early".
You cannot anticipate any future needs of callers (e.g. only-to-die
flag might want to be passed when doing follow-symlinks in the
future) and clean-ups that may be added to the later part of the
function that you are skipping with this early return.

In other words,

	if (new_filename)
        	filename = new_filename;
-	ret = get_tree_entry(...);
+	if (flags & GET_SHA1_FOLLOW_SYMLINKS) {
+		ret = ... whatever new thing you want to do ...
+	} else {
+		ret = get_tree_entry(... just reindented ... );
+	}
	if (ret && ...)

is what we would prefer to see here.

> @@ -1469,5 +1475,7 @@ void maybe_die_on_misspelt_object_name(const char *name, const char *prefix)
>  
>  int get_sha1_with_context(const char *str, unsigned flags, unsigned char *sha1, struct object_context *orc)
>  {
> +	if (flags & GET_SHA1_FOLLOW_SYMLINKS && flags & GET_SHA1_ONLY_TO_DIE)
> +		die(_("internal error: bad flags for get_sha1_with_context"));
>  	return get_sha1_with_context_1(str, flags, NULL, sha1, orc);
>  }