[PATCH] shrink git-shell by avoiding redundant dependencies

[PATCH] shrink git-shell by avoiding redundant dependencies

[Dmitry Potapov]

A lot of modules that have nothing to do with git-shell functionality
were linked in, bloating git-shell more than 8 times.

This patch cuts off redundant dependencies by:
1. providing stubs for three functions that make no sense for git-shell;
2. moving quote_path_fully from environment.c to quote.c to make the
   later self sufficient;
3. moving make_absolute_path into a new separate file.

The following numbers have been received with the default optimization
settings on master using GCC 4.1.2:

Before:
   text    data     bss     dec     hex filename
 143915    1348   93168  238431   3a35f git-shell

After:
   text    data     bss     dec     hex filename
  17670     788    8232   26690    6842 git-shell
---
 Makefile      |    1 +
 abspath.c     |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 environment.c |    1 -
 path.c        |   67 -----------------------------------------------
 quote.c       |    2 +
 shell.c       |    8 +++++
 6 files changed, 91 insertions(+), 68 deletions(-)
 create mode 100644 abspath.c

diff --git a/Makefile b/Makefile
index 3584b8c..bf77292 100644
--- a/Makefile
+++ b/Makefile
@@ -378,6 +378,7 @@ LIB_H += unpack-trees.h
 LIB_H += utf8.h
 LIB_H += wt-status.h
 
+LIB_OBJS += abspath.o
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
 LIB_OBJS += archive.o
diff --git a/abspath.c b/abspath.c
new file mode 100644
index 0000000..4becedf
--- /dev/null
+++ b/abspath.c
@@ -0,0 +1,80 @@
+/*
+ * I'm tired of doing "vsnprintf()" etc just to open a
+ * file, so here's a "return static buffer with printf"
+ * interface for paths.
+ *
+ * It's obviously not thread-safe. Sue me. But it's quite
+ * useful for doing things like
+ *
+ *   f = open(mkpath("%s/%s.git", base, name), O_RDONLY);
+ *
+ * which is what it's designed for.
+ */
+#include "cache.h"
+
+/* We allow "recursive" symbolic links. Only within reason, though. */
+#define MAXDEPTH 5
+
+const char *make_absolute_path(const char *path)
+{
+	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
+	char cwd[1024] = "";
+	int buf_index = 1, len;
+
+	int depth = MAXDEPTH;
+	char *last_elem = NULL;
+	struct stat st;
+
+	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
+		die ("Too long path: %.*s", 60, path);
+
+	while (depth--) {
+		if (stat(buf, &st) || !S_ISDIR(st.st_mode)) {
+			char *last_slash = strrchr(buf, '/');
+			if (last_slash) {
+				*last_slash = '\0';
+				last_elem = xstrdup(last_slash + 1);
+			} else {
+				last_elem = xstrdup(buf);
+				*buf = '\0';
+			}
+		}
+
+		if (*buf) {
+			if (!*cwd && !getcwd(cwd, sizeof(cwd)))
+				die ("Could not get current working directory");
+
+			if (chdir(buf))
+				die ("Could not switch to '%s'", buf);
+		}
+		if (!getcwd(buf, PATH_MAX))
+			die ("Could not get current working directory");
+
+		if (last_elem) {
+			int len = strlen(buf);
+			if (len + strlen(last_elem) + 2 > PATH_MAX)
+				die ("Too long path name: '%s/%s'",
+						buf, last_elem);
+			buf[len] = '/';
+			strcpy(buf + len + 1, last_elem);
+			free(last_elem);
+			last_elem = NULL;
+		}
+
+		if (!lstat(buf, &st) && S_ISLNK(st.st_mode)) {
+			len = readlink(buf, next_buf, PATH_MAX);
+			if (len < 0)
+				die ("Invalid symlink: %s", buf);
+			next_buf[len] = '\0';
+			buf = next_buf;
+			buf_index = 1 - buf_index;
+			next_buf = bufs[buf_index];
+		} else
+			break;
+	}
+
+	if (*cwd && chdir(cwd))
+		die ("Could not change back to '%s'", cwd);
+
+	return buf;
+}
diff --git a/environment.c b/environment.c
index 084ac8a..4a88a17 100644
--- a/environment.c
+++ b/environment.c
@@ -13,7 +13,6 @@ char git_default_email[MAX_GITNAME];
 char git_default_name[MAX_GITNAME];
 int user_ident_explicitly_given;
 int trust_executable_bit = 1;
