submodule: drop the top-level requirement
cover submodule: drop the top-level requirement
With the recent discussion, I wondered how hard it would be to add SUBDIRECTORY_OK=Yes to git-submodule.sh and it doesn't seem that bad.
Note that this series currently lacks both tests and documentation updates. Also I have made no attempt to change the output from any commands, so submodule paths are always printed relative to the top-level of the repository.
Consider this series a proof-of-concept, to see whether I'm on completely the wrong course.
John Keeping (2):
rev-parse: add --filename-prefix option
submodule: drop the top-level requirement
builtin/rev-parse.c | 17 ++++++++++++----- git-submodule.sh | 7 +++++++ 2 files changed, 19 insertions(+), 5 deletions(-)
Signature · 1 line
1.8.2.694.ga76e9c3.dirty
Am 07.04.2013 21:55, schrieb John Keeping:
Quoted 10 lines
With the recent discussion, I wondered how hard it would be to add SUBDIRECTORY_OK=Yes to
git-submodule.shand it doesn't seem that bad.Note that this series currently lacks both tests and documentation updates. Also I have made no attempt to change the output from any commands, so submodule paths are always printed relative to the top-level of the repository.
Consider this series a proof-of-concept, to see whether I'm on completely the wrong course.
Cool! Thanks for working on that, I'll take a deeper look in my next Git timeslot.
Quoted 8 lines
John Keeping (2):
rev-parse: add --filename-prefix option
submodule: drop the top-level requirementbuiltin/rev-parse.c | 17 ++++++++++++----- git-submodule.sh | 7 +++++++ 2 files changed, 19 insertions(+), 5 deletions(-)
PATCH 1/2 rev-parse: add --filename-prefix option
In reply to John KeepingThis adds a prefix string to any filename arguments encountered after it has been specified.
--- builtin/rev-parse.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
builtin/rev-parse.c +12 -5
@@ -212,11 +212,12 @@ 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,const char *prefix) {show_default ();if ((filter & (DO_NONFLAGS |DO_NOREV )) == (DO_NONFLAGS |DO_NOREV )) { -show (arg); +size_t prefixlen = prefix ?strlen (prefix) :0 ; +show (prefix_filename (prefix, prefixlen, arg));return 1 ; }return 0 ;
@@ -470,6 +471,7 @@ 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 ; +const char *filename_prefix =NULL ;unsigned char sha1[20 ];const char *name =NULL ;
@@ -503,7 +505,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) && as_is <2 ) +if (show_file (arg, filename_prefix) && as_is <2 )verify_filename (prefix, arg,0 );continue ; }
@@ -527,7 +529,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); +show_file (arg,NULL );continue ; }if (!strcmp (arg,"--default" )) {
@@ -535,6 +537,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) i++;continue ; } +if (!strcmp (arg,"--filename-prefix" )) { + filename_prefix = argv[i+1 ]; + i++; +continue ; + }if (!strcmp (arg,"--revs-only" )) { filter &= ~DO_NOREV ;continue ;
@@ -754,7 +761,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)) +if (!show_file (arg, filename_prefix))continue ;verify_filename (prefix, arg,1 ); }
Signature · 1 line
1.8.2.694.ga76e9c3.dirty
Re: [PATCH 1/2] rev-parse: add --filename-prefix option
Hi,
Quoted 2 lines · John Keeping
This adds a prefix string to any filename arguments encountered after it has been specified.
I assume this is a way of passing the prefix in? In that case, I think a good UI would be
git rev-parse --prefix=Documentation/ <usual rev-parse args>
That sounds like a useful thing and would make the meaning very clear.
How does this interact with the following options?
* --resolve-git-dir some/relative/path
* master:./path
As for the patch itself, I haven't looked at it closely. My only immediate reaction is that I wish it touched Documentation/ and t/. :)
Hope that helps,
Jonathan
Re: [PATCH 1/2] rev-parse: add --filename-prefix option
Quoted 11 lines · Jonathan Nieder
Quoted 2 lines · John Keeping
This adds a prefix string to any filename arguments encountered after it has been specified.
I assume this is a way of passing the prefix in? In that case, I think a good UI would be
git rev-parse --prefix=Documentation/ <usual rev-parse args>That sounds like a useful thing and would make the meaning very clear.
Yes (ish), the intended usage is something like this:
prefix=$(git rev-parse --show-prefix)
cd_to_toplevel
... parse options here ...
# Convert remaining arguments (filenames) into top-level paths:
eval "set $(git rev-parse --prefix "$prefix" --sq -- "$@")"
The "ish" is that my current implementation introduced a new variable instead of simply resetting the existing "prefix" variable, which I assume is what you mean. That is probably simpler than my implementation, but loses the ability to be at an intermediate level, for example:
cd Documentation/
eval "set $(git rev-parse --prefix technical/ --sq -- api-strbuf.txt)"Quoted 3 lines
How does this interact with the following options?
* --resolve-git-dir some/relative/path
It doesn't change this since --resolve-git-dir is handled separately from the other argument parsing at the moment and you cannot specify any other options with it.
Quoted 1 line
* master:./path
I hadn't considered this case, but I think it should be inserting the prefix into the path. I suspect the easiest thing to do is simply make the path part of that absolute, by combining both the prefix based on $PWD and the prefix specified on the command line, but I haven't looked at doing this yet.
The other think that's missing at the moment is that the prefix passed to verify_filename should be modified by the one specified on the command line.
Quoted 2 lines
As for the patch itself, I haven't looked at it closely. My only immediate reaction is that I wish it touched
Documentation/and t/. :)
I'll make sure the next version does. This version was doing the minimum required to make patch 2/2 possible, it certainly needs some polish before it's more than a proof-of-concept.
Re: [PATCH 1/2] rev-parse: add --filename-prefix option
Quoted 11 lines · John Keeping
Yes (ish), the intended usage is something like this:
prefix=$(git rev-parse --show-prefix)
cd_to_toplevel
... parse options here ...
# Convert remaining arguments (filenames) into top-level paths:
eval "set $(git rev-parse --prefix "$prefix" --sq -- "$@")"The "ish" is that my current implementation introduced a new variable instead of simply resetting the existing "prefix" variable, which I assume is what you mean.
This is very sensible.
Quoted 6 lines
That is probably simpler than my
implementation, but loses the ability to be at an intermediate level, for example:cd Documentation/ eval "set $(git rev-parse --prefix technical/ --sq -- api-strbuf.txt)"
I am not sure in what situation it makes sense to do this. Where does "technical/" come from? For a script that wants to work from a subdirectory, end-user input would come in the form relative to the current directory, i.e. "Documentation/" from the top-level, so...
Re: [PATCH 1/2] rev-parse: add --filename-prefix option
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · John Keeping
Yes (ish), the intended usage is something like this:
prefix=$(git rev-parse --show-prefix)
cd_to_toplevel
... parse options here ...
# Convert remaining arguments (filenames) into top-level paths:
eval "set $(git rev-parse --prefix "$prefix" --sq -- "$@")"
Quoted 5 lines
Quoted 3 lines
The "ish" is that my current implementation introduced a new variable instead of simply resetting the existing "prefix" variable, which I assume is what you mean.
This is very sensible.
Which bit specifically? I assume you agree with the intended usage, but do you also mean that resetting the prefix returned from setup_git_directory is the right way to approach this?
Re: [PATCH 1/2] rev-parse: add --filename-prefix option
Quoted 15 lines · John Keeping
Quoted 14 lines · Junio C Hamano
Quoted 11 lines · John Keeping
Yes (ish), the intended usage is something like this:
prefix=$(git rev-parse --show-prefix)
cd_to_toplevel
... parse options here ...
# Convert remaining arguments (filenames) into top-level paths:
eval "set $(git rev-parse --prefix "$prefix" --sq -- "$@")"The "ish" is that my current implementation introduced a new variable instead of simply resetting the existing "prefix" variable, which I assume is what you mean.
Quoted 5 lines
Quoted 1 line
This is very sensible.
Which bit specifically? I assume you agree with the intended usage, but do you also mean that resetting the prefix returned from
setup_git_directoryis the right way to approach this?
My gut feeling says yes, but you can persuade me easily why it is a bad idea if you have an example of why it would not work well.
PATCH 2/2 submodule: drop the top-level requirement
In reply to John KeepingUse the new rev-parse --filename-prefix option to process all paths given to the submodule command, dropping the requirement that it be run from the top-level of the repository.
--- git-submodule.sh | 7 +++++++ 1 file changed, 7 insertions(+)
git-submodule.sh +7 -0
@@ -14,10 +14,13 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <re or: $dashless [--quiet] foreach [--recursive] <command> or: $dashless [--quiet] sync [--recursive] [--] [<path>...]" OPTIONS_SPEC= +SUBDIRECTORY_OK=Yes . git-sh-setup . git-sh-i18n . git-parse-remote require_work_tree +wt_prefix=$(git rev-parse --show-prefix) +cd_to_toplevel command= branch=
@@ -112,6 +115,7 @@ resolve_relative_url () # module_list() { + eval "set $(git rev-parse --sq --filename-prefix "$wt_prefix" -- "$@")" ( git ls-files --error-unmatch --stage -- "$@" || echo "unmatched pathspec exists"
@@ -335,6 +339,8 @@ cmd_add() usage fi + sm_path="$wt_prefix$sm_path" + # assure repo is absolute or relative to parent case "$repo" in ./*|../*)
@@ -942,6 +948,7 @@ cmd_summary() { fi cd_to_toplevel + eval "set $(git rev-parse --sq --filename-prefix "$wt_prefix" -- "$@")" # Get modified modules cared by user modules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- "$@" | sane_egrep '^:([0-7]* )?160000' |
Signature · 1 line
1.8.2.694.ga76e9c3.dirty
v2 cover submodule: drop the top-level requirement
In reply to John KeepingSince version 1, patch 1 has been completely re-written using the approach proposed by Jonathan and Junio.
Also, there's now a documentation update and some tests.
John Keeping (2):
rev-parse: add --filename-prefix option
submodule: drop the top-level requirement
Documentation/git-rev-parse.txt | 16 ++++++++ builtin/rev-parse.c | 24 ++++++++--- git-submodule.sh | 7 ++++ t/t1513-rev-parse-prefix.sh | 90 +++++++++++++++++++++++++++++++++++++++++ t/t7400-submodule-basic.sh | 26 ++++++++++++ t/t7401-submodule-summary.sh | 9 +++++ 6 files changed, 167 insertions(+), 5 deletions(-) create mode 100755 t/t1513-rev-parse-prefix.sh
Signature · 1 line
1.8.2.694.ga76e9c3.dirty
PATCH v2 1/2 rev-parse: add --filename-prefix option
This adds a prefix string to any filename arguments encountered after it has been specified.
--- Documentation/git-rev-parse.txt | 16 ++++++++ builtin/rev-parse.c | 24 ++++++++--- t/t1513-rev-parse-prefix.sh | 90 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 5 deletions(-) create mode 100755 t/t1513-rev-parse-prefix.sh
Documentation/git-rev-parse.txt +16 -0
@@ -59,6 +59,22 @@ OPTIONS If there is no parameter given by the user, use `<arg>` instead. +--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" "$@")" +---- + --verify:: Verify that exactly one parameter is provided, and that it can be turned into a raw 20-byte SHA-1 that can be used to
builtin/rev-parse.c +19 -5
@@ -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) {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);return 1 ; }return 0 ;
@@ -470,6 +476,7 @@ 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 ;
@@ -503,7 +510,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) && as_is <2 ) +if (show_file (arg, output_prefix) && as_is <2 )verify_filename (prefix, arg,0 );continue ; }
@@ -527,7 +534,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); +show_file (arg,0 );continue ; }if (!strcmp (arg,"--default" )) {
@@ -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 ; + }if (!strcmp (arg,"--revs-only" )) { filter &= ~DO_NOREV ;continue ;
@@ -754,7 +768,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)) +if (!show_file (arg, output_prefix))continue ;verify_filename (prefix, arg,1 ); }
t/t1513-rev-parse-prefix.sh new file +90 -0
90 lines not on this page · raw message
Signature · 1 line
1.8.2.694.ga76e9c3.dirty
Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
Quoted 5 lines · John Keeping
This adds a prefix string to any filename arguments encountered after it has been specified.
Signed-off-by John Keeping <[email protected]>---
Stale subject?
Quoted 4 lines
+--prefix <arg>::
Documentation/git-rev-parse.txt +3
-0+ 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
Documentation/git-rev-parse.txt OPTIONS · line 75
Quoted 10 lines
Documentation/git-rev-parse.txt +10
-0+ 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 "$@"?
Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
Quoted 9 lines · Junio C Hamano
Quoted 5 lines · John Keeping
This adds a prefix string to any filename arguments encountered after it has been specified.
Signed-off-by John Keeping <[email protected]>---
Stale subject?
Yep. Sorry.
Quoted 19 lines
Quoted 4 lines
+--prefix <arg>::
Documentation/git-rev-parse.txt +3
-0+ 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 printedDocumentation/git-rev-parse.txt OPTIONS · line 75
Quoted 10 lines
Documentation/git-rev-parse.txt +10
-0+ 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
Quoted 5 lines
git mylog -U20 master..next -- README
inside
Documentation/directory, "git-mylog" script would want to see README prefixed withDocumentation/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
Quoted 17 lines
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 ofgit 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:
Quoted 7 lines
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.
Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
Quoted 2 lines · John Keeping
It's not guessing on all of "$@" in git-submodule - we know that everything left is a path.
OK, then.
Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
In reply to John KeepingQuoted 2 lines · John Keeping
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?
builtin/rev-parse.c static void show_datestring(const char *flag, const char *datestr) · line 215
Quoted 10 lines
builtin/rev-parse.c +1
-1@@ -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.
builtin/rev-parse.c static void show_datestring(const char *flag, const char *datestr) · line 225
Quoted 11 lines
builtin/rev-parse.c +7
-1{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()?
builtin/rev-parse.c int cmd_rev_parse(int argc, const char **argv, const char *prefix) · line 551
Quoted 13 lines
builtin/rev-parse.c +7
-0@@ -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?
builtin/rev-parse.c int cmd_rev_parse(int argc, const char **argv, const char *prefix) · line 554
Quoted 3 lines
builtin/rev-parse.c +3
-0+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.
t/t1513-rev-parse-prefix.sh line 18
Quoted 8 lines
t/t1513-rev-parse-prefix.sh new file +3
-0+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.
t/t1513-rev-parse-prefix.sh line 48
Quoted 3 lines
t/t1513-rev-parse-prefix.sh new file +3
-0+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?
Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
Quoted 9 lines · Ramkumar Ramachandra
Quoted 2 lines · John Keeping
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 29 lines
builtin/rev-parse.c static void show_datestring(const char *flag, const char *datestr) · line 215
Quoted 10 lines
builtin/rev-parse.c +1
-1@@ -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 supposeshow_(rev|reference|default|flag|rev|with_type|datestring|abbrev)don't need to be patched, as they are path-independent.builtin/rev-parse.c static void show_datestring(const char *flag, const char *datestr) · line 225
Quoted 11 lines
builtin/rev-parse.c +7
-1{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);
Quoted 2 lines
Uh, why do you need
output_prefix? Ifstartup_info->prefixis set, use it. Isstartup_info->prefixset by anyone bycmd_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 11 lines
Quoted 11 lines
builtin/rev-parse.c +5
-0@@ -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++;
Quoted 8 lines
builtin/rev-parse.c int cmd_rev_parse(int argc, const char **argv, const char *prefix) · line 551
Quoted 2 lines
builtin/rev-parse.c +2
-0+continue ; + }Wait, why isn't prefix filled in when
run_builtin()calls this? Oh,
right: because we didn't mark this builtin withRUN_SETUPorRUN_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.
Quoted 4 lines
Why are you setting prefix to argv[i+1], and then setting
startup_info->prefixto that? Is anyone else incmd_rev_parse()going to use it?
Quoted 10 lines
builtin/rev-parse.c int cmd_rev_parse(int argc, const char **argv, const char *prefix) · line 554
Quoted 3 lines
builtin/rev-parse.c +3
-0+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 11 lines
t/t1513-rev-parse-prefix.sh line 18
Quoted 8 lines
t/t1513-rev-parse-prefix.sh new file +3
-0+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 6 lines
t/t1513-rev-parse-prefix.sh line 48
Quoted 3 lines
t/t1513-rev-parse-prefix.sh new file +3
-0+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.
PATCH v2 2/2 submodule: drop the top-level requirement
In reply to John KeepingUse the new rev-parse --prefix option to process all paths given to the submodule command, dropping the requirement that it be run from the top-level of the repository.
--- git-submodule.sh | 7 +++++++ t/t7400-submodule-basic.sh | 26 ++++++++++++++++++++++++++ t/t7401-submodule-summary.sh | 9 +++++++++ 3 files changed, 42 insertions(+)
git-submodule.sh +7 -0
@@ -14,10 +14,13 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <re or: $dashless [--quiet] foreach [--recursive] <command> or: $dashless [--quiet] sync [--recursive] [--] [<path>...]" OPTIONS_SPEC= +SUBDIRECTORY_OK=Yes . git-sh-setup . git-sh-i18n . git-parse-remote require_work_tree +wt_prefix=$(git rev-parse --show-prefix) +cd_to_toplevel command= branch=
@@ -112,6 +115,7 @@ resolve_relative_url () # module_list() { + eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")" ( git ls-files --error-unmatch --stage -- "$@" || echo "unmatched pathspec exists"
@@ -335,6 +339,8 @@ cmd_add() usage fi + sm_path="$wt_prefix$sm_path" + # assure repo is absolute or relative to parent case "$repo" in ./*|../*)
@@ -942,6 +948,7 @@ cmd_summary() { fi cd_to_toplevel + eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")" # Get modified modules cared by user modules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- "$@" | sane_egrep '^:([0-7]* )?160000' |
t/t7400-submodule-basic.sh +26 -0
@@ -212,6 +212,23 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' test_cmp empty untracked ' +test_expect_success 'submodule add in subdir' ' + echo "refs/heads/master" >expect && + >empty && + ( + mkdir addtest/sub && + cd addtest/sub && + git submodule add "$submodurl" ../realsubmod3 && + git submodule init + ) && + + rm -f heads head untracked && + inspect addtest/realsubmod3 ../.. && + test_cmp expect heads && + test_cmp expect head && + test_cmp empty untracked +' + test_expect_success 'setup - add an example entry to .gitmodules' ' GIT_CONFIG=.gitmodules \ git config submodule.example.url git://example.com/init.git
@@ -319,6 +336,15 @@ test_expect_success 'status should be "up-to-date" after update' ' grep "^ $rev1" list ' +test_expect_success 'status works correctly from a subdirectory' ' + mkdir sub && + ( + cd sub && + git submodule status >../list + ) && + grep "^ $rev1" list +' + test_expect_success 'status should be "modified" after submodule commit' ' ( cd init &&
t/t7401-submodule-summary.sh +9 -0
@@ -45,6 +45,15 @@ EOF test_cmp expected actual " +test_expect_success 'run summary from subdir' ' + mkdir sub && + ( + cd sub && + git submodule summary >../actual + ) && + test_cmp expected actual +' + commit_file sm1 && head2=$(add_file sm1 foo3)
Signature · 1 line
1.8.2.694.ga76e9c3.dirty
Review
Quoted 1 line · John Keeping
(quoted diff) +1
-0+ eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
This may be handier than having to do the "for arg" loop git-am uses yourself.
git-submodule.sh cmd_add() · line 342
Quoted 8 lines
git-submodule.sh +1
-0( git ls-files --error-unmatch --stage -- "$@" || echo "unmatched pathspec exists"@@ -335,6 +339,8 @@ cmd_add() usage fi + sm_path="$wt_prefix$sm_path"
But this is doing fine without "rev-parse --prefix" at all.
Quoted 14 lines · Junio C Hamano
Review
Quoted 1 line · John Keeping
(quoted diff) +1
-0+ eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"This may be handier than having to do the "for arg" loop git-am uses yourself.
Quoted 7 lines
git-submodule.sh +0
-0( git ls-files --error-unmatch --stage -- "$@" || echo "unmatched pathspec exists"@@ -335,6 +339,8 @@ cmd_add() usage fi
Quoted 3 lines
git-submodule.sh cmd_add() · line 342
Quoted 1 line
git-submodule.sh +1
-0+ sm_path="$wt_prefix$sm_path"But this is doing fine without "rev-parse --prefix" at all.
In this case we only have a single argument (and it must have a value). In the cases using "rev-parse --prefix" we can have any number of arguments (including zero).
Quoted 3 lines · John Keeping
Use the new rev-parse --prefix option to process all paths given to the submodule command, dropping the requirement that it be run from the top-level of the repository.
Yay!
git-submodule.sh resolve_relative_url () · line 118
Quoted 9 lines
git-submodule.sh +1
-0@@ -112,6 +115,7 @@ resolve_relative_url () # module_list() { + eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
Nit: Why not use "--" to disambiguate between options and arguments, like you showed in your intended usage?
So, this essentially converts all the paths specified in "$@" to toplevel-relative paths. Beautiful, as this is practically the only change you require in each function.
git-submodule.sh cmd_add() · line 342
Quoted 1 line
git-submodule.sh +1
-0+ sm_path="$wt_prefix$sm_path"
Wait, isn't sm_path the value of submodule.<name>.path in .gitmodules?
Why does it need to be prefixed?
git-submodule.sh cmd_summary() { · line 951
Quoted 5 lines
git-submodule.sh +1
-0@@ -942,6 +948,7 @@ cmd_summary() { fi cd_to_toplevel + eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
Um, what about other functions? Yeah, it looks like all of them except this one call module_list().
t/t7400-submodule-basic.sh test_expect_success 'submodule add with ./, /.. and // in path' ' · line 217
Quoted 11 lines
t/t7400-submodule-basic.sh +3
-0@@ -212,6 +212,23 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' test_cmp empty untracked ' +test_expect_success 'submodule add in subdir' ' + echo "refs/heads/master" >expect && + >empty &&
Nit: Use : >empty. It's clearer.
t/t7400-submodule-basic.sh test_expect_success 'submodule add with ./, /.. and // in path' ' · line 219
Quoted 2 lines
t/t7400-submodule-basic.sh +2
-0+ ( + mkdir addtest/sub &&
Why is the mkdir inside the subshell? The next statement (cd) onwards should be in the subshell, to save you cd'ing back.
t/t7400-submodule-basic.sh test_expect_success 'submodule add with ./, /.. and // in path' ' · line 225
Quoted 6 lines
t/t7400-submodule-basic.sh +6
-0+ cd addtest/sub && + git submodule add "$submodurl" ../realsubmod3 && + git submodule init + ) && + + rm -f heads head untracked &&
Huh? What is this in the middle? The next statement (call to inspect) write-truncates them anyway, so this is unnecessary.
Quoted 7 lines · Ramkumar Ramachandra
Quoted 3 lines · John Keeping
Use the new rev-parse --prefix option to process all paths given to the submodule command, dropping the requirement that it be run from the top-level of the repository.
Yay!
Quoted 12 lines
git-submodule.sh resolve_relative_url () · line 118
Quoted 9 lines
git-submodule.sh +1
-0@@ -112,6 +115,7 @@ resolve_relative_url () # module_list() { + eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"Nit: Why not use "--" to disambiguate between options and arguments, like you showed in your intended usage?
Erm... it does. What's before "$@"?
Ah, you mean after "set". In this case, rev-parse will keep the "--" we supply to it, so we get that from there and do not want to give set an extra one.
Quoted 8 lines
So, this essentially converts all the paths specified in "$@" to toplevel-relative paths. Beautiful, as this is practically the only change you require in each function.
git-submodule.sh cmd_add() · line 342
Quoted 1 line
git-submodule.sh +1
-0+ sm_path="$wt_prefix$sm_path"Wait, isn't
sm_paththe value of submodule.<name>.path in .gitmodules?
Why does it need to be prefixed?
I think you've lost too much context here. This is in cmd_add and at this point sm_path holds the argument supplied by the user, which we must adjust as it will be relative to the current directory.
We should probably be testing for an absolute path here though.
Quoted 8 lines
git-submodule.sh cmd_summary() { · line 951
Quoted 5 lines
git-submodule.sh +1
-0@@ -942,6 +948,7 @@ cmd_summary() { fi cd_to_toplevel + eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"Um, what about other functions? Yeah, it looks like all of them except this one call
module_list().
Exactly. Apart from this and cmd_add everything uses module_list.
Quoted 27 lines
t/t7400-submodule-basic.sh test_expect_success 'submodule add with ./, /.. and // in path' ' · line 217
Quoted 11 lines
t/t7400-submodule-basic.sh +3
-0@@ -212,6 +212,23 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' test_cmp empty untracked ' +test_expect_success 'submodule add in subdir' ' + echo "refs/heads/master" >expect && + >empty &&Nit: Use : >empty. It's clearer.
t/t7400-submodule-basic.sh test_expect_success 'submodule add with ./, /.. and // in path' ' · line 219
Quoted 2 lines
t/t7400-submodule-basic.sh +2
-0+ ( + mkdir addtest/sub &&Why is the mkdir inside the subshell? The next statement (cd) onwards should be in the subshell, to save you cd'ing back.
t/t7400-submodule-basic.sh test_expect_success 'submodule add with ./, /.. and // in path' ' · line 225
Quoted 6 lines
t/t7400-submodule-basic.sh +6
-0+ cd addtest/sub && + git submodule add "$submodurl" ../realsubmod3 && + git submodule init + ) && + + rm -f heads head untracked &&
Quoted 2 lines
Huh? What is this in the middle? The next statement (call to inspect) write-truncates them anyway, so this is unnecessary.
I just followed the surrounding tests here. I think it's better for follow the pattern of the surrounding code here than get this one test perfect. A cleanup can follow on top if someone wants to do it.
v3 cover submodule: drop the top-level requirement
In reply to John KeepingThanks to Ram and Jens for the feedback on v2. I've addressed most of their comments in this version, but I've ignored a couple of Ram's nits about test cases where leaving it how it is makes the added tests consistent with those already in the same file.
Since v2, the main improvement is that the output from git-submodule now contains paths relative to the directory in which the command is run, not the top-level of the working tree.
John Keeping (2):
rev-parse: add --prefix option
submodule: drop the top-level requirement
Documentation/git-rev-parse.txt | 16 ++++++ builtin/rev-parse.c | 24 +++++++-- git-submodule.sh | 113 ++++++++++++++++++++++++++++++---------- t/t1513-rev-parse-prefix.sh | 96 ++++++++++++++++++++++++++++++++++ t/t7400-submodule-basic.sh | 27 ++++++++++ t/t7401-submodule-summary.sh | 14 +++++ 6 files changed, 258 insertions(+), 32 deletions(-) create mode 100755 t/t1513-rev-parse-prefix.sh
Signature · 1 line
1.8.2.1.424.g1899e5e.dirty
PATCH v3 1/2 rev-parse: add --prefix option
This makes 'git rev-parse' behave as if it were invoked from the specified subdirectory of a repository, with the difference that any file paths which it prints are prefixed with the full path from the top of the working tree.
This is useful for shell scripts where we may want to cd to the top of the working tree but need to handle relative paths given by the user on the command line.
---
Changes since v2:
- Rewrite commit message
- Add a new test for 'prefix ignored with HEAD:top'
- Use '<<-\EOF' where appropriate in t1513
Documentation/git-rev-parse.txt | 16 +++++++ builtin/rev-parse.c | 24 ++++++++--- t/t1513-rev-parse-prefix.sh | 96 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 5 deletions(-) create mode 100755 t/t1513-rev-parse-prefix.sh
Documentation/git-rev-parse.txt +16 -0
@@ -59,6 +59,22 @@ OPTIONS If there is no parameter given by the user, use `<arg>` instead. +--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" "$@")" +---- + --verify:: Verify that exactly one parameter is provided, and that it can be turned into a raw 20-byte SHA-1 that can be used to
builtin/rev-parse.c +19 -5
@@ -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) {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);return 1 ; }return 0 ;
@@ -470,6 +476,7 @@ 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 ;
@@ -503,7 +510,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) && as_is <2 ) +if (show_file (arg, output_prefix) && as_is <2 )verify_filename (prefix, arg,0 );continue ; }
@@ -527,7 +534,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); +show_file (arg,0 );continue ; }if (!strcmp (arg,"--default" )) {
@@ -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 ; + }if (!strcmp (arg,"--revs-only" )) { filter &= ~DO_NOREV ;continue ;
@@ -754,7 +768,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)) +if (!show_file (arg, output_prefix))continue ;verify_filename (prefix, arg,1 ); }
t/t1513-rev-parse-prefix.sh new file +96 -0
96 lines not on this page · raw message
Signature · 1 line
1.8.2.1.424.g1899e5e.dirty
Re: [PATCH v3 1/2] rev-parse: add --prefix option
Quoted 4 lines · John Keeping
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?
-- 8< --
builtin/rev-parse.c +17 -9
@@ -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 ); }
Re: [PATCH v3 1/2] rev-parse: add --prefix option
Quoted 17 lines · Ramkumar Ramachandra
Quoted 4 lines · John Keeping
Changes since v2:
- Rewrite commit message
- Add a new test for 'prefix ignored with HEAD:top'
- Use '<<-\EOF' where appropriate in t1513Thanks 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_SETUPand letrun_builtin_command()take care of the prefix-setting. Unfortunately, as 5410a02 (git-rev-parse --parseopt, 2007-11-06) indicates, we have to runsetup_git_directory()incmd_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.
Quoted 5 lines
Other than that, I just have one small suggestion: it's possible to avoid passing
output_prefixaround, and simplifyshow_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.]
Quoted 93 lines
Signed-off-by Ramkumar Ramachandra <[email protected]>-- 8< --
builtin/rev-parse.c +17
-9@@ -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 ); }
Re: [PATCH v3 1/2] rev-parse: add --prefix option
Quoted 8 lines · John Keeping
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_prefixaround, 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 "--".
Yeah, it would probably make more sense to extract the conditional. I just thought it was unnecessary to pass the argument around, but feel free to go either way on this one.
Quoted 2 lines
[Also, you introduce a potential segfault via passing a NULL prefix to strlen.]
Isn't prefix guaranteed to be set by setup_git_directory()? If I wanted to check it nevertheless, I'd probably put in a die("internal error") before this line. Feel free to go either way though.
Re: [PATCH v3 1/2] rev-parse: add --prefix option
Quoted 14 lines · Ramkumar Ramachandra
Quoted 8 lines · John Keeping
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_prefixaround, 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 "--".
Yeah, it would probably make more sense to extract the conditional. I just thought it was unnecessary to pass the argument around, but feel free to go either way on this one.
Quoted 6 lines
Quoted 2 lines
[Also, you introduce a potential segfault via passing a NULL prefix to strlen.]
Isn't prefix guaranteed to be set by
setup_git_directory()? If I wanted to check it nevertheless, I'd probably put in adie("internal error")before this line. Feel free to go either way though.
It looks to me like setup_git_directory() returns NULL if we're at the top of the working tree so we do need that check. I suspect that's so that "!prefix" does the right thing.
Re: [PATCH v3 1/2] rev-parse: add --prefix option
Quoted 3 lines · John Keeping
It looks to me like
setup_git_directory()returns NULL if we're at the top of the working tree so we do need that check. I suspect that's so that "!prefix" does the right thing.
Ah, yes. My bad. I expected it to return "/" on the toplevel directory actually.
PATCH v3 2/2 submodule: drop the top-level requirement
In reply to John KeepingUse the new rev-parse --prefix option to process all paths given to the submodule command, dropping the requirement that it be run from the top-level of the repository.
---
Changes since v2:
- Handle relative paths given to "submodule add --reference=./..."
- Check for absolute path before prepending $wt_prefix in cmd_add
- Export relative sm_path in cmd_foreach
- Change displayed sm_path to be relative
- Move a mkdir out of a subshell in t7400
I'm not sure if the relative_path function here is sufficient on Windows. We should be okay with the "target" argument since that comes from git-ls-files but I think we're in trouble if "git rev-parse --show-prefix" output the prefix with '\' instead of '/'.
git-submodule.sh | 113 ++++++++++++++++++++++++++++++++----------- t/t7400-submodule-basic.sh | 27 +++++++++++ t/t7401-submodule-summary.sh | 14 ++++++ 3 files changed, 127 insertions(+), 27 deletions(-)
git-submodule.sh +86 -27
@@ -14,10 +14,13 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <re or: $dashless [--quiet] foreach [--recursive] <command> or: $dashless [--quiet] sync [--recursive] [--] [<path>...]" OPTIONS_SPEC= +SUBDIRECTORY_OK=Yes . git-sh-setup . git-sh-i18n . git-parse-remote require_work_tree +wt_prefix=$(git rev-parse --show-prefix) +cd_to_toplevel command= branch=
@@ -106,12 +109,48 @@ resolve_relative_url () echo "${is_relative:+${up_path}}${remoteurl#./}" } +# Resolve a path to be relative to another path. This is intended for +# converting submodule paths when git-submodule is run in a subdirectory +# and only handles paths where the directory separator is '/'. +# +# The output is the first argument as a path relative to the second argument, +# which defaults to $wt_prefix if it is omitted. +relative_path () +{ + local target curdir result + target=$1 + curdir=${2-$wt_prefix} + curdir=${curdir%/} + result= + + while test -n "$curdir" + do + case "$target" in + "$curdir/"*) + target=${target#$curdir/} + break + ;; + esac + + result="${result}../" + if test "$curdir" = "${curdir%/*}" + then + curdir= + else + curdir="${curdir%/*}" + fi + done + + echo "$result$target" +} + # # Get submodule info for registered submodules # $@ = path to limit submodule list # module_list() { + eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")" ( git ls-files --error-unmatch --stage -- "$@" || echo "unmatched pathspec exists"
@@ -282,6 +321,7 @@ isnumber() cmd_add() { # parse $args after "submodule ... add". + reference_path= while test $# -ne 0 do case "$1" in
@@ -298,11 +338,11 @@ cmd_add() ;; --reference) case "$2" in '') usage ;; esac - reference="--reference=$2" + reference_path=$2 shift ;; --reference=*) - reference="$1" + reference_path="${1#--reference=}" ;; --name) case "$2" in '') usage ;; esac
@@ -323,6 +363,14 @@ cmd_add() shift done + if test -n "$reference_path" + then + is_absolute_path "$reference_path" || + reference_path="$wt_prefix$reference_path" + + reference="--reference=$reference_path" + fi + repo=$1 sm_path=$2
@@ -335,6 +383,8 @@ cmd_add() usage fi + is_absolute_path "$sm_path" || sm_path="$wt_prefix$sm_path" + # assure repo is absolute or relative to parent case "$repo" in ./*|../*)
@@ -479,6 +529,7 @@ cmd_foreach() # we make $path available to scripts ... path=$sm_path cd "$sm_path" && + sm_path=$(relative_path "$sm_path") && eval "$@" && if test -n "$recursive" then
@@ -594,27 +645,29 @@ cmd_deinit() die_if_unmatched "$mode" name=$(module_name "$sm_path") || exit + displaypath=$(relative_path "$sm_path") + # Remove the submodule work tree (unless the user already did it) if test -d "$sm_path" then # Protect submodules containing a .git directory if test -d "$sm_path/.git" then - echo >&2 "$(eval_gettext "Submodule work tree '\$sm_path' contains a .git directory")" + echo >&2 "$(eval_gettext "Submodule work tree '\$displaypath' contains a .git directory")" die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")" fi if test -z "$force" then git rm -qn "$sm_path" || - die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")" + die "$(eval_gettext "Submodule work tree '\$displaypath' contains local modifications; use '-f' to discard them")" fi rm -rf "$sm_path" && - say "$(eval_gettext "Cleared directory '\$sm_path'")" || - say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")" + say "$(eval_gettext "Cleared directory '\$displaypath'")" || + say "$(eval_gettext "Could not remove submodule work tree '\$displaypath'")" fi - mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")" + mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$displaypath'")" # Remove the .git/config entries (unless the user already did it) if test -n "$(git config --get-regexp submodule."$name\.")"
@@ -623,7 +676,7 @@ cmd_deinit() # the user later decides to init this submodule again url=$(git config submodule."$name".url) git config --remove-section submodule."$name" 2>/dev/null && - say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$sm_path'")" + say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")" fi done }
@@ -717,9 +770,11 @@ cmd_update() update_module=$(git config submodule."$name".update) fi + displaypath=$(relative_path "$prefix$sm_path") + if test "$update_module" = "none" then - echo "Skipping submodule '$prefix$sm_path'" + echo "Skipping submodule '$displaypath'" continue fi
@@ -728,7 +783,7 @@ cmd_update() # Only mention uninitialized submodules when its # path have been specified test "$#" != "0" && - say "$(eval_gettext "Submodule path '\$prefix\$sm_path' not initialized + say "$(eval_gettext "Submodule path '\$displaypath' not initialized Maybe you want to use 'update --init'?")" continue fi
@@ -741,7 +796,7 @@ Maybe you want to use 'update --init'?")" else subsha1=$(clear_local_git_env; cd "$sm_path" && git rev-parse --verify HEAD) || - die "$(eval_gettext "Unable to find current revision in submodule path '\$prefix\$sm_path'")" + die "$(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")" fi if test -n "$remote"
@@ -774,7 +829,7 @@ Maybe you want to use 'update --init'?")" (clear_local_git_env; cd "$sm_path" && ( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) && test -z "$rev") || git-fetch)) || - die "$(eval_gettext "Unable to fetch in submodule path '\$prefix\$sm_path'")" + die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")" fi # Is this something we just cloned?
@@ -788,20 +843,20 @@ Maybe you want to use 'update --init'?")" case "$update_module" in rebase) command="git rebase" - die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path '\$prefix\$sm_path'")" - say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': rebased into '\$sha1'")" + die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path '\$displaypath'")" + say_msg="$(eval_gettext "Submodule path '\$displaypath': rebased into '\$sha1'")" must_die_on_failure=yes ;; merge) command="git merge" - die_msg="$(eval_gettext "Unable to merge '\$sha1' in submodule path '\$prefix\$sm_path'")" - say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': merged in '\$sha1'")" + die_msg="$(eval_gettext "Unable to merge '\$sha1' in submodule path '\$displaypath'")" + say_msg="$(eval_gettext "Submodule path '\$displaypath': merged in '\$sha1'")" must_die_on_failure=yes ;; *) command="git checkout $subforce -q" - die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$prefix\$sm_path'")" - say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': checked out '\$sha1'")" + die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$displaypath'")" + say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out '\$sha1'")" ;; esac
@@ -828,7 +883,7 @@ Maybe you want to use 'update --init'?")" res=$? if test $res -gt 0 then - die_msg="$(eval_gettext "Failed to recurse into submodule path '\$prefix\$sm_path'")" + die_msg="$(eval_gettext "Failed to recurse into submodule path '\$displaypath'")" if test $res -eq 1 then err="${err};$die_msg"
@@ -942,6 +997,7 @@ cmd_summary() { fi cd_to_toplevel + eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")" # Get modified modules cared by user modules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- "$@" | sane_egrep '^:([0-7]* )?160000' |
@@ -991,16 +1047,18 @@ cmd_summary() { ! GIT_DIR="$name/.git" git-rev-parse -q --verify $sha1_dst^0 >/dev/null && missing_dst=t + display_name=$(relative_path "$name") + total_commits= case "$missing_src,$missing_dst" in t,) - errmsg="$(eval_gettext " Warn: \$name doesn't contain commit \$sha1_src")" + errmsg="$(eval_gettext " Warn: \$display_name doesn't contain commit \$sha1_src")" ;; ,t) - errmsg="$(eval_gettext " Warn: \$name doesn't contain commit \$sha1_dst")" + errmsg="$(eval_gettext " Warn: \$display_name doesn't contain commit \$sha1_dst")" ;; t,t) - errmsg="$(eval_gettext " Warn: \$name doesn't contain commits \$sha1_src and \$sha1_dst")" + errmsg="$(eval_gettext " Warn: \$display_name doesn't contain commits \$sha1_src and \$sha1_dst")" ;; *) errmsg=
@@ -1029,12 +1087,12 @@ cmd_summary() { submodule="$(gettext "submodule")" if test $mod_dst = 160000 then - echo "* $name $sha1_abbr_src($blob)->$sha1_abbr_dst($submodule)$total_commits:" + echo "* $display_name $sha1_abbr_src($blob)->$sha1_abbr_dst($submodule)$total_commits:" else - echo "* $name $sha1_abbr_src($submodule)->$sha1_abbr_dst($blob)$total_commits:" + echo "* $display_name $sha1_abbr_src($submodule)->$sha1_abbr_dst($blob)$total_commits:" fi else - echo "* $name $sha1_abbr_src...$sha1_abbr_dst$total_commits:" + echo "* $display_name $sha1_abbr_src...$sha1_abbr_dst$total_commits:" fi if test -n "$errmsg" then
@@ -1118,7 +1176,7 @@ cmd_status() die_if_unmatched "$mode" name=$(module_name "$sm_path") || exit url=$(git config submodule."$name".url) - displaypath="$prefix$sm_path" + displaypath=$(relative_path "$prefix$sm_path") if test "$stage" = U then say "U$sha1 $displaypath"
@@ -1213,7 +1271,8 @@ cmd_sync() if git config "submodule.$name.url" >/dev/null 2>/dev/null then - say "$(eval_gettext "Synchronizing submodule url for '\$prefix\$sm_path'")" + displaypath=$(relative_path "$prefix$sm_path") + say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")" git config submodule."$name".url "$super_config_url" if test -e "$sm_path"/.git
t/t7400-submodule-basic.sh +27 -0
@@ -212,6 +212,24 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' test_cmp empty untracked ' +test_expect_success 'submodule add in subdir' ' + echo "refs/heads/master" >expect && + >empty && + + mkdir addtest/sub && + ( + cd addtest/sub && + git submodule add "$submodurl" ../realsubmod3 && + git submodule init + ) && + + rm -f heads head untracked && + inspect addtest/realsubmod3 ../.. && + test_cmp expect heads && + test_cmp expect head && + test_cmp empty untracked +' + test_expect_success 'setup - add an example entry to .gitmodules' ' GIT_CONFIG=.gitmodules \ git config submodule.example.url git://example.com/init.git
@@ -319,6 +337,15 @@ test_expect_success 'status should be "up-to-date" after update' ' grep "^ $rev1" list ' +test_expect_success 'status works correctly from a subdirectory' ' + mkdir sub && + ( + cd sub && + git submodule status >../list + ) && + grep "^ $rev1" list +' + test_expect_success 'status should be "modified" after submodule commit' ' ( cd init &&
t/t7401-submodule-summary.sh +14 -0
@@ -45,6 +45,20 @@ EOF test_cmp expected actual " +test_expect_success 'run summary from subdir' ' + mkdir sub && + ( + cd sub && + git submodule summary >../actual + ) && + cat >expected <<-EOF && +* ../sm1 0000000...$head1 (2): + > Add foo2 + +EOF + test_cmp expected actual +' + commit_file sm1 && head2=$(add_file sm1 foo3)
Signature · 1 line
1.8.2.1.424.g1899e5e.dirty
Quoted 6 lines · John Keeping
git-submodule.sh +6
-0+relative_path () +{ + local target curdir result + target=$1 + curdir=${2-$wt_prefix} + curdir=${curdir%/}
git-submodule.sh resolve_relative_url () · line 133
Quoted 10 lines
git-submodule.sh +10
-0+ result= + + while test -n "$curdir" + do + case "$target" in + "$curdir/"*) + target=${target#$curdir/} + break + ;; + esac
Could $curdir have glob wildcard to throw this part of the logic off? It is OK to have limitations like "you cannot have a glob characters in your path to submodule working tree" (at least until we start rewriting these in C or Perl or Python), but we need to be aware of them.
Review
Quoted 3 lines
module_list()
{(quoted diff) +1
-0+ eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
An efficient reuse of "--" ;-)
t/t7401-submodule-summary.sh EOF · line 58
Quoted 11 lines
t/t7401-submodule-summary.sh +11
-0+test_expect_success 'run summary from subdir' ' + mkdir sub && + ( + cd sub && + git submodule summary >../actual + ) && + cat >expected <<-EOF && +* ../sm1 0000000...$head1 (2): + > Add foo2 + +EOF
It somewhat looks strange to start with "<<-EOF" and then not to indent the body nor EOF.
Quoted 16 lines · Junio C Hamano
Quoted 14 lines · John Keeping
git-submodule.sh +14
-0+relative_path () +{ + local target curdir result + target=$1 + curdir=${2-$wt_prefix} + curdir=${curdir%/} + result= + + while test -n "$curdir" + do + case "$target" in + "$curdir/"*) + target=${target#$curdir/} + break
Quoted 8 lines
git-submodule.sh resolve_relative_url () · line 133
Quoted 2 lines
git-submodule.sh +2
-0+ ;; + esacCould $curdir have glob wildcard to throw this part of the logic off? It is OK to have limitations like "you cannot have a glob characters in your path to submodule working tree" (at least until we start rewriting these in C or Perl or Python), but we need to be aware of them.
I think the use of "#" instead of "##" saves us here because even with a wildcard in $curdir the case statement matches literally, so we know that "$target" starts with "$curdir/", so "${target#$curdir/}" can't match anything longer than the literal "$curdir" prefix.
Quoted 6 lines
Review
Quoted 3 lines
module_list()
{(quoted diff) +1
-0+ eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"An efficient reuse of "--" ;-)
Quoted 14 lines
t/t7401-submodule-summary.sh EOF · line 58
Quoted 11 lines
t/t7401-submodule-summary.sh +11
-0+test_expect_success 'run summary from subdir' ' + mkdir sub && + ( + cd sub && + git submodule summary >../actual + ) && + cat >expected <<-EOF && +* ../sm1 0000000...$head1 (2): + > Add foo2 + +EOFIt somewhat looks strange to start with "<<
-EOF" and then not to indent the body nor EOF.
Yes, but I copied this from the preceding test. I'd rather leave this as it is for consistency and let later style fixes change all of the tests in this file.
Quoted 26 lines · John Keeping
Quoted 16 lines · Junio C Hamano
Quoted 14 lines · John Keeping
git-submodule.sh +14
-0+relative_path () +{ + local target curdir result + target=$1 + curdir=${2-$wt_prefix} + curdir=${curdir%/} + result= + + while test -n "$curdir" + do + case "$target" in + "$curdir/"*) + target=${target#$curdir/} + breakQuoted 8 lines
git-submodule.sh resolve_relative_url () · line 133
Quoted 2 lines
git-submodule.sh +2
-0+ ;; + esacCould $curdir have glob wildcard to throw this part of the logic off? It is OK to have limitations like "you cannot have a glob characters in your path to submodule working tree" (at least until we start rewriting these in C or Perl or Python), but we need to be aware of them.
Quoted 2 lines
I think the use of "#" instead of "##" saves us here because even with a wildcard in $curdir the case statement matches literally,
If you have curdir=a*b and target=adropb/c/d/e, the chopping itself
target=${target#$curdir/}would happily chop "adropb/" from the target, but because the dq around "$curdir/"* in the case arm label enforces that target must literally match curdir followed by a slash, we do not even come to the chomping part.
I still have not convinced myself that it is impossible for somebody more clever than I to craft a pair of target and curdir that breaks it, though. (target="a*b/c/d", curdir="a*b") is correctly chopped, so that is not it.
Am 19.04.2013 18:45, schrieb Junio C Hamano:
Quoted 40 lines
Quoted 26 lines · John Keeping
Quoted 16 lines · Junio C Hamano
Quoted 14 lines · John Keeping
git-submodule.sh +14
-0+relative_path () +{ + local target curdir result + target=$1 + curdir=${2-$wt_prefix} + curdir=${curdir%/} + result= + + while test -n "$curdir" + do + case "$target" in + "$curdir/"*) + target=${target#$curdir/} + breakQuoted 8 lines
git-submodule.sh resolve_relative_url () · line 133
Quoted 2 lines
git-submodule.sh +2
-0+ ;; + esacCould $curdir have glob wildcard to throw this part of the logic off? It is OK to have limitations like "you cannot have a glob characters in your path to submodule working tree" (at least until we start rewriting these in C or Perl or Python), but we need to be aware of them.
Quoted 2 lines
I think the use of "#" instead of "##" saves us here because even with a wildcard in $curdir the case statement matches literally,
If you have curdir=a*b and target=adropb/c/d/e, the chopping itself
target=${target#$curdir/}would happily chop "adropb/" from the target, but because the dq around "$curdir/"* in the case arm label enforces that target must literally match curdir followed by a slash, we do not even come to the chomping part.
Quoted 4 lines
I still have not convinced myself that it is impossible for somebody more clever than I to craft a pair of target and curdir that breaks it, though. (target="a*b/c/d", curdir="a*b") is correctly chopped, so that is not it.
Why not just replace the six-liner by this one-liner:
target=${target#"$curdir"/}-- Hannes
Quoted 3 lines · Johannes Sixt
Why not just replace the six-liner by this one-liner:
target=${target#"$curdir"/}
Simple enough ;-)
PATCH 1/1 submodule: fix quoting in relative_path()
Commit caca2c1 (submodule: drop the top-level requirement) introduced a relative_path helper but does not quote $curdir when it is stripped from the front of a target path. In this particular case this should be safe even with special characters because we only do this after checking that $target begins with "$curdir/" which is quoted correctly, but we should quote the variable to be certain that there is not some obscure case where we this could strip more or less than we want.
---
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Johannes Sixt
Why not just replace the six-liner by this one-liner:
target=${target#"$curdir"/}Simple enough ;-)
This seems to have arrived on next without this fix, so here's a patch on top.
git-submodule.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
git-submodule.sh +1 -1
@@ -127,7 +127,7 @@ relative_path () do case "$target" in "$curdir/"*) - target=${target#$curdir/} + target=${target#"$curdir"/} break ;; esac
Signature · 1 line
1.8.2.1.715.gb260f47
Re: [PATCH] submodule: fix quoting in relative_path()
Quoted 17 lines · John Keeping
Quoted 5 lines
Quoted 3 lines
Why not just replace the six-liner by this one-liner:
target=${target#"$curdir"/}Simple enough ;-)
This seems to have arrived on next without this fix, so here's a patch on top.
git-submodule.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)git-submodule.sh +0
-0
git-submodule.sh relative_path () · line 133
Quoted 8 lines
git-submodule.sh +1
-1@@ -127,7 +127,7 @@ relative_path () do case "$target" in "$curdir/"*) - target=${target#$curdir/} + target=${target#"$curdir"/} break ;; esac
J6t meant a patch to remove the entire case...esac and replace it with a single liner (target=${target#"$curdir"/}).
Re: [PATCH] submodule: fix quoting in relative_path()
Quoted 14 lines · Junio C Hamano
Quoted 12 lines · John Keeping
Quoted 5 lines
Quoted 3 lines
Why not just replace the six-liner by this one-liner:
target=${target#"$curdir"/}Simple enough ;-)
This seems to have arrived on next without this fix, so here's a patch on top.
git-submodule.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 16 lines
git-submodule.sh relative_path () · line 133
Quoted 13 lines
git-submodule.sh +1
-1@@ -127,7 +127,7 @@ relative_path () do case "$target" in "$curdir/"*) - target=${target#$curdir/} + target=${target#"$curdir"/} break ;; esacJ6t meant a patch to remove the entire case...esac and replace it with a single liner (target=${target#"$curdir"/}).
Ah, I missed the "six-liner" part. But that doesn't work because we break out of the do...while loop when "$target" starts with "$curdir/" and replacing the case statement will remove that.
Re: [PATCH] submodule: fix quoting in relative_path()
Am 24.04.2013 18:28, schrieb John Keeping:
Quoted 7 lines · Junio C Hamano
Quoted 2 lines
J6t meant a patch to remove the entire case...esac and replace it with a single liner (target=${target#"$curdir"/}).
Ah, I missed the "six-liner" part. But that doesn't work because we break out of the do...while loop when "$target" starts with "$curdir/" and replacing the case statement will remove that.
Yeah, right, _that_ is what I missed ;-)
-- Hannes
Quoted 11 lines · John Keeping
Use the new rev-parse --prefix option to process all paths given to the submodule command, dropping the requirement that it be run from the top-level of the repository.
Signed-off-by John Keeping <[email protected]>---
t/t7400-submodule-basic.sh +0
-0
t/t7400-submodule-basic.sh test_expect_success 'submodule add with ./, /.. and // in path' ' · line 215
Quoted 4 lines
t/t7400-submodule-basic.sh +1
-0@@ -212,6 +212,24 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' test_cmp empty untracked ' +test_expect_success 'submodule add in subdir' '
A particularly minor nit. Existing subdirectory-related tests in t7400 spell out "subdirectory" fully, so perhaps for consistency:
s/subdir/subdirectory/
Quoted 21 lines
t/t7400-submodule-basic.sh +17
-0+ echo "refs/heads/master" >expect && + >empty && + + mkdir addtest/sub && + ( + cd addtest/sub && + git submodule add "$submodurl" ../realsubmod3 && + git submodule init + ) && + + rm -f heads head untracked && + inspect addtest/realsubmod3 ../.. && + test_cmp expect heads && + test_cmp expect head && + test_cmp empty untracked +' + test_expect_success 'setup - add an example entry to .gitmodules' ' GIT_CONFIG=.gitmodules \ git config submodule.example.url git://example.com/init.git
t/t7400-submodule-basic.sh test_expect_success 'status should be "up-to-date" after update' ' · line 340
Quoted 4 lines
t/t7400-submodule-basic.sh +1
-0@@ -319,6 +337,15 @@ test_expect_success 'status should be "up-to-date" after update' ' grep "^ $rev1" list ' +test_expect_success 'status works correctly from a subdirectory' '
Good: "subdirectory"
Quoted 16 lines
t/t7400-submodule-basic.sh +8
-0+ mkdir sub && + ( + cd sub && + git submodule status >../list + ) && + grep "^ $rev1" list +' + test_expect_success 'status should be "modified" after submodule commit' ' ( cd init &&t/t7401-submodule-summary.sh +0
-0
t/t7401-submodule-summary.sh EOF · line 48
Quoted 4 lines
t/t7401-submodule-summary.sh +1
-0@@ -45,6 +45,20 @@ EOF test_cmp expected actual " +test_expect_success 'run summary from subdir' '
t7401 does not have any existing subdirectory-related tests, but for consistency with t7400, perhaps: s/subdir/subdirectory/
Quoted 15 lines
t/t7401-submodule-summary.sh +13
-0+ mkdir sub && + ( + cd sub && + git submodule summary >../actual + ) && + cat >expected <<-EOF && +* ../sm1 0000000...$head1 (2): + > Add foo2 + +EOF + test_cmp expected actual +' + commit_file sm1 && head2=$(add_file sm1 foo3)