* git-credentials-store.exe crash
@ 2016-06-30 21:24 dmh
2016-07-01 4:07 ` Jeff King
0 siblings, 1 reply; 21+ messages in thread
From: dmh @ 2016-06-30 21:24 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: git-credentials-store.exe crash 2016-06-30 21:24 git-credentials-store.exe crash dmh @ 2016-07-01 4:07 ` Jeff King 2016-07-01 5:55 ` [PATCH 0/5] consistent setup code for external commands Jeff King 2016-07-01 7:38 ` git-credentials-store.exe crash Johannes Schindelin 0 siblings, 2 replies; 21+ messages in thread From: Jeff King @ 2016-07-01 4:07 UTC (permalink / raw) To: dmh; +Cc: git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 0/5] consistent setup code for external commands 2016-07-01 4:07 ` Jeff King @ 2016-07-01 5:55 ` Jeff King 2016-07-01 5:58 ` [PATCH 1/5] add an extra level of indirection to main() Jeff King ` (6 more replies) 2016-07-01 7:38 ` git-credentials-store.exe crash Johannes Schindelin 1 sibling, 7 replies; 21+ messages in thread From: Jeff King @ 2016-07-01 5:55 UTC (permalink / raw) To: dmh; +Cc: git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/5] add an extra level of indirection to main() 2016-07-01 5:55 ` [PATCH 0/5] consistent setup code for external commands Jeff King @ 2016-07-01 5:58 ` Jeff King 2016-07-01 8:04 ` Johannes Schindelin 2016-07-01 6:04 ` [PATCH 2/5] common-main: call git_extract_argv0_path() Jeff King ` (5 subsequent siblings) 6 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2016-07-01 5:58 UTC (permalink / raw) To: dmh; +Cc: git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] add an extra level of indirection to main() 2016-07-01 5:58 ` [PATCH 1/5] add an extra level of indirection to main() Jeff King @ 2016-07-01 8:04 ` Johannes Schindelin 2016-07-01 8:19 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Johannes Schindelin @ 2016-07-01 8:04 UTC (permalink / raw) To: Jeff King; +Cc: dmh, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] add an extra level of indirection to main() 2016-07-01 8:04 ` Johannes Schindelin @ 2016-07-01 8:19 ` Jeff King 2016-07-01 13:39 ` Johannes Schindelin 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2016-07-01 8:19 UTC (permalink / raw) To: Johannes Schindelin; +Cc: dmh, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] add an extra level of indirection to main() 2016-07-01 8:19 ` Jeff King @ 2016-07-01 13:39 ` Johannes Schindelin 2016-07-01 22:38 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Johannes Schindelin @ 2016-07-01 13:39 UTC (permalink / raw) To: Jeff King; +Cc: dmh, git [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] add an extra level of indirection to main() 2016-07-01 13:39 ` Johannes Schindelin @ 2016-07-01 22:38 ` Jeff King 2016-07-02 6:52 ` Johannes Schindelin 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2016-07-01 22:38 UTC (permalink / raw) To: Johannes Schindelin; +Cc: dmh, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] add an extra level of indirection to main() 2016-07-01 22:38 ` Jeff King @ 2016-07-02 6:52 ` Johannes Schindelin 0 siblings, 0 replies; 21+ messages in thread From: Johannes Schindelin @ 2016-07-02 6:52 UTC (permalink / raw) To: Jeff King; +Cc: dmh, git [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/5] common-main: call git_extract_argv0_path() 2016-07-01 5:55 ` [PATCH 0/5] consistent setup code for external commands Jeff King 2016-07-01 5:58 ` [PATCH 1/5] add an extra level of indirection to main() Jeff King @ 2016-07-01 6:04 ` Jeff King 2016-07-01 8:05 ` Johannes Schindelin 2016-07-01 6:06 ` [PATCH 3/5] common-main: call sanitize_stdfds() Jeff King ` (4 subsequent siblings) 6 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2016-07-01 6:04 UTC (permalink / raw) To: dmh; +Cc: git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] common-main: call git_extract_argv0_path() 2016-07-01 6:04 ` [PATCH 2/5] common-main: call git_extract_argv0_path() Jeff King @ 2016-07-01 8:05 ` Johannes Schindelin 0 siblings, 0 replies; 21+ messages in thread From: Johannes Schindelin @ 2016-07-01 8:05 UTC (permalink / raw) To: Jeff King; +Cc: dmh, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/5] common-main: call sanitize_stdfds() 2016-07-01 5:55 ` [PATCH 0/5] consistent setup code for external commands Jeff King 2016-07-01 5:58 ` [PATCH 1/5] add an extra level of indirection to main() Jeff King 2016-07-01 6:04 ` [PATCH 2/5] common-main: call git_extract_argv0_path() Jeff King @ 2016-07-01 6:06 ` Jeff King 2016-07-01 6:06 ` [PATCH 4/5] common-main: call restore_sigpipe_to_default() Jeff King ` (3 subsequent siblings) 6 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2016-07-01 6:06 UTC (permalink / raw) To: dmh; +Cc: git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/5] common-main: call restore_sigpipe_to_default() 2016-07-01 5:55 ` [PATCH 0/5] consistent setup code for external commands Jeff King ` (2 preceding siblings ...) 2016-07-01 6:06 ` [PATCH 3/5] common-main: call sanitize_stdfds() Jeff King @ 2016-07-01 6:06 ` Jeff King 2016-07-01 6:07 ` [PATCH 5/5] common-main: call git_setup_gettext() Jeff King ` (2 subsequent siblings) 6 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2016-07-01 6:06 UTC (permalink / raw) To: dmh; +Cc: git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/5] common-main: call git_setup_gettext() 2016-07-01 5:55 ` [PATCH 0/5] consistent setup code for external commands Jeff King ` (3 preceding siblings ...) 2016-07-01 6:06 ` [PATCH 4/5] common-main: call restore_sigpipe_to_default() Jeff King @ 2016-07-01 6:07 ` Jeff King 2016-07-01 7:45 ` [PATCH 0/5] consistent setup code for external commands Johannes Schindelin 2016-07-01 22:19 ` Junio C Hamano 6 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2016-07-01 6:07 UTC (permalink / raw) To: dmh; +Cc: git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 0/5] consistent setup code for external commands 2016-07-01 5:55 ` [PATCH 0/5] consistent setup code for external commands Jeff King ` (4 preceding siblings ...) 2016-07-01 6:07 ` [PATCH 5/5] common-main: call git_setup_gettext() Jeff King @ 2016-07-01 7:45 ` Johannes Schindelin 2016-07-01 22:19 ` Junio C Hamano 6 siblings, 0 replies; 21+ messages in thread From: Johannes Schindelin @ 2016-07-01 7:45 UTC (permalink / raw) To: Jeff King; +Cc: dmh, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/5] consistent setup code for external commands 2016-07-01 5:55 ` [PATCH 0/5] consistent setup code for external commands Jeff King ` (5 preceding siblings ...) 2016-07-01 7:45 ` [PATCH 0/5] consistent setup code for external commands Johannes Schindelin @ 2016-07-01 22:19 ` Junio C Hamano 2016-07-01 22:39 ` Jeff King 6 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2016-07-01 22:19 UTC (permalink / raw) To: Jeff King; +Cc: dmh, git 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/5] consistent setup code for external commands 2016-07-01 22:19 ` Junio C Hamano @ 2016-07-01 22:39 ` Jeff King 2016-07-06 15:17 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2016-07-01 22:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: dmh, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/5] consistent setup code for external commands 2016-07-01 22:39 ` Jeff King @ 2016-07-06 15:17 ` Junio C Hamano 2016-07-06 15:36 ` Johannes Schindelin 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2016-07-06 15:17 UTC (permalink / raw) To: Jeff King, Johannes Schindelin; +Cc: dmh, git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 0/5] consistent setup code for external commands 2016-07-06 15:17 ` Junio C Hamano @ 2016-07-06 15:36 ` Johannes Schindelin 0 siblings, 0 replies; 21+ messages in thread From: Johannes Schindelin @ 2016-07-06 15:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, dmh, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: git-credentials-store.exe crash 2016-07-01 4:07 ` Jeff King 2016-07-01 5:55 ` [PATCH 0/5] consistent setup code for external commands Jeff King @ 2016-07-01 7:38 ` Johannes Schindelin 2016-07-01 7:43 ` Jeff King 1 sibling, 1 reply; 21+ messages in thread From: Johannes Schindelin @ 2016-07-01 7:38 UTC (permalink / raw) To: Jeff King; +Cc: dmh, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: git-credentials-store.exe crash 2016-07-01 7:38 ` git-credentials-store.exe crash Johannes Schindelin @ 2016-07-01 7:43 ` Jeff King 0 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2016-07-01 7:43 UTC (permalink / raw) To: Johannes Schindelin; +Cc: dmh, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2016-07-06 15:37 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-30 21:24 git-credentials-store.exe crash dmh 2016-07-01 4:07 ` Jeff King 2016-07-01 5:55 ` [PATCH 0/5] consistent setup code for external commands Jeff King 2016-07-01 5:58 ` [PATCH 1/5] add an extra level of indirection to main() Jeff King 2016-07-01 8:04 ` Johannes Schindelin 2016-07-01 8:19 ` Jeff King 2016-07-01 13:39 ` Johannes Schindelin 2016-07-01 22:38 ` Jeff King 2016-07-02 6:52 ` Johannes Schindelin 2016-07-01 6:04 ` [PATCH 2/5] common-main: call git_extract_argv0_path() Jeff King 2016-07-01 8:05 ` Johannes Schindelin 2016-07-01 6:06 ` [PATCH 3/5] common-main: call sanitize_stdfds() Jeff King 2016-07-01 6:06 ` [PATCH 4/5] common-main: call restore_sigpipe_to_default() Jeff King 2016-07-01 6:07 ` [PATCH 5/5] common-main: call git_setup_gettext() Jeff King 2016-07-01 7:45 ` [PATCH 0/5] consistent setup code for external commands Johannes Schindelin 2016-07-01 22:19 ` Junio C Hamano 2016-07-01 22:39 ` Jeff King 2016-07-06 15:17 ` Junio C Hamano 2016-07-06 15:36 ` Johannes Schindelin 2016-07-01 7:38 ` git-credentials-store.exe crash Johannes Schindelin 2016-07-01 7:43 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.