-int quote_path_fully = 1;
 int has_symlinks = 1;
 int ignore_case;
 int assume_unchanged;
diff --git a/path.c b/path.c
index 6e3df18..496123c 100644
--- a/path.c
+++ b/path.c
@@ -327,9 +327,6 @@ const char *make_nonrelative_path(const char *path)
 	return buf;
 }
 
-/* We allow "recursive" symbolic links. Only within reason, though. */
-#define MAXDEPTH 5
-
 const char *make_relative_path(const char *abs, const char *base)
 {
 	static char buf[PATH_MAX + 1];
@@ -346,67 +343,3 @@ const char *make_relative_path(const char *abs, const char *base)
 	strcpy(buf, abs + baselen);
 	return buf;
 }
-
-const char *make_absolute_path(const char *path)
-{
-	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
-	char cwd[1024] = "";
-	int buf_index = 1, len;
-
-	int depth = MAXDEPTH;
-	char *last_elem = NULL;
-	struct stat st;
-
-	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
-		die ("Too long path: %.*s", 60, path);
-
-	while (depth--) {
-		if (stat(buf, &st) || !S_ISDIR(st.st_mode)) {
-			char *last_slash = strrchr(buf, '/');
-			if (last_slash) {
-				*last_slash = '\0';
-				last_elem = xstrdup(last_slash + 1);
-			} else {
-				last_elem = xstrdup(buf);
-				*buf = '\0';
-			}
-		}
-
-		if (*buf) {
-			if (!*cwd && !getcwd(cwd, sizeof(cwd)))
-				die ("Could not get current working directory");
-
-			if (chdir(buf))
-				die ("Could not switch to '%s'", buf);
-		}
-		if (!getcwd(buf, PATH_MAX))
-			die ("Could not get current working directory");
-
-		if (last_elem) {
-			int len = strlen(buf);
-			if (len + strlen(last_elem) + 2 > PATH_MAX)
-				die ("Too long path name: '%s/%s'",
-						buf, last_elem);
-			buf[len] = '/';
-			strcpy(buf + len + 1, last_elem);
-			free(last_elem);
-			last_elem = NULL;
-		}
-
-		if (!lstat(buf, &st) && S_ISLNK(st.st_mode)) {
-			len = readlink(buf, next_buf, PATH_MAX);
-			if (len < 0)
-				die ("Invalid symlink: %s", buf);
-			next_buf[len] = '\0';
-			buf = next_buf;
-			buf_index = 1 - buf_index;
-			next_buf = bufs[buf_index];
-		} else
-			break;
-	}
-
-	if (*cwd && chdir(cwd))
-		die ("Could not change back to '%s'", cwd);
-
-	return buf;
-}
diff --git a/quote.c b/quote.c
index d5cf9d8..6a52085 100644
--- a/quote.c
+++ b/quote.c
@@ -1,6 +1,8 @@
 #include "cache.h"
 #include "quote.h"
 
