Re: [PATCH 0/3] recursive support for ls-files

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 25, 2016, 18:38 UTC
Message-ID: <xmqqzimvygdt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160925163244.GB66080@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 23 quoted lines

> 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.

Show 15 quoted lines

>> > 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.

>> There are some other options that are ignored (neither disabled nor
>> passed along to children). Most of them are related to exclusions, which
>> I _think_ are safe to ignore (they do not do anything interesting unless
>> you specify "-o", which is explicitly disabled).

Sure. I agree that some options fall outside of "safe and reasonably useful to pass through" criteria and it is OK not to support passing them through. I however think we should detect mistakes in the caller, though.

I wonder if this will be common enough in the future that we are better off adding a bit or two to the parse-options infrastructure. Thinking out loud, would an enhancement like this be sufficient for this particular series, and still useful in more general cases?

 - allow the caller of parse_options() to mark each entry with a
   handful of bits with no meaning to parse-options API.  "ls-files"
   may use this mechanism to mark options that to be passed through
   and options that must be rejected when --recurse-submodules is
   asked.

 - parse_options() will mark each entry as "this option was given"
   while doing its work.  Note that "--no-foo" counts as "option
   'foo' was given".

With these, after your call to parse_options() returns, you could notice that recurse-submodules was asked for by inspecting your own "static int recurse_submodules" global variable, and then iterate over builtin_ls_files_options[] array yourself to see if an option that you marked with "must be rejected while recursing" bit using mechanism #1 was seen by parse_options() by relying on mechanism #2.

You could also add a new helper macro to parse-options API that iterates over the options[] array, i.e.

	for_each_parse_options_array(opt) {
		if (!opt_was_used(opt))
                	continue;
		if (opt_custom_bit(opt) & NOT_IN_RECURSIVE)
                	die("'%s' cannot be used with --recurse-submodules",
			    parse_options_name(opt))
	}

or something like that, but we'd need to gain experience with the mechanism #1 and #2 first before deciding if such a helper is useful (iow, I do not think we need it from day one, if we go this route).