Re: [PATCH v2 1/2] convert: fix normalization of foreign idents

From: Marcus Comstedt <marcus@mc.pp.se>
Date: Sep 12, 2010, 22:06 UTC
Message-ID: <yf9d3siiplc.fsf@chiyo.mc.pp.se>
In-Reply-To: <7vd3sid4bo.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Imagine there isn't any "$Id$" (or "$ident$" as it was originally known)
> expansion in git.  You can implement it easily using a smudge/clean pair,

Sorry, but here I have to go off a little at a tangent. Yes, you could implement the ident-expansion currently provided by git as a smudge/clean pair. However, you could not implement an ident which actually puts something more useful (such as the id of the commit where the file was last modified) into the id string by using smudge/clean. I know, because I tried to do just that. ;-) The reason: smudge/clean do not get the pathname, so they are not able to query any information about the file from the repository. I might submit a patch adressing this issue later.

Show 7 quoted lines

> and the smudge and clean should be conditionally applied in the codepath
> you touched using exactly the same logic as your patch uses, no?
>
> That is what I meant.  It smells wrong to make this "sometime we do,
> sometimes we don't" as a special case for "$Id$".  Specifically, the
> parameter name "normalize_foreign_ident" feels wrong; the concept that the
> parameter tries to convey covers much wider than just "foreign ident", no?

Ok, I think I follow where you are going. _If_ we say that clean (and smudge?) should be able to run in different "modes", with cleaning for a commit being such an mode, then this ought to be triggered by the same parameter, yes. The parameter name describes what the parameter does now, but not necessarily what it would do in a possible future where such new concepts as modal clean scripts have been introduced.

Generally, I'm kind of wondering if the parameters of convert_to_git wouldn't be better off just specifying a mode (like the, perhaps also slightly mis-named, write_object paremeter to index_mem) rather than trying to micro-manage specific features like they have before. Was that what you had in mind?

  // Marcus