+int quote_path_fully = 1;
+
 /* Help to copy the thing properly quoted for the shell safety.
  * any single quote is replaced with '\'', any exclamation point
  * is replaced with '\!', and the whole thing is enclosed in a
diff --git a/shell.c b/shell.c
index b27d01c..91ca7de 100644
--- a/shell.c
+++ b/shell.c
@@ -3,6 +3,14 @@
 #include "exec_cmd.h"
 #include "strbuf.h"
 
+/* Stubs for functions that make no sense for git-shell. These stubs
+ * are provided here to avoid linking in external redundant modules.
+ */
+void release_pack_memory(size_t need, int fd){}
+void trace_argv_printf(const char **argv, const char *fmt, ...){}
+void trace_printf(const char *fmt, ...){}
+
+
 static int do_generic_cmd(const char *me, char *arg)
 {
 	const char *my_argv[4];
-- 
1.5.6.1

Re: [PATCH] shrink git-shell by avoiding redundant dependencies

[Junio C Hamano]

Dmitry Potapov <dpotapov@gmail.com> writes:

> diff --git a/abspath.c b/abspath.c
> new file mode 100644
> index 0000000..4becedf
> --- /dev/null
> +++ b/abspath.c
> @@ -0,0 +1,80 @@
> +/*
> + * I'm tired of doing "vsnprintf()" etc just to open a
> + * file, so here's a "return static buffer with printf"
> + * interface for paths.
> + *
> + * It's obviously not thread-safe. Sue me. But it's quite
> + * useful for doing things like
> + *
> + *   f = open(mkpath("%s/%s.git", base, name), O_RDONLY);
> + *
> + * which is what it's designed for.
> + */

This is not a comment you would want to move to the resulting file that
contains only make_absolute_path().

Re: [PATCH] shrink git-shell by avoiding redundant dependencies

[Dmitry Potapov]

On Fri, Jun 27, 2008 at 02:55:32PM -0700, Junio C Hamano wrote:
> Dmitry Potapov <dpotapov@gmail.com> writes:
> 
> > diff --git a/abspath.c b/abspath.c
> > new file mode 100644
> > index 0000000..4becedf
> > --- /dev/null
> > +++ b/abspath.c
> > @@ -0,0 +1,80 @@
> > +/*
> > + * I'm tired of doing "vsnprintf()" etc just to open a
> > + * file, so here's a "return static buffer with printf"
> > + * interface for paths.
> > + *
> > + * It's obviously not thread-safe. Sue me. But it's quite
> > + * useful for doing things like
> > + *
> > + *   f = open(mkpath("%s/%s.git", base, name), O_RDONLY);
> > + *
> > + * which is what it's designed for.
> > + */
> 
> This is not a comment you would want to move to the resulting file that
> contains only make_absolute_path().

I am sorry... I removed this comment. Here is the corrected patch.

-- 8< --
From: Dmitry Potapov <dpotapov@gmail.com>
Date: Sat, 28 Jun 2008 00:46:42 +0400
Subject: [PATCH] shrink git-shell by avoiding redundant dependencies

A lot of modules that have nothing to do with git-shell functionality
were linked in, bloating git-shell more than 8 times.

This patch cuts off redundant dependencies by:
1. providing stubs for three functions that make no sense for git-shell;
2. moving quote_path_fully from environment.c to quote.c to make the
   later self sufficient;
3. moving make_absolute_path into a new separate file.

The following numbers have been received with the default optimization
settings on master using GCC 4.1.2:

Before:
   text    data     bss     dec     hex filename
 143915    1348   93168  238431   3a35f git-shell

After:
   text    data     bss     dec     hex filename
  17670     788    8232   26690    6842 git-shell
---
 Makefile      |    1 +
 abspath.c     |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 environment.c |    1 -
 path.c        |   67 --------------------------------------------------------
 quote.c       |    2 +
 shell.c       |    8 ++++++
 6 files changed, 79 insertions(+), 68 deletions(-)
 create mode 100644 abspath.c

diff --git a/Makefile b/Makefile
index 3584b8c..bf77292 100644
--- a/Makefile
+++ b/Makefile
@@ -378,6 +378,7 @@ LIB_H += unpack-trees.h
 LIB_H += utf8.h
 LIB_H += wt-status.h
 
+LIB_OBJS += abspath.o
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
 LIB_OBJS += archive.o
diff --git a/abspath.c b/abspath.c
new file mode 100644
index 0000000..4f95a95
--- /dev/null
+++ b/abspath.c
@@ -0,0 +1,68 @@
+#include "cache.h"
+
+/* We allow "recursive" symbolic links. Only within reason, though. */
+#define MAXDEPTH 5
+
+const char *make_absolute_path(const char *path)
+{
+	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
+	char cwd[1024] = "";
+	int buf_index = 1, len;
+
+	int depth = MAXDEPTH;
+	char *last_elem = NULL;
+	struct stat st;
+
+	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
+		die ("Too long path: %.*s", 60, path);
+
+	while (depth--) {
+		if (stat(buf, &st) || !S_ISDIR(st.st_mode)) {
+			char *last_slash = strrchr(buf, '/');
+			if (last_slash) {
+				*last_slash = '\0';
+				last_elem = xstrdup(last_slash + 1);
+			} else {
+				last_elem = xstrdup(buf);
+				*buf = '\0';
+			}
+		}
+
+		if (*buf) {
+			if (!*cwd && !getcwd(cwd, sizeof(cwd)))
+				die ("Could not get current working directory");
+
+			if (chdir(buf))
+				die ("Could not switch to '%s'", buf);
+		}
+		if (!getcwd(buf, PATH_MAX))
+			die ("Could not get current working directory");
+
+		if (last_elem) {
+			int len = strlen(buf);
+			if (len + strlen(last_elem) + 2 > PATH_MAX)
+				die ("Too long path name: '%s/%s'",
+						buf, last_elem);
+			buf[len] = '/';
+			strcpy(buf + len + 1, last_elem);
+			free(last_elem);
+			last_elem = NULL;
+		}
+
+		if (!lstat(buf, &st) && S_ISLNK(st.st_mode)) {
+			len = readlink(buf, next_buf, PATH_MAX);
+			if (len < 0)
+				die ("Invalid symlink: %s", buf);
+			next_buf[len] = '\0';
+			buf = next_buf;
+			buf_index = 1 - buf_index;
+			next_buf = bufs[buf_index];
+		} else
+			break;
+	}
+
+	if (*cwd && chdir(cwd))
+		die ("Could not change back to '%s'", cwd);
+
+	return buf;
+}
diff --git a/environment.c b/environment.c
index 084ac8a..4a88a17 100644
--- a/environment.c
+++ b/environment.c
@@ -13,7 +13,6 @@ char git_default_email[MAX_GITNAME];
 char git_default_name[MAX_GITNAME];
 int user_ident_explicitly_given;
 int trust_executable_bit = 1;
-int quote_path_fully = 1;
 int has_symlinks = 1;
 int ignore_case;
 int assume_unchanged;
diff --git a/path.c b/path.c
index 6e3df18..496123c 100644
--- a/path.c
+++ b/path.c
@@ -327,9 +327,6 @@ const char *make_nonrelative_path(const char *path)
 	return buf;
 }
 
