Re: [PATCH v3 1/2] rev-parse: add --prefix option

[John Keeping <john@keeping.me.uk>]

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@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);
>  	}