Re: [PATCH v3 1/2] rev-parse: add --prefix option
From: John Keeping <hidden>
Date: 2016-06-15 22:56:54
On Fri, Apr 19, 2013 at 03:23:37PM +0530, Ramkumar Ramachandra wrote:
John Keeping wrote:quoted
Changes since v2: - Rewrite commit message - Add a new test for 'prefix ignored with HEAD:top' - Use '<<-\EOF' where appropriate in t1513Thanks for the re-roll. In the previous iteration, I wasn't sure this was the right approach because I thought it would be better to pass RUN_SETUP and let run_builtin_command() take care of the prefix-setting. Unfortunately, as 5410a02 (git-rev-parse --parseopt, 2007-11-06) indicates, we have to run setup_git_directory() in cmd_rev_parse() after parsing --parseopt, as 'git rev-parse --parseopt' can be run outside a git repository. You might want to include this note in your commit message for the benefit of other readers. Other than that, I just have one small suggestion: it's possible to avoid passing output_prefix around, and simplify show_file() a bit if we acknowledge that printing "--" is not the same as printing a file (although the condition is the same). Would you like to squash this in?
I'm not actually sure this is better. I'm more afraid of the condition for outputting files changing in the future than of passing output_prefix around, so I'd much rather keep that condition in one place. If you really feel strongly about it, I'd prefer to extract the if condition to a function and use that directly when deciding whether to print "--". [Also, you introduce a potential segfault via passing a NULL prefix to strlen.]
quoted hunk ↗ jump to hunk
Signed-off-by: Ramkumar Ramachandra <redacted> -- 8< --diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index de894c7..7e69b3f 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c@@ -29,6 +29,7 @@ static int abbrev; static int abbrev_ref; static int abbrev_ref_strict; static int output_sq; +static int output_prefix; /* * Some arguments are relevant "revision" arguments,@@ -212,15 +213,13 @@ static void show_datestring(const char *flag, const char *datestr) show(buffer); } -static int show_file(const char *arg, int output_prefix) +static int show_file(const char *arg) { show_default(); if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) { if (output_prefix) { const char *prefix = startup_info->prefix; - show(prefix_filename(prefix, - prefix ? strlen(prefix) : 0, - arg)); + show(prefix_filename(prefix, strlen(prefix), arg)); } else show(arg); return 1;@@ -228,6 +227,16 @@ static int show_file(const char *arg, int output_prefix) return 0; } +static int show_dashdash() +{ + show_default(); + if ((filter & (DO_NONFLAGS | DO_NOREV)) == (DO_NONFLAGS | DO_NOREV)) { + show("--"); + return 1; + } + return 0; +} + static int try_difference(const char *arg) { char *dotdot;@@ -476,7 +485,6 @@ N_("git rev-parse --parseopt [options] -- [<args>...]\n" int cmd_rev_parse(int argc, const char **argv, const char *prefix) { int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0; - int output_prefix = 0; unsigned char sha1[20]; const char *name = NULL;@@ -510,7 +518,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) const char *arg = argv[i]; if (as_is) { - if (show_file(arg, output_prefix) && as_is < 2) + if (show_file(arg) && as_is < 2) verify_filename(prefix, arg, 0); continue; }@@ -534,7 +542,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) as_is = 2; /* Pass on the "--" if we show anything but files.. */ if (filter & (DO_FLAGS | DO_REVS)) - show_file(arg, 0); + show_dashdash(); continue; } if (!strcmp(arg, "--default")) {@@ -543,7 +551,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) continue; } if (!strcmp(arg, "--prefix")) { - prefix = argv[i+1]; + prefix = argv[i + 1]; startup_info->prefix = prefix; output_prefix = 1; i++;@@ -768,7 +776,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) if (verify) die_no_single_rev(quiet); as_is = 1; - if (!show_file(arg, output_prefix)) + if (!show_file(arg)) continue; verify_filename(prefix, arg, 1); }