-/* We allow "recursive" symbolic links. Only within reason, though. */
-#define MAXDEPTH 5
-
 const char *make_relative_path(const char *abs, const char *base)
 {
 	static char buf[PATH_MAX + 1];
@@ -346,67 +343,3 @@ const char *make_relative_path(const char *abs, const char *base)
 	strcpy(buf, abs + baselen);
 	return buf;
 }
-
-const char *make_absolute_path(const char *path)
-{
-	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
-	char cwd[1024] = "";
-	int buf_index = 1, len;
-
-	int depth = MAXDEPTH;
-	char *last_elem = NULL;
-	struct stat st;
-
-	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
-		die ("Too long path: %.*s", 60, path);
-
-	while (depth--) {
-		if (stat(buf, &st) || !S_ISDIR(st.st_mode)) {
-			char *last_slash = strrchr(buf, '/');
-			if (last_slash) {
-				*last_slash = '\0';
-				last_elem = xstrdup(last_slash + 1);
-			} else {
-				last_elem = xstrdup(buf);
-				*buf = '\0';
-			}
-		}
-
-		if (*buf) {
-			if (!*cwd && !getcwd(cwd, sizeof(cwd)))
-				die ("Could not get current working directory");
-
-			if (chdir(buf))
-				die ("Could not switch to '%s'", buf);
-		}
-		if (!getcwd(buf, PATH_MAX))
-			die ("Could not get current working directory");
-
-		if (last_elem) {
-			int len = strlen(buf);
-			if (len + strlen(last_elem) + 2 > PATH_MAX)
-				die ("Too long path name: '%s/%s'",
-						buf, last_elem);
-			buf[len] = '/';
-			strcpy(buf + len + 1, last_elem);
-			free(last_elem);
-			last_elem = NULL;
-		}
-
-		if (!lstat(buf, &st) && S_ISLNK(st.st_mode)) {
-			len = readlink(buf, next_buf, PATH_MAX);
-			if (len < 0)
-				die ("Invalid symlink: %s", buf);
-			next_buf[len] = '\0';
-			buf = next_buf;
-			buf_index = 1 - buf_index;
-			next_buf = bufs[buf_index];
-		} else
-			break;
-	}
-
-	if (*cwd && chdir(cwd))
-		die ("Could not change back to '%s'", cwd);
-
-	return buf;
-}
diff --git a/quote.c b/quote.c
index d5cf9d8..6a52085 100644
--- a/quote.c
+++ b/quote.c
@@ -1,6 +1,8 @@
 #include "cache.h"
 #include "quote.h"
 
