git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Sean Barag <sean@barag.org>
Cc: git@vger.kernel.org, gitgitgadget@gmail.com, stolee@gmail.com
Subject: Re: [PATCH] clone: add remote.cloneDefault config option
Date: Wed, 26 Aug 2020 21:21:30 -0700	[thread overview]
Message-ID: <xmqqpn7csoqd.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200827033854.130005-1-sean@barag.org> (Sean Barag's message of "Wed, 26 Aug 2020 20:38:54 -0700")

Sean Barag <sean@barag.org> 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
series:

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

Thanks.



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

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=xmqqpn7csoqd.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=sean@barag.org \
    --cc=stolee@gmail.com \
    --subject='Re: [PATCH] clone: add remote.cloneDefault config option' \
    /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

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