Re: Remove diff machinery dependency from read-cache

11 messages, 6 authors, 2016-06-15 · open the first message on its own page

Re: Remove diff machinery dependency from read-cache

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:04

Linus Torvalds [off-list ref] writes:

On Thu, 21 Jan 2010, Linus Torvalds wrote:

quoted

We could fix it a few ways

 - ignore it. Most git programs will get the pack handling functions 
   anyway, since they want to get object reading.

In fact, we should probably remove git-show-index. It may have some 
historical significance as a pack-file index debugger, but it has no 
actual redeeming features now, considering that the binary is a megabyte 
of useless crud with debugging info.

However, we do actually use it in t/t5302-pack-index.sh. So in the 
meantime, how about this hacky patch to simply just avoid xmalloc, and 
separating out the trivial hex functions into "hex.o".

This results in

  [torvalds@nehalem git]$ size git-show-index 
       text    data     bss     dec     hex filename
     222818    2276  112688  337782   52776 git-show-index (before)
       5696     624    1264    7584    1da0 git-show-index (after)

which is a whole lot better, no?

(Or make it a built-in, if we actually think we want to carry it along in 
the long run)

We tend to not remove things unless we are absolutely certain nobody uses
it, so probably making it built-in would be preferrable.  I don't think
show-index is used very often if ever, but scripts that use hash-object
would use it really often and would do so via its --stdin interface if it
knows that it is creating more than a dozen objects, so start-up time
required to map the whole git is probably not an issue.

By the way, do you think anybody still uses "git merge-trees"?

Re: Remove diff machinery dependency from read-cache

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:48:04

Hi,

On Thu, 21 Jan 2010, Junio C Hamano wrote:

By the way, do you think anybody still uses "git merge-trees"?

IMO this is the only viable way to a non-broken merge-recursive.  Removing 
it would be counterproductive.

Ciao,
Dscho

Re: Remove diff machinery dependency from read-cache

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:48:04


On Thu, 21 Jan 2010, Junio C Hamano wrote:

We tend to not remove things unless we are absolutely certain nobody uses
it, so probably making it built-in would be preferrable.  I don't think
show-index is used very often if ever, but scripts that use hash-object
would use it really often and would do so via its --stdin interface if it
knows that it is creating more than a dozen objects, so start-up time
required to map the whole git is probably not an issue.

git-show-index should certainly be easy to turn into a built-in too. Patch 
appended.

By the way, do you think anybody still uses "git merge-trees"?

I dunno. I think it has some conceptual advantages, but realistically, I 
doubt anybody is willing to go through the pain to make it grow up enough 
to become a viable alternative to our current situation.

		Linus

---
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu Jan 21 18:16:49 2010 -0800
Subject: Turn 'show-index' into a builtin

.. rather than being a huge executable just because it sucks in all the
git library code and then does something really trivial.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 Makefile                             |    2 +-
 show-index.c => builtin-show-index.c |    2 +-
 builtin.h                            |    1 +
 git.c                                |    3 ++-
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index ad890ec..3439d2c 100644
--- a/Makefile
+++ b/Makefile

@@ -396,7 +396,6 @@ PROGRAMS += git-mktag$X
 PROGRAMS += git-pack-redundant$X
 PROGRAMS += git-patch-id$X
 PROGRAMS += git-shell$X
-PROGRAMS += git-show-index$X
 PROGRAMS += git-unpack-file$X
 PROGRAMS += git-upload-pack$X
 PROGRAMS += git-var$X

@@ -693,6 +692,7 @@ BUILTIN_OBJS += builtin-rm.o
 BUILTIN_OBJS += builtin-send-pack.o
 BUILTIN_OBJS += builtin-shortlog.o
 BUILTIN_OBJS += builtin-show-branch.o
+BUILTIN_OBJS += builtin-show-index.o
 BUILTIN_OBJS += builtin-show-ref.o
 BUILTIN_OBJS += builtin-stripspace.o
 BUILTIN_OBJS += builtin-symbolic-ref.o