+int quote_path_fully = 1;
+
 /* Help to copy the thing properly quoted for the shell safety.
  * any single quote is replaced with '\'', any exclamation point
  * is replaced with '\!', and the whole thing is enclosed in a
diff --git a/shell.c b/shell.c
index b27d01c..91ca7de 100644
--- a/shell.c
+++ b/shell.c
@@ -3,6 +3,14 @@
 #include "exec_cmd.h"
 #include "strbuf.h"
 
+/* Stubs for functions that make no sense for git-shell. These stubs
+ * are provided here to avoid linking in external redundant modules.
+ */
+void release_pack_memory(size_t need, int fd){}
+void trace_argv_printf(const char **argv, const char *fmt, ...){}
+void trace_printf(const char *fmt, ...){}
+
+
 static int do_generic_cmd(const char *me, char *arg)
 {
 	const char *my_argv[4];
-- 
1.5.6.1

Re: [PATCH] shrink git-shell by avoiding redundant dependencies

[Junio C Hamano]

Dmitry Potapov <dpotapov@gmail.com> writes:

> On Fri, Jun 27, 2008 at 02:55:32PM -0700, Junio C Hamano wrote:
>> Dmitry Potapov <dpotapov@gmail.com> writes:
>> 
>> > diff --git a/abspath.c b/abspath.c
>> > new file mode 100644
>> > index 0000000..4becedf
>> > --- /dev/null
>> > +++ b/abspath.c
>> > @@ -0,0 +1,80 @@
>> > +/*
>> > + * I'm tired of doing "vsnprintf()" etc just to open a
>> > + * file, so here's a "return static buffer with printf"
>> > + * interface for paths.
>> > + *
>> > + * It's obviously not thread-safe. Sue me. But it's quite
>> > + * useful for doing things like
>> > + *
>> > + *   f = open(mkpath("%s/%s.git", base, name), O_RDONLY);
>> > + *
>> > + * which is what it's designed for.
>> > + */
>> 
>> This is not a comment you would want to move to the resulting file that
>> contains only make_absolute_path().
>
> I am sorry... I removed this comment. Here is the corrected patch.

No need to say nor be sorry.  The rest looks sane from a quick glance,
except that we may want to have a separate clean-up patch that
consolidates MAXDEPTH in many places that limits our manual symlink
resolution.

Re: [PATCH] shrink git-shell by avoiding redundant dependencies

[Stephan Beyer]

Hi,

Dmitry Potapov wrote:
> diff --git a/shell.c b/shell.c
> index b27d01c..91ca7de 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -3,6 +3,14 @@
>  #include "exec_cmd.h"
>  #include "strbuf.h"
>  
> +/* Stubs for functions that make no sense for git-shell. These stubs
> + * are provided here to avoid linking in external redundant modules.
> + */
> +void release_pack_memory(size_t need, int fd){}
> +void trace_argv_printf(const char **argv, const char *fmt, ...){}
> +void trace_printf(const char *fmt, ...){}
> +
> +

I don't really understand why this works.
You redefine libgit.a functions here

So the linker should complain like that:
	libgit.a(sha1_file.o): In function `release_pack_memory':
	/home/sbeyer/src/git/sha1_file.c:624: multiple definition of `release_pack_memory'
	shell.o:/home/sbeyer/src/git/shell.c:9: first defined here
	collect2: ld returned 1 exit status

And, in fact, it does when I move a function from a builtin to a lib
source file, for example launch_editor() from builtin-tag.c to strbuf.c,
like the following one:

---
 builtin-tag.c |   53 -----------------------------------------------------
 strbuf.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 53 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index c2cca6c..219f51d 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -23,59 +23,6 @@ static const char * const git_tag_usage[] = {
 
 static char signingkey[1000];
 
-void launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
-{
-	const char *editor, *terminal;
-
-	editor = getenv("GIT_EDITOR");
-	if (!editor && editor_program)
-		editor = editor_program;
-	if (!editor)
-		editor = getenv("VISUAL");
-	if (!editor)
-		editor = getenv("EDITOR");
-
-	terminal = getenv("TERM");
-	if (!editor && (!terminal || !strcmp(terminal, "dumb"))) {
-		fprintf(stderr,
-		"Terminal is dumb but no VISUAL nor EDITOR defined.\n"
-		"Please supply the message using either -m or -F option.\n");
-		exit(1);
-	}
-
-	if (!editor)
-		editor = "vi";
-
-	if (strcmp(editor, ":")) {
-		size_t len = strlen(editor);
-		int i = 0;
-		const char *args[6];
-		struct strbuf arg0;
-
-		strbuf_init(&arg0, 0);
-		if (strcspn(editor, "$ \t'") != len) {
-			/* there are specials */
-			strbuf_addf(&arg0, "%s \"$@\"", editor);
-			args[i++] = "sh";
-			args[i++] = "-c";
-			args[i++] = arg0.buf;
-		}
-		args[i++] = editor;
-		args[i++] = path;
-		args[i] = NULL;
-
-		if (run_command_v_opt_cd_env(args, 0, NULL, env))
-			die("There was a problem with the editor %s.", editor);
-		strbuf_release(&arg0);
-	}
-
-	if (!buffer)
-		return;
-	if (strbuf_read_file(buffer, path, 0) < 0)
-		die("could not read message file '%s': %s",
-		    path, strerror(errno));
-}
-
 struct tag_filter {
 	const char *pattern;
 	int lines;
diff --git a/strbuf.c b/strbuf.c
index 720737d..6419e02 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "run-command.h"
 
 int prefixcmp(const char *str, const char *prefix)
 {
@@ -308,3 +309,56 @@ int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
 
 	return len;
 }
+
+void launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	const char *editor, *terminal;
+
+	editor = getenv("GIT_EDITOR");
+	if (!editor && editor_program)
+		editor = editor_program;
+	if (!editor)
+		editor = getenv("VISUAL");
+	if (!editor)
+		editor = getenv("EDITOR");
+
+	terminal = getenv("TERM");
+	if (!editor && (!terminal || !strcmp(terminal, "dumb"))) {
+		fprintf(stderr,
+		"Terminal is dumb but no VISUAL nor EDITOR defined.\n"
+		"Please supply the message using either -m or -F option.\n");
+		exit(1);
+	}
+
+	if (!editor)
+		editor = "vi";
+
+	if (strcmp(editor, ":")) {
+		size_t len = strlen(editor);
+		int i = 0;
+		const char *args[6];
+		struct strbuf arg0;
+
+		strbuf_init(&arg0, 0);
+		if (strcspn(editor, "$ \t'") != len) {
+			/* there are specials */
+			strbuf_addf(&arg0, "%s \"$@\"", editor);
+			args[i++] = "sh";
+			args[i++] = "-c";
+			args[i++] = arg0.buf;
+		}
+		args[i++] = editor;
+		args[i++] = path;
+		args[i] = NULL;
+
+		if (run_command_v_opt_cd_env(args, 0, NULL, env))
+			die("There was a problem with the editor %s.", editor);
+		strbuf_release(&arg0);
+	}
+
+	if (!buffer)
+		return;
+	if (strbuf_read_file(buffer, path, 0) < 0)
+		die("could not read message file '%s': %s",
+		    path, strerror(errno));
+}
-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH] shrink git-shell by avoiding redundant dependencies

[Shawn O. Pearce]

Stephan Beyer <s-beyer@gmx.net> wrote:
> >  
> > +/* Stubs for functions that make no sense for git-shell. These stubs
> > + * are provided here to avoid linking in external redundant modules.
> > + */
> > +void release_pack_memory(size_t need, int fd){}
> > +void trace_argv_printf(const char **argv, const char *fmt, ...){}
> > +void trace_printf(const char *fmt, ...){}
> 
> I don't really understand why this works.
> You redefine libgit.a functions here

On Solaris you cannot compile git with the Solaris compiler
and linker, as the linker will not put up with the duplicate
definition of these functions.

I told my co-worker who is taking over "that git stuff" from
me at day-job to post a message to the list, or look at the
Solaris manual pages and figure out what he needs to do in
the Makefile to get it to work right.  Neither has happened
yet, and those day-job systems are the only Solaris boxen I
touch, so I won't be fixing it anytime soon myself.

I have to wonder why its important we avoid linking to
all of libgit.a here.  So what if git-shell is a little
bigger?  This is certainly not fully portable, and does
give warnings on some systems.

-- 
Shawn.

[PATCH] Link git-shell only to a subset of libgit.a

[Stephan Beyer]

Commit 5b8e6f85 introduced stubs for three functions that make no sense
for git-shell. But those stubs defined libgit.a functions a second time
so that a linker can complain.

Now git-shell is only linked to a subset of libgit.a.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Hi Shawn,

does this compile on Solaris?


For Dmitry: this is even smaller, but not significant ;-)

