threads / patch / 21156

patchls-files: make option parser keep argv[0]

Subject: [PATCH] ls-files: make option parser keep argv[0]

## tl;dr

4 messages between Oct 8, 2009 and Oct 9, 2009. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Ben Walton· Oct 8, 2009, 14:20 UTC · lore

The ls-files built-in was not asking the option parser to maintain argv[0]. This led to the possibility of fprintf(stderr, "...", NULL). On Solaris, this was causing a segfault. On glibc systems, printed error messages didn't contain proper strings, but rather, "(null)":...

A trigger for this bug was: `git ls-files -i`

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 builtin-ls-files.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Show changes to builtin-ls-files.c +2 −2

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index f473220..9a3705a 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -482,7 +482,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
-			ls_files_usage, 0);
+			ls_files_usage, PARSE_OPT_KEEP_ARGV0);
 	if (show_tag || show_valid_bit) {
 		tag_cached = "H ";
 		tag_unmerged = "M ";
@@ -505,7 +505,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
-	pathspec = get_pathspec(prefix, argv);
+	pathspec = get_pathspec(prefix, argv + 1);
 
 	/* be nice with submodule paths ending in a slash */
 	read_cache();

-- 
1.6.4.4

Stephen Boyd· Oct 8, 2009, 20:32 UTC · re: Ben Walton · lore

Re: [PATCH] ls-files: make option parser keep argv[0]

Ben Walton wrote:

Show 9 quoted lines

> The ls-files built-in was not asking the option parser to maintain
> argv[0].  This led to the possibility of fprintf(stderr, "...", NULL).
> On Solaris, this was causing a segfault.  On glibc systems, printed
> error messages didn't contain proper strings, but rather, "(null)":...
>
> A trigger for this bug was: `git ls-files -i`
>
> Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
>

Patch looks good.

Just a thought, maybe we should change the fprintf(stderr,...) and exit(1) call to a die() and replace the argv[0] with "ls-files" similar to the die() on line 546. Then your diffstat becomes -1 instead of 0.

Ben Walton· Oct 9, 2009, 01:53 UTC · re: Stephen Boyd · lore

[PATCH] ls-files: die instead of fprintf/exit in -i error

When ls-files was called with -i but no exclude pattern, it was calling fprintf(stderr, "...", NULL) and then exiting. On Solaris, passing NULL into fprintf was causing a segfault. On glibc systems, it was simply producing incorrect output (eg: "(null)": ...). The NULL pointer was a result of argv[0] not being preserved by the option parser. Instead of requesting that the option parser preserve argv[0], use die() with a constant string.

A trigger for this bug was: `git ls-files -i`

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
This is the alternate solution to this bug as proposed earlier today.
I don't have a preference either way for which solution is better or
more inline with the 'git way,' so please choose the most appropriate.

I've run the test suite with both patches on Linux and Solaris and everything passes.

 builtin-ls-files.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

Show changes to builtin-ls-files.c +2 −5

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index f473220..2c95ca6 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -524,11 +524,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		ps_matched = xcalloc(1, num);
 	}
 
-	if ((dir.flags & DIR_SHOW_IGNORED) && !exc_given) {
-		fprintf(stderr, "%s: --ignored needs some exclude pattern\n",
-			argv[0]);
-		exit(1);
-	}
+	if ((dir.flags & DIR_SHOW_IGNORED) && !exc_given)
+		die("ls-files --ignored needs some exclude pattern");
 
 	/* With no flags, we default to showing the cached files */
 	if (!(show_stage | show_deleted | show_others | show_unmerged |

-- 
1.6.4.4

Junio C Hamano· Oct 9, 2009, 06:53 UTC · re: Ben Walton · lore

Re: [PATCH] ls-files: die instead of fprintf/exit in -i error

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> This is the alternate solution to this bug as proposed earlier today.
> I don't have a preference either way for which solution is better or
> more inline with the 'git way,' so please choose the most appropriate.

I think die() is better for consistency reasons if nothing else.

Thanks.