git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Sean Barag <sean@barag.org>
To: sean@barag.org, gitster@pobox.com
Cc: git@vger.kernel.org, gitgitgadget@gmail.com, johannes.schindelin@gmx.de
Subject: Re: [PATCH 3/4] clone: validate --origin option before use
Date: Mon, 21 Sep 2020 09:13:23 -0700	[thread overview]
Message-ID: <20200921161323.2381099-1-sean@barag.org> (raw)
In-Reply-To: <20200916171151.1890682-1-sean@barag.org>

On Wed, 16 Sep 2020 at 10:11:51 -0700, Sean Barag writes:
> > > validation for the provided `--origin` option, but notably
> > > _doesn't_ include a multi-level check (e.g. "foo/bar") that was
> > > present in the original `git-clone.sh`.  It seems `git remote`
> > > allows multi-level remote names, so applying that same validation
> > > in `git clone` seems reasonable.
> >
> > Even though I suspect "git remote" is being overly loose and
> > careless here, I am not sure if it is a good idea to tighten it
> > retroactively.  But if this is meant as a bugfix for misconversion
> > made in 8434c2f1, we should be as strict as the original.  I dunno.
>
>
> To be honest, I'm struggling to decide which route to go.  It seems
> like multilevel fetch and push refspecs are allowed as far back as
> 46220ca100 (remote.c: Fix overtight refspec validation, 2008-03-20) or
> ef00d150e4 (Tighten refspec processing, 2008-03-17), so perhaps
> removing the multilevel check in 8434c2f1 (Build in clone, 2008-04-27)
> was intentional?  If removing that check in 8434c2f1 was a mistake and
> we reintroduce it, that's probably a breaking change for some users.
> What sort of accommodations would I need to include in this patchset
> to ease that pain for users?

Thinking about this more, I'd be very surprised as a `git` user if
introducing a new config option broke backwards compatibility.  Other
`git` UIs have mixed support for slashes in remote names:

* GitHub Desktop has an open issue regarding remote names that contain
  slashes: https://github.com/desktop/desktop/issues/3618
* Sublime Merge (as-of build 2032) renders remote names with slashes as
  a tree of remotes, e.g.:

      $ git remote -v
      foo/bar     /tmp/example_repo
      foo/baz     /tmp/example_repo2

  is rendered with as a collapsible tree, roughly:

      REMOTES (2)
      v foo
        bar
        baz

* `tig` (2.4.1) renders remote names with slashes identically to those
  without slashes

Retroactively tightening those rules would cause more harm than good
(both for end-users and for downstream projects), especially with no
safe way to fix existing /-containing remote names.  I'm going to keep
the multilevel check out of this patchset (thus continuing to allowing
multilevel remote names), but if anyone feels strongly either way please
let me know! :)

Sean

  reply	other threads:[~2020-09-21 16:13 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 [this message]
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       ` [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

  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 \
    --in-reply-to=20200921161323.2381099-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 \
    --subject='Re: [PATCH 3/4] clone: validate --origin option before use' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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