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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2017, 03:35 UTC
Message-ID: <xmqqa7zhmxld.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171119184113.16630-1-bedhanger@gmx.de>

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

Show 9 quoted lines

> 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.

Show 23 quoted lines

> 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.