[PATCH 1/2] submodule: whitespace fix

[PATCH 1/2] submodule: whitespace fix

[Tay Ray Chuan]

Replace SPs with TAB.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 git-submodule.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 928a62f..ebea35b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -104,9 +104,9 @@ module_name()
 	re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
 	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
 		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
-       test -z "$name" &&
-       die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$path'")"
-       echo "$name"
+	test -z "$name" &&
+	die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$path'")"
+	echo "$name"
 }
 
 #
-- 
1.7.7.584.g16d0ea

[PATCH 2/2] submodule::module_clone(): silence die() message from module_name()

[Tay Ray Chuan]

The die() message that may occur in module_name() is not really relevant
to the user when called from module_clone(); the latter handles the
"failure" (no submodule mapping) anyway.

Leave other callers of module_name() unchanged, as the die() message
shown is either relevant for user consumption (such as those that exit()
when the call fails), or will not occur at all (when called with paths
returned by module_list()).

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 git-submodule.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ebea35b..3adab93 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -130,7 +130,7 @@ module_clone()
 
 	gitdir=
 	gitdir_base=
-	name=$(module_name "$path")
+	name=$(module_name "$path" 2>/dev/null)
 	base_path=$(dirname "$path")
 
 	gitdir=$(git rev-parse --git-dir)
-- 
1.7.7.584.g16d0ea

Re: [PATCH 2/2] submodule::module_clone(): silence die() message from module_name()

[Jens Lehmann]

BTW: this patch applies to next

Am 07.10.2011 11:04, schrieb Tay Ray Chuan:
> The die() message that may occur in module_name() is not really relevant
> to the user when called from module_clone(); the latter handles the
> "failure" (no submodule mapping) anyway.

Makes tons of sense, especially as adding a new submodule currently always
spews out the "No submodule mapping found in .gitmodules for path 'sub'"
message right before that mapping is added there. Thanks for noticing that
and ACK on that change from my side.

> Leave other callers of module_name() unchanged, as the die() message
> shown is either relevant for user consumption (such as those that exit()
> when the call fails), or will not occur at all (when called with paths
> returned by module_list()).

Hmm, while I agree on the first reasoning I'm not sure about the second.
module_list() asks the index for the submodule paths while module_name()
gets it's input from .gitmodules, so they can (and sometimes will)
disagree. When cmd_foreach() passes an empty "name" variable to the
spawned command that might still work (and even make sense), but using the
empty name in cmd_sync() to access the config is looking like an error to
me. It might make sense to add an "|| exit" at least to the callsite in
cmd_sync(). Or am I missing something here?

> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
>  git-submodule.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index ebea35b..3adab93 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -130,7 +130,7 @@ module_clone()
>  
>  	gitdir=
>  	gitdir_base=
> -	name=$(module_name "$path")
> +	name=$(module_name "$path" 2>/dev/null)
>  	base_path=$(dirname "$path")
>  
>  	gitdir=$(git rev-parse --git-dir)

Re: [PATCH 2/2] submodule::module_clone(): silence die() message from module_name()

[Tay Ray Chuan]

On Tue, Oct 11, 2011 at 3:34 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> BTW: this patch applies to next
>
> Am 07.10.2011 11:04, schrieb Tay Ray Chuan:
>> The die() message that may occur in module_name() is not really relevant
>> to the user when called from module_clone(); the latter handles the
>> "failure" (no submodule mapping) anyway.
>
> Makes tons of sense, especially as adding a new submodule currently always
> spews out the "No submodule mapping found in .gitmodules for path 'sub'"
> message right before that mapping is added there. Thanks for noticing that
> and ACK on that change from my side.

Thanks for the review.

>> Leave other callers of module_name() unchanged, as the die() message
>> shown is either relevant for user consumption (such as those that exit()
>> when the call fails), or will not occur at all (when called with paths
>> returned by module_list()).
>
> Hmm, while I agree on the first reasoning I'm not sure about the second.
> module_list() asks the index for the submodule paths while module_name()
> gets it's input from .gitmodules, so they can (and sometimes will)
> disagree.

Oh, you're right. I neglected to see how module_list() actually worked.

> When cmd_foreach() passes an empty "name" variable to the
> spawned command that might still work (and even make sense), but using the
> empty name in cmd_sync() to access the config is looking like an error to
> me. It might make sense to add an "|| exit" at least to the callsite in
> cmd_sync(). Or am I missing something here?

Cc-ed David, who authored cmd_sync().

David, what do you think of Jens' analysis?

In the meantime, I'll probably reword the second paragraph to say that
future work will be needed to analyze non- || exit callsites.

-- 
Cheers,
Ray Chuan

Re: [PATCH 2/2] submodule::module_clone(): silence die() message from module_name()

[Jens Lehmann]

Am 11.10.2011 10:44, schrieb Tay Ray Chuan:
> On Tue, Oct 11, 2011 at 3:34 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> When cmd_foreach() passes an empty "name" variable to the
>> spawned command that might still work (and even make sense), but using the
>> empty name in cmd_sync() to access the config is looking like an error to
>> me. It might make sense to add an "|| exit" at least to the callsite in
>> cmd_sync(). Or am I missing something here?
> 
> Cc-ed David, who authored cmd_sync().
> 
> David, what do you think of Jens' analysis?
> 
> In the meantime, I'll probably reword the second paragraph to say that
> future work will be needed to analyze non- || exit callsites.

Yeah, me too thinks the missing "|| exit" should be subject of another
patch.

[PATCH v2 0/2] submodule patches

[Tay Ray Chuan]

Junio, this goes on top of 'fg/submodule-git-file-git-dir' (particularly
the second patch).

Changed in v2: reworded 2nd paragraph of 2nd patch, as recommended by
Jens.

Tay Ray Chuan (2):
  submodule: whitespace fix
  submodule::module_clone(): silence die() message from module_name()

 git-submodule.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

-- 
1.7.6.msysgit.0.584.g2cbf

[PATCH v2 1/2] submodule: whitespace fix

[Tay Ray Chuan]

Replace SPs with TAB.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 git-submodule.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 7576d14..8e9e5ea 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -104,9 +104,9 @@ module_name()
 	re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
 	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
 		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
-       test -z "$name" &&
-       die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$path'")"
-       echo "$name"
+	test -z "$name" &&
+	die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$path'")"
+	echo "$name"
 }
 
 #
-- 
1.7.6.msysgit.0.584.g2cbf

[PATCH v2 2/2] submodule::module_clone(): silence die() message from module_name()

[Tay Ray Chuan]

The die() message that may occur in module_name() is not really relevant
to the user when called from module_clone(); the latter handles the
"failure" (no submodule mapping) anyway.

Analysis of other callsites is left to future work.

Acked-by: Jens Lehmann <Jens.Lehmann@web.de>
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 git-submodule.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 8e9e5ea..5d29f82 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -124,7 +124,7 @@ module_clone()
 	reference="$3"
 	gitdir=
 	gitdir_base=
-	name=$(module_name "$path")
+	name=$(module_name "$path" 2>/dev/null)
 	base_path=$(dirname "$path")
 
 	gitdir=$(git rev-parse --git-dir)
-- 
1.7.6.msysgit.0.584.g2cbf

Re: [PATCH 1/2] submodule: whitespace fix

[Jens Lehmann]

Am 07.10.2011 11:04, schrieb Tay Ray Chuan:
> Replace SPs with TAB.

As these are the only lines where spaces are used for indentation in this
file this might be a worthwhile cleanup, but I really don't care that much.
Junio, what do you think?

> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
>  git-submodule.sh |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 928a62f..ebea35b 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -104,9 +104,9 @@ module_name()
>  	re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
>  	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
>  		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
> -       test -z "$name" &&
> -       die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$path'")"
> -       echo "$name"
> +	test -z "$name" &&
> +	die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$path'")"
> +	echo "$name"
>  }
>  
>  #