Re: [PATCH 2/6] git rm: Support for removing submodules
I will collect your feedback on the whole series, then resend it from scratch - sounds good?
On Fri, Sep 12, 2008 at 02:49:56PM -0700, Junio C Hamano wrote:
Show 35 quoted lines
> Petr Baudis <pasky@suse.cz> writes:
>
> > @@ -20,7 +20,8 @@ and no updates to their contents can be staged in the index,
> > though that default behavior can be overridden with the `-f` option.
> > When '--cached' is given, the staged content has to
> > match either the tip of the branch or the file on disk,
> > -allowing the file to be removed from just the index.
> > +allowing the file to be removed from just the index;
> > +this is always the case when removing submodules.
>
> Sorry, I read this three times but "this" is unclear to me. Different and
> mutually incompatible interpretations I tried to understand it are:
>
> (1) When removing submodules, whether --cached or not, the index can
> match either HEAD or the work tree; this is different from removing
> regular blobs where the index must match with HEAD without --cached
> nor -f;
>
> (2) When removing submodules with --cached, the index can match either
> HEAD or the work tree and it is removed only from the index. You
> cannot remove submodules without --cached;
>
> (3) When removing submodules, the index can match either HEAD or the work
> tree and it is removed only from the index, even if you did not give
> --cached;
>
> It later becomes clear that you meant (3) in the second hunk, but the
> first time reader of the resulting document (not this patch) won't be
> reading from bottom to top.
>
> This is a leftover issue from ealier documentation 25dc720 (Clarify and
> fix English in "git-rm" documentation, 2008-04-16), but the description is
> unclear what should happen while working towards the initial commit
> (i.e. no HEAD yet). I think check_local_mod() allows removal in such a
> case. Perhaps you can clarify the point while at it, please?
Show 15 quoted lines
> > diff --git a/builtin-rm.c b/builtin-rm.c
> > index 6bd8211..7475de2 100644
> > --- a/builtin-rm.c
> > +++ b/builtin-rm.c
> > ...
> > -static void add_list(const char *name)
> > +static void add_list(const char *name, int is_gitlink)
> > {
> > if (list.nr >= list.alloc) {
> > list.alloc = alloc_nr(list.alloc);
> > - list.name = xrealloc(list.name, list.alloc * sizeof(const char *));
> > + list.info = xrealloc(list.info, list.alloc * sizeof(*list.info));
> > }
>
> ALLOC_GROW()?Show 13 quoted lines
> > @@ -38,6 +44,13 @@ static int remove_file(const char *name)
> > if (ret && errno == ENOENT)
> > /* The user has removed it from the filesystem by hand */
> > ret = errno = 0;
> > + if (ret && errno == EISDIR) {
> > + /* This is a gitlink entry; try to remove at least the
> > + * directory if the submodule is not checked out; we always
> > + * leave the checked out ones as they are */
>
> /*
> * Style?
> * for a multi-line comment.
> */Right - I will have to get used to this. ;-)
Show 17 quoted lines
> > +static void remove_submodule(const char *name)
> > +{
> > + char *key = submodule_by_path(name);
> > + char *sectend = strrchr(key, '.');
> > +
> > + assert(sectend);
> > + *sectend = 0;
>
> Here is one caller I questioned in my comments on [1/6]. It is clear this
> caller wants to use "submodule.xyzzy" out of "submodule.xyzzy.path". The
> function returning "submodule.xyzzy.path" does not feel like a clean and
> reusable interface to me. I'd suggest either returning "submodule.xyzzy"
> (that's too specialized only for this caller to my taste, though), or just
> "xyzzy" and have the caller synthesize whatever string it wants to use
> (yes, it loses microoptimization but do we really care about it in this
> codepath?), if you have other callers that want different strings around
> "xyzzy".This is still easier to use than explicit snprintf(), but in the long run, you're right that returning just the submodule name is the right thing. I will change the API.
Show 11 quoted lines
> > @@ -140,7 +169,7 @@ static struct option builtin_rm_options[] = {
> >
> > int cmd_rm(int argc, const char **argv, const char *prefix)
> > {
> > - int i, newfd;
> > + int i, newfd, subs;
>
> Perhaps hoist "int removed" up one scope level and reuse it? I misread
> that you are counting the number of gitlinks in the index, not the number
> of gitlinks that is being removed, on my first read. The variable is used
> for the latter.Sensible idea.
On Fri, Sep 12, 2008 at 02:59:12PM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> +{
> >> + char *key = submodule_by_path(name);
> >> + char *sectend = strrchr(key, '.');
> >> +
> >> + assert(sectend);
> >> + *sectend = 0;
> >
> > Here is one caller I questioned in my comments on [1/6]...
>
> Another thing --- can submodule_by_path() ever return NULL saying "I do
> not see one in the configuration"?No, it would rather die().
--
Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC. -- Bill Gates
Petr Baudis