Before:
  text    data     bss     dec     hex filename
 24798    1304    8232   34334    861e git-shell

After:
   text    data     bss     dec     hex filename
  24504    1264    8248   34016    84e0 git-shell

*hide&run*
Stephan.

 Makefile |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 4bec4b3..a5626dc 100644
--- a/Makefile
+++ b/Makefile
@@ -1204,6 +1204,9 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
+git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^)
+
 $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)
 builtin-revert.o wt-status.o: wt-status.h
-- 
1.5.6.3.390.g7b30

Re: [PATCH] Link git-shell only to a subset of libgit.a

[Shawn O. Pearce]

Stephan Beyer <s-beyer@gmx.net> wrote:
> Commit 5b8e6f85 introduced stubs for three functions that make no sense
> for git-shell. But those stubs defined libgit.a functions a second time
> so that a linker can complain.
> 
> Now git-shell is only linked to a subset of libgit.a.
> 
> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> ---
> Hi Shawn,
> 
> does this compile on Solaris?

Can't, and won't test it.  Tomorrow is my last day at work and
I won't have time to test it on Solaris.  I don't have any other
Solaris boxes to test on, and don't care to either.

-- 
Shawn.

Re: [PATCH] Link git-shell only to a subset of libgit.a

[Dmitry Potapov]

