drafnel@gmail.com writes:
From: Brandon Casey <redacted>
Signed-off-by: Brandon Casey <redacted>
quoted hunk
@@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
BUILT_INS += git-get-tar-commit-id$X
BUILT_INS += git-init$X
BUILT_INS += git-merge-subtree$X
+BUILT_INS += git-mktag$X
BUILT_INS += git-peek-remote$X
BUILT_INS += git-repo-config$X
BUILT_INS += git-show$X
@@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
LIB_OBJS += mailmap.o
LIB_OBJS += match-trees.o
LIB_OBJS += merge-file.o
+LIB_OBJS += mktag.o
This is unusual for a builtin. Why didn't it migrate to builtin-mktag?
Junio C Hamano wrote:
drafnel@gmail.com writes:
quoted
From: Brandon Casey <redacted>
Signed-off-by: Brandon Casey <redacted>
quoted
@@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
BUILT_INS += git-get-tar-commit-id$X
BUILT_INS += git-init$X
BUILT_INS += git-merge-subtree$X
+BUILT_INS += git-mktag$X
BUILT_INS += git-peek-remote$X
BUILT_INS += git-repo-config$X
BUILT_INS += git-show$X
@@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
LIB_OBJS += mailmap.o
LIB_OBJS += match-trees.o
LIB_OBJS += merge-file.o
+LIB_OBJS += mktag.o
This is unusual for a builtin. Why didn't it migrate to builtin-mktag?
I didn't know how to do it.
I was trying not to do a code move and a code change at the same time.
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.
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"?
-brandon
Hi,
On Mon, 12 May 2008, Brandon Casey wrote:
Junio C Hamano wrote:
quoted
drafnel@gmail.com writes:
quoted
From: Brandon Casey <redacted>
Signed-off-by: Brandon Casey <redacted>
quoted
@@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
BUILT_INS += git-get-tar-commit-id$X
BUILT_INS += git-init$X
BUILT_INS += git-merge-subtree$X
+BUILT_INS += git-mktag$X
BUILT_INS += git-peek-remote$X
BUILT_INS += git-repo-config$X
BUILT_INS += git-show$X
@@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
LIB_OBJS += mailmap.o
LIB_OBJS += match-trees.o
LIB_OBJS += merge-file.o
+LIB_OBJS += mktag.o
This is unusual for a builtin. Why didn't it migrate to builtin-mktag?
I didn't know how to do it.
I was trying not to do a code move and a code change at the same time.
Why did you not consult Git's own history for guidance? See e.g.
$ git log next --diff-filter=A builtin-*.c
Ciao,
Dscho
Johannes Schindelin wrote:
Hi,
On Mon, 12 May 2008, Brandon Casey wrote:
quoted
Junio C Hamano wrote:
quoted
drafnel@gmail.com writes:
quoted
From: Brandon Casey <redacted>
Signed-off-by: Brandon Casey <redacted>
@@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
BUILT_INS += git-get-tar-commit-id$X
BUILT_INS += git-init$X
BUILT_INS += git-merge-subtree$X
+BUILT_INS += git-mktag$X
BUILT_INS += git-peek-remote$X
BUILT_INS += git-repo-config$X
BUILT_INS += git-show$X
@@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
LIB_OBJS += mailmap.o
LIB_OBJS += match-trees.o
LIB_OBJS += merge-file.o
+LIB_OBJS += mktag.o
This is unusual for a builtin. Why didn't it migrate to builtin-mktag?
I didn't know how to do it.
I was trying not to do a code move and a code change at the same time.
Why did you not consult Git's own history for guidance? See e.g.
I did, but not exhaustively. I found only examples of converting a
shell/perl script to a c version. That is straight-forward.
$ git log next --diff-filter=A builtin-*.c
And indeed the first two results here are examples of converting scripts
to builtin c versions.
The third result is applicable and converts merge-recursive from a
standalone c version to builtin.
If I had known about (or looked for) the --diff-filter option I would
have used the 'R' argument, since I am interested in a rename event,
but it produces no results.
$ git log next --diff-filter=R builtin-*.c
Adding -M does not help.
-brandon
Brandon Casey [off-list ref] 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.
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?