From: Junio C Hamano <firstname.lastname@example.org>
To: Sean Barag <email@example.com>
Cc: firstname.lastname@example.org, email@example.com, firstname.lastname@example.org
Subject: Re: [PATCH] clone: add remote.cloneDefault config option
Date: Wed, 26 Aug 2020 21:21:30 -0700 [thread overview]
Message-ID: <email@example.com> (raw)
In-Reply-To: <firstname.lastname@example.org> (Sean Barag's message of "Wed, 26 Aug 2020 20:38:54 -0700")
Sean Barag <email@example.com> writes:
>> It is somewhat sad that we have the git_config(git_default_config)
>> call so late in the control flow. I wonder if we can update the
>> start-up sequence to match the usual flow
>> One oddity "git clone" has is that it wants to delay the reading of
>> configuration files (they are read only once, and second and
>> subsequent git_config() calls will reuse what was read before [*]) so
>> that it can read what clone.c::write_config() wrote, so if we were to
>> "fix" the start-up sequence to match the usual flow, we need to
>> satisfy what that odd arrangement wanted to achieve in some other way
>> (e.g. feed what is in option_config to git_default_config ourselves,
>> without using git_config(), as part of the "main control flow uses the
>> variable" part), but it should be doable.
> Sounds like a pretty big change! I'm willing to take a crack at it,
> but given that this is my first patch I'm frankly a bit intimidated :)
> How would you feel about that being a separate patch?
It definitely needs to be a separate patch.
In order to realize any new feature that needs to read the existing
(i.e. per-machine or per-user) configuration files to affect the
behaviour of "git clone", whether it is the "give default to
--origin option" or any other thing, first needs to fix the start-up
sequence so that the configuration is read once before we process
command line options, which is the norm. Only after that is done,
we can build the clone.defaultRemoteName and other features that
would be affected by the settings of clone.* variables on top, and
it is not wise to mix the foundation with a new feature that uses
the foundation. So I would imagine it would at least be 3-patch
- [1/3] would be to whip the start-up sequence into the normal
order. This may be the most tricky part. I identified that the
handling of option_config might need adjustment, but there may be
others. This may not need any new tests, but if the existing
tests for "clone -c var=val" do not catch breakage when we
naively move the git_config(git_default_config) call early
without doing any other adjustment, we might need to add more
tests to cover the option. If we find things other than
option_config needs adjustment, they also need test coverage.
- [2/3] would be to tighten the error checking of option_origin
that was lost when the command was reimplemented in C (already
discussed). This would need new tests to see "--origin $bogus"
command line option is flagged as an error.
- [3/3] would be to read option_origin from the configuration file.
The start-up sequence fixed by [1/3] would allow us to write the
config callback in a more natural way, compared to what you wrote
and what I suggested to rewrite. Error checking code in [2/3]
would directly be reusable in the code added in this step. We'd
need tests similar to we add in [2/3] for the configuration, and
combination of configuration and command line (you already wrote
most of these).
next prev parent reply other threads:[~2020-08-27 4:21 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-26 15:45 [PATCH] clone: add remote.cloneDefault config option Sean Barag via GitGitGadget
2020-08-26 18:46 ` Junio C Hamano
2020-08-26 19:04 ` Derrick Stolee
2020-08-26 19:59 ` Junio C Hamano
2020-08-27 3:38 ` Sean Barag
2020-08-27 4:21 ` Junio C Hamano [this message]
2020-08-27 14:00 ` Sean Barag
2020-09-29 19:59 [PATCH v2 7/7] clone: allow configurable default for `-o`/`--origin` Junio C Hamano
2020-09-29 23:47 ` [PATCH] clone: add remote.cloneDefault config option Sean Barag
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:
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 \
* 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
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).