git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Sean Barag via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Derrick Stolee <stolee@gmail.com>, Jeff King <peff@peff.net>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Taylor Blau <me@ttaylorr.com>, Sean Barag <sean@barag.org>,
	Andrei Rybak <rybak.a.v@gmail.com>
Subject: Re: [PATCH v2 7/7] clone: allow configurable default for `-o`/`--origin`
Date: Tue, 29 Sep 2020 12:59:22 -0700	[thread overview]
Message-ID: <xmqqa6x8icbp.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <737f91c6244220eec196c327fcea9a6548c45310.1601350615.git.gitgitgadget@gmail.com> (Sean Barag via GitGitGadget's message of "Tue, 29 Sep 2020 03:36:55 +0000")

"Sean Barag via GitGitGadget" <gitgitgadget@gmail.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.

> @@ -976,6 +982,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	int submodule_progress;
>  
>  	struct strvec ref_prefixes = STRVEC_INIT;

Missing blank line here.

> +	remote_name = default_remote_name;

Isn't the reason why the git_config() callback we saw above has an
unusual special case for default_remote_name because we have this
assignment way too early?  Would it make the control flow in a more
natural way if we removed this, and then after parse_options() did
the "-o" handling, add something like

	if (!remote_name)
		remote_name = xstrdup("origin");

instead?

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

>  	git_config(git_clone_config, NULL);
>  
> +	/*
> +	 * apply the remote name provided by --origin only after this second
> +	 * call to git_config, to ensure it overrides all config-based values.
> +	 */
> +	if (option_origin)
> +		remote_name = option_origin;

And here would be where you'd fall back

	if (!remote_name)
		remote_name = xstrdup("origin");

Note that you'd need to dup the option_origin for consistency if you
want to free() it at the end.

> +	if (!valid_remote_name(remote_name))
> +		die(_("'%s' is not a valid remote name"), remote_name);
> +

  reply	other threads:[~2020-09-29 19:59 UTC|newest]

Thread overview: 49+ 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 [this message]
2020-09-29 23:47       ` [PATCH] clone: add remote.cloneDefault config option Sean Barag
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

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=xmqqa6x8icbp.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    --cc=rybak.a.v@gmail.com \
    --cc=sean@barag.org \
    --cc=stolee@gmail.com \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH v2 7/7] clone: allow configurable default for `-o`/`--origin`' \
    /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).