Re: [PATCH 2/5] Make mktag a builtin.

From: Junio C Hamano <gitster@pobox.com>
Date: May 12, 2008, 18:41 UTC
Message-ID: <7vej87z7p2.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <48285DAB.2040707@nrlssc.navy.mil>

Brandon Casey <casey@nrlssc.navy.mil> writes:

> I didn't think I should move the non-builtin mktag.c to builtin-mktag.c,
> and then after I modified mktag to be a builtin I knew I was moving it
> to builtin-tag.c so I didn't see a point to renaming it.

I see. I did not realize that the eventual shape would be to have both mktag and tag to be in builtin-tag.c, just like builtin-log.c supports many other commands from the log family, and that was where my question came from.

But I think the arrangement to have both in builtin-tag.c actually makes sense --- mktag needs to be kept supported but most of its internal should be shared with tag anyway.

Show 5 quoted lines

> Also, I decided about those things _before_ I realized how small the changes
> would be to mktag to make it a builtin.
>
> Do you think the modified patch you posted conflicts with the idea that
> "code move should be separate from code change"?

Yes, it does, but I do not subscribe to the "idea". Therefor I do not see any problem.

If you were to stop at making mktag a builtin, then the patch I sent would be the change that is necessary to do so. A code movement can and often does need some adjustment (e.g. if you move "a.c" to "src/a.c", its '#include "a.h"' may need to become '#include "../a.h"' (or preferably to '#include <a.h>' with appropriate -I.. option in the Makefile). It does not help anybody to insist on a blanket dogma that forbids modification and movement at the same time.

We do discourage rolling unrelated things in one commit, but creating a builtin "foo" typically involves creation of builtin-foo.c and associated changes to the Makefile and builtin.h, and in this case the initial contents of builtin-mktag.c happens to come from an existing file mktag.c, while making the original mktag.c obsolete and unnecessary along the way. That is a single logical change, and I do not think there is anything wrong to do it in one commit. In fact, splitting such a change into more than one commit is just plain silly and wrong, isn't it?