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

2013-04-18 Thread Ramkumar Ramachandra 
John Keeping wrote:
 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?
 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.

  {
 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()?

 @@ -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.

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?

 +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.

 diff --git a/t/t1513-rev-parse-prefix.sh b/t/t1513-rev-parse-prefix.sh
 new file mode 100755
 index 000..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.

 +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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-04-18 Thread John Keeping 
On Thu, Apr 18, 2013 at 07:58:25PM +0530, Ramkumar Ramachandra wrote:
 John Keeping wrote:
  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.

  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.
 
   {
  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.

  @@ -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?
 
  +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.

  diff --git a/t/t1513-rev-parse-prefix.sh b/t/t1513-rev-parse-prefix.sh
  new file mode 100755
  index 000..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.
 
  +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

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

2013-04-09 Thread Junio C Hamano 
John Keeping j...@keeping.me.uk writes:

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

 Signed-off-by: John Keeping j...@keeping.me.uk
 ---

Stale subject?

 +--prefix arg::
 + Behave as if 'git rev-parse' was invoked from the `arg`
 + subdirectory of the working tree.  Any relative filenames are
 + resolved as if they are prefixed by `arg` and will be printed
 + in that form.
 ++
 +This can be used to convert arguments to a command run in a subdirectory
 +so that they can still be used after moving to the top-level of the
 +repository.  For example:
 ++
 +
 +prefix=$(git rev-parse --show-prefix)
 +cd $(git rev-parse --show-toplevel)
 +eval set -- $(git rev-parse --sq --prefix $prefix $@)

I think you should tighten rev-parse parameter to reject options and
revisions, especially as an example to teach how to use it.  When
the user said

git mylog -U20 master..next -- README

inside Documentation/ directory,  git-mylog script would want to
see README prefixed with Documentation/ but want to see -U20 or
master..next untouched.  Historically, rev-parse was a way to sift
options and args meant for rev-list from those mant for diff-tree
so that a variant of

git rev-list $(git rev-parse --revs) $@ |
git diff-tree --stdin $(git rev-parse --no-revs)

can be used to implement such git mylog script.

I think --no-revs --no-flags is how you ask it to give you only
the paths, but I am writing from memory so please double check.

Having said all that.

Existing scripts (e.g. git am) do this kind of things without such
an option added to rev-parse.  They first do:

prefix=$(git rev-parse --show-prefix)

and refer to $prefix/$1, $prefix/$2, etc., I think.

Is this option really necessary to update git submodule?  Don't we
have a much better idea which parameter holds user-supplied path in the
script than having rev-parse make a guess on the entire $@?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-04-09 Thread John Keeping 
On Tue, Apr 09, 2013 at 01:57:21PM -0700, Junio C Hamano wrote:
 John Keeping j...@keeping.me.uk writes:
 
  This adds a prefix string to any filename arguments encountered after it
  has been specified.
 
  Signed-off-by: John Keeping j...@keeping.me.uk
  ---
 
 Stale subject?

Yep.  Sorry.

  +--prefix arg::
  +   Behave as if 'git rev-parse' was invoked from the `arg`
  +   subdirectory of the working tree.  Any relative filenames are
  +   resolved as if they are prefixed by `arg` and will be printed
  +   in that form.
  ++
  +This can be used to convert arguments to a command run in a subdirectory
  +so that they can still be used after moving to the top-level of the
  +repository.  For example:
  ++
  +
  +prefix=$(git rev-parse --show-prefix)
  +cd $(git rev-parse --show-toplevel)
  +eval set -- $(git rev-parse --sq --prefix $prefix $@)
 
 I think you should tighten rev-parse parameter to reject options and
 revisions, especially as an example to teach how to use it.  When
 the user said
 
   git mylog -U20 master..next -- README
 
 inside Documentation/ directory,  git-mylog script would want to
 see README prefixed with Documentation/ but want to see -U20 or
 master..next untouched.

And it will if it runs:

git rev-parse --prefix Documentation/ -U20 master..next -- README

Which gives:

-U20
f131fb6eb2a1e09f7ce53d148e21ce6960f42422
^fa7285dc3dce8bd01fd8c665b032603ed55348e5
--
Documentation/README

  Historically, rev-parse was a way to sift
 options and args meant for rev-list from those mant for diff-tree
 so that a variant of
 
   git rev-list $(git rev-parse --revs) $@ |
 git diff-tree --stdin $(git rev-parse --no-revs)
 
 can be used to implement such git mylog script.
 
 I think --no-revs --no-flags is how you ask it to give you only
 the paths, but I am writing from memory so please double check.
 
 Having said all that.
 
 Existing scripts (e.g. git am) do this kind of things without such
 an option added to rev-parse.  They first do:
 
 prefix=$(git rev-parse --show-prefix)
 
 and refer to $prefix/$1, $prefix/$2, etc., I think.
 
 Is this option really necessary to update git submodule?  Don't we
 have a much better idea which parameter holds user-supplied path in the
 script than having rev-parse make a guess on the entire $@?

It's not guessing on all of $@ in git-submodule - we know that
everything left is a path.

I've looked at git-am and hadn't thought of doing that, just thought
this was a reasonably elegant way of processing the arguments.  I'm
happy to try another approach if that's going to be more acceptable.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-04-09 Thread Junio C Hamano 
John Keeping j...@keeping.me.uk writes:

 It's not guessing on all of $@ in git-submodule - we know that
 everything left is a path.

OK, then.


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html