Re: [PATCH v2] Let submodule command exit with error status if path does not exist

2012-08-13 Thread Junio C Hamano
Heiko Voigt hvo...@hvoigt.net writes:

 How about I update CodingGuidelines according to the rules you
 suggested? Then other people know how we prefer bash functions and if
 statements to look like.

OK.  I was hoping that imitate surrounding code was sufficient,
but it seems many parts of the codebase have deteriorated enough to
make that rule no longer easy to follow.

 Style:
 
  if test -z $sm_path
  then
  exit 1

 See above. If you agree I would add this style to the guidelines.

Likewise.

 I know module_list would have said error: pathspec 'no-such' did
 not match any file(s) known to git.  Did you forget to git add
 already, but because that comes at the very end of the input to the
 filter written in perl (and with the way the filter is coded, it
 will stay at the end), I am not sure if the user would notice it if
 we exit like this.  By the time we hit this exit, we would have seen
 Entering $sm_path... followed by whatever message given while in
 the submodule for all the submodules that comes out of module_list,
 no?
 
 How about doing it this way to avoid that issue, to make sure we die
 immediately after the typo is diagnosed without touching anything?

 I like it, your approach combines the atomicity of my first patch with
 the efficiency of not calling ls-files twice. I was hesitant to change
 to much code just to get the exit code right, but your approach looks
 good to me.

 Should I send an updated patch? Or do you just want to squash this in.
 Since now only the tests are from me what should we do with the
 ownership?

That is your itch and the idea and the bulk of the changes remains
to be yours in any case, methinks.

By the way, there is no reason for my patch to be unshifting the #unmatch
token into the array and spewing the array out, if the readers are always
going to stop upon seeing #unmatch without touching any submodule
that is named correctly on the command line.  In other words, the
following should suffice:

while () {
... accumulate in @out ...
}
if ($unmatched) {
print #unmatched\n;
} else {
print for (@out);
}

--
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] Let submodule command exit with error status if path does not exist

2012-08-11 Thread Junio C Hamano
Heiko Voigt hvo...@hvoigt.net writes:

 I did not know that you prefer a space after the function name. I simply
 imitated the style from C and there we do not have spaces. It makes the
 style rules a bit more complicated. Wouldn't it be nicer to have the
 same as in C so we have less rules?

I do not think so, as they are different languages.  The call site
of C functions have name and opening parenthesis without a SP in
between.  The call site of shell functions do not even have
parentheses.

In any case, personal preferences (including mine) do not matter
much, as there is no this is scientificly superiour in styles.

 diff --git a/git-submodule.sh b/git-submodule.sh
 index aac575e..48014f2 100755
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -109,7 +109,8 @@ resolve_relative_url ()
  #
  module_list()
  {
 - git ls-files --error-unmatch --stage -- $@ |
 + (git ls-files --error-unmatch --stage -- $@ ||
 + echo '16  0 ') |

Is there a reason why the sentinel has to have the same mode pattern
as a GITLINK entry, NULL SHA-1, stage #0?  Or is the path being
empty all that matters?

Ah, OK, you did not want to touch the perl script downstream.  I
would have preferred a comment to document that, i.e.

(
git ls-files --error-unmatch --stage -- $@ ||
# an entry with an empty $path used as a signal
echo '16 0 0 '
) |
perl -e '...

or

(
git ls-files --error-unmatch --stage -- $@ ||
echo 'unmatched pathspec exists'
) |
perl -e '
...
while (STDIN) {
if (/^unmatched pathspec/) {
print;
next;
}
chomp;
my ($mode, $sha1, $stage, $path) = ...

Either way, the reader of the code would not have to scratch her
head that way.

   perl -e '
   my %unmerged = ();
   my ($null_sha1) = (0 x 40);
 @@ -385,6 +386,10 @@ cmd_foreach()
   module_list |
   while read mode sha1 stage sm_path
   do
 + if test -z $sm_path; then
 + exit 1

Style:

if test -z $sm_path
then
exit 1

I know module_list would have said error: pathspec 'no-such' did
not match any file(s) known to git.  Did you forget to git add
already, but because that comes at the very end of the input to the
filter written in perl (and with the way the filter is coded, it
will stay at the end), I am not sure if the user would notice it if
we exit like this.  By the time we hit this exit, we would have seen
Entering $sm_path... followed by whatever message given while in
the submodule for all the submodules that comes out of module_list,
no?

How about doing it this way to avoid that issue, to make sure we die
immediately after the typo is diagnosed without touching anything?

 git-submodule.sh | 32 +---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 3aa7644..3f99d71 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -109,26 +109,47 @@ resolve_relative_url ()
 #
 module_list()
 {
-   git ls-files --error-unmatch --stage -- $@ |
+   (
+   git ls-files --error-unmatch --stage -- $@ ||
+   echo unmatched pathspec exists
+   ) |
perl -e '
my %unmerged = ();
my ($null_sha1) = (0 x 40);
+   my @out = ();
+   my $unmatched = 0;
while (STDIN) {
+   if (/^unmatched pathspec/) {
+   $unmatched = 1;
+   next;
+   }
chomp;
my ($mode, $sha1, $stage, $path) =
/^([0-7]+) ([0-9a-f]{40}) ([0-3])\t(.*)$/;
next unless $mode eq 16;
if ($stage ne 0) {
if (!$unmerged{$path}++) {
-   print $mode $null_sha1 U\t$path\n;
+   push @out, $mode $null_sha1 U\t$path\n;
}
next;
}
-   print $_\n;
+   push @out, $_\n;
+   }
+   if ($unmatched) {
+   unshift @out, #unmatched\n;
}
+   print for (@out);
'
 }
 
+check_unmatched ()
+{
+   if test $1 = #unmatched
+   then
+   exit 1
+   fi
+}
+
 #
 # Map submodule path to submodule name
 #
@@ -385,6 +406,7 @@ cmd_foreach()
module_list |
while read mode sha1 stage sm_path
do
+   check_unmatched $mode
if test -e $sm_path/.git
then
say $(eval_gettext Entering '\$prefix\$sm_path')
@@ -437,6 +459,7 @@ cmd_init()
module_list $@ |
while read mode sha1 stage sm_path
do
+