On Fri, Jul 18, 2008 at 03:04:30AM +0200, Stephan Beyer wrote:
> Commit 5b8e6f85 introduced stubs for three functions that make no sense
> for git-shell. But those stubs defined libgit.a functions a second time
> so that a linker can complain.
> 
> Now git-shell is only linked to a subset of libgit.a.
> 
> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>

I agree, it is probably better to specify explicitly what files to link
for git-shell if we want to keep it small and avoid the problem with
some linkers.

Dmitry

Re: [PATCH] Link git-shell only to a subset of libgit.a

[Johannes Schindelin]

Hi,

On Fri, 18 Jul 2008, Dmitry Potapov wrote:

> On Fri, Jul 18, 2008 at 03:04:30AM +0200, Stephan Beyer wrote:
> > Commit 5b8e6f85 introduced stubs for three functions that make no 
> > sense for git-shell. But those stubs defined libgit.a functions a 
> > second time so that a linker can complain.
> > 
> > Now git-shell is only linked to a subset of libgit.a.
> > 
> > Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> 
> I agree, it is probably better to specify explicitly what files to link 
> for git-shell if we want to keep it small and avoid the problem with 
> some linkers.

It is also more elegant in general, as it does not play tricks to do what 
we want, but it does explicitely what we want.

FWIW I only removed almost the exact launch_editor() patch Stephan posted 
from my personal Git fork because of this trickery (which I did not have 
the time to fix).

Ciao,
Dscho

[PATCH] Remove function stubs in shell.c

[Stephan Beyer]

These function stubs may be useful, but because they redefine
functions, they provoke a linker error.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Or would you rather like this one? ;)

 shell.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/shell.c b/shell.c
index 91ca7de..b27d01c 100644
--- a/shell.c
+++ b/shell.c
@@ -3,14 +3,6 @@
 #include "exec_cmd.h"
 #include "strbuf.h"
 
-/* Stubs for functions that make no sense for git-shell. These stubs
- * are provided here to avoid linking in external redundant modules.
- */
-void release_pack_memory(size_t need, int fd){}
-void trace_argv_printf(const char **argv, const char *fmt, ...){}
-void trace_printf(const char *fmt, ...){}
-
-
 static int do_generic_cmd(const char *me, char *arg)
 {
 	const char *my_argv[4];
-- 
1.5.6.3.390.g7b30

Re: [PATCH] Remove function stubs in shell.c

[Dmitry Potapov]

On Fri, Jul 18, 2008 at 03:06:37AM +0200, Stephan Beyer wrote:
> These function stubs may be useful, but because they redefine
> functions, they provoke a linker error.
> 
> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> ---
> Or would you rather like this one? ;)

Unfortunately, this one will increase the size of git-shell
considerably:

   text    data     bss     dec     hex filename
  18182     824    8232   27238    6a66 git-shell
===
   text    data     bss     dec     hex filename
 146738    1376   93164  241278   3ae7e git-shell

So, personally, I don't like it.

Dmitry

Re: [PATCH] shrink git-shell by avoiding redundant dependencies

[Dmitry Potapov]

On Fri, Jul 18, 2008 at 02:26:20AM +0200, Stephan Beyer wrote:
> 
> Dmitry Potapov wrote:
> > diff --git a/shell.c b/shell.c
> > index b27d01c..91ca7de 100644
> > --- a/shell.c
> > +++ b/shell.c
> > @@ -3,6 +3,14 @@
> >  #include "exec_cmd.h"
> >  #include "strbuf.h"
> >  
> > +/* Stubs for functions that make no sense for git-shell. These stubs
> > + * are provided here to avoid linking in external redundant modules.
> > + */
> > +void release_pack_memory(size_t need, int fd){}
> > +void trace_argv_printf(const char **argv, const char *fmt, ...){}
> > +void trace_printf(const char *fmt, ...){}
> > +
> > +
> 
> I don't really understand why this works.
> You redefine libgit.a functions here
> 
> So the linker should complain like that:
> 	libgit.a(sha1_file.o): In function `release_pack_memory':
> 	/home/sbeyer/src/git/sha1_file.c:624: multiple definition of `release_pack_memory'
> 	shell.o:/home/sbeyer/src/git/shell.c:9: first defined here
> 	collect2: ld returned 1 exit status
> 
> And, in fact, it does when I move a function from a builtin to a lib
> source file, for example launch_editor() from builtin-tag.c to strbuf.c,
> like the following one:

It works because these functions are defined in object files, so the
linker should not search for them in libraries. However, if the linker
is forced to link sha1_file.c for some other reason, you will get the
conflict.

Dmitry

Re: [PATCH] shrink git-shell by avoiding redundant dependencies

[Johannes Schindelin]

Hi,

On Sat, 28 Jun 2008, Dmitry Potapov wrote:

> A lot of modules that have nothing to do with git-shell functionality
> were linked in, bloating git-shell more than 8 times.

/me wonders if "strip git-shell" would not take care of all that.

Just curious,
Dscho

Re: [PATCH] shrink git-shell by avoiding redundant dependencies

[Dmitry Potapov]

On Sat, Jun 28, 2008 at 6:51 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> /me wonders if "strip git-shell" would not take care of all that.

"strip" removes only debug information. It has no affect on text, data, and bss
sections. You can try to run "size git-shell" before and after
"strip", and you'll
see the same numbers.

BTW, it is possible to reduce the "text" and "data" size twice more
using the whole
program optimization, as it will discard some functions that are not
actually used,
but I don't think it is worth pursuing as it will complicate complication.

Dmitry

Re: [PATCH] shrink git-shell by avoiding redundant dependencies

[Johannes Schindelin]

Hi,

Thanks for the explanation.  Somehow I thought "strip" would do what an 
efficient linker should have done before, but I was wrong.

On Sat, 28 Jun 2008, Dmitry Potapov wrote:

> BTW, it is possible to reduce the "text" and "data" size twice more 
> using the whole program optimization, as it will discard some functions 
> that are not actually used, but I don't think it is worth pursuing as it 
> will complicate complication.

Heh.  Indeed, the complication will be complicated ;-)

Ciao,
Dscho