git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Mark Levedahl <mlevedahl@gmail.com>
Cc: David Aguilar <davvid@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH 1/2] git-submodule: replace duplicated code with a module_list function
Date: Fri, 22 Aug 2008 18:01:40 -0700	[thread overview]
Message-ID: <7vpro04k97.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <48AF5BA0.5050001@gmail.com> (Mark Levedahl's message of "Fri, 22 Aug 2008 20:36:48 -0400")

Mark Levedahl <mlevedahl@gmail.com> writes:

> As the command is required, while pathspec is optional, the latter
> should require the option, not the former. How about...
>
>    $ git submodule foreach [-l pathspec] 'command'
>
> or
>
>    $ git submodule foreach [<pathspec> --] 'command'

Neither would fly well.  For all normal git command, pathspec comes after
the double dash.

And I think it is probably a mistake to think "the command is required" as
anything holy.  "submodule foreach" looks very much like an index aware
"find".  Perhaps we could even make the lack of 'command' to default to
"echo its name", just like "find ." and "find . -print" are equivalent (I
am not seriously suggesting this, though.  Read on).

Having said all that, I think you can have best of both worlds by doing
something like this:

 - If you do not have -c 'command' or any option, then everything is
   command and you cannot limit with pathspecs.  This is the original
   syntax.

 - If you do have options to "foreach", then -c 'command' string is the
   command, and perhaps the foreach subcommand will gain other options
   over time, including possible -- pathspec separator.  The remainder of
   the command line after options are processed are pathspecs that limit
   the ls-files.

I do not think it is a defect that we do not allow pathspec limited
foreach subcommand.  Teaching foreach to take pathspecs is an independent
topic if somebody has such an itch.

As a conclusion of this discussion, I think David's [1/2] is fine as-is.

Thanks.

      reply	other threads:[~2008-08-23  1:02 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-08-22  7:30 [PATCH 1/2] git-submodule: replace duplicated code with a module_list function David Aguilar
2008-08-22  7:30 ` [PATCH 2/2] git-submodule: add "sync" command David Aguilar
2008-08-22 23:13   ` Junio C Hamano
2008-08-22 22:53 ` [PATCH 1/2] git-submodule: replace duplicated code with a module_list function Junio C Hamano
2008-08-23  0:01   ` Mark Levedahl
2008-08-23  0:08     ` Junio C Hamano
2008-08-23  0:36       ` Mark Levedahl
2008-08-23  1:01         ` Junio C Hamano [this message]

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=7vpro04k97.fsf@gitster.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=davvid@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=mlevedahl@gmail.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).