Repository: iterative/dvc
Pull request: exp run: add experiment name check.
fix #6674 and fix #6652 

1. Add experiment name check (https://git-scm.com/docs/git-check-ref-format)
2. Add duplicate exp name check.
3. Add some unit test for it.

* [x] ❗ I have followed the [Contributing to DVC](https://dvc.org/doc/user-guide/contributing/core) checklist.

* [x] 📖 If this PR requires [documentation](https://dvc.org/doc) updates, I have created a separate PR (or issue, at least) in [dvc.org](https://github.com/iterative/dvc.org) and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@pmrowla , in #6819,  exp name with '/' inside are request. And these types of exp names are stored in a subdirectory.  For example in #6843, it is stored in `.git/refs/heads/dependabot/pip/pylint-2.11.1`. The subdirectory here is used for grouping branches. Currently, our `dvc exp` did not support this. It involves not only name checking here, maybe we should reopen #6819 and create a new PR for it?

Problem statement:
exp run: Results cannot be found if experiment name includes space
# Bug Report

## Description 
When running an experiment with white space in the name, the resulting experiment does not show up in `exp show` later.

### Reproduce

1. `dvc exp run --name "some name": Experiments runs as expected.
2. `dvc exp show`: Experiment is not listed.

### Expected

I would expect that either

- `dvc exp run` should refuse to assign names with white space in them,
- or `dvc exp show` shows this experiment in the summary regardless of the white space in the name.

### Environment information

**Output of `dvc doctor`:**

```console
DVC version: 2.7.2 (pip)
---------------------------------
Platform: Python 3.7.10 on Linux-5.14.5-arch1-1-x86_64-with-arch
```

exp run -n: check for conflicts before running experiment
`dvc exp run -n` will throw an error if a naming conflict arises, but it won't check until after the experiment has completed. 

I guess someone could re-run the experiment later with a new name and the run-cache should prevent the whole experiment from needing to be executed, but this doesn't seem intuitive, and it would be better to know immediately that the experiment name conflicts with an existing experiment.

Steps to reproduce:

```
$ dvc init
$ echo "p: 0" > params.yaml
$ dvc stage add -p p -n test "echo test"
$ dvc exp run -n test
Running stage 'test':
> echo test
test
Generating lock file 'dvc.lock'
Updating lock file 'dvc.lock'

To track the changes with git, run:

        git add dvc.yaml dvc.lock params.yaml

Reproduced experiment(s): test
Experiment results have been applied to your workspace.

To promote an experiment to a Git branch run:

        dvc exp branch <exp> <branch>
$ dvc exp run -n test -S p=1
Running stage 'test':
> echo test
test
Updating lock file 'dvc.lock'

To track the changes with git, run:

        git add params.yaml dvc.yaml dvc.lock
ERROR: Reproduced experiment conflicts with existing experiment 'refs/exps/d4/5e189b914d70882bd67b041110f3f27a30f941/test'. To overwrite the existing experiment run:

        dvc exp run -f ...

To run this experiment with a different name run:

        dvc exp run -n <new_name> ...
```

Hints from issue discussion:
For reference, the intended behavior is:

> dvc exp run should refuse to assign names with white space in them,

Experiment names must be valid Git ref/branch names and cannot contain whitespace (https://git-scm.com/docs/git-check-ref-format)
related: https://github.com/iterative/dvc/issues/6652

We need to validate user-provided exp names in general to make sure they conform with `git check-ref-format` rules in addition to checking for duplicates

Commit message: Some bug fix

Patch to review:
diff --git a/dvc/repo/experiments/__init__.py b/dvc/repo/experiments/__init__.py
index 30b9a5d55e..19f7891f18 100644
--- a/dvc/repo/experiments/__init__.py
+++ b/dvc/repo/experiments/__init__.py
@@ -473,6 +473,21 @@ def _log_reproduced(self, revs: Iterable[str], tmp_dir: bool = False):
             "\tdvc exp branch <exp> <branch>\n"
         )
 
+    def _ref_name_validation(self, name: Optional[str], **kwargs):
+        from .utils import check_ref_format
+
+        if name is None:
+            return
+
+        baseline_sha = self.repo.scm.get_rev()
+        exp_ref = ExpRefInfo(baseline_sha=baseline_sha, name=name)
+        check_ref_format(self.scm, exp_ref)
+
+        reset = kwargs.get("reset", False)
+        force = kwargs.get("force", False)
+        if not (force or reset) and self.scm.get_ref(str(exp_ref)):
+            raise ExperimentExistsError(name)
+
     @scm_locked
     def new(self, *args, checkpoint_resume: Optional[str] = None, **kwargs):
         """Create a new experiment.
@@ -485,7 +500,10 @@ def new(self, *args, checkpoint_resume: Optional[str] = None, **kwargs):
                 *args, resume_rev=checkpoint_resume, **kwargs
             )
 
-        return self._stash_exp(*args, **kwargs)
+        name = kwargs.pop("name", None)
+        self._ref_name_validation(name, **kwargs)
+
+        return self._stash_exp(*args, name=name, **kwargs)
 
     def _resume_checkpoint(
         self, *args, resume_rev: Optional[str] = None, **kwargs
diff --git a/dvc/repo/experiments/utils.py b/dvc/repo/experiments/utils.py
index c2a8f9cb05..1c68a2b0a9 100644
--- a/dvc/repo/experiments/utils.py
+++ b/dvc/repo/experiments/utils.py
@@ -157,3 +157,12 @@ def resolve_exp_ref(
         msg.extend([f"\t{info}" for info in exp_ref_list])
         raise InvalidArgumentError("\n".join(msg))
     return exp_ref_list[0]
+
+
+def check_ref_format(scm: "Git", ref: ExpRefInfo):
+    # "/" forbidden, only in dvc exp as we didn't support it for now.
+    if not scm.check_ref_format(str(ref)) or "/" in ref.name:
+        raise InvalidArgumentError(
+            f"Invalid exp name {ref.name}, the exp name must follow rules in "
+            "https://git-scm.com/docs/git-check-ref-format"
+        )
diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py
index 5d60ec232f..ba8e76c4ce 100644
--- a/dvc/scm/git/__init__.py
+++ b/dvc/scm/git/__init__.py
@@ -346,6 +346,7 @@ def get_fs(self, rev: str):
     status = partialmethod(_backend_func, "status")
     merge = partialmethod(_backend_func, "merge")
     validate_git_remote = partialmethod(_backend_func, "validate_git_remote")
+    check_ref_format = partialmethod(_backend_func, "check_ref_format")
 
     def resolve_rev(self, rev: str) -> str:
         from dvc.repo.experiments.utils import exp_refs_by_name
diff --git a/dvc/scm/git/backend/dulwich/__init__.py b/dvc/scm/git/backend/dulwich/__init__.py
index 84bfd2f4f7..1f706a8e2d 100644
--- a/dvc/scm/git/backend/dulwich/__init__.py
+++ b/dvc/scm/git/backend/dulwich/__init__.py
@@ -681,3 +681,8 @@ def validate_git_remote(self, url: str, **kwargs):
             os.path.join("", path)
         ):
             raise InvalidRemoteSCMRepo(url)
+
+    def check_ref_format(self, refname: str):
+        from dulwich.refs import check_ref_format
+
+        return check_ref_format(refname.encode())
diff --git a/tests/func/experiments/test_experiments.py b/tests/func/experiments/test_experiments.py
index cf94871f5b..ee44e83753 100644
--- a/tests/func/experiments/test_experiments.py
+++ b/tests/func/experiments/test_experiments.py
@@ -53,6 +53,7 @@ def test_experiment_exists(tmp_dir, scm, dvc, exp_stage, mocker, workspace):
         tmp_dir=not workspace,
     )
 
+    new_mock = mocker.spy(dvc.experiments, "_stash_exp")
     with pytest.raises(ExperimentExistsError):
         dvc.experiments.run(
             exp_stage.addressing,
@@ -60,6 +61,7 @@ def test_experiment_exists(tmp_dir, scm, dvc, exp_stage, mocker, workspace):
             params=["foo=3"],
             tmp_dir=not workspace,
         )
+    new_mock.assert_not_called()
 
     results = dvc.experiments.run(
         exp_stage.addressing,
@@ -685,3 +687,16 @@ def test_exp_run_recursive(tmp_dir, scm, dvc, run_copy_metrics):
     )
     assert dvc.experiments.run(".", recursive=True)
     assert (tmp_dir / "metric.json").parse() == {"foo": 1}
+
+
+def test_experiment_name_invalid(tmp_dir, scm, dvc, exp_stage, mocker):
+    from dvc.exceptions import InvalidArgumentError
+
+    new_mock = mocker.spy(dvc.experiments, "_stash_exp")
+    with pytest.raises(InvalidArgumentError):
+        dvc.experiments.run(
+            exp_stage.addressing,
+            name="fo^o",
+            params=["foo=3"],
+        )
+    new_mock.assert_not_called()
diff --git a/tests/unit/repo/experiments/test_utils.py b/tests/unit/repo/experiments/test_utils.py
index b0fd3da808..4fce74244c 100644
--- a/tests/unit/repo/experiments/test_utils.py
+++ b/tests/unit/repo/experiments/test_utils.py
@@ -1,7 +1,8 @@
 import pytest
 
+from dvc.exceptions import InvalidArgumentError
 from dvc.repo.experiments.base import EXPS_NAMESPACE, ExpRefInfo
-from dvc.repo.experiments.utils import resolve_exp_ref
+from dvc.repo.experiments.utils import check_ref_format, resolve_exp_ref
 
 
 def commit_exp_ref(tmp_dir, scm, file="foo", contents="foo", name="foo"):
@@ -25,3 +26,22 @@ def test_resolve_exp_ref(tmp_dir, scm, git_upstream, name_only, use_url):
     remote_ref_info = resolve_exp_ref(scm, "foo" if name_only else ref, remote)
     assert isinstance(remote_ref_info, ExpRefInfo)
     assert str(remote_ref_info) == ref
+
+
+def test_run_check_ref_format(scm):
+    baseline_rev = "b05eecc666734e899f79af228ff49a7ae5a18cc0"
+
+    def fun(name):
+        ref = ExpRefInfo(baseline_rev, name)
+        check_ref_format(scm, ref)
+
+    fun("name")
+
+    # Forbid slash / here because we didn't support it for now.
+    with pytest.raises(InvalidArgumentError):
+        fun("group/name")
+
+    with pytest.raises(InvalidArgumentError):
+        fun("na\05me")
+    with pytest.raises(InvalidArgumentError):
+        fun("na me")


Code review comments:
diff_hunk: @@ -25,3 +26,22 @@ def test_resolve_exp_ref(tmp_dir, scm, git_upstream, name_only, use_url):
     remote_ref_info = resolve_exp_ref(scm, "foo" if name_only else ref, remote)
     assert isinstance(remote_ref_info, ExpRefInfo)
     assert str(remote_ref_info) == ref
+
+
+def test_run_check_ref_format(scm):
path: tests/unit/repo/experiments/test_utils.py
text: @user2:
This test could just be parametrized (and the baseline SHA doesn't have to be a valid SHA for the purposes of this test)
```py
@user1.mark.parametrize("name", [...])
def test_run_check_ref_format(scm, name):
    with pytest.raises(InvalidArgumentError):
        check_ref_format(scm, ExpRefInfo("abc123", name))
```

diff_hunk: @@ -473,6 +473,21 @@ def _log_reproduced(self, revs: Iterable[str], tmp_dir: bool = False):
             "\tdvc exp branch <exp> <branch>\n"
         )
 
+    def _ref_name_validation(self, name: Optional[str], **kwargs):
+        from .utils import check_ref_format
+
+        if name is None:
+            return
+
+        baseline_sha = self.repo.scm.get_rev()
path: dvc/repo/experiments/__init__.py
text: @user1:
```suggestion
        baseline_sha = kwargs.get("baseline_rev") or self.repo.scm.get_rev()
```

diff_hunk: @@ -473,6 +473,21 @@ def _log_reproduced(self, revs: Iterable[str], tmp_dir: bool = False):
             "\tdvc exp branch <exp> <branch>\n"
         )
 
+    def _ref_name_validation(self, name: Optional[str], **kwargs):
path: dvc/repo/experiments/__init__.py
text: @user1:
```suggestion
    def _validate_ref_name(self, name: Optional[str], **kwargs):
```

method names should be verbs to keep consistency w/the rest of the code base