threads / patch / 14193

patchshrink git-shell by avoiding redundant dependencies

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

## tl;dr

16 messages between Jun 27, 2008 and Jul 18, 2008. Diffs are folded; open one to read it.

replies: 15people: 5as markdown or json

Dmitry Potapov· Jun 27, 2008, 21:35 UTC · lore

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
Show changes to 6 files +91 −68

Makefile, abspath.c, environment.c, path.c, quote.c, shell.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
Junio C Hamano· Jun 27, 2008, 21:55 UTC · re: Dmitry Potapov · lore

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

Dmitry Potapov <dpotapov@gmail.com> writes:
Show 18 quoted lines
> 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().

Dmitry Potapov· Jun 27, 2008, 22:31 UTC · re: Junio C Hamano · lore

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

On Fri, Jun 27, 2008 at 02:55:32PM -0700, Junio C Hamano wrote:
Show 23 quoted lines
> 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
Show changes to 6 files +79 −68

Makefile, abspath.c, environment.c, path.c, quote.c, shell.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
Junio C Hamano· Jun 27, 2008, 22:34 UTC · re: Dmitry Potapov · lore

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

Dmitry Potapov <dpotapov@gmail.com> writes:
Show 26 quoted lines
> 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.

Stephan Beyer· Jul 18, 2008, 00:26 UTC · re: Dmitry Potapov · lore

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

Hi,
Dmitry Potapov wrote:
Show 16 quoted lines
> 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(-)
Show changes to 2 files +54 −53

builtin-tag.c, strbuf.c

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
Shawn O. Pearce· Jul 18, 2008, 00:58 UTC · re: Stephan Beyer · lore

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

Stephan Beyer <s-beyer@gmx.net> wrote:
Show 10 quoted lines
> >  
> > +/* 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.
Stephan Beyer· Jul 18, 2008, 01:04 UTC · re: Shawn O. Pearce · lore

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

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(-)
Show changes to Makefile +3 −0
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
Shawn O. Pearce· Jul 18, 2008, 01:06 UTC · re: Stephan Beyer · lore

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

Stephan Beyer <s-beyer@gmx.net> wrote:
Show 11 quoted lines
> 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.
Dmitry Potapov· Jul 18, 2008, 06:03 UTC · re: Stephan Beyer · lore

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

On Fri, Jul 18, 2008 at 03:04:30AM +0200, Stephan Beyer wrote:
Show 7 quoted lines
> 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
Johannes Schindelin· Jul 18, 2008, 10:55 UTC · re: Dmitry Potapov · lore

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

Hi,
On Fri, 18 Jul 2008, Dmitry Potapov wrote:
Show 12 quoted lines
> 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

Stephan Beyer· Jul 18, 2008, 01:06 UTC · re: Shawn O. Pearce · lore

[PATCH] Remove function stubs in shell.c

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(-)
Show changes to shell.c +0 −8
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
Dmitry Potapov· Jul 18, 2008, 06:06 UTC · re: Stephan Beyer · lore

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

On Fri, Jul 18, 2008 at 03:06:37AM +0200, Stephan Beyer wrote:
Show 6 quoted lines
> 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
Dmitry Potapov· Jul 18, 2008, 05:59 UTC · re: Stephan Beyer · lore

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

On Fri, Jul 18, 2008 at 02:26:20AM +0200, Stephan Beyer wrote:
Show 31 quoted lines
> 
> 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
Johannes Schindelin· Jun 28, 2008, 14:51 UTC · re: Dmitry Potapov · lore

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

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

Dmitry Potapov· Jun 28, 2008, 16:48 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Jun 28, 2008, 17:31 UTC · re: Dmitry Potapov · lore

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

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

← back to recent threads