[PATCH] add definitions for global variables to shell.c

[PATCH] add definitions for global variables to shell.c

[Robert Schiele]

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.  While commit 78568448 fixes this problem
it introduces a new issue on the affected systems: Some versions of the
Sun compiler generate references to global variables when they see
extern declarations for those, even when they are never used in the
code.

This patch does a similar workaround for this problem as commit 5b8e6f85
did for the functions.

Signed-off-by: Robert Schiele <rschiele@gmail.com>
---
 shell.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/shell.c b/shell.c
index 6a48de0..8902ea5 100644
--- a/shell.c
+++ b/shell.c
@@ -3,12 +3,17 @@
 #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.
+/* Stubs for functions and external variables 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, ...){}
+int trust_executable_bit;
+const unsigned char null_sha1[20];
+const signed char hexval_table[256];
+int has_symlinks;
 
 
 static int do_generic_cmd(const char *me, char *arg)
-- 
1.5.4.5

Re: [PATCH] add definitions for global variables to shell.c

[Junio C Hamano]

Robert Schiele <rschiele@gmail.com> writes:

> 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.  While commit 78568448 fixes this problem
> it introduces a new issue on the affected systems: Some versions of the
> Sun compiler generate references to global variables when they see
> extern declarations for those, even when they are never used in the
> code.

Haven't looked at the real declarations but if the decl are "extern" and
nobody refers to them, why should the resulting object file require them
to be defined anywhere?  If the decl are not and in (fortran-ish) "common"
section, on the other hand, you shouldn't have to define them yourself
like this either.

This sounds like a compiler bug to me.

Re: [PATCH] add definitions for global variables to shell.c

[Robert Schiele]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

On Mon, Aug 18, 2008 at 05:29:11PM -0700, Junio C Hamano wrote:
> Haven't looked at the real declarations but if the decl are "extern" and
> nobody refers to them, why should the resulting object file require them
> to be defined anywhere?  If the decl are not and in (fortran-ish) "common"
> section, on the other hand, you shouldn't have to define them yourself
> like this either.
> 
> This sounds like a compiler bug to me.

This was my first thought as well but after more inspection there are two
things to consider:

1. I was not really precise enough in my description since I didn't spot that
   when I looked into the issue first: Actually there are references to these
   variables in static inline functions in cache.h.  Thus there actually is a
   reference though one that will never be used since abspath.c (that includes
   cache.h) is not calling any of these functions.

2. Since these symbols turn out to be referenced though in dead code only I
   wouldn't call it a compiler bug.  Obviously a smart compiler would do dead
   code elimination here but the fact that this compiler is not doing so is
   bad but not really a bug.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]

Re: [PATCH] add definitions for global variables to shell.c

[Junio C Hamano]

Robert Schiele <rschiele@gmail.com> writes:

> On Mon, Aug 18, 2008 at 05:29:11PM -0700, Junio C Hamano wrote:
>> Haven't looked at the real declarations but if the decl are "extern" and
>> nobody refers to them, why should the resulting object file require them
>> to be defined anywhere?  If the decl are not and in (fortran-ish) "common"
>> section, on the other hand, you shouldn't have to define them yourself
>> like this either.
>> 
>> This sounds like a compiler bug to me.
>
> This was my first thought as well but after more inspection there are two
> things to consider:
>
> 1. I was not really precise enough in my description since I didn't spot that
>    when I looked into the issue first: Actually there are references to these
>    variables in static inline functions in cache.h.  Thus there actually is a
>    reference though one that will never be used since abspath.c (that includes
>    cache.h) is not calling any of these functions.
>
> 2. Since these symbols turn out to be referenced though in dead code only I
>    wouldn't call it a compiler bug.

Ok, as I said, I didn't look.  If they are indeed referenced, that is a
different story.

Even if that is the case, I do not like the prospect of having to maintain
a set of duplicated variable definitions.  If we really wanted to address
this issue, maybe we would want a separate source file that is linked to
both git-shell and to the rest of the system that has nothing but
definitions of these variables?  I thought environment.c was meant to be
something like that -- would linking environment.o pull in too many extra
references these days (again, I didn't try)?

Re: [PATCH] add definitions for global variables to shell.c

[Robert Schiele]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Tue, Aug 19, 2008 at 12:53:21AM -0700, Junio C Hamano wrote:
> Even if that is the case, I do not like the prospect of having to maintain
> a set of duplicated variable definitions.  If we really wanted to address
> this issue, maybe we would want a separate source file that is linked to
> both git-shell and to the rest of the system that has nothing but
> definitions of these variables?  I thought environment.c was meant to be

Ok, so how about globals.c and move just global variables that have at least
two references there?

> something like that -- would linking environment.o pull in too many extra
> references these days (again, I didn't try)?

This pulls in almost everything these days.  In our specific case even the
functions that were redefined in shell.c to start the whole mess.  Thus as
soon as we add environment.c we end up with duplicate symbols on linking
git-shell again.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]

Re: [PATCH] add definitions for global variables to shell.c

[Johannes Sixt]

Junio C Hamano schrieb:
> Even if that is the case, I do not like the prospect of having to maintain
> a set of duplicated variable definitions.  If we really wanted to address
> this issue, maybe we would want a separate source file that is linked to
> both git-shell and to the rest of the system that has nothing but
> definitions of these variables?  I thought environment.c was meant to be
> something like that -- would linking environment.o pull in too many extra
> references these days (again, I didn't try)?

Why not just revert 5b8e6f85f (shrink git-shell)? It was a nice try. If it
had not proved as a maintainance burden, it would have had merits. But who
these days cares whether git-shell takes 300K or 30K in the light of that
it goes out of the way anyway by execing some other process at the first
opportunity?

-- Hannes

Re: [PATCH] add definitions for global variables to shell.c

[Robert Schiele]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]

On Tue, Aug 19, 2008 at 10:49:53AM +0200, Johannes Sixt wrote:
> Why not just revert 5b8e6f85f (shrink git-shell)? It was a nice try. If it
> had not proved as a maintainance burden, it would have had merits. But who
> these days cares whether git-shell takes 300K or 30K in the light of that
> it goes out of the way anyway by execing some other process at the first
> opportunity?

Sounds reasonable to me.  Doing this in a clean way would require more
restructuring in the code.  So far this change started a chain of changes
where each change tried to solve one issue and caused a more severe one.

Though it would not only be 5b8e6f85 to be reverted but also 78568448 that
tried to fix up problems that 5b8e6f85 caused.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]

Re: [PATCH] add definitions for global variables to shell.c

[Junio C Hamano]

Robert Schiele <rschiele@gmail.com> writes:

> On Tue, Aug 19, 2008 at 10:49:53AM +0200, Johannes Sixt wrote:
>
>> Why not just revert 5b8e6f85f (shrink git-shell)? It was a nice try. If it
>> had not proved as a maintainance burden, it would have had merits. But who
>> these days cares whether git-shell takes 300K or 30K in the light of that
>> it goes out of the way anyway by execing some other process at the first
>> opportunity?
>
> Sounds reasonable to me.  Doing this in a clean way would require more
> restructuring in the code.  So far this change started a chain of changes
> where each change tried to solve one issue and caused a more severe one.
>
> Though it would not only be 5b8e6f85 to be reverted but also 78568448 that
> tried to fix up problems that 5b8e6f85 caused.

I have no issue with that.  Some people also mumbled about auditability,
which I did not find particularly convincing.

[PATCH 1/2] shell: do not play duplicated definition games to shrink the executable

[Junio C Hamano]

Playing with linker games to shrink git-shell did not go well with various
other platforms and compilers.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile |    9 +--------
 shell.c  |    8 --------
 2 files changed, 1 insertions(+), 16 deletions(-)

diff --git a/Makefile b/Makefile
index 53ab4b5..71339e1 100644
--- a/Makefile
+++ b/Makefile
@@ -333,7 +333,6 @@ endif
 export PERL_PATH
 
 LIB_FILE=libgit.a
-COMPAT_LIB = compat/lib.a
 XDIFF_LIB=xdiff/lib.a
 
 LIB_H += archive.h
@@ -1223,12 +1222,6 @@ 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)
 
-$(COMPAT_LIB): $(COMPAT_OBJS)
-	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(COMPAT_OBJS)
-
-git-shell$X: abspath.o ctype.o exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_LIB)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(COMPAT_LIB)
-
 $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)
 builtin-revert.o wt-status.o: wt-status.h
@@ -1441,7 +1434,7 @@ distclean: clean
 
 clean:
 	$(RM) *.o mozilla-sha1/*.o arm/*.o ppc/*.o compat/*.o xdiff/*.o \
-		$(LIB_FILE) $(XDIFF_LIB) $(COMPAT_LIB)
+		$(LIB_FILE) $(XDIFF_LIB)
 	$(RM) $(ALL_PROGRAMS) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h TAGS tags cscope*
diff --git a/shell.c b/shell.c
index 6a48de0..0f6a727 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.6.0.6.gc6670b

[PATCH 2/2] Build-in "git-shell"

[Junio C Hamano]

This trivially makes "git-shell" a built-in.  It makes the executable even
fatter, though.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile                   |    1 +
 shell.c => builtin-shell.c |    5 +++--
 builtin.h                  |    1 +
 git.c                      |    1 +
 4 files changed, 6 insertions(+), 2 deletions(-)
 rename shell.c => builtin-shell.c (94%)

diff --git a/Makefile b/Makefile
index 71339e1..1a52f71 100644
--- a/Makefile
+++ b/Makefile
@@ -546,6 +546,7 @@ BUILTIN_OBJS += builtin-rev-parse.o
 BUILTIN_OBJS += builtin-revert.o
 BUILTIN_OBJS += builtin-rm.o
 BUILTIN_OBJS += builtin-send-pack.o
+BUILTIN_OBJS += builtin-shell.o
 BUILTIN_OBJS += builtin-shortlog.o
 BUILTIN_OBJS += builtin-show-branch.o
 BUILTIN_OBJS += builtin-show-ref.o
diff --git a/shell.c b/builtin-shell.c
similarity index 94%
rename from shell.c
rename to builtin-shell.c
index 0f6a727..3cf97d4 100644
--- a/shell.c
+++ b/builtin-shell.c
@@ -2,6 +2,7 @@
 #include "quote.h"
 #include "exec_cmd.h"
 #include "strbuf.h"
+#include "builtin.h"
 
 static int do_generic_cmd(const char *me, char *arg)
 {
@@ -44,7 +45,7 @@ static struct commands {
 	{ NULL },
 };
 
-int main(int argc, char **argv)
+int cmd_shell(int argc, const char **argv, const char *prefix)
 {
 	char *prog;
 	struct commands *cmd;
@@ -62,7 +63,7 @@ int main(int argc, char **argv)
 	else if (argc != 3 || strcmp(argv[1], "-c"))
 		die("What do you think I am? A shell?");
 
-	prog = argv[2];
+	prog = xstrdup(argv[2]);
 	if (!strncmp(prog, "git", 3) && isspace(prog[3]))
 		/* Accept "git foo" as if the caller said "git-foo". */
 		prog[3] = '-';
diff --git a/builtin.h b/builtin.h
index f3502d3..2b57a5e 100644
--- a/builtin.h
+++ b/builtin.h
@@ -88,6 +88,7 @@ extern int cmd_rev_parse(int argc, const char **argv, const char *prefix);
 extern int cmd_revert(int argc, const char **argv, const char *prefix);
 extern int cmd_rm(int argc, const char **argv, const char *prefix);
 extern int cmd_send_pack(int argc, const char **argv, const char *prefix);
+extern int cmd_shell(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);
diff --git a/git.c b/git.c
index 37b1d76..89e4645 100644
--- a/git.c
+++ b/git.c
@@ -338,6 +338,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
 		{ "rm", cmd_rm, RUN_SETUP },
 		{ "send-pack", cmd_send_pack, RUN_SETUP },
+		{ "shell", cmd_shell },
 		{ "shortlog", cmd_shortlog, USE_PAGER },
 		{ "show-branch", cmd_show_branch, RUN_SETUP },
 		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
-- 
1.6.0.6.gc6670b

Re: [PATCH] add definitions for global variables to shell.c

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Robert Schiele <rschiele@gmail.com> writes:
>
>> On Tue, Aug 19, 2008 at 10:49:53AM +0200, Johannes Sixt wrote:
>>
>>> Why not just revert 5b8e6f85f (shrink git-shell)? It was a nice try. If it
>>> had not proved as a maintainance burden, it would have had merits. But who
>>> these days cares whether git-shell takes 300K or 30K in the light of that
>>> it goes out of the way anyway by execing some other process at the first
>>> opportunity?
>>
>> Sounds reasonable to me.  Doing this in a clean way would require more
>> restructuring in the code.  So far this change started a chain of changes
>> where each change tried to solve one issue and caused a more severe one.
>>
>> Though it would not only be 5b8e6f85 to be reverted but also 78568448 that
>> tried to fix up problems that 5b8e6f85 caused.
>
> I have no issue with that.  Some people also mumbled about auditability,
> which I did not find particularly convincing.

The result would look like this two-patch series.

 [1/2] shell: do not play duplicated definition games to shrink the executable
 [2/2] Build-in "git-shell"

Re: [PATCH] add definitions for global variables to shell.c

[Robert Schiele]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

On Tue, Aug 19, 2008 at 06:06:24PM -0700, Junio C Hamano wrote:
> The result would look like this two-patch series.
> 
>  [1/2] shell: do not play duplicated definition games to shrink the executable
>  [2/2] Build-in "git-shell"

Thanks, this makes the Sun compilers happy again without the need of weired
hacks.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]

Re: [PATCH 2/2] Build-in "git-shell"

[Johannes Sixt]

Junio C Hamano schrieb:
> This trivially makes "git-shell" a built-in.  It makes the executable even
> fatter, though.
...
> diff --git a/Makefile b/Makefile
> index 71339e1..1a52f71 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -546,6 +546,7 @@ BUILTIN_OBJS += builtin-rev-parse.o
>  BUILTIN_OBJS += builtin-revert.o
>  BUILTIN_OBJS += builtin-rm.o
>  BUILTIN_OBJS += builtin-send-pack.o
> +BUILTIN_OBJS += builtin-shell.o
>  BUILTIN_OBJS += builtin-shortlog.o
>  BUILTIN_OBJS += builtin-show-branch.o
>  BUILTIN_OBJS += builtin-show-ref.o

You must squash this in:

diff --git a/Makefile b/Makefile
index 57d16cb..fae9b22 100644
--- a/Makefile
+++ b/Makefile
@@ -826,7 +826,6 @@ EXTLIBS += -lz
 ifndef NO_POSIX_ONLY_PROGRAMS
 	PROGRAMS += git-daemon$X
 	PROGRAMS += git-imap-send$X
-	PROGRAMS += git-shell$X
 endif
 ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl

We removed git-shell from the MinGW build only because of the compat
dependencies. We don't have problems building it as a built-in.

-- Hannes