diff --git a/show-index.c b/builtin-show-index.c
similarity index 96%
rename from show-index.c
rename to builtin-show-index.c
index 63f9da5..92202b8 100644
--- a/show-index.c
+++ b/builtin-show-index.c

@@ -4,7 +4,7 @@
 static const char show_index_usage[] =
 "git show-index < <packed archive index>";
 
-int main(int argc, char **argv)
+int cmd_show_index(int argc, const char **argv, const  char *prefix)
 {
 	int i;
 	unsigned nr;

diff --git a/builtin.h b/builtin.h
index c3f83c0..4d73d7c 100644
--- a/builtin.h
+++ b/builtin.h

@@ -93,6 +93,7 @@ extern int cmd_send_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_shortlog(int argc, const char **argv, const char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
+extern int cmd_show_index(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);

diff --git a/git.c b/git.c
index 194471f..5fabf18 100644
--- a/git.c
+++ b/git.c

@@ -358,8 +358,9 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "rm", cmd_rm, RUN_SETUP },
 		{ "send-pack", cmd_send_pack, RUN_SETUP },
 		{ "shortlog", cmd_shortlog, USE_PAGER },
-		{ "show-branch", cmd_show_branch, RUN_SETUP },
 		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
+		{ "show-branch", cmd_show_branch, RUN_SETUP },
+		{ "show-index", cmd_show_index },
 		{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 		{ "stripspace", cmd_stripspace },
 		{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },

Re: Remove diff machinery dependency from read-cache

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:48:04


On Thu, 21 Jan 2010, Linus Torvalds wrote:

quoted

By the way, do you think anybody still uses "git merge-trees"?

I dunno. I think it has some conceptual advantages, but realistically, I 
doubt anybody is willing to go through the pain to make it grow up enough 
to become a viable alternative to our current situation.

This makes it a built-in, at least, so it doesn't waste the diskspace.

		Linus

---
 Makefile                             |    2 +-
 merge-tree.c => builtin-merge-tree.c |    4 +---
 builtin.h                            |    1 +
 git.c                                |    1 +
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 3439d2c..35aea16 100644
--- a/Makefile
+++ b/Makefile

@@ -391,7 +391,6 @@ PROGRAMS += git-hash-object$X
 PROGRAMS += git-imap-send$X
 PROGRAMS += git-index-pack$X
 PROGRAMS += git-merge-index$X
-PROGRAMS += git-merge-tree$X
 PROGRAMS += git-mktag$X
 PROGRAMS += git-pack-redundant$X
 PROGRAMS += git-patch-id$X

@@ -670,6 +669,7 @@ BUILTIN_OBJS += builtin-merge-base.o
 BUILTIN_OBJS += builtin-merge-file.o
 BUILTIN_OBJS += builtin-merge-ours.o
 BUILTIN_OBJS += builtin-merge-recursive.o
+BUILTIN_OBJS += builtin-merge-tree.o
 BUILTIN_OBJS += builtin-mktree.o
 BUILTIN_OBJS += builtin-mv.o
 BUILTIN_OBJS += builtin-name-rev.o

diff --git a/merge-tree.c b/builtin-merge-tree.c
similarity index 99%
rename from merge-tree.c
rename to builtin-merge-tree.c
index 37b94d9..8e16c3e 100644
--- a/merge-tree.c
+++ b/builtin-merge-tree.c

@@ -337,7 +337,7 @@ static void *get_tree_descriptor(struct tree_desc *desc, const char *rev)
 	return buf;
 }
 
-int main(int argc, char **argv)
+int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
 	struct tree_desc t[3];
 	void *buf1, *buf2, *buf3;

@@ -347,8 +347,6 @@ int main(int argc, char **argv)
 
 	git_extract_argv0_path(argv[0]);
 
-	setup_git_directory();
-
 	buf1 = get_tree_descriptor(t+0, argv[1]);
 	buf2 = get_tree_descriptor(t+1, argv[2]);
 	buf3 = get_tree_descriptor(t+2, argv[3]);

diff --git a/builtin.h b/builtin.h
index 4d73d7c..06bf04e 100644
--- a/builtin.h
+++ b/builtin.h

@@ -70,6 +70,7 @@ extern int cmd_merge_base(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
+extern int cmd_merge_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_mktree(int argc, const char **argv, const char *prefix);
 extern int cmd_mv(int argc, const char **argv, const char *prefix);
 extern int cmd_name_rev(int argc, const char **argv, const char *prefix);

diff --git a/git.c b/git.c
index 5fabf18..e5964a8 100644
--- a/git.c
+++ b/git.c

@@ -335,6 +335,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
 		{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
 		{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
+		{ "merge-tree", cmd_merge_tree, RUN_SETUP },
 		{ "mktree", cmd_mktree, RUN_SETUP },
 		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
 		{ "name-rev", cmd_name_rev, RUN_SETUP },

Re: Remove diff machinery dependency from read-cache

From: Nicolas Pitre <nico@fluxnic.net>
Date: 2016-06-15 22:48:04

On Thu, 21 Jan 2010, Junio C Hamano wrote:

Linus Torvalds [off-list ref] writes:

quoted

On Thu, 21 Jan 2010, Linus Torvalds wrote:

quoted

We could fix it a few ways

 - ignore it. Most git programs will get the pack handling functions 
   anyway, since they want to get object reading.

In fact, we should probably remove git-show-index. It may have some 
historical significance as a pack-file index debugger, but it has no 
actual redeeming features now, considering that the binary is a megabyte 
of useless crud with debugging info.

However, we do actually use it in t/t5302-pack-index.sh. So in the 
meantime, how about this hacky patch to simply just avoid xmalloc, and 
separating out the trivial hex functions into "hex.o".

This results in

  [torvalds@nehalem git]$ size git-show-index 
       text    data     bss     dec     hex filename
     222818    2276  112688  337782   52776 git-show-index (before)
       5696     624    1264    7584    1da0 git-show-index (after)

which is a whole lot better, no?

(Or make it a built-in, if we actually think we want to carry it along in 
the long run)

We tend to not remove things unless we are absolutely certain nobody uses
it, so probably making it built-in would be preferrable.  I don't think
show-index is used very often if ever, but scripts that use hash-object
would use it really often and would do so via its --stdin interface if it
knows that it is creating more than a dozen objects, so start-up time
required to map the whole git is probably not an issue.

I do use it, but for developing/debugging pack stuff.
I don't suggest removing it, but I don't think making it a built-in has 
value either.

So I really think that Linus' patch (which is missing hex.c btw) is a 
good thing to do, even if only for the cleanup value.

Then, git-show-index could probably become test-show-index and no longer 
leave the build directory.


Nicolas

Re: Remove diff machinery dependency from read-cache

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:48:04


On Thu, 21 Jan 2010, Nicolas Pitre wrote:

So I really think that Linus' patch (which is missing hex.c btw) is a 
good thing to do, even if only for the cleanup value.

Gaah. hex.c was the obvious code movement with just a "cache.h" include at 
the top. However, I already threw away that whole tree in favor of the 
built-in version, and now I'm too lazy to re-create it.

		Linus

Re: Remove diff machinery dependency from read-cache

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:48:04


On Thu, 21 Jan 2010, Linus Torvalds wrote:

This makes it a built-in, at least, so it doesn't waste the diskspace.

.. and here's 'git hash-object' as a built-in.

		Linus

---
 Makefile                               |    2 +-
 hash-object.c => builtin-hash-object.c |    5 +----
 builtin.h                              |    1 +
 git.c                                  |    1 +
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 35aea16..f9e4aa3 100644
--- a/Makefile
+++ b/Makefile

@@ -387,7 +387,6 @@ EXTRA_PROGRAMS =
 # ... and all the rest that could be moved out of bindir to gitexecdir
 PROGRAMS += $(EXTRA_PROGRAMS)
 PROGRAMS += git-fast-import$X
-PROGRAMS += git-hash-object$X
 PROGRAMS += git-imap-send$X
 PROGRAMS += git-index-pack$X
 PROGRAMS += git-merge-index$X

@@ -656,6 +655,7 @@ BUILTIN_OBJS += builtin-for-each-ref.o
 BUILTIN_OBJS += builtin-fsck.o
 BUILTIN_OBJS += builtin-gc.o
 BUILTIN_OBJS += builtin-grep.o
+BUILTIN_OBJS += builtin-hash-object.o
 BUILTIN_OBJS += builtin-help.o
 BUILTIN_OBJS += builtin-init-db.o
 BUILTIN_OBJS += builtin-log.o

diff --git a/hash-object.c b/builtin-hash-object.c
similarity index 97%
rename from hash-object.c
rename to builtin-hash-object.c
index 9455dd0..6a5f5b5 100644
--- a/hash-object.c
+++ b/builtin-hash-object.c

@@ -73,17 +73,14 @@ static const struct option hash_object_options[] = {
 	OPT_END()
 };
 
-int main(int argc, const char **argv)
+int cmd_hash_object(int argc, const char **argv, const char *prefix)
 {
 	int i;
-	const char *prefix = NULL;
 	int prefix_length = -1;
 	const char *errstr = NULL;
 
 	type = blob_type;
 
-	git_extract_argv0_path(argv[0]);
-
 	argc = parse_options(argc, argv, NULL, hash_object_options,
 			     hash_object_usage, 0);

diff --git a/builtin.h b/builtin.h
index 06bf04e..3aa6b6c 100644
--- a/builtin.h
+++ b/builtin.h

@@ -55,6 +55,7 @@ extern int cmd_fsck(int argc, const char **argv, const char *prefix);
 extern int cmd_gc(int argc, const char **argv, const char *prefix);
 extern int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix);
 extern int cmd_grep(int argc, const char **argv, const char *prefix);
+extern int cmd_hash_object(int argc, const char **argv, const char *prefix);
 extern int cmd_help(int argc, const char **argv, const char *prefix);
 extern int cmd_http_fetch(int argc, const char **argv, const char *prefix);
 extern int cmd_init_db(int argc, const char **argv, const char *prefix);

diff --git a/git.c b/git.c
index e5964a8..a952663 100644
--- a/git.c
+++ b/git.c

@@ -318,6 +318,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "gc", cmd_gc, RUN_SETUP },
 		{ "get-tar-commit-id", cmd_get_tar_commit_id },
 		{ "grep", cmd_grep, USE_PAGER },
+		{ "hash-object", cmd_hash_object },
 		{ "help", cmd_help },
 		{ "init", cmd_init_db },
 		{ "init-db", cmd_init_db },

Re: Remove diff machinery dependency from read-cache

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:48:05


On Thu, 21 Jan 2010, Nicolas Pitre wrote:

So I really think that Linus' patch (which is missing hex.c btw) is a 
good thing to do, even if only for the cleanup value.

Btw, it looks like the separate hex.c would fix not just git-show-index 
(together with de-xmalloc), but also make git-patch-id shrink down. Except 
git-patch-id for some reason does git_extract_argv0_path(), which brings 
in exec_cmd.o, which brings in strbuf, and xmalloc, and now it's all the 
same old pain again.

So rather than try to solve it all (xmalloc in particular is pretty 
hairy), here's another patch.

		Linus
---
 Makefile                         |    2 +-
 patch-id.c => builtin-patch-id.c |    4 +---
 builtin.h                        |    1 +
 git.c                            |    1 +
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 398b5fb..5b614e4 100644
--- a/Makefile
+++ b/Makefile

@@ -392,7 +392,6 @@ PROGRAMS += git-index-pack$X
 PROGRAMS += git-merge-index$X
 PROGRAMS += git-mktag$X
 PROGRAMS += git-pack-redundant$X
-PROGRAMS += git-patch-id$X
 PROGRAMS += git-shell$X
 PROGRAMS += git-unpack-file$X
 PROGRAMS += git-upload-pack$X

@@ -674,6 +673,7 @@ BUILTIN_OBJS += builtin-mv.o
 BUILTIN_OBJS += builtin-name-rev.o
 BUILTIN_OBJS += builtin-pack-objects.o
 BUILTIN_OBJS += builtin-pack-refs.o
+BUILTIN_OBJS += builtin-patch-id.o
 BUILTIN_OBJS += builtin-prune-packed.o
 BUILTIN_OBJS += builtin-prune.o
 BUILTIN_OBJS += builtin-push.o

diff --git a/patch-id.c b/builtin-patch-id.c
similarity index 95%
rename from patch-id.c
rename to builtin-patch-id.c
index 0df4cb0..af0911e 100644
--- a/patch-id.c
+++ b/builtin-patch-id.c

@@ -75,13 +75,11 @@ static void generate_id_list(void)
 
 static const char patch_id_usage[] = "git patch-id < patch";
 
-int main(int argc, char **argv)
+int cmd_patch_id(int argc, const char **argv, const char *prefix)
 {
 	if (argc != 1)
 		usage(patch_id_usage);
 
-	git_extract_argv0_path(argv[0]);
-
 	generate_id_list();
 	return 0;
 }

diff --git a/builtin.h b/builtin.h
index 0c9c396..ab723f8 100644
--- a/builtin.h
+++ b/builtin.h

@@ -76,6 +76,7 @@ extern int cmd_mktree(int argc, const char **argv, const char *prefix);
 extern int cmd_mv(int argc, const char **argv, const char *prefix);
 extern int cmd_name_rev(int argc, const char **argv, const char *prefix);
 extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
+extern int cmd_patch_id(int argc, const char **argv, const char *prefix);
 extern int cmd_pickaxe(int argc, const char **argv, const char *prefix);
 extern int cmd_prune(int argc, const char **argv, const char *prefix);
 extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);

diff --git a/git.c b/git.c
index 09d3272..e38f201 100644
--- a/git.c
+++ b/git.c

@@ -341,6 +341,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
 		{ "name-rev", cmd_name_rev, RUN_SETUP },
 		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
+		{ "patch-id", cmd_patch_id },
 		{ "peek-remote", cmd_ls_remote },
 		{ "pickaxe", cmd_blame, RUN_SETUP },
 		{ "prune", cmd_prune, RUN_SETUP },

Re: Remove diff machinery dependency from read-cache

From: Johannes Sixt <hidden>
Date: 2016-06-15 22:48:05

Linus Torvalds schrieb:

quoted hunk

@@ -347,8 +347,6 @@ int main(int argc, char **argv)
 
 	git_extract_argv0_path(argv[0]);

This line must go away as well.

-	setup_git_directory();
-

-- Hannes

[PATCH] merge-tree: remove unnecessary call of git_extract_argv0_path

From: Johannes Sixt <hidden>
Date: 2016-06-15 22:48:05

This call should have been removed when the utility was made a builtin by
907a7cb.

Signed-off-by: Johannes Sixt <redacted>
---
 builtin-merge-tree.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/builtin-merge-tree.c b/builtin-merge-tree.c
index 8e16c3e..a4a4f2c 100644
--- a/builtin-merge-tree.c
+++ b/builtin-merge-tree.c

@@ -345,8 +345,6 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 	if (argc != 4)
 		usage(merge_tree_usage);
 
-	git_extract_argv0_path(argv[0]);
-
 	buf1 = get_tree_descriptor(t+0, argv[1]);
 	buf2 = get_tree_descriptor(t+1, argv[2]);
 	buf3 = get_tree_descriptor(t+2, argv[3]);

-- 
1.6.6.1.372.g084d

Re: [PATCH] merge-tree: remove unnecessary call of git_extract_argv0_path

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:48:05


On Fri, 22 Jan 2010, Johannes Sixt wrote:

This call should have been removed when the utility was made a builtin by
907a7cb.

Ack.

			Linus

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help