On Fri, Apr 19, 2013 at 03:23:37PM +0530, Ramkumar Ramachandra wrote:
> 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.]
> Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
> -- 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);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html