From: Sean Barag <sean@barag.org>
To: gitster@pobox.com
Cc: git@vger.kernel.org, gitgitgadget@gmail.com,
johannes.schindelin@gmx.de, me@ttaylorr.com, peff@peff.net,
rybak.a.v@gmail.com, sean@barag.org, stolee@gmail.com,
sunshine@sunshineco.com
Subject: Re: [PATCH] clone: add remote.cloneDefault config option
Date: Tue, 29 Sep 2020 16:47:50 -0700 [thread overview]
Message-ID: <20200929234750.879484-1-sean@barag.org> (raw)
In-Reply-To: <xmqqa6x8icbp.fsf@gitster.c.googlers.com>
"Junio C Hamano" <gitster@pobox.com> writes:
> > static int git_clone_config(const char *k, const char *v, void *cb)
> > {
> > + if (!strcmp(k, "clone.defaultremotename")) {
> > + if (remote_name != default_remote_name)
> > + free(remote_name);
> > + remote_name = xstrdup(v);
>
> This feels strange. The usual arrangement is
>
> - initialize the variable to NULL (or any value that the code
> can tell that nobody touched it);
>
> - let git_config() callback to update the variable, taking care
> of freeing and strduping as needed. Note that free(NULL) is
> kosher.
>
> - let parse_options() to further update the variable, taking
> care of freeing and strduping as needed.
>
> - finally, if the variable is still NULL, give it its default
> value.
>
> so there is no room for the "if the variable has the value of the
> fallback default, do things differently" logic to be in the
> git_config() callback function.
I agree, it felt pretty awkward while writing it! I was hoping to match
the start-up sequence you'd mentioned in my original attempt [1] but I'm
happy to move the default value assignment down.
> > @@ -1153,10 +1154,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >
> > /*
> > * re-read config after init_db and write_config to pick up any config
> > - * injected by --template and --config, respectively
> > + * injected by --template and --config, respectively.
> > */
>
> Squash this "oops, I forgot to finish the sentence" to the step the
> mistake was introduced, i.e. "use more conventional..."
How embarrassing, I thought I'd gotten all of those. Will do.
Sean
--
[1] Original attempt at this feature, in separate thread because of the
divergence: https://lore.kernel.org/git/xmqqlfi1utwi.fsf@gitster.c.googlers.com/
next prev parent reply other threads:[~2020-09-29 23:48 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-11 18:25 [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag via GitGitGadget
2020-09-11 18:25 ` [PATCH 1/4] clone: add tests for --template and some disallowed option pairs Sean Barag via GitGitGadget
2020-09-11 18:57 ` Derrick Stolee
2020-09-11 19:56 ` Jeff King
2020-09-11 20:07 ` Eric Sunshine
2020-09-16 3:15 ` [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag
2020-09-12 3:17 ` [PATCH 1/4] clone: add tests for --template and some disallowed option pairs Taylor Blau
2020-09-15 16:09 ` Sean Barag
2020-09-16 16:36 ` Jeff King
2020-09-11 21:02 ` Junio C Hamano
2020-09-12 0:41 ` Derrick Stolee
2020-09-11 18:25 ` [PATCH 2/4] clone: call git_config before parse_options Sean Barag via GitGitGadget
2020-09-11 18:59 ` Derrick Stolee
2020-09-11 20:26 ` Junio C Hamano
2020-09-16 16:12 ` Sean Barag
2020-09-11 18:25 ` [PATCH 3/4] clone: validate --origin option before use Sean Barag via GitGitGadget
2020-09-11 19:24 ` Derrick Stolee
2020-09-16 16:28 ` Sean Barag
2020-09-11 20:39 ` Junio C Hamano
2020-09-16 17:11 ` Sean Barag
2020-09-21 16:13 ` Sean Barag
2020-09-11 18:25 ` [PATCH 4/4] clone: allow configurable default for `-o`/`--origin` Sean Barag via GitGitGadget
2020-09-11 19:13 ` Derrick Stolee
2020-09-28 16:04 ` Sean Barag
2020-09-11 21:00 ` Junio C Hamano
2020-09-28 16:02 ` Sean Barag
2020-09-17 15:25 ` Andrei Rybak
2020-09-11 19:25 ` [PATCH 0/4] clone: allow configurable default for -o/--origin Derrick Stolee
2020-09-11 19:34 ` Junio C Hamano
2020-09-29 3:36 ` [PATCH v2 0/7] " Sean Barag via GitGitGadget
2020-09-29 3:36 ` [PATCH v2 1/7] clone: add tests for --template and some disallowed option pairs Sean Barag via GitGitGadget
2020-09-29 3:36 ` [PATCH v2 2/7] clone: use more conventional config/option layering Sean Barag via GitGitGadget
2020-09-29 3:36 ` [PATCH v2 3/7] remote: add tests for add and rename with invalid names Sean Barag via GitGitGadget
2020-09-29 3:36 ` [PATCH v2 4/7] refs: consolidate remote name validation Sean Barag via GitGitGadget
2020-09-29 3:36 ` [PATCH v2 5/7] clone: validate --origin option before use Sean Barag via GitGitGadget
2020-09-29 3:36 ` [PATCH v2 6/7] clone: read new remote name from remote_name instead of option_origin Sean Barag via GitGitGadget
2020-09-29 3:36 ` [PATCH v2 7/7] clone: allow configurable default for `-o`/`--origin` Sean Barag via GitGitGadget
2020-09-29 19:59 ` Junio C Hamano
2020-09-29 23:47 ` Sean Barag [this message]
2020-09-29 3:44 ` [PATCH v2 0/7] clone: allow configurable default for -o/--origin Sean Barag
2020-10-01 3:46 ` [PATCH v3 " Sean Barag via GitGitGadget
2020-10-01 3:46 ` [PATCH v3 1/7] clone: add tests for --template and some disallowed option pairs Sean Barag via GitGitGadget
2020-10-01 3:46 ` [PATCH v3 2/7] clone: use more conventional config/option layering Sean Barag via GitGitGadget
2020-10-01 3:46 ` [PATCH v3 3/7] remote: add tests for add and rename with invalid names Sean Barag via GitGitGadget
2020-10-01 3:46 ` [PATCH v3 4/7] refs: consolidate remote name validation Sean Barag via GitGitGadget
2020-10-01 3:46 ` [PATCH v3 5/7] clone: validate --origin option before use Sean Barag via GitGitGadget
2020-10-01 3:46 ` [PATCH v3 6/7] clone: read new remote name from remote_name instead of option_origin Sean Barag via GitGitGadget
2020-10-01 3:46 ` [PATCH v3 7/7] clone: allow configurable default for `-o`/`--origin` Sean Barag via GitGitGadget
2020-10-02 12:56 ` [PATCH v3 0/7] clone: allow configurable default for -o/--origin Derrick Stolee
-- strict thread matches above, loose matches on Subject: below --
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
2020-08-27 14:00 ` 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=20200929234750.879484-1-sean@barag.org \
--to=sean@barag.org \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
--cc=me@ttaylorr.com \
--cc=peff@peff.net \
--cc=rybak.a.v@gmail.com \
--cc=stolee@gmail.com \
--cc=sunshine@sunshineco.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).