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

On Thu, Apr 18, 2013 at 07:58:25PM +0530, Ramkumar Ramachandra wrote:

John Keeping wrote:

quoted

This adds a prefix string to any filename arguments encountered after it
has been specified.

Very nice.  I thought we'd have to resort to path mangling in shell to
fix git-submodule.sh.  Glad to see that we can go with something
cleaner.

Perhaps pull some bits from your nice Documentation into the commit message?

Good idea.  I intended to re-write the commit message for v2 since the
patch was completely re-written but forgot by the time I'd sorted out
patch 2 as well.  I will do for v3.

quoted

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index f267a1d..de894c7 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c

@@ -212,11 +212,17 @@ static void show_datestring(const char *flag, const char *datestr)
        show(buffer);
 }

-static int show_file(const char *arg)
+static int show_file(const char *arg, int output_prefix)

Okay, so you've essentially patched show_file() to accept an
additional argument, and modified callers to call with this additional
argument.  I suppose
show_(rev|reference|default|flag|rev|with_type|datestring|abbrev)
don't need to be patched, as they are path-independent.

quoted

 {
        show_default();
        if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) {
-               show(arg);
+               if (output_prefix) {
+                       const char *prefix = startup_info->prefix;
+                       show(prefix_filename(prefix,
+                                            prefix ? strlen(prefix) : 0,
+                                            arg));
+               } else
+                       show(arg);

Uh, why do you need output_prefix?  If startup_info->prefix is set,
use it.  Is startup_info->prefix set by anyone by cmd_rev_parse()?

output_prefix is a flag to say "do we want to show the prefix".  We need
it because show_file is used for the "--" argument separator as well as
file paths.  Without a separate flag we end up prefixing "--" with the
prefix path.

quoted

@@ -470,6 +476,7 @@ N_("git rev-parse --parseopt [options] -- [<args>...]\n"
 int cmd_rev_parse(int argc, const char **argv, const char *prefix)

@@ -535,6 +542,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
                                i++;
                                continue;
                        }
+                       if (!strcmp(arg, "--prefix")) {
+                               prefix = argv[i+1];
+                               startup_info->prefix = prefix;
+                               output_prefix = 1;
+                               i++;
+                               continue;
+                       }

Wait, why isn't prefix filled in when run_builtin() calls this?  Oh,
right: because we didn't mark this builtin with RUN_SETUP or
RUN_SETUP_GENTLY.  Okay, now why didn't we change that?  Because it
would be a major problem (all our scripts would break) if rev-parse
did cd-to-toplevel.

prefix is already set, by setup_git_git_directory.  The point is that we
just change the values set in setup_git_directory so that the command
behaves as if it were run from a subdirectory.

Why are you setting prefix to argv[i+1], and then setting
startup_info->prefix to that?  Is anyone else in cmd_rev_parse() going
to use it?

quoted

+prefix=$(git rev-parse --show-prefix)
+cd "$(git rev-parse --show-toplevel)"
+eval "set -- $(git rev-parse --sq --prefix "$prefix" "$@")"

I'm wondering if you need such a convoluted usage though.  Will you
ever need to specify a prefix by hand that is different from what git
rev-parse --show-toplevel returns?  If not, why don't you just
rev-parse --emulate-toplevel, and get rid of specifying prefix by hand
altogether?  Then again, this is a plumbing command, so the simplicity
is probably more valuable.

How does that work?  When we run rev-parse with the --prefix argument
we're no longer in the subdirectory.

While this may look convoluted here, I don't think it is in normal usage
inside a script.  If you look at the way it's used in patch 2 we're
careful not to just remap all the arguments but to extract the flags
before remapping file paths when we know that everything we have is a
file path.

quoted

diff --git a/t/t1513-rev-parse-prefix.sh b/t/t1513-rev-parse-prefix.sh
new file mode 100755
index 0000000..5ef48d2
--- /dev/null
+++ b/t/t1513-rev-parse-prefix.sh
+test_expect_success 'empty prefix -- file' '
+       git rev-parse --prefix "" -- top sub1/file1 >actual &&
+       cat <<-EOF >expected &&

Nit: when you're not putting in variables, you can cat <<-\EOF.

quoted

+test_expect_success 'empty prefix HEAD:./path' '
+       git rev-parse --prefix "" HEAD:./top >actual &&
+       git rev-parse HEAD:top >expected &&

Nit: why did you change "./top" to "top"?  Your --prefix option
doesn't require you to change your arguments accordingly, does it?

The point is to show that the case where a prefix is applied
("HEAD:./top") is the same as the canonical form ("HEAD:top").

I should probably add a test for "HEAD:top" with a prefix to ensure that
we don't modify arguments like that.