Re: [PATCH 5/5] log: decorate "replaced" on to replaced commits

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 18, 2011, 18:16 UTC
Message-ID: <7vippua433.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1313670578-2939-6-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Old code also decorates "new" commits with "refs/replace/SHA1". This
> is now gone, but I guess no one will miss it.

Makes sense but...

Show 8 quoted lines

> +	if (!prefixcmp(refname, "refs/replace/")) {
> +		unsigned char original_sha1[20];
> +		if (get_sha1_hex(refname + 13, original_sha1)) {
> +			warning("invalid replace ref %s", refname);
> +			return 0;
> +		}
> +		obj = parse_object(original_sha1);
> +		if (obj && (obj->type == OBJ_COMMIT || obj->type == OBJ_TAG))

... is it necessary to check and limit the types like this here?

If the argument is "we know only commits and tags are listed and blobs and trees are not shown with --decorate option" and "excluding the decoration that we know will never be used will avoid bloating the decorate hashtable with unused cruft", then add_name_decoration() should be doing the check for all of its callers, not just this one, no?

Show 7 quoted lines

> +			add_name_decoration(DECORATION_GRAFTED, "replaced", obj);
> +		return 0;
> +	}
> +
> +	obj = parse_object(sha1);
>  	if (!obj)
>  		return 0;