All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Mark Levedahl <mlevedahl@gmail.com>
Cc: git@vger.kernel.org, pkufranky@gmail.com,
	Sven Verdoolaege <skimo@kotnet.org>
Subject: Re: [PATCH] git-submodule - Allow adding a submodule in-place
Date: Mon, 03 Mar 2008 23:09:20 -0800	[thread overview]
Message-ID: <7vod9v9d9b.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: 1204596383-4040-1-git-send-email-mlevedahl@gmail.com

Mark Levedahl <mlevedahl@gmail.com> writes:

> When working in top-level project, it is useful to create a new submodule
> as a git repo in a subdirectory, then add that submodule to top-level in
> place.  This allows "git submodule add <intended url> subdir" to add the
> existing subdir to the current project.  The presumption is the user will
> later push / clone the subdir to the <intended url> so that future
> submodule init / updates will work.
>
> Absent this patch, "git submodule add" insists upon cloning the subdir
> from a repository at the given url, which is fine for adding an existing
> project in but less useful when adding a new submodule from scratch to an
> existing project.  The former functionality remains, and the clone is
> attempted if the subdir does not already exist as a valid git repo.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>

This is a very well written commit log message with an appropriate title,
and a convincing justification why this is a good idea.  Even I (who does
not heavily use submodules himself) can look at the patch and tell that
the existing check and die was too limiting to the users after reading
these two paragraphs.

I wish everybody wrote his commit log message like this.

> +-r remote::
> +	Name of remote to use or define when working with relative submodules
> +	(i.e., submodules whose url is given relative to the top-level
> +	project). If this value is undefined, the top-level project's
> +	branch.<name>.remote is used, and if that is undefined the default
> +	"origin" is used. The remote will be defined in each relative
> +	submodule as needed by appending the relative url to the top level
> +	project's url. This option has no effect upon submodules defined
> +	using an absolute url: such project's are cloned using the default
> +	"origin," and are updated using the submodule's branch.<name>.remote
> +	machinery and defaulting to "origin."
> +

However, this part is not mentioned in the commit log message at all.

Is the enhancement advertised on the title line be useful _without_ this?

If so, this is a commit with two unrelated changes, and needs to be split
into two patches.  Also the other change that adds "-r remote" needs to be
explained and defended separately.

If not, the additional option should be described (what it does) justified
(why it is needed), and also there needs an explanation why this is an
integral part of the addition of this "add existing subdirectory" feature.

Yes, I _can_ guess that this option is related to your earlier f31a522
(git-submodule - allow a relative path as the subproject url).  Because
"submodule add" is used for setting up the initial .gitmodules entry for
the new submodule, you would need to give a clue to the command if you
want to set it up as a relative thing or an absolute thing, and you use
the relativeness of the URL parameter for that.  If you give a relative
path to the URL, however, you would need a way to pass in another piece of
information to let the command determine what it is relative to, and that
is the reason why this parameter exists.

But you _shouldn't_ be making me (or others, for that matter) wonder why
and justify it for you.  It should be explained in the commit log message.



  parent reply	other threads:[~2008-03-04  7:10 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-03-04  2:06 [PATCH] git-submodule - Allow adding a submodule in-place Mark Levedahl
2008-03-04  5:16 ` Ping Yin
2008-03-04  7:09 ` Junio C Hamano [this message]
2008-03-04 12:39   ` Mark Levedahl
2008-03-04 11:22 ` Sven Verdoolaege
2008-03-05  1:15 ` Mark Levedahl

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7vod9v9d9b.fsf@gitster.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=mlevedahl@gmail.com \
    --cc=pkufranky@gmail.com \
    --cc=skimo@kotnet.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.