From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 18 Aug 2011 18:16:55 GMT
Subject: Re: [PATCH 5/5] log: decorate "replaced" on to replaced commits
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...

> +	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?

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