From: Brandon Williams <bmwill@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>, git@vger.kernel.org
Subject: Re: [PATCH 0/3] recursive support for ls-files
Date: Mon, 26 Sep 2016 10:04:29 -0700 [thread overview]
Message-ID: <20160926170429.GA3624@google.com> (raw)
In-Reply-To: <xmqqzimvygdt.fsf@gitster.mtv.corp.google.com>
On 09/25, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > On 09/25, Jeff King wrote:
> >> On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote:
> >>
> >> > After looking at the feedback I rerolled a few things, in particular the
> >> > --submodule_prefix option that existed to give a submodule context about where
> >> > it had been invoked from. People didn't seem to like the idea of exposing this
> >> > to the users (yet anyways) so I removed it as an option and instead have it
> >> > being passed to a child process via an environment variable
> >> > GIT_INTERNAL_SUBMODULE_PREFIX. This way we don't have to support anything to
> >> > external users at the moment.
> >>
> >> I think we can still have it as a command-line argument and declare it
> >> internal. It's not like environment variables cannot also be set by our
> >> callers. :)
> >>
> >> I don't mind it as an environment variable, though. In some ways it
> >> makes things easier. I just think "internal versus external" and the
> >> exact implementation are orthogonal.
> >
> > We may still want it to be an option at some point in the future. This
> > way we can revisit making it an option once we know more about the other
> > uses it could have (aside from just being for submodules as someone
> > suggested).
>
> I do not think it makes too much of a difference between environment
> and command line option. We need an update to the "git" potty to
> say "you told me to use the submodule-prefix feature, but this
> subcommand is not prepared to accept it (yet)" and cause it to error
> out either way, which would mean that a series that introduces the
> feature needs to touch "git.c" anyway, so I would have expected us
> to add command line option first, simply because "git.c" is where it
> happens, optionally with the support for the environment variable,
> not the other way around.
In a previous email you mentioned that this feature should be completely
hidden from users, which is why I removed the command line option for
this latest series. If that isn't what you intended that I can
definitely add the option to git.c. And you would rather we perform the
checking in git.c to see if a subcommand supports the prefix versus
silently ignoring it if it hasn't? I'm assuming this checking would
also be done in git.c?
>
> >> > Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
> >> > King.
> >>
> >> Hmm. It is broken after patch 2, and then fixed in patch 3. Usually we'd
> >> try not to have a broken state in the history. It's less important in
> >> this case, because the breakage is not a regression
> >> (--recurse-submodules is a new feature, so you could consider it "not
> >> working" until the 3rd patch). But I think it's still a good rule to
> >> follow, because it makes the commits easier to review, look at later,
> >> etc.
> >>
> >> For that matter, I do not understand why options like "-s" get enabled
> >> in patch 3. I do not mind them starting as disabled in patch 2, but it
> >> seems like "pass along some known-safe options" should be its own patch
> >> somewhere between patches 2 and 3.
>
> Yes, exactly.
>
> An obvious lazy way out to avoid breakage-in-the-middle and make
> incremental progress would be to squash everything into one patch,
> but we should and we should be able to do better.
>
> I'd imagine this three-patch series would be more pleasant for
> future readers if it were structured like:
>
> [1/3] introduces the submodule-prefix as a global feature; at the
> least it needs a way to invoke (either an environment, or an
> option to "git" potty, or both) and prevent mistakes by
> erroring out when it is attempted to call a subcommand that
> does not support the feature (yet).
>
> [2/3] adds the --recurse-submodule feature in a limited form to
> "ls-files". I'd suggest for this step to pass through all
> options and arguments that are safe and reasonably useful
> to pass through without needing anything more than "ah, this
> option was given, so let's stuff it to the argv-array". An
> attempt to give things that are not yet passed through until
> 3/3 to lead to an error that says it is not allowed (yet).
>
> [3-N] each of the remaining steps after 3/N adds support for one
> more thing to be passed that 2/3 refrained from doing, by
> doing more than just "pass it in argv-array", and then remove
> the "not yet supported" error that added by 2/3 for that one
> thing. The first of these "more things" would be to support
> pathspecs as the receiving side would need code changes for
> the matching logic. There may be more, or there may be
> nothing else that requires 4/N, 5/N, etc.
I can do another rework and structure the patch series more inline with
what you are suggesting here.
--
Brandon Williams
next prev parent reply other threads:[~2016-09-26 17:04 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-24 0:13 [PATCH 0/3] recursive support for ls-files Brandon Williams
2016-09-24 0:13 ` [PATCH 1/3 v3] submodules: make submodule-prefix option an envvar Brandon Williams
2016-09-25 23:34 ` Junio C Hamano
2016-09-24 0:13 ` [PATCH 2/3 v3] ls-files: optionally recurse into submodules Brandon Williams
2016-09-24 0:13 ` [PATCH 3/3 v3] ls-files: add pathspec matching for submodules Brandon Williams
2016-09-25 7:17 ` [PATCH 0/3] recursive support for ls-files Jeff King
2016-09-25 16:32 ` Brandon Williams
2016-09-25 18:38 ` Junio C Hamano
2016-09-26 17:04 ` Brandon Williams [this message]
2016-09-26 18:17 ` Junio C Hamano
2016-09-26 18:38 ` Brandon Williams
2016-09-26 18:48 ` Junio C Hamano
2016-09-26 22:46 ` [PATCH 0/4 v4] " Brandon Williams
2016-09-26 22:46 ` [PATCH 1/4 v4] submodules: make submodule-prefix option Brandon Williams
2016-09-27 18:17 ` Junio C Hamano
2016-09-27 20:29 ` Brandon Williams
2016-09-27 20:35 ` Junio C Hamano
2016-09-27 20:43 ` Brandon Williams
2016-09-26 22:46 ` [PATCH 2/4 v4] ls-files: optionally recurse into submodules Brandon Williams
2016-09-27 18:29 ` Junio C Hamano
2016-09-27 20:33 ` Brandon Williams
2016-09-26 22:46 ` [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules Brandon Williams
2016-09-27 18:40 ` Junio C Hamano
2016-09-27 20:11 ` Junio C Hamano
2016-09-27 20:52 ` Brandon Williams
2016-09-27 20:58 ` Junio C Hamano
2016-09-27 20:59 ` Stefan Beller
2016-09-28 17:24 ` Brandon Williams
2016-09-28 18:59 ` Junio C Hamano
2016-09-27 20:49 ` Brandon Williams
2016-09-27 18:43 ` Junio C Hamano
2016-09-27 20:44 ` Brandon Williams
2016-09-27 20:59 ` Junio C Hamano
2016-09-26 22:46 ` [PATCH 4/4 v4] ls-files: add pathspec matching for submodules Brandon Williams
2016-09-27 20:01 ` Junio C Hamano
2016-09-27 20:40 ` Brandon Williams
2016-09-28 21:50 ` [PATCH v5 0/4] recursive support for ls-files Brandon Williams
2016-09-28 21:50 ` [PATCH v5 1/4] git: make super-prefix option Brandon Williams
2016-09-28 22:01 ` Stefan Beller
2016-09-28 22:19 ` Junio C Hamano
2016-09-29 18:39 ` Jeff King
2016-09-29 18:44 ` Brandon Williams
2016-09-28 21:50 ` [PATCH v5 2/4] ls-files: optionally recurse into submodules Brandon Williams
2016-09-28 22:11 ` Stefan Beller
2016-09-28 22:22 ` Junio C Hamano
2016-09-28 21:50 ` [PATCH v5 3/4] ls-files: pass through safe options for --recurse-submodules Brandon Williams
2016-09-28 21:50 ` [PATCH v5 4/4] ls-files: add pathspec matching for submodules Brandon Williams
2016-09-29 21:48 ` [PATCH v6 0/4] recursive support for ls-files Brandon Williams
2016-09-29 21:48 ` [PATCH v6 1/4] git: make super-prefix option Brandon Williams
2016-10-04 17:31 ` Stefan Beller
2016-10-04 17:35 ` Junio C Hamano
2016-10-04 17:39 ` Jeff King
2016-09-29 21:48 ` [PATCH v6 2/4] ls-files: optionally recurse into submodules Brandon Williams
2016-09-29 21:48 ` [PATCH v6 3/4] ls-files: pass through safe options for --recurse-submodules Brandon Williams
2016-09-30 0:14 ` Junio C Hamano
2016-09-30 16:33 ` Brandon Williams
2016-09-30 17:01 ` Brandon Williams
2016-09-29 21:48 ` [PATCH v6 4/4] ls-files: add pathspec matching for submodules Brandon Williams
2016-10-04 17:56 ` Stefan Beller
2016-10-07 18:18 ` [PATCH v7 0/4] recursive support for ls-files Brandon Williams
2016-10-07 18:18 ` [PATCH v7 1/4] git: make super-prefix option Brandon Williams
2016-10-07 18:18 ` [PATCH v7 2/4] ls-files: optionally recurse into submodules Brandon Williams
2016-10-07 18:18 ` [PATCH v7 3/4] ls-files: pass through safe options for --recurse-submodules Brandon Williams
2016-10-07 18:18 ` [PATCH v7 4/4] ls-files: add pathspec matching for submodules Brandon Williams
2016-10-07 18:34 ` [PATCH v7 0/4] recursive support for ls-files Stefan Beller
2016-10-07 18:35 ` Stefan Beller
2016-10-07 18:45 ` Brandon Williams
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160926170429.GA3624@google.com \
--to=bmwill@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).