Re: [PATCH 3/4] clone: validate --origin option before use

From: Sean Barag <sean@barag.org>
Date: Sep 21, 2020, 16:13 UTC
Message-ID: <20200921161323.2381099-1-sean@barag.org>
In-Reply-To: <20200916171151.1890682-1-sean@barag.org>

On Wed, 16 Sep 2020 at 10:11:51 -0700, Sean Barag writes:

Show 21 quoted lines

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