Hi Junio,
thanks for such a thorough review.
On Sat, Aug 11, 2012 at 10:43:22PM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@xxxxxxxxxx> 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.
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.
> > 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 '160000 0000000000000000000000000000000000000000 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.
I only described it in the commit message, sorry. Next time I will add a
comment.
> > @@ -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
See above. If you agree I would add this style to the guidelines.
> 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?
> git-submodule.sh | 32 +++++++++++++++++++++++++++++---
> 1 file changed, 29 insertions(+), 3 deletions(-)
[...]
Cheers Heiko
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html