git-credentials-store.exe crash

git-credentials-store.exe crash

[dmh]

Since I cannot submit a github issue, I will try here.



  - Which version of Git for Windows are you using? 32-bit or 64-bit? 
Include the
    output of `git version` as well.
git version 2.9.0.windows.1
32-bit

  - Which version of Windows are you running? 32-bit or 64-bit?
64-bit windows 7

  - What options did you set as part of the installation? Or did you 
choose the
    defaults?
no git credentials manager

  - Any other interesting things about your environment that might be 
related
    to the issue you're seeing?
Unknown

### Details

  - Which terminal/shell are you running Git from? e.g 
Bash/CMD/PowerShell/other

  cygwin bash

  - What commands did you run to trigger this issue? If you can provide a
    [Minimal, Complete, and Verifiable 
example](http://stackoverflow.com/help/mcve)
    this will help us understand the issue.

I kept getting complaints by git about credentials lock existing.
deleted it and then git-credentials-store crashed as follows.
```
Carson:part2: git push
fatal: unable to get credential storage lock: File exists
Everything up-to-date
Carson:part2: ls ~/.git*
/cygdrive/c/Users/dmh/.gitconfig* 
/cygdrive/c/Users/dmh/.gitignore*
/cygdrive/c/Users/dmh/.git-credentials*       /cygdrive/c/Users/dmh/.gitk*
/cygdrive/c/Users/dmh/.git-credentials.lock*
Carson:part2: rm ~/.git-credentials.lock
Carson:part2: git push
This application has requested the Runtime to terminate it in an unusual 
way.
Please contact the application's support team for more information.
A s s e r t i o n   f a i l e d !

  P r o g r a m :   C : \ t o o l s \ G i t \ m i n g w 3 2 \ l i b e x 
e c \ g i
  t - c o r e \ g i t - c r e d e n t i a l - s t o r e . e x e
  F i l e :   e x e c _ c m d . c ,   L i n e   2 3

  E x p r e s s i o n :   a r g v 0 _ p a t h
  Everything up-to-date
Carson:part2:

```
  - What did you expect to occur after running these commands?
completion with no errors

  - What actually happened instead?

  git-credentials-store failed.

  - If the problem was occurring with a specific repository, can you 
provide the
    URL to that repository to help us with testing?
	url = https://github.com/Unidata/thredds.git

Re: git-credentials-store.exe crash

[Jeff King]

On Thu, Jun 30, 2016 at 03:24:41PM -0600, dmh@ucar.edu wrote:

> Carson:part2: git push
> This application has requested the Runtime to terminate it in an unusual
> way.
> Please contact the application's support team for more information.
> A s s e r t i o n   f a i l e d !
> 
>  P r o g r a m :   C : \ t o o l s \ G i t \ m i n g w 3 2 \ l i b e x e c \
> g i
>  t - c o r e \ g i t - c r e d e n t i a l - s t o r e . e x e
>  F i l e :   e x e c _ c m d . c ,   L i n e   2 3
> 
>  E x p r e s s i o n :   a r g v 0 _ p a t h
>  Everything up-to-date

Interesting. It's failing on the assert(argv0_path) in system_path().

That's part of the RUNTIME_PREFIX code which is built only on Windows,
so this is a Windows-specific issue.

I can guess the reason that argv0_path is not set is that
credential-store has its own main() function and does not call
git_extract_argv0_path(). It never mattered before, but as of v2.8.0,
one of the library functions it calls wants to use system_path(), which
assumes that the argv0 stuff has been set up.

I'm preparing some patches to fix this case (and some other related
ones).

-Peff

[PATCH 0/5] consistent setup code for external commands

[Jeff King]

On Fri, Jul 01, 2016 at 12:07:15AM -0400, Jeff King wrote:

> Interesting. It's failing on the assert(argv0_path) in system_path().
> 
> That's part of the RUNTIME_PREFIX code which is built only on Windows,
> so this is a Windows-specific issue.
> 
> I can guess the reason that argv0_path is not set is that
> credential-store has its own main() function and does not call
> git_extract_argv0_path(). It never mattered before, but as of v2.8.0,
> one of the library functions it calls wants to use system_path(), which
> assumes that the argv0 stuff has been set up.
> 
> I'm preparing some patches to fix this case (and some other related
> ones).

Here they are:

  [1/5]: add an extra level of indirection to main()
  [2/5]: common-main: call git_extract_argv0_path()
  [3/5]: common-main: call sanitize_stdfds()
  [4/5]: common-main: call restore_sigpipe_to_default()
  [5/5]: common-main: call git_setup_gettext()

The first patch is refactoring so we can fix this problem once, rather
than in the dozens of programs that need it.

The second should fix the problem you saw. It's unfortunate that the
tests didn't detect it; there's some discussion of that in the commit
message.

Patches 3-5 are other places where we can use the refactoring to be more
consistent and remove boilerplate code.

I almost did a patch 6 moving trace_command_performance(), but I'm not
sure if people would care or not. Running "git foo" already covers that,
even for an external command, because the git wrapper waits until the
sub-process finishes. Setting it up in each sub-program would mean:

  - you get it for dashed externals you run directly, too

  - for external programs run as "git foo", you get _two_ times. One for
    the time the actual sub-program ran, and one with the overhead of
    the git wrapper process.

    I'm not sure if people actually care about that distinction, or
    whether the extra number would simply be annoying.

So I punted. It's easy to move it over later if anybody cares.

-Peff

[PATCH 1/5] add an extra level of indirection to main()

[Jeff King]

There are certain startup tasks that we expect every git
process to do. In some cases this is just to improve the
quality of the program (e.g., setting up gettext()). In
others it is a requirement for using certain functions in
libgit.a (e.g., system_path() expects that you have called
git_extract_argv0_path()).

Most commands are builtins and are covered by the git.c
version of main(). However, there are still a few external
commands that use their own main(). Each of these has to
remember to include the correct startup sequence, and we are
not always consistent.

Rather than just fix the inconsistencies, let's make this
harder to get wrong by providing a common main() that can
run this standard startup.

We basically have two options to do this:

 - the compat/mingw.h file already does something like this by
   adding a #define that replaces the definition of main with a
   wrapper that calls mingw_startup().

   The upside is that the code in each program doesn't need
   to be changed at all; it's rewritten on the fly by the
   preprocessor.

   The downside is that it may make debugging of the startup
   sequence a bit more confusing, as the preprocessor is
   quietly inserting new code.

 - the builtin functions are all of the form cmd_foo(),
   and git.c's main() calls them.

   This is much more explicit, which may make things more
   obvious to somebody reading the code. It's also more
   flexible (because of course we have to figure out _which_
   cmd_foo() to call).

   The downside is that each of the builtins must define
   cmd_foo(), instead of just main().

This patch chooses the latter option, preferring the more
explicit approach, even though it is more invasive. We
introduce a new file common-main.c, with the "real" main. It
expects to call cmd_main() from whatever other objects it is
linked against.

We link common-main.o against anything that links against
libgit.a, since we know that such programs will need to do
this setup. Note that common-main.o can't actually go inside
libgit.a, as the linker would not pick up its main()
function automatically (it has no callers).

The rest of the patch is just adjusting all of the various
external programs (mostly in t/helper) to use cmd_main().
I've provided a global declaration for cmd_main(), which
means that all of the programs also need to match its
signature. In particular, many functions need to switch to
"const char **" instead of "char **" for argv. This effect
ripples out to a few other variables and functions, as well.

This makes the patch even more invasive, but the end result
is much better. We should be treating argv strings as const
anyway, and now all programs conform to the same signature
(which also matches the way builtins are defined).

Signed-off-by: Jeff King <peff@peff.net>
---
I waffled between the two mechanisms. Opinions welcome.

 Makefile                               | 17 +++++++++++++----
 common-main.c                          | 12 ++++++++++++
 credential-cache--daemon.c             |  2 +-
 credential-cache.c                     |  2 +-
 credential-store.c                     |  2 +-
 daemon.c                               |  8 ++++----
 fast-import.c                          |  4 ++--
 git-compat-util.h                      |  2 ++
 git.c                                  |  3 +--
 http-backend.c                         |  2 +-
 http-fetch.c                           |  2 +-
 http-push.c                            |  6 +++---
 imap-send.c                            |  2 +-
 remote-curl.c                          |  2 +-
 remote-testsvn.c                       |  2 +-
 sh-i18n--envsubst.c                    |  2 +-
 shell.c                                |  2 +-
 show-index.c                           |  2 +-
 t/helper/test-chmtime.c                |  2 +-
 t/helper/test-config.c                 |  2 +-
 t/helper/test-ctype.c                  |  2 +-
 t/helper/test-date.c                   |  8 ++++----
 t/helper/test-delta.c                  |  2 +-
 t/helper/test-dump-cache-tree.c        |  2 +-
 t/helper/test-dump-split-index.c       |  2 +-
 t/helper/test-dump-untracked-cache.c   |  2 +-
 t/helper/test-fake-ssh.c               |  2 +-
 t/helper/test-genrandom.c              |  2 +-
 t/helper/test-hashmap.c                |  2 +-
 t/helper/test-index-version.c          |  2 +-
 t/helper/test-line-buffer.c            |  2 +-
 t/helper/test-match-trees.c            |  2 +-
 t/helper/test-mergesort.c              |  2 +-
 t/helper/test-mktemp.c                 |  2 +-
 t/helper/test-parse-options.c          |  2 +-
 t/helper/test-path-utils.c             |  4 ++--
 t/helper/test-prio-queue.c             |  2 +-
 t/helper/test-read-cache.c             |  2 +-
 t/helper/test-regex.c                  |  2 +-
 t/helper/test-revision-walking.c       |  2 +-
 t/helper/test-run-command.c            |  2 +-
 t/helper/test-scrap-cache-tree.c       |  2 +-
 t/helper/test-sha1-array.c             |  2 +-
 t/helper/test-sha1.c                   |  2 +-
 t/helper/test-sigchain.c               |  2 +-
 t/helper/test-string-list.c            |  2 +-
 t/helper/test-submodule-config.c       |  6 +++---
 t/helper/test-subprocess.c             |  2 +-
 t/helper/test-svn-fe.c                 |  4 ++--
 t/helper/test-urlmatch-normalization.c |  2 +-
 t/helper/test-wildmatch.c              |  2 +-
 upload-pack.c                          |  2 +-
 52 files changed, 89 insertions(+), 67 deletions(-)
 create mode 100644 common-main.c

diff --git a/Makefile b/Makefile
index de5a030..717dc34 100644
--- a/Makefile
+++ b/Makefile
@@ -939,7 +939,7 @@ BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/worktree.o
 BUILTIN_OBJS += builtin/write-tree.o
 
-GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
+GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB)
 EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)
@@ -1572,7 +1572,15 @@ TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
 DIFF_SQ = $(subst ','\'',$(DIFF))
 PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
 
-LIBS = $(GITLIBS) $(EXTLIBS)
+# We must filter out any object files from $(GITLIBS),
+# as it is typically used like:
+#
+#   foo: foo.o $(GITLIBS)
+#	$(CC) $(filter %.o,$^) $(LIBS)
+#
+# where we use it as a dependency. Since we also pull object files
+# from the dependency list, that would make each entry appear twice.
+LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
 	$(COMPAT_CFLAGS)
@@ -1708,8 +1716,8 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
 git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) git.o \
-		$(BUILTIN_OBJS) $(LIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
+		$(filter %.o,$^) $(LIBS)
 
 help.sp help.s help.o: common-cmds.h
 
@@ -1902,6 +1910,7 @@ TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS))
 OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
 	$(XDIFF_OBJS) \
 	$(VCSSVN_OBJS) \
+	common-main.o \
 	git.o
 ifndef NO_CURL
 	OBJECTS += http.o http-walker.o remote-curl.o
diff --git a/common-main.c b/common-main.c
new file mode 100644
index 0000000..2b96bbf
--- /dev/null
+++ b/common-main.c
@@ -0,0 +1,12 @@
+#include "git-compat-util.h"
+
+int main(int argc, char **av)
+{
+	/*
+	 * This const trickery is explained in
+	 * 84d32bf7678259c08406571cd6ce4b7a6724dcba
+	 */
+	const char **argv = (const char **)av;
+
+	return cmd_main(argc, argv);
+}
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 1f14d56..1e5f16a 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -257,7 +257,7 @@ static void init_socket_directory(const char *path)
 	free(path_copy);
 }
 
-int main(int argc, const char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	const char *socket_path;
 	int ignore_sighup = 0;
diff --git a/credential-cache.c b/credential-cache.c
index 86e21de..cc8a6ee 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -83,7 +83,7 @@ static void do_cache(const char *socket, const char *action, int timeout,
 	strbuf_release(&buf);
 }
 
-int main(int argc, const char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	char *socket_path = NULL;
 	int timeout = 900;
diff --git a/credential-store.c b/credential-store.c
index 5714167..55ca1b1 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -142,7 +142,7 @@ static void lookup_credential(const struct string_list *fns, struct credential *
 			return; /* Found credential */
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	const char * const usage[] = {
 		"git credential-store [<options>] <action>",
diff --git a/daemon.c b/daemon.c
index 46dddac..4326cd0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -32,7 +32,7 @@ static const char daemon_usage[] =
 "           [<directory>...]";
 
 /* List of acceptable pathname prefixes */
-static char **ok_paths;
+static const char **ok_paths;
 static int strict_paths;
 
 /* If this is set, git-daemon-export-ok is not required */
@@ -240,7 +240,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 	}
 
 	if ( ok_paths && *ok_paths ) {
-		char **pp;
+		const char **pp;
 		int pathlen = strlen(path);
 
 		/* The validation is done on the paths after enter_repo
@@ -1192,7 +1192,7 @@ static int serve(struct string_list *listen_addr, int listen_port,
 	return service_loop(&socklist);
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	int listen_port = 0;
 	struct string_list listen_addr = STRING_LIST_INIT_NODUP;
@@ -1207,7 +1207,7 @@ int main(int argc, char **argv)
 	git_extract_argv0_path(argv[0]);
 
 	for (i = 1; i < argc; i++) {
-		char *arg = argv[i];
+		const char *arg = argv[i];
 		const char *v;
 
 		if (skip_prefix(arg, "--listen=", &v)) {
diff --git a/fast-import.c b/fast-import.c
index 59630ce..7cb7434 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -302,7 +302,7 @@ static int failure;
 static FILE *pack_edges;
 static unsigned int show_stats = 1;
 static int global_argc;
-static char **global_argv;
+static const char **global_argv;
 
 /* Memory pools */
 static size_t mem_pool_alloc = 2*1024*1024 - sizeof(struct mem_pool);
@@ -3445,7 +3445,7 @@ static void parse_argv(void)
 		read_marks();
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	unsigned int i;
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 49d4029..1930444 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1045,3 +1045,5 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #endif
 
 #endif
+
+extern int cmd_main(int, const char **);
diff --git a/git.c b/git.c
index 968a8a4..e244404 100644
--- a/git.c
+++ b/git.c
@@ -630,9 +630,8 @@ static void restore_sigpipe_to_default(void)
 	signal(SIGPIPE, SIG_DFL);
 }
 
-int main(int argc, char **av)
+int cmd_main(int argc, const char **argv)
 {
-	const char **argv = (const char **) av;
 	const char *cmd;
 	int done_help = 0;
 
diff --git a/http-backend.c b/http-backend.c
index 2148814..411c835 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -632,7 +632,7 @@ static struct service_cmd {
 	{"POST", "/git-receive-pack$", service_rpc}
 };
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	char *method = getenv("REQUEST_METHOD");
 	char *dir;
diff --git a/http-fetch.c b/http-fetch.c
index ba3ea10..eb559eb 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -6,7 +6,7 @@
 static const char http_fetch_usage[] = "git http-fetch "
 "[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin] commit-id url";
 
-int main(int argc, const char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct walker *walker;
 	int commits_on_stdin = 0;
diff --git a/http-push.c b/http-push.c
index a092f02..df20e44 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1692,12 +1692,12 @@ static void run_request_queue(void)
 #endif
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct transfer_request *request;
 	struct transfer_request *next_request;
 	int nr_refspec = 0;
-	char **refspec = NULL;
+	const char **refspec = NULL;
 	struct remote_lock *ref_lock = NULL;
 	struct remote_lock *info_ref_lock = NULL;
 	struct rev_info revs;
@@ -1717,7 +1717,7 @@ int main(int argc, char **argv)
 
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
-		char *arg = *argv;
+		const char *arg = *argv;
 
 		if (*arg == '-') {
 			if (!strcmp(arg, "--all")) {
diff --git a/imap-send.c b/imap-send.c
index 938c691..890e1cb 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1494,7 +1494,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 }
 #endif
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct strbuf all_msgs = STRBUF_INIT;
 	int total;
diff --git a/remote-curl.c b/remote-curl.c
index 672b382..a068340 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -984,7 +984,7 @@ static void parse_push(struct strbuf *buf)
 	free(specs);
 }
 
-int main(int argc, const char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int nongit;
diff --git a/remote-testsvn.c b/remote-testsvn.c
index f05ff45..32631eb 100644
--- a/remote-testsvn.c
+++ b/remote-testsvn.c
@@ -284,7 +284,7 @@ static int do_command(struct strbuf *line)
 	return 0;
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct strbuf buf = STRBUF_INIT, url_sb = STRBUF_INIT,
 			private_ref_sb = STRBUF_INIT, marksfilename_sb = STRBUF_INIT,
diff --git a/sh-i18n--envsubst.c b/sh-i18n--envsubst.c
index 2842a22..e06b2c1 100644
--- a/sh-i18n--envsubst.c
+++ b/sh-i18n--envsubst.c
@@ -64,7 +64,7 @@ static void note_variables (const char *string);
 static void subst_from_stdin (void);
 
 int
-main (int argc, char *argv[])
+cmd_main (int argc, const char *argv[])
 {
   /* Default values for command line options.  */
   /* unsigned short int show_variables = 0; */
diff --git a/shell.c b/shell.c
index c5439a6..3dd7fdc 100644
--- a/shell.c
+++ b/shell.c
@@ -138,7 +138,7 @@ static struct commands {
 	{ NULL },
 };
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	char *prog;
 	const char **user_argv;
diff --git a/show-index.c b/show-index.c
index acf8d54..575f9c5 100644
--- a/show-index.c
+++ b/show-index.c
@@ -4,7 +4,7 @@
 static const char show_index_usage[] =
 "git show-index";
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	int i;
 	unsigned nr;
diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c
index dfe8a83..e760256 100644
--- a/t/helper/test-chmtime.c
+++ b/t/helper/test-chmtime.c
@@ -56,7 +56,7 @@ static int timespec_arg(const char *arg, long int *set_time, int *set_eq)
 	return 1;
 }
 
-int main(int argc, char *argv[])
+int cmd_main(int argc, const char **argv)
 {
 	static int verbose;
 
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 6a77552..d143cd7 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -33,7 +33,7 @@
  */
 
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	int i, val;
 	const char *v;
diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
index 707a821..bb72c47 100644
--- a/t/helper/test-ctype.c
+++ b/t/helper/test-ctype.c
@@ -28,7 +28,7 @@ static int is_in(const char *s, int ch)
 #define LOWER "abcdefghijklmnopqrstuvwxyz"
 #define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	TEST_CLASS(isdigit, DIGIT);
 	TEST_CLASS(isspace, " \n\r\t");
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index 63f3735..0f3cfb1 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -5,7 +5,7 @@ static const char *usage_msg = "\n"
 "  test-date parse [date]...\n"
 "  test-date approxidate [date]...\n";
 
-static void show_dates(char **argv, struct timeval *now)
+static void show_dates(const char **argv, struct timeval *now)
 {
 	struct strbuf buf = STRBUF_INIT;
 
@@ -17,7 +17,7 @@ static void show_dates(char **argv, struct timeval *now)
 	strbuf_release(&buf);
 }
 
-static void parse_dates(char **argv, struct timeval *now)
+static void parse_dates(const char **argv, struct timeval *now)
 {
 	struct strbuf result = STRBUF_INIT;
 
@@ -36,7 +36,7 @@ static void parse_dates(char **argv, struct timeval *now)
 	strbuf_release(&result);
 }
 
-static void parse_approxidate(char **argv, struct timeval *now)
+static void parse_approxidate(const char **argv, struct timeval *now)
 {
 	for (; *argv; argv++) {
 		time_t t;
@@ -45,7 +45,7 @@ static void parse_approxidate(char **argv, struct timeval *now)
 	}
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct timeval now;
 	const char *x;
diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 4595cd6..59937dc 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -15,7 +15,7 @@
 static const char usage_str[] =
 	"test-delta (-d|-p) <from_file> <data_file> <out_file>";
 
-int main(int argc, char *argv[])
+int cmd_main(int argc, const char **argv)
 {
 	int fd;
 	struct stat st;
diff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c
index bb53c0a..44f3290 100644
--- a/t/helper/test-dump-cache-tree.c
+++ b/t/helper/test-dump-cache-tree.c
@@ -54,7 +54,7 @@ static int dump_cache_tree(struct cache_tree *it,
 	return errs;
 }
 
-int main(int ac, char **av)
+int cmd_main(int ac, const char **av)
 {
 	struct index_state istate;
 	struct cache_tree *another = cache_tree();
diff --git a/t/helper/test-dump-split-index.c b/t/helper/test-dump-split-index.c
index 861d28c..d168924 100644
--- a/t/helper/test-dump-split-index.c
+++ b/t/helper/test-dump-split-index.c
@@ -7,7 +7,7 @@ static void show_bit(size_t pos, void *data)
 	printf(" %d", (int)pos);
 }
 
-int main(int ac, char **av)
+int cmd_main(int ac, const char **av)
 {
 	struct split_index *si;
 	int i;
diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index 0a1c285..50112cc 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -40,7 +40,7 @@ static void dump(struct untracked_cache_dir *ucd, struct strbuf *base)
 	strbuf_setlen(base, len);
 }
 
-int main(int ac, char **av)
+int cmd_main(int ac, const char **av)
 {
 	struct untracked_cache *uc;
 	struct strbuf base = STRBUF_INIT;
diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c
index 980de21..12beee9 100644
--- a/t/helper/test-fake-ssh.c
+++ b/t/helper/test-fake-ssh.c
@@ -2,7 +2,7 @@
 #include "run-command.h"
 #include "strbuf.h"
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	const char *trash_directory = getenv("TRASH_DIRECTORY");
 	struct strbuf buf = STRBUF_INIT;
diff --git a/t/helper/test-genrandom.c b/t/helper/test-genrandom.c
index 54824d0..8d11d22 100644
--- a/t/helper/test-genrandom.c
+++ b/t/helper/test-genrandom.c
@@ -6,7 +6,7 @@
 
 #include "git-compat-util.h"
 
-int main(int argc, char *argv[])
+int cmd_main(int argc, const char **argv)
 {
 	unsigned long count, next = 0;
 	unsigned char *c;
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index cc2891d..7aa9440 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -138,7 +138,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)
  *
  * perfhashmap method rounds -> test hashmap.[ch] performance
  */
-int main(int argc, char *argv[])
+int cmd_main(int argc, const char **argv)
 {
 	char line[1024];
 	struct hashmap map;
diff --git a/t/helper/test-index-version.c b/t/helper/test-index-version.c
index 05d4699..f569f6b 100644
--- a/t/helper/test-index-version.c
+++ b/t/helper/test-index-version.c
@@ -1,6 +1,6 @@
 #include "cache.h"
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct cache_header hdr;
 	int version;
diff --git a/t/helper/test-line-buffer.c b/t/helper/test-line-buffer.c
index 1e58f04..81575fe 100644
--- a/t/helper/test-line-buffer.c
+++ b/t/helper/test-line-buffer.c
@@ -50,7 +50,7 @@ static void handle_line(const char *line, struct line_buffer *stdin_buf)
 	handle_command(line, arg + 1, stdin_buf);
 }
 
-int main(int argc, char *argv[])
+int cmd_main(int argc, const char **argv)
 {
 	struct line_buffer stdin_buf = LINE_BUFFER_INIT;
 	struct line_buffer file_buf = LINE_BUFFER_INIT;
diff --git a/t/helper/test-match-trees.c b/t/helper/test-match-trees.c
index d446b8e..e939502 100644
--- a/t/helper/test-match-trees.c
+++ b/t/helper/test-match-trees.c
@@ -1,7 +1,7 @@
 #include "cache.h"
 #include "tree.h"
 
-int main(int ac, char **av)
+int cmd_main(int ac, const char **av)
 {
 	struct object_id hash1, hash2, shifted;
 	struct tree *one, *two;
diff --git a/t/helper/test-mergesort.c b/t/helper/test-mergesort.c
index ea3b959..335cf6b 100644
--- a/t/helper/test-mergesort.c
+++ b/t/helper/test-mergesort.c
@@ -22,7 +22,7 @@ static int compare_strings(const void *a, const void *b)
 	return strcmp(x->text, y->text);
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct line *line, *p = NULL, *lines = NULL;
 	struct strbuf sb = STRBUF_INIT;
diff --git a/t/helper/test-mktemp.c b/t/helper/test-mktemp.c
index c8c5421..89d9b2f 100644
--- a/t/helper/test-mktemp.c
+++ b/t/helper/test-mktemp.c
@@ -3,7 +3,7 @@
  */
 #include "git-compat-util.h"
 
-int main(int argc, char *argv[])
+int cmd_main(int argc, const char **argv)
 {
 	if (argc != 2)
 		usage("Expected 1 parameter defining the temporary file template");
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 8a1235d..d51d292 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -94,7 +94,7 @@ static void show(struct string_list *expect, int *status, const char *fmt, ...)
 	strbuf_release(&buf);
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	const char *prefix = "prefix/";
 	const char *usage[] = {
diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index ba805b3..1ebe0f7 100644
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -156,7 +156,7 @@ static struct test_data dirname_data[] = {
 	{ NULL,              NULL     }
 };
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
 		char *buf = xmallocz(strlen(argv[2]));
@@ -213,7 +213,7 @@ int main(int argc, char **argv)
 	}
 
 	if (argc >= 4 && !strcmp(argv[1], "prefix_path")) {
-		char *prefix = argv[2];
+		const char *prefix = argv[2];
 		int prefix_len = strlen(prefix);
 		int nongit_ok;
 		setup_git_directory_gently(&nongit_ok);
diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
index 7be72f0..ae58fff 100644
--- a/t/helper/test-prio-queue.c
+++ b/t/helper/test-prio-queue.c
@@ -16,7 +16,7 @@ static void show(int *v)
 	free(v);
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct prio_queue pq = { intcmp };
 
diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index b25bcf1..2a7990e 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -1,6 +1,6 @@
 #include "cache.h"
 
-int main (int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	int i, cnt = 1;
 	if (argc == 2)
diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
index 0dc598e..37b7f06 100644
--- a/t/helper/test-regex.c
+++ b/t/helper/test-regex.c
@@ -1,6 +1,6 @@
 #include "git-compat-util.h"
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	char *pat = "[^={} \t]+";
 	char *str = "={}\nfred";
diff --git a/t/helper/test-revision-walking.c b/t/helper/test-revision-walking.c
index 3d03133..b8e6fe1 100644
--- a/t/helper/test-revision-walking.c
+++ b/t/helper/test-revision-walking.c
@@ -45,7 +45,7 @@ static int run_revision_walk(void)
 	return got_revision;
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	if (argc < 2)
 		return 1;
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 30a64a9..c71ea4f 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -49,7 +49,7 @@ static int task_finished(int result,
 	return 1;
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
 	int jobs;
diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index 6efee31..5b2fd09 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -5,7 +5,7 @@
 
 static struct lock_file index_lock;
 
-int main(int ac, char **av)
+int cmd_main(int ac, const char **av)
 {
 	hold_locked_index(&index_lock, 1);
 	if (read_cache() < 0)
diff --git a/t/helper/test-sha1-array.c b/t/helper/test-sha1-array.c
index 60ea1d5..09f7790 100644
--- a/t/helper/test-sha1-array.c
+++ b/t/helper/test-sha1-array.c
@@ -6,7 +6,7 @@ static void print_sha1(const unsigned char sha1[20], void *data)
 	puts(sha1_to_hex(sha1));
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct sha1_array array = SHA1_ARRAY_INIT;
 	struct strbuf line = STRBUF_INIT;
diff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c
index e57eae1..a1c13f5 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-sha1.c
@@ -1,6 +1,6 @@
 #include "cache.h"
 
-int main(int ac, char **av)
+int cmd_main(int ac, const char **av)
 {
 	git_SHA_CTX ctx;
 	unsigned char sha1[20];
diff --git a/t/helper/test-sigchain.c b/t/helper/test-sigchain.c
index e499fce..b71edbd 100644
--- a/t/helper/test-sigchain.c
+++ b/t/helper/test-sigchain.c
@@ -13,7 +13,7 @@ X(two)
 X(three)
 #undef X
 
-int main(int argc, char **argv) {
+int cmd_main(int argc, const char **argv) {
 	sigchain_push(SIGTERM, one);
 	sigchain_push(SIGTERM, two);
 	sigchain_push(SIGTERM, three);
diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 14bdf9d..4a68967 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -41,7 +41,7 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)
 	return starts_with(item->string, prefix);
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	if (argc == 5 && !strcmp(argv[1], "split")) {
 		struct string_list list = STRING_LIST_INIT_DUP;
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index dab8c27..2cffded 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -2,7 +2,7 @@
 #include "submodule-config.h"
 #include "submodule.h"
 
-static void die_usage(int argc, char **argv, const char *msg)
+static void die_usage(int argc, const char **argv, const char *msg)
 {
 	fprintf(stderr, "%s\n", msg);
 	fprintf(stderr, "Usage: %s [<commit> <submodulepath>] ...\n", argv[0]);
@@ -14,9 +14,9 @@ static int git_test_config(const char *var, const char *value, void *cb)
 	return parse_submodule_config_option(var, value);
 }
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
-	char **arg = argv;
+	const char **arg = argv;
 	int my_argc = argc;
 	int output_url = 0;
 	int lookup_name = 0;
diff --git a/t/helper/test-subprocess.c b/t/helper/test-subprocess.c
index 56881a0..30c5765 100644
--- a/t/helper/test-subprocess.c
+++ b/t/helper/test-subprocess.c
@@ -1,7 +1,7 @@
 #include "cache.h"
 #include "run-command.h"
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int nogit = 0;
diff --git a/t/helper/test-svn-fe.c b/t/helper/test-svn-fe.c
index 120ec96..7667c08 100644
--- a/t/helper/test-svn-fe.c
+++ b/t/helper/test-svn-fe.c
@@ -11,7 +11,7 @@
 static const char test_svnfe_usage[] =
 	"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)";
 
-static int apply_delta(int argc, char *argv[])
+static int apply_delta(int argc, const char **argv)
 {
 	struct line_buffer preimage = LINE_BUFFER_INIT;
 	struct line_buffer delta = LINE_BUFFER_INIT;
@@ -35,7 +35,7 @@ static int apply_delta(int argc, char *argv[])
 	return 0;
 }
 
-int main(int argc, char *argv[])
+int cmd_main(int argc, const char **argv)
 {
 	if (argc == 2) {
 		if (svndump_init(argv[1]))
diff --git a/t/helper/test-urlmatch-normalization.c b/t/helper/test-urlmatch-normalization.c
index 090bf21..49b6e83 100644
--- a/t/helper/test-urlmatch-normalization.c
+++ b/t/helper/test-urlmatch-normalization.c
@@ -1,7 +1,7 @@
 #include "git-compat-util.h"
 #include "urlmatch.h"
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	const char usage[] = "test-urlmatch-normalization [-p | -l] <url1> | <url1> <url2>";
 	char *url1, *url2;
diff --git a/t/helper/test-wildmatch.c b/t/helper/test-wildmatch.c
index 578b164..52be876 100644
--- a/t/helper/test-wildmatch.c
+++ b/t/helper/test-wildmatch.c
@@ -1,6 +1,6 @@
 #include "cache.h"
 
-int main(int argc, char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	int i;
 	for (i = 2; i < argc; i++) {
diff --git a/upload-pack.c b/upload-pack.c
index 9e03c27..80dda75 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -820,7 +820,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
-int main(int argc, const char **argv)
+int cmd_main(int argc, const char **argv)
 {
 	const char *dir;
 	int strict = 0;
-- 
2.9.0.317.g65b4e7c

[PATCH 2/5] common-main: call git_extract_argv0_path()

[Jeff King]

Every program which links against libgit.a must call this
function, or risk hitting an assert() in system_path() that
checks whether we have configured argv0_path (though only
when RUNTIME_PREFIX is defined, so essentially only on
Windows).

Looking at the diff, you can see that putting it into the
common main() saves us having to do it individually in each
of the external commands. But what you can't see are the
cases where we _should_ have been doing so, but weren't
(e.g., git-credential-store, and all of the t/helper test
programs).

This has been an accident-waiting-to-happen for a long time,
but wasn't triggered until recently because it involves one
of those programs actually calling system_path(). That
happened with git-credential-store in v2.8.0 with ae5f677
(lazily load core.sharedrepository, 2016-03-11). The
program:

  - takes a lock file, which...

  - opens a tempfile, which...

  - calls adjust_shared_perm to fix permissions, which...

  - lazy-loads the config (as of ae5f677), which...

  - calls system_path() to find the location of
    /etc/gitconfig

On systems with RUNTIME_PREFIX, this means credential-store
reliably hits that assert() and cannot be used.

We never noticed in the test suite, because we set
GIT_CONFIG_NOSYSTEM there, which skips the system_path()
lookup entirely.  But if we were to tweak git_config() to
find /etc/gitconfig even when we aren't going to open it,
then the test suite shows multiple failures (for
credential-store, and for some other test helpers). I didn't
include that tweak here because it's way too specific to
this particular call to be worth carrying around what is
essentially dead code.

The implementation is fairly straightforward, with one
exception: there is exactly one caller (git.c) that actually
cares about the result of the function, and not the
side-effect of setting up argv0_path. We can accommodate
that by simply replacing the value of argv[0] in the array
we hand down to cmd_main().

Signed-off-by: Jeff King <peff@peff.net>
---
 common-main.c    | 3 +++
 daemon.c         | 3 ---
 fast-import.c    | 3 ---
 git.c            | 2 +-
 http-backend.c   | 1 -
 http-fetch.c     | 2 --
 http-push.c      | 2 --
 imap-send.c      | 2 --
 remote-curl.c    | 1 -
 remote-testsvn.c | 1 -
 shell.c          | 2 --
 upload-pack.c    | 1 -
 12 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/common-main.c b/common-main.c
index 2b96bbf..57c912a 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,4 +1,5 @@
 #include "git-compat-util.h"
+#include "exec_cmd.h"
 
 int main(int argc, char **av)
 {
@@ -8,5 +9,7 @@ int main(int argc, char **av)
 	 */
 	const char **argv = (const char **)av;
 
+	argv[0] = git_extract_argv0_path(argv[0]);
+
 	return cmd_main(argc, argv);
 }
diff --git a/daemon.c b/daemon.c
index 4326cd0..d33ee5a 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,6 +1,5 @@
 #include "cache.h"
 #include "pkt-line.h"
-#include "exec_cmd.h"
 #include "run-command.h"
 #include "strbuf.h"
 #include "string-list.h"
@@ -1204,8 +1203,6 @@ int cmd_main(int argc, const char **argv)
 
 	git_setup_gettext();
 
-	git_extract_argv0_path(argv[0]);
-
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		const char *v;
diff --git a/fast-import.c b/fast-import.c
index 7cb7434..f12cd00 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -164,7 +164,6 @@ Format of STDIN stream:
 #include "refs.h"
 #include "csum-file.h"
 #include "quote.h"
-#include "exec_cmd.h"
 #include "dir.h"
 #include "run-command.h"
 
@@ -3449,8 +3448,6 @@ int cmd_main(int argc, const char **argv)
 {
 	unsigned int i;
 
-	git_extract_argv0_path(argv[0]);
-
 	git_setup_gettext();
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
diff --git a/git.c b/git.c
index e244404..3b4e12d 100644
--- a/git.c
+++ b/git.c
@@ -635,7 +635,7 @@ int cmd_main(int argc, const char **argv)
 	const char *cmd;
 	int done_help = 0;
 
-	cmd = git_extract_argv0_path(argv[0]);
+	cmd = argv[0];
 	if (!cmd)
 		cmd = "git-help";
 
diff --git a/http-backend.c b/http-backend.c
index 411c835..5375cbc 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -642,7 +642,6 @@ int cmd_main(int argc, const char **argv)
 
 	git_setup_gettext();
 
-	git_extract_argv0_path(argv[0]);
 	set_die_routine(die_webcgi);
 	set_die_is_recursing_routine(die_webcgi_recursing);
 
diff --git a/http-fetch.c b/http-fetch.c
index eb559eb..244cd0d 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -24,8 +24,6 @@ int cmd_main(int argc, const char **argv)
 
 	git_setup_gettext();
 
-	git_extract_argv0_path(argv[0]);
-
 	while (arg < argc && argv[arg][0] == '-') {
 		if (argv[arg][1] == 't') {
 			get_tree = 1;
diff --git a/http-push.c b/http-push.c
index df20e44..3a5fecf 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1711,8 +1711,6 @@ int cmd_main(int argc, const char **argv)
 
 	git_setup_gettext();
 
-	git_extract_argv0_path(argv[0]);
-
 	repo = xcalloc(1, sizeof(*repo));
 
 	argv++;
diff --git a/imap-send.c b/imap-send.c
index 890e1cb..125b218 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1500,8 +1500,6 @@ int cmd_main(int argc, const char **argv)
 	int total;
 	int nongit_ok;
 
-	git_extract_argv0_path(argv[0]);
-
 	git_setup_gettext();
 
 	setup_git_directory_gently(&nongit_ok);
diff --git a/remote-curl.c b/remote-curl.c
index a068340..d39f4cf 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -991,7 +991,6 @@ int cmd_main(int argc, const char **argv)
 
 	git_setup_gettext();
 
-	git_extract_argv0_path(argv[0]);
 	setup_git_directory_gently(&nongit);
 	if (argc < 2) {
 		error("remote-curl: usage: git remote-curl <remote> [<url>]");
diff --git a/remote-testsvn.c b/remote-testsvn.c
index 32631eb..f87bf85 100644
--- a/remote-testsvn.c
+++ b/remote-testsvn.c
@@ -292,7 +292,6 @@ int cmd_main(int argc, const char **argv)
 	static struct remote *remote;
 	const char *url_in;
 
-	git_extract_argv0_path(argv[0]);
 	setup_git_directory();
 	if (argc < 2 || argc > 3) {
 		usage("git-remote-svn <remote-name> [<url>]");
diff --git a/shell.c b/shell.c
index 3dd7fdc..ca00807 100644
--- a/shell.c
+++ b/shell.c
@@ -147,8 +147,6 @@ int cmd_main(int argc, const char **argv)
 
 	git_setup_gettext();
 
-	git_extract_argv0_path(argv[0]);
-
 	/*
 	 * Always open file descriptors 0/1/2 to avoid clobbering files
 	 * in die().  It also avoids messing up when the pipes are dup'ed
diff --git a/upload-pack.c b/upload-pack.c
index 80dda75..681fd2f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -839,7 +839,6 @@ int cmd_main(int argc, const char **argv)
 	git_setup_gettext();
 
 	packet_trace_identity("upload-pack");
-	git_extract_argv0_path(argv[0]);
 	check_replace_refs = 0;
 
 	argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);
-- 
2.9.0.317.g65b4e7c

[PATCH 3/5] common-main: call sanitize_stdfds()

[Jeff King]

This is setup that should be done in every program for
safety, but we never got around to adding it everywhere (so
builtins benefited from the call in git.c, but any external
commands did not). Putting it in the common main() gives us
this safety everywhere.

Note that the case in daemon.c is a little funny. We wait
until we know whether we want to daemonize, and then either:

 - call daemonize(), which will close stdio and reopen it to
   /dev/null under the hood

 - sanitize_stdfds(), to fix up any odd cases

But that is way too late; the point of sanitizing is to give
us reliable descriptors on 0/1/2, and we will already have
executed code, possibly called die(), etc. The sanitizing
should be the very first thing that happens.

With this patch, git-daemon will sanitize first, and can
remove the call in the non-daemonize case. It does mean that
daemonize() may just end up closing the descriptors we
opened, but that's not a big deal (it's not wrong to do so,
nor is it really less optimal than the case where our parent
process redirected us from /dev/null ahead of time).

Signed-off-by: Jeff King <peff@peff.net>
---
 common-main.c | 9 ++++++++-
 daemon.c      | 3 +--
 git.c         | 7 -------
 shell.c       | 7 -------
 4 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/common-main.c b/common-main.c
index 57c912a..353c6ea 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,4 +1,4 @@
-#include "git-compat-util.h"
+#include "cache.h"
 #include "exec_cmd.h"
 
 int main(int argc, char **av)
@@ -9,6 +9,13 @@ int main(int argc, char **av)
 	 */
 	const char **argv = (const char **)av;
 
+	/*
+	 * Always open file descriptors 0/1/2 to avoid clobbering files
+	 * in die().  It also avoids messing up when the pipes are dup'ed
+	 * onto stdin/stdout/stderr in the child processes we spawn.
+	 */
+	sanitize_stdfds();
+
 	argv[0] = git_extract_argv0_path(argv[0]);
 
 	return cmd_main(argc, argv);
diff --git a/daemon.c b/daemon.c
index d33ee5a..8646f33 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1378,8 +1378,7 @@ int cmd_main(int argc, const char **argv)
 	if (detach) {
 		if (daemonize())
 			die("--detach not supported on this platform");
-	} else
-		sanitize_stdfds();
+	}
 
 	if (pid_file)
 		write_file(pid_file, "%"PRIuMAX, (uintmax_t) getpid());
diff --git a/git.c b/git.c
index 3b4e12d..b65083c 100644
--- a/git.c
+++ b/git.c
@@ -639,13 +639,6 @@ int cmd_main(int argc, const char **argv)
 	if (!cmd)
 		cmd = "git-help";
 
-	/*
-	 * Always open file descriptors 0/1/2 to avoid clobbering files
-	 * in die().  It also avoids messing up when the pipes are dup'ed
-	 * onto stdin/stdout/stderr in the child processes we spawn.
-	 */
-	sanitize_stdfds();
-
 	restore_sigpipe_to_default();
 
 	git_setup_gettext();
diff --git a/shell.c b/shell.c
index ca00807..5e70acb 100644
--- a/shell.c
+++ b/shell.c
@@ -147,13 +147,6 @@ int cmd_main(int argc, const char **argv)
 
 	git_setup_gettext();
 
-	/*
-	 * Always open file descriptors 0/1/2 to avoid clobbering files
-	 * in die().  It also avoids messing up when the pipes are dup'ed
-	 * onto stdin/stdout/stderr in the child processes we spawn.
-	 */
-	sanitize_stdfds();
-
 	/*
 	 * Special hack to pretend to be a CVS server
 	 */
-- 
2.9.0.317.g65b4e7c

[PATCH 4/5] common-main: call restore_sigpipe_to_default()

[Jeff King]

This is another safety/sanity setup that should be in force
everywhere, but which we only applied in git.c. This did
catch most cases, since even external commands are typically
run via "git ..." (and the restoration applies to
sub-processes, too). But there were cases we missed, such as
somebody calling git-upload-pack directly via ssh, or
scripts which use dashed external commands directly.

Signed-off-by: Jeff King <peff@peff.net>
---
 common-main.c | 23 +++++++++++++++++++++++
 git.c         | 23 -----------------------
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/common-main.c b/common-main.c
index 353c6ea..20e55ef 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,6 +1,27 @@
 #include "cache.h"
 #include "exec_cmd.h"
 
+/*
+ * Many parts of Git have subprograms communicate via pipe, expect the
+ * upstream of a pipe to die with SIGPIPE when the downstream of a
+ * pipe does not need to read all that is written.  Some third-party
+ * programs that ignore or block SIGPIPE for their own reason forget
+ * to restore SIGPIPE handling to the default before spawning Git and
+ * break this carefully orchestrated machinery.
+ *
+ * Restore the way SIGPIPE is handled to default, which is what we
+ * expect.
+ */
+static void restore_sigpipe_to_default(void)
+{
+	sigset_t unblock;
+
+	sigemptyset(&unblock);
+	sigaddset(&unblock, SIGPIPE);
+	sigprocmask(SIG_UNBLOCK, &unblock, NULL);
+	signal(SIGPIPE, SIG_DFL);
+}
+
 int main(int argc, char **av)
 {
 	/*
@@ -18,5 +39,7 @@ int main(int argc, char **av)
 
 	argv[0] = git_extract_argv0_path(argv[0]);
 
+	restore_sigpipe_to_default();
+
 	return cmd_main(argc, argv);
 }
diff --git a/git.c b/git.c
index b65083c..ccb24fd 100644
--- a/git.c
+++ b/git.c
@@ -609,27 +609,6 @@ static int run_argv(int *argcp, const char ***argv)
 	return done_alias;
 }
 
-/*
- * Many parts of Git have subprograms communicate via pipe, expect the
- * upstream of a pipe to die with SIGPIPE when the downstream of a
- * pipe does not need to read all that is written.  Some third-party
- * programs that ignore or block SIGPIPE for their own reason forget
- * to restore SIGPIPE handling to the default before spawning Git and
- * break this carefully orchestrated machinery.
- *
- * Restore the way SIGPIPE is handled to default, which is what we
- * expect.
- */
-static void restore_sigpipe_to_default(void)
-{
-	sigset_t unblock;
-
-	sigemptyset(&unblock);
-	sigaddset(&unblock, SIGPIPE);
-	sigprocmask(SIG_UNBLOCK, &unblock, NULL);
-	signal(SIGPIPE, SIG_DFL);
-}
-
 int cmd_main(int argc, const char **argv)
 {
 	const char *cmd;
@@ -639,8 +618,6 @@ int cmd_main(int argc, const char **argv)
 	if (!cmd)
 		cmd = "git-help";
 
-	restore_sigpipe_to_default();
-
 	git_setup_gettext();
 
 	trace_command_performance(argv);
-- 
2.9.0.317.g65b4e7c

[PATCH 5/5] common-main: call git_setup_gettext()

[Jeff King]

This should be part of every program, as otherwise users do
not get translated error messages. However, some external
commands forgot to do so (e.g., git-credential-store). This
fixes them, and eliminates the repeated code in programs
that did remember to use it.

Signed-off-by: Jeff King <peff@peff.net>
---
 common-main.c  | 2 ++
 daemon.c       | 2 --
 fast-import.c  | 2 --
 git.c          | 2 --
 http-backend.c | 2 --
 http-fetch.c   | 2 --
 http-push.c    | 2 --
 imap-send.c    | 2 --
 remote-curl.c  | 2 --
 shell.c        | 2 --
 show-index.c   | 2 --
 upload-pack.c  | 2 --
 12 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/common-main.c b/common-main.c
index 20e55ef..3be5ad1 100644
--- a/common-main.c
+++ b/common-main.c
@@ -37,6 +37,8 @@ int main(int argc, char **av)
 	 */
 	sanitize_stdfds();
 
+	git_setup_gettext();
+
 	argv[0] = git_extract_argv0_path(argv[0]);
 
 	restore_sigpipe_to_default();
diff --git a/daemon.c b/daemon.c
index 8646f33..e647254 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1201,8 +1201,6 @@ int cmd_main(int argc, const char **argv)
 	struct credentials *cred = NULL;
 	int i;
 
-	git_setup_gettext();
-
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		const char *v;
diff --git a/fast-import.c b/fast-import.c
index f12cd00..bf53ac9 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3448,8 +3448,6 @@ int cmd_main(int argc, const char **argv)
 {
 	unsigned int i;
 
-	git_setup_gettext();
-
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(fast_import_usage);
 
diff --git a/git.c b/git.c
index ccb24fd..0f1937f 100644
--- a/git.c
+++ b/git.c
@@ -618,8 +618,6 @@ int cmd_main(int argc, const char **argv)
 	if (!cmd)
 		cmd = "git-help";
 
-	git_setup_gettext();
-
 	trace_command_performance(argv);
 
 	/*
diff --git a/http-backend.c b/http-backend.c
index 5375cbc..0d59499 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -640,8 +640,6 @@ int cmd_main(int argc, const char **argv)
 	char *cmd_arg = NULL;
 	int i;
 
-	git_setup_gettext();
-
 	set_die_routine(die_webcgi);
 	set_die_is_recursing_routine(die_webcgi_recursing);
 
diff --git a/http-fetch.c b/http-fetch.c
index 244cd0d..3b556d6 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -22,8 +22,6 @@ int cmd_main(int argc, const char **argv)
 	int get_verbosely = 0;
 	int get_recover = 0;
 
-	git_setup_gettext();
-
 	while (arg < argc && argv[arg][0] == '-') {
 		if (argv[arg][1] == 't') {
 			get_tree = 1;
diff --git a/http-push.c b/http-push.c
index 3a5fecf..dacada9 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1709,8 +1709,6 @@ int cmd_main(int argc, const char **argv)
 	int new_refs;
 	struct ref *ref, *local_refs;
 
-	git_setup_gettext();
-
 	repo = xcalloc(1, sizeof(*repo));
 
 	argv++;
diff --git a/imap-send.c b/imap-send.c
index 125b218..9cbe27f 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1500,8 +1500,6 @@ int cmd_main(int argc, const char **argv)
 	int total;
 	int nongit_ok;
 
-	git_setup_gettext();
-
 	setup_git_directory_gently(&nongit_ok);
 	git_imap_config();
 
diff --git a/remote-curl.c b/remote-curl.c
index d39f4cf..6b83b77 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -989,8 +989,6 @@ int cmd_main(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	int nongit;
 
-	git_setup_gettext();
-
 	setup_git_directory_gently(&nongit);
 	if (argc < 2) {
 		error("remote-curl: usage: git remote-curl <remote> [<url>]");
diff --git a/shell.c b/shell.c
index 5e70acb..464ee1a 100644
--- a/shell.c
+++ b/shell.c
@@ -145,8 +145,6 @@ int cmd_main(int argc, const char **argv)
 	struct commands *cmd;
 	int count;
 
-	git_setup_gettext();
-
 	/*
 	 * Special hack to pretend to be a CVS server
 	 */
diff --git a/show-index.c b/show-index.c
index 575f9c5..1ead41e 100644
--- a/show-index.c
+++ b/show-index.c
@@ -11,8 +11,6 @@ int cmd_main(int argc, const char **argv)
 	unsigned int version;
 	static unsigned int top_index[256];
 
-	git_setup_gettext();
-
 	if (argc != 1)
 		usage(show_index_usage);
 	if (fread(top_index, 2 * 4, 1, stdin) != 1)
diff --git a/upload-pack.c b/upload-pack.c
index 681fd2f..8769b1b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -836,8 +836,6 @@ int cmd_main(int argc, const char **argv)
 		OPT_END()
 	};
 
-	git_setup_gettext();
-
 	packet_trace_identity("upload-pack");
 	check_replace_refs = 0;
 
-- 
2.9.0.317.g65b4e7c

Re: git-credentials-store.exe crash

[Johannes Schindelin]

Hi Peff,

it is sad when bug reporters simply delete the part of our bug reporting
template that says: "I searched for existing bug reports but did not find
any".

It saves the reporters a few minutes... and spends the time of you and me
in the hour range.

In this particular case, the bug report is an obvious duplicate of
https://github.com/git-for-windows/git/issues/766

The saddest part is that I already spent time to investigate this and to
come up with a work-around, and I already committed and pushed it.

So those hours are now thoroughly wasted.

Disappointed,
Dscho

Re: git-credentials-store.exe crash

[Jeff King]

On Fri, Jul 01, 2016 at 09:38:33AM +0200, Johannes Schindelin wrote:

> it is sad when bug reporters simply delete the part of our bug reporting
> template that says: "I searched for existing bug reports but did not find
> any".
> 
> It saves the reporters a few minutes... and spends the time of you and me
> in the hour range.
> 
> In this particular case, the bug report is an obvious duplicate of
> https://github.com/git-for-windows/git/issues/766
> 
> The saddest part is that I already spent time to investigate this and to
> come up with a work-around, and I already committed and pushed it.
> 
> So those hours are now thoroughly wasted.

Yeah, you're right. Same bug, and your patch obviously is the right
thing to do as an immediate fix.

If it's any consolation, I diagnosed the bug quite quickly, and the
extra hours went to solving the greater problem that you mentioned in
https://github.com/git-for-windows/git/issues/766#issuecomment-226985175.

So hopefully there is still some value in my series.

-Peff

Re: [PATCH 0/5] consistent setup code for external commands

[Johannes Schindelin]

Hi Peff,

On Fri, 1 Jul 2016, Jeff King wrote:

>   - for external programs run as "git foo", you get _two_ times.

I considered doing this in my work-around, but decided against it because
I deemed it too inelegant to call it twice.

But your patch series has more benefits than just fixing the assertion in
the credential-store.

For one, it makes test programs such as test-dump-untracked-cache.exe
usable outside Git's test suite without any major envvar jujitsu.

So once the dust settles, I will revert my commit in git-for-windows.git
and merge in your patches.

Thanks,
Dscho

Re: [PATCH 1/5] add an extra level of indirection to main()

[Johannes Schindelin]

Hi Peff,

On Fri, 1 Jul 2016, Jeff King wrote:

> I waffled between the two mechanisms. Opinions welcome.

I am happy you took the cmd_main() approach: we do have to play some
tricks on Windows, in particular in some upcoming changes that support
building with MS Visual C++ (we want to ensure that `argv` is in UTF-8,
which means that we actually have to use the UTF-16 versions and convert
them manually lest argv has the current Windows encoding of strings).
Which means that we still have to use that mingw_startup() trick you
mentioned, and which would have interfered had you chosen a similar
method.

> diff --git a/common-main.c b/common-main.c
> new file mode 100644
> index 0000000..2b96bbf
> --- /dev/null
> +++ b/common-main.c
> @@ -0,0 +1,12 @@
> +#include "git-compat-util.h"
> +
> +int main(int argc, char **av)
> +{
> +	/*
> +	 * This const trickery is explained in
> +	 * 84d32bf7678259c08406571cd6ce4b7a6724dcba

This commit message says that mingw_main() is declared with char **argv,
and that is the reason why we have to convert. Maybe spell that out here,
and then in a subsequent commit, we can fix the mingw_main() declaration?

> +	 */
> +	const char **argv = (const char **)av;
> +
> +	return cmd_main(argc, argv);
> +}

Ciao,
Dscho

Re: [PATCH 2/5] common-main: call git_extract_argv0_path()

[Johannes Schindelin]

Hi Peff,

On Fri, 1 Jul 2016, Jeff King wrote:

> This has been an accident-waiting-to-happen for a long time,
> but wasn't triggered until recently because it involves one
> of those programs actually calling system_path(). That
> happened with git-credential-store in v2.8.0 with ae5f677
> (lazily load core.sharedrepository, 2016-03-11). The
> program:
> 
>   - takes a lock file, which...
> 
>   - opens a tempfile, which...
> 
>   - calls adjust_shared_perm to fix permissions, which...
> 
>   - lazy-loads the config (as of ae5f677), which...
> 
>   - calls system_path() to find the location of
>     /etc/gitconfig
> 
> On systems with RUNTIME_PREFIX, this means credential-store
> reliably hits that assert() and cannot be used.

Thank you for that thorough write-up. I am now even more upset that we had
to go through the same steps (it took me an hour to figure out what was
going wrong, mostly due to abort() *not* spitting out a stack trace, so I
had to wield some gdb magic).

I am partly to blame here, of course, because I did not report what I did
to this mailing list. But then: 1) I considered this a Windows-only
problem, and 2) I was really already swamped, as it were.

The patch is good, of course, as are the rest of the patches (I did not
really look at them very thoroughly, but then, they are pretty obvious
improvements).

Ciao,
Dscho

Re: [PATCH 1/5] add an extra level of indirection to main()

[Jeff King]

On Fri, Jul 01, 2016 at 10:04:44AM +0200, Johannes Schindelin wrote:

> Hi Peff,
> 
> On Fri, 1 Jul 2016, Jeff King wrote:
> 
> > I waffled between the two mechanisms. Opinions welcome.
> 
> I am happy you took the cmd_main() approach: we do have to play some
> tricks on Windows, in particular in some upcoming changes that support
> building with MS Visual C++ (we want to ensure that `argv` is in UTF-8,
> which means that we actually have to use the UTF-16 versions and convert
> them manually lest argv has the current Windows encoding of strings).
> Which means that we still have to use that mingw_startup() trick you
> mentioned, and which would have interfered had you chosen a similar
> method.

I actually wondered if it would make sense to get rid of the
mingw_main() macro, and do it here as just:

    #ifdef MINGW
    mingw_startup();
    #endif

or something. But I didn't look deeply at it, and anyway I am afraid to
touch anything in that area because I can't even compile-test it.

> > diff --git a/common-main.c b/common-main.c
> > new file mode 100644
> > index 0000000..2b96bbf
> > --- /dev/null
> > +++ b/common-main.c
> > @@ -0,0 +1,12 @@
> > +#include "git-compat-util.h"
> > +
> > +int main(int argc, char **av)
> > +{
> > +	/*
> > +	 * This const trickery is explained in
> > +	 * 84d32bf7678259c08406571cd6ce4b7a6724dcba
> 
> This commit message says that mingw_main() is declared with char **argv,
> and that is the reason why we have to convert. Maybe spell that out here,
> and then in a subsequent commit, we can fix the mingw_main() declaration?

The description was sufficiently long that I didn't want to try
repeating it, and opted for a reference instead. If you're planning to
fix mingw_main(), I'd be happy to do that as a preparatory patch, and
then just skip this trickery entirely. :)

-Peff

Re: [PATCH 1/5] add an extra level of indirection to main()

[Johannes Schindelin]

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

Hi Peff,

On Fri, 1 Jul 2016, Jeff King wrote:

> On Fri, Jul 01, 2016 at 10:04:44AM +0200, Johannes Schindelin wrote:
> 
> > On Fri, 1 Jul 2016, Jeff King wrote:
> > 
> > > I waffled between the two mechanisms. Opinions welcome.
> > 
> > I am happy you took the cmd_main() approach: we do have to play some
> > tricks on Windows, in particular in some upcoming changes that support
> > building with MS Visual C++ (we want to ensure that `argv` is in UTF-8,
> > which means that we actually have to use the UTF-16 versions and convert
> > them manually lest argv has the current Windows encoding of strings).
> > Which means that we still have to use that mingw_startup() trick you
> > mentioned, and which would have interfered had you chosen a similar
> > method.
> 
> I actually wondered if it would make sense to get rid of the
> mingw_main() macro, and do it here as just:
> 
>     #ifdef MINGW
>     mingw_startup();
>     #endif
> 
> or something. But I didn't look deeply at it, and anyway I am afraid to
> touch anything in that area because I can't even compile-test it.

Sure. There are a couple of patches in flight to support MSVC better, and
one part is a duplication of mingw_startup(). I would like to fix that
before merging, of course, and hope that it will naturally be helped by
your patch series.

> > > diff --git a/common-main.c b/common-main.c
> > > new file mode 100644
> > > index 0000000..2b96bbf
> > > --- /dev/null
> > > +++ b/common-main.c
> > > @@ -0,0 +1,12 @@
> > > +#include "git-compat-util.h"
> > > +
> > > +int main(int argc, char **av)
> > > +{
> > > +	/*
> > > +	 * This const trickery is explained in
> > > +	 * 84d32bf7678259c08406571cd6ce4b7a6724dcba
> > 
> > This commit message says that mingw_main() is declared with char **argv,
> > and that is the reason why we have to convert. Maybe spell that out here,
> > and then in a subsequent commit, we can fix the mingw_main() declaration?
> 
> The description was sufficiently long that I didn't want to try
> repeating it, and opted for a reference instead. If you're planning to
> fix mingw_main(), I'd be happy to do that as a preparatory patch, and
> then just skip this trickery entirely. :)

Deal:

½/5 is in 5c54dff5c54e68a1101d8fe37aefc6158fddd7f2 and the fixup for 1/5
is in 7b74f7aabb56b428c74f5983c066dc9ea8fe5116 in the 'common-main' branch
on https://github.com/dscho/git.

(I had to resolve merge conflicts in a couple of the later patches, so
feel free to just use the branch, but please note that I cherry-picked a
patch to let me compile with DEVELOPER=1 on Windows, so you might want to
drop 7b74f7aabb56b428c74f5983c066dc9ea8fe5116.)

Ciao,
Dscho

Re: [PATCH 0/5] consistent setup code for external commands

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 01, 2016 at 12:07:15AM -0400, Jeff King wrote:
>
>> Interesting. It's failing on the assert(argv0_path) in system_path().
>> 
>> That's part of the RUNTIME_PREFIX code which is built only on Windows,
>> so this is a Windows-specific issue.
>> 
>> I can guess the reason that argv0_path is not set is that
>> credential-store has its own main() function and does not call
>> git_extract_argv0_path(). It never mattered before, but as of v2.8.0,
>> one of the library functions it calls wants to use system_path(), which
>> assumes that the argv0 stuff has been set up.
>> 
>> I'm preparing some patches to fix this case (and some other related
>> ones).
>
> Here they are:
>
>   [1/5]: add an extra level of indirection to main()
>   [2/5]: common-main: call git_extract_argv0_path()
>   [3/5]: common-main: call sanitize_stdfds()
>   [4/5]: common-main: call restore_sigpipe_to_default()
>   [5/5]: common-main: call git_setup_gettext()

As this is also a fix to maint-2.8 track, I tweaked them to ensure
that they apply there, and queued the result as jk/common-main.  I
double checked the result by comparing the result of applying these
five patches directly on top of master, and the result of merging
that jk/common-main (based on maint-2.8) into master, and they seem
to match.

Thanks.

Re: [PATCH 1/5] add an extra level of indirection to main()

[Jeff King]

On Fri, Jul 01, 2016 at 03:39:03PM +0200, Johannes Schindelin wrote:

> > The description was sufficiently long that I didn't want to try
> > repeating it, and opted for a reference instead. If you're planning to
> > fix mingw_main(), I'd be happy to do that as a preparatory patch, and
> > then just skip this trickery entirely. :)
> 
> Deal:
> 
> ½/5 is in 5c54dff5c54e68a1101d8fe37aefc6158fddd7f2 and the fixup for 1/5
> is in 7b74f7aabb56b428c74f5983c066dc9ea8fe5116 in the 'common-main' branch
> on https://github.com/dscho/git.

Hrm. I didn't find 5c54dff5c when I fetched from dscho/git. However,
having seen 7b74f7aa, the one that comes on top, I actually think that's
what we really want anyway. It's really my first patch that makes your
cleanup safe to do.

-Peff

Re: [PATCH 0/5] consistent setup code for external commands

[Jeff King]

On Fri, Jul 01, 2016 at 03:19:37PM -0700, Junio C Hamano wrote:

> > Here they are:
> >
> >   [1/5]: add an extra level of indirection to main()
> >   [2/5]: common-main: call git_extract_argv0_path()
> >   [3/5]: common-main: call sanitize_stdfds()
> >   [4/5]: common-main: call restore_sigpipe_to_default()
> >   [5/5]: common-main: call git_setup_gettext()
> 
> As this is also a fix to maint-2.8 track, I tweaked them to ensure
> that they apply there, and queued the result as jk/common-main.  I
> double checked the result by comparing the result of applying these
> five patches directly on top of master, and the result of merging
> that jk/common-main (based on maint-2.8) into master, and they seem
> to match.

Thanks, this obviously is a regression in v2.8, but I didn't even think
about that and just built it on top of master.

I think the cleanup that Dscho suggested elsewhere in the thread is a
good idea on top, but that can also just wait and come separately.

-Peff

Re: [PATCH 1/5] add an extra level of indirection to main()

[Johannes Schindelin]

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

Hi Peff,

On Fri, 1 Jul 2016, Jeff King wrote:

> On Fri, Jul 01, 2016 at 03:39:03PM +0200, Johannes Schindelin wrote:
> 
> > > The description was sufficiently long that I didn't want to try
> > > repeating it, and opted for a reference instead. If you're planning to
> > > fix mingw_main(), I'd be happy to do that as a preparatory patch, and
> > > then just skip this trickery entirely. :)
> > 
> > Deal:
> > 
> > ½/5 is in 5c54dff5c54e68a1101d8fe37aefc6158fddd7f2 and the fixup for 1/5
> > is in 7b74f7aabb56b428c74f5983c066dc9ea8fe5116 in the 'common-main' branch
> > on https://github.com/dscho/git.
> 
> Hrm. I didn't find 5c54dff5c when I fetched from dscho/git. However,
> having seen 7b74f7aa, the one that comes on top, I actually think that's
> what we really want anyway. It's really my first patch that makes your
> cleanup safe to do.

The reason is obvious: I forgot to push :-( Hopefully you did not
duplicate all my work to move my patch in front of the branch, to make it
a preparatory patch.

It is pushed now.

Sorry!
Dscho

Re: [PATCH 0/5] consistent setup code for external commands

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 01, 2016 at 03:19:37PM -0700, Junio C Hamano wrote:
>
>> > Here they are:
>> >
>> >   [1/5]: add an extra level of indirection to main()
>> >   [2/5]: common-main: call git_extract_argv0_path()
>> >   [3/5]: common-main: call sanitize_stdfds()
>> >   [4/5]: common-main: call restore_sigpipe_to_default()
>> >   [5/5]: common-main: call git_setup_gettext()
>> 
>> As this is also a fix to maint-2.8 track, I tweaked them to ensure
>> that they apply there, and queued the result as jk/common-main.  I
>> double checked the result by comparing the result of applying these
>> five patches directly on top of master, and the result of merging
>> that jk/common-main (based on maint-2.8) into master, and they seem
>> to match.
>
> Thanks, this obviously is a regression in v2.8, but I didn't even think
> about that and just built it on top of master.
>
> I think the cleanup that Dscho suggested elsewhere in the thread is a
> good idea on top, but that can also just wait and come separately.

OK.

I think that amounts to the following single patch, which I cherry
picked from the topic in Dscho's repository he mentioned earlier.

With this applied on top of jk/common-main-2.8, when I merge it to
his b764cdf, the result matches his common-main topic, so we three
are on the same page, I'd think, and the result can be fed to the
2.8.x maintenance track.

-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 1 Jul 2016 15:01:28 +0200
Subject: [PATCH] mingw: declare main()'s argv as const

In 84d32bf (sparse: Fix mingw_main() argument number/type errors,
2013-04-27), we addressed problems identified by the 'sparse' tool where
argv was declared inconsistently. The way we addressed it was by casting
from the non-const version to the const-version.

This patch is long overdue, fixing compat/mingw.h's declaration to
make the "argv" parameter const.  This also allows us to lose the
"const" trickery introduced earlier to common-main.c:main().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 common-main.c  | 8 +-------
 compat/mingw.h | 2 +-
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/common-main.c b/common-main.c
index 3be5ad1..44a29e8 100644
--- a/common-main.c
+++ b/common-main.c
@@ -22,15 +22,9 @@ static void restore_sigpipe_to_default(void)
 	signal(SIGPIPE, SIG_DFL);
 }
 
-int main(int argc, char **av)
+int main(int argc, const char **argv)
 {
 	/*
-	 * This const trickery is explained in
-	 * 84d32bf7678259c08406571cd6ce4b7a6724dcba
-	 */
-	const char **argv = (const char **)av;
-
-	/*
 	 * Always open file descriptors 0/1/2 to avoid clobbering files
 	 * in die().  It also avoids messing up when the pipes are dup'ed
 	 * onto stdin/stdout/stderr in the child processes we spawn.
diff --git a/compat/mingw.h b/compat/mingw.h
index 69bb43d..1ac9086 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -535,7 +535,7 @@ extern CRITICAL_SECTION pinfo_cs;
 void mingw_startup();
 #define main(c,v) dummy_decl_mingw_main(); \
 static int mingw_main(c,v); \
-int main(int argc, char **argv) \
+int main(int argc, const char **argv) \
 { \
 	mingw_startup(); \
 	return mingw_main(__argc, (void *)__argv); \
-- 
2.9.0-457-ge524329

Re: [PATCH 0/5] consistent setup code for external commands

[Johannes Schindelin]

Hi Junio,

On Wed, 6 Jul 2016, Junio C Hamano wrote:

> I think that amounts to the following single patch, which I cherry
> picked from the topic in Dscho's repository he mentioned earlier.

That patch would be fine by me, too.

Thanks,
Dscho