From: Lars Hjemli <hjemli@gmail.com>
Date: Sat, 26 May 2007 09:39:22 GMT
Subject: Re: [PATCH] Add git-submodule command
Message-ID: <8c5c35580705260239x4acf0318vfda3651fb315f8c5@mail.gmail.com>
In-Reply-To: <7v1wh4ped4.fsf@assigned-by-dhcp.cox.net>

On 5/26/07, Junio C Hamano <junkio@cox.net> wrote:
> Lars Hjemli <hjemli@gmail.com> writes:
>
> > With this entry in .gitmodules (and a commit reference in the index entry for
> > the path "git"), the command 'git submodule init' will clone the repository
> > at kernel.org into the directory "git".
> >
> > Signed-off-by: Lars Hjemli <hjemli@gmail.com>
> > ---
> >
> > On 5/26/07, Lars Hjemli <hjemli@gmail.com> wrote:
> >> I'll redo the patch, removing the branch-specific things, and try to
> >> shut up :)
>
> Hey, don't shut up.  Starting small and covering corner cases
> incrementally is really the right approach.

Ok, my meds have kicked in and I'm ready for more ;-)

>
> > +status::
> > +     Show the status of the submodules. This will print the sha1 of the
> > +     currently checked out commit for each submodule, along with the
> > +     submodule path and the output of gitlink:git-describe[1] for the
> > +     sha1. Each sha1 will be prefixed with '-' if the submodule is not
> > +     initialized and '+' if the currently checked out submodule commit
> > +     does not match the sha1 found in the index of the containing
> > +     repository. This command is the default command for git-submodule.
>
> (markup) probably you would want `` there for typewriter face.

Ok, will fix

>
> (wording) didn't we have "the name of the hash function is
> SHA-1" patch earlier?  I'd personally prefer calling them
> "object names", though...

Some quick grepping wasn't helpfull:

~/src/git/Documentation$ git grep sha1 | wc -l
98
~/src/git/Documentation$ git grep SHA1 | wc -l
80
~/src/git/Documentation$ git grep SHA-1 | wc -l
22

Would you prefer SHA-1?

>
> Other than that, the command description is very nicely done,
> which means the design of the command set hence the semantics is
> cleanly done.  Good job.

Thanks!

>
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > new file mode 100755
> > index 0000000..247b1ee
> > --- /dev/null
> > +++ b/git-submodule.sh
> > @@ -0,0 +1,172 @@
> > ...
> > +#
> > +# print stuff on stdout unless -q was specified
> > +#
> > +say()
> > +{
> > +     if test -z "$quiet"
> > +     then
> > +             echo -e "$@"
> > +     fi
> > +}
>
> We tend to avoid "echo -e" (not POSIX).  I do not see any string
> you feed to this function that you would _want_ backslash
> escaped sequences (actually I would suspect you would not want
> them).

I do use \t between submodule path and the result of git-describe, but
it's not really needed. I'll drop it.

>
> > +
> > +#
> > +# Run clone + checkout on missing submodules
> > +#
> > +# $@ = requested paths (default to all)
> > +#
> > +modules_init()
> > +{
> > +     git ls-files --stage -- $@ | grep -e '^160000 ' |
>
> Did you mean "$@", i.e. inside double-quotes?

That looks right, yes


> > +             test -d "$path/.git" && continue
> > +
> > +             if test -d "$path"
> > +             then
> > +                     rmdir "$path" 2>/dev/null ||
> > +                     die "Directory '$path' exist, but not as a submodule"
> > +             fi
>
> Could the currently checked-out $path be a symlink to another
> directory, and what does the code do in such a case?

This I will have to test, but I _suspect_ it would fail on "test -e
path" _unless_ the "test -d $path/.git" kicks in.

>
> > +
> > +             test -e "$path" &&
> > +             die "A file already exist at path '$path'"
>
> "test -e" is a relatively new invention and I tended to stay
> away from it, but it should be safe to use these days...

Would you prefer separate -f and -l instead? Hmm, that would match up
nicely with your concern about symlinks ;-)

>
> > +             url=$(GIT_CONFIG=.gitmodules git-config module."$path".url)
> > +             test -z "$url" &&
> > +             die "No url found for submodule '$path' in .gitmodules"
> > +
> > +             git-clone "$url" "$path" ||
> > +             die "Clone of submodule '$path' failed"
>
> "git-clone -n" please, as you will checkout something different
> in the next step anyway.

Nice, I hadn't noticed -n, will fix

>
> > +
> > +             $(unset GIT_DIR && cd "$path" && git-checkout -q "$sha1") ||
> > +             die "Checkout of submodule '$path' failed"
>
> Lose $() around this, as it is not producing a string which is
> the name of the command to run.  You do want a subshell here
> because of chdir, so instead of losing $(), replace them with
> ().

Heh, there's always something new to learn on this list, thanks.

--
larsh