Re: [PATCH v3 1/2] rev-parse: add --prefix option
- From
John Keeping <john@keeping.me.uk>
- Date
- Apr 19, 2013, 10:22 UTC
- Message-ID
- <20130419102207.GF2278@serenity.lan>
- In-Reply-To
- <1366365217-17674-1-git-send-email-artagnon@gmail.com>
On Fri, Apr 19, 2013 at 03:23:37PM +0530, Ramkumar Ramachandra wrote:
Show 22 quoted lines
> John Keeping wrote: > > Changes since v2: > > - Rewrite commit message > > - Add a new test for 'prefix ignored with HEAD:top' > > - Use '<<-\EOF' where appropriate in t1513 > > Thanks 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.]
Show 93 quoted lines
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> -- 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);
> }