Re: [PATCH v3 1/5] checkout: describe_detached_head: remove ellipsis after committish

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

Ann T Ropea <bedhanger@gmx.de> writes:

> We do not want an ellipsis displayed following an (abbreviated) SHA-1
> value.
>
> The days when this was necessary to indicate the truncation to
> lower-level Git commands and/or the user are bygone.
>
> However, to ease the transition, the ellipsis will still be printed if
> the user (actively!) sets the environment variable PRINT_SHA1_ELLIPSIS
> to "yes" (case does not matter).

Does "(actively!)" add any value here?  For that matter, it appears
to me that "(case does not matter)" does not, either.  I thought
you'd be also reacting to Print_SHA1_Ellipsis variable, too, but the
code does not seem to be doing so (for obvious reasons).  If the users
can get what they want by setting it to "yes", that is sufficient to
say, whether your implementation accepts "Yes" as a synonym or not,
especially for something like this that we would want to remove in
an year or two.

> The transient nature of this fallback suggests that we should not prefix
> the variable by "GIT_".

Hmph.  That nature does not suggest anything like it to me.  When I
find an unfamiliar environment variable in my ~/.login I defined or
added to an existing script a few years back and forgot what it was
for, with a GIT_ prefix I would have one extra clue to help me
recall that it was once needed but no longer supported one that I
can safely remove.

I do agree that moving to an environment variable is a more useful
escape hatch for existing scripts that could be broken with this
change.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 7d8bcc383351..e6d3a28fe26e 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -400,10 +400,17 @@ static void show_local_changes(struct object *head,
>  static void describe_detached_head(const char *msg, struct commit *commit)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> +	const char *env_printsha1ellipsis = getenv("PRINT_SHA1_ELLIPSIS");
> +
>  	if (!parse_commit(commit))
>  		pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);
> -	fprintf(stderr, "%s %s... %s\n", msg,
> -		find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV), sb.buf);
> +	if (env_printsha1ellipsis && !strcasecmp(env_printsha1ellipsis, "yes")) {
> +		fprintf(stderr, "%s %s... %s\n", msg,
> +			find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV), sb.buf);
> +	} else {
> +		fprintf(stderr, "%s %s %s\n", msg,
> +			find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV), sb.buf);
> +	}
>  	strbuf_release(&sb);
>  }

I would actually have expected a helper function like

int print_sha1_ellipsis(void)
{
        static int cached_result = -1; /* unknown */

	if (cached_result < 0) {
		const char *v = getenv("PRINT_SHA1_ELLIPSIS");
                cached_result = (v && !strcasecmp(v, "yes"));
	}
	return cached_result;
}

so that you can reuse that in here and in quite a different place
like in diff.c.