adopt-project.py: a --work-dir that is, or runs through, a symlink round a loop is refused in words that name the link and the loop, not "(File exists)", and a symlink to a file is named as one (#978)

adopt-project.py

@@ -5179,26 +5179,27 @@ def _checked_flag_inputs(args, naming: NamingPolicy) -> None:
     source.
 
     NOT EVERY REFUSAL A FLAG DECIDES IS HERE. Still after the source, for
-    an `org/repo` source as for a local one: `naming-unclassified` and
-    `naming-role-mismatch`, which `_check_names` asks of the names
-    `--project` makes, with a remedy about the source; an empty
-    `--elected-by`, which is a value `unwritable_values` holds and the
-    manifest validator refuses (`adopt-manifest-invalid`, in
-    `_refuse_unholdable_values`), and an empty `--reference` with it
-    (#902); and a missing `--path-policy` (`yaml-unreadable`). A passed
-    `--id` that is no id is here only where `naming` holds the names
-    `--project` makes (`_names_held`, #902): beside a name it refuses, the
-    naming refusal speaks first, and its remedy needs the source.
+    an `org/repo` source as for a local one, among them:
+    `naming-unclassified` and `naming-role-mismatch`, which `_check_names`
+    asks of the names `--project` makes, with a remedy about the source
+    where it names a repository; an empty `--elected-by`, which is a value
+    `unwritable_values` holds and the manifest validator refuses
+    (`adopt-manifest-invalid`, in `_refuse_unholdable_values`), and an
+    empty `--reference` with it (#902); and a missing `--path-policy`
+    (`yaml-unreadable`). A passed `--id` that is no id is here only where
+    `naming` holds the names `--project` makes (`_names_held`, #902):
+    beside a name it refuses, the naming refusal speaks first, and its
+    remedy needs the source.
     """
     args.project = checked_value("--project", args.project)
     # A `--tracking-branch` that was passed: PASSED IS ON THE COMMAND LINE,
     # `''` INCLUDED, as #479 reads it. Git's own question follows
     # `checked_value` (#420): a name inside its alphabet that git will not
     # create (`a..b`, `~`, `x.lock`, `a/`) is refused here, in git's words,
     # and is not written for `check` to find it no branch of the source and
-    # the plan to be written again. That question is asked of a throwaway
-    # repository in the system temp folder, which it removes, and never of
-    # the source.
+    # the plan to be written again. That question is asked with an empty
+    # temporary directory as its `--git-dir`, which `plan` removes, and
+    # never of the source.
     if args.tracking_branch is not None:
         tracking = _flag_named(TRACKING_KEY, {TRACKING_KEY})
         args.tracking_branch = checked_value(tracking, args.tracking_branch)
@@ -6026,8 +6027,9 @@ def cmd_plan(args) -> int:
     # made, git had read the source and an `org/repo` one was cloned, and
     # with a clone that failed they were never reached: no work directory
     # is made, git reads nothing of the source and no `gh` runs before
-    # them. A passed `--tracking-branch` runs `git check-ref-format` in a
-    # throwaway repository first, and never touches the source.
+    # them. A passed `--tracking-branch` runs `git check-ref-format` first,
+    # with an empty temporary directory as its `--git-dir`, which `plan`
+    # removes, and never touches the source.
     # The naming policy is read first, a file of this checkout: a passed
     # `--id` is refused with the flags only where the names `--project`
     # makes are ones it holds (#902).
@@ -12280,13 +12282,14 @@ def _refuse_an_occupied_work_dir(work_root: Path, names: dict,
 NO_LEG_YET = "no leg repository was created and nothing was pushed. "
 
 
-def _work_dir_refusal(detail: str, done: str = "") -> Refusal:
+def _work_dir_refusal(detail: str, done: str = "",
+                      fix: str = WORK_DIR_FIX) -> Refusal:
     """`adopt-work-dir-unwritable` for `detail`, why the `--work-dir`
     cannot be used: `_work_root`'s or `_unwritable_work_dir`'s. The one
     refusal of a work directory, in all three commands (#650); `done` is
-    what the refusing command left as it was, said first."""
-    return Refusal(WORK_DIR_UNWRITABLE, detail,
-                   f"Remediation: {done}{WORK_DIR_FIX}")
+    what the refusing command left as it was, said first, and `fix` the way
+    out, which is #524's but for a symlink in the way (#978)."""
+    return Refusal(WORK_DIR_UNWRITABLE, detail, f"Remediation: {done}{fix}")
 
 
 def _unwritable_work_dir(args, work_root: Path,
@@ -16046,8 +16049,11 @@ def _work_root(args, done: str = "") -> Path:
     FileExistsError, NotADirectoryError or PermissionError traceback, exit
     1, and on a loop Python 3.12's `Path.resolve` raised RuntimeError
     first; `os.path.realpath` is the same reading without that raise. The
-    code and the way out are #524's, for one this user cannot write in;
-    `done` is what the refusing command left as it was, said first.
+    code and the way out are #524's, for one this user cannot write in, but
+    a symlink in the way is named, with a way out of its own: one at the
+    `--work-dir` or on the way to it that leads round a loop, or one at it
+    that leads to what is no directory (`_unmade_work_dir`, #978); `done`
+    is what the refusing command left as it was, said first.
 
     A `--work-dir` ANOTHER USER CAN RENAME IN IS REFUSED NEXT, before
     anything is made in it (#803): one another user owns, or one every user
@@ -16071,9 +16077,8 @@ def _work_root(args, done: str = "") -> Path:
         _made_0755(root)
         status = os.lstat(root)
     except OSError as exc:
-        raise _work_dir_refusal(
-            f"--work-dir {_spelled(str(root))} cannot be made: making it a "
-            f"directory failed ({exc.strerror or exc})", done) from None
+        detail, fix = _unmade_work_dir(args.work_dir, root, exc)
+        raise _work_dir_refusal(detail, done, fix) from None
     _refuse_a_shared_chain(root, done)
     shared = _shared_with_another_user(status)
     if shared:
@@ -16084,6 +16089,89 @@ def _work_root(args, done: str = "") -> Path:
     return root
 
 
+#: Why a `--work-dir` that is a symlink round a loop cannot be made (#978).
+SYMLINK_LOOP = "it is a symbolic link that leads round a loop"
+
+
+def _unmade_work_dir(work_dir: Path, root: Path,
+                     exc: OSError) -> tuple[str, str]:
+    """Why the `--work-dir` `work_dir`, which `_work_root` read as `root`
+    (`os.path.realpath`), could not be made, where making it failed with
+    `exc`, and the way out: `_work_root`'s refusal (#650).
+
+    A SYMLINK IN THE WAY IS NAMED AS ONE (#978). `os.mkdir` of a symlink
+    that leads round a loop fails with EEXIST, the error a file in the way
+    gets, so the reason was "(File exists)": the person was told a file is
+    there, and found a link, and the way out was for a folder this user
+    cannot write in. Where the `--work-dir` is a symlink, or the way to it
+    runs through one, that leads round a loop, the reason says so and names
+    the link, and the link in the loop where that is another; where it is a
+    symlink that leads to a file, or to anything else that is no directory,
+    the reason names the link and what it leads to. The way out is to
+    remove the link or make it lead to a directory: the `--work-dir` itself
+    where it is a symlink, else the link in the loop. The `--work-dir` is
+    named as given, made absolute and not resolved, since
+    `os.path.realpath` stops at a loop on a path the person may never have
+    typed.
+
+    A DANGLING SYMLINK IS NO LOOP: `os.path.realpath` reads it as the path
+    it leads to, which `_made_0755` makes, and only where that fails does
+    it get here. That failure, and every other, a file at the `--work-dir`
+    or above it among them, is said in the system's own words, of `root`,
+    as before."""
+    given = work_dir.absolute()
+    link = given if os.path.islink(given) else None
+    looped = (_the_loop_in(root)
+              if exc.errno in (errno.EEXIST, errno.ELOOP) else None)
+    if looped is not None:
+        if link is None:
+            why = ("the way to it leads round a loop, at the symbolic link "
+                   f"{_spelled(str(looped))}")
+            link = looped
+        elif _same_entry(link, looped):
+            why = SYMLINK_LOOP
+        else:
+            why = (f"{SYMLINK_LOOP}, at the symbolic link "
+                   f"{_spelled(str(looped))}")
+    elif link is not None and exc.errno == errno.EEXIST:
+        why = (f"it is a symbolic link that leads to {_spelled(str(root))}, "
+               "which is not a directory")
+    else:
+        return (f"--work-dir {_spelled(str(root))} cannot be made: making it "
+                f"a directory failed ({exc.strerror or exc})", WORK_DIR_FIX)
+    return (f"--work-dir {_spelled(str(given))} cannot be made: {why}",
+            f"Remove the symbolic link {_spelled(str(link))}, or make it "
+            "lead to a directory, or pass another --work-dir, or none: "
+            "without one, each run makes a new one.")
+
+
+def _the_loop_in(root: Path) -> Path | None:
+    """The symlink in `root`, a `--work-dir` as `os.path.realpath` read it,
+    that leads round a loop; None where there is none (#978).
+    `os.path.realpath` follows every symlink but one it meets again before
+    it has resolved it, which it leaves in the path as it found it, with
+    the rest of the path unread after it. So the first symlink in `root`,
+    from `/` down, is that one, and the system is asked whether following
+    it loops (ELOOP), so that nothing else is called a loop."""
+    for part in (*reversed(root.parents), root):
+        if os.path.islink(part):
+            try:
+                os.stat(part)
+            except OSError as exc:
+                return part if exc.errno == errno.ELOOP else None
+            return None
+    return None
+
+
+def _same_entry(one: Path, other: Path) -> bool:
+    """Whether `one` and `other` are the same directory entry, a symlink
+    taken as itself and not followed."""
+    try:
+        return os.path.samestat(os.lstat(one), os.lstat(other))
+    except OSError:
+        return False
+
+
 def _holds_a_file(root: Path) -> bool:
     """Whether anything but directories is in `root`, at any depth: a file,
     a symlink, anything else that is not a directory. A directory that

tests/test_adopt_clone_claimed.py

@@ -39,7 +39,8 @@
 import pytest
 
 from conftest import ADOPT, WINDOWS_SKIP, make_source_repo, rmtree, spelled
-from test_adopt_work_dir_refusal import refused_alike
+from test_adopt_work_dir_refusal import (refused_alike, refused_for_a_link,
+                                         through_a_loop)
 from test_adopt_source_clone_record import (  # noqa: F401
     MINE, OCCUPIED, SOURCE_READONLY, WORK, WORK_FLAG, clone_dir, modes_hold,
     on_github, plan_args, run, symlinks_are_made,
@@ -731,20 +732,25 @@ def test_a_loop_or_a_file_above_the_work_dir_keeps_its_650_refusal(
         tmp_path, command, above):
     """`--work-dir <T>/loop/W` (`loop -> loop`) and `<T>/notes.txt/W`, a
     file every user can write: no directory can be made there, and each
-    command refuses it as #650 does, in main's words. On `bba7e445` each was
-    `adopt-work-dir-shared`, naming the link or the file as a directory
-    every user can write in: a symlink always reads as mode 0777."""
+    command refuses it as #650 does, `adopt-work-dir-unwritable`: the file
+    in the system's words, and the loop in #978's, which name the link. On
+    `bba7e445` each was `adopt-work-dir-shared`, naming the link or the
+    file as a directory every user can write in: a symlink always reads as
+    mode 0777."""
     planned_locally(tmp_path, {}, {}, {})
     if above == "a-symlink-loop":
         in_the_way = tmp_path / "loop"
         try:
             in_the_way.symlink_to(in_the_way.name)
         except OSError:
             pytest.skip("this machine cannot make a symlink")
-    else:
-        in_the_way = tmp_path / "notes.txt"
-        in_the_way.write_text(MINE, encoding="utf-8")
-        os.chmod(in_the_way, 0o666)
+        loop = real(tmp_path) / in_the_way.name
+        refused_for_a_link(command, tmp_path, in_the_way / WORK,
+                           through_a_loop(loop), loop)
+        return
+    in_the_way = tmp_path / "notes.txt"
+    in_the_way.write_text(MINE, encoding="utf-8")
+    os.chmod(in_the_way, 0o666)
     refused_alike(command, tmp_path, in_the_way / WORK)
 
 

tests/test_adopt_plan_flag_refusals_first.py

@@ -26,18 +26,19 @@
 
 Now the refusals of a passed flag's own value are asked first, as
 `--pin-source` and `--out` are: no work directory, no `git` on the source and
-no `gh` before them (a passed `--tracking-branch` is asked of git alone, in a
-throwaway repository). An `org/repo` source is held to its derived id, `--org`
-and `--project` before the clone, and its unwritable folder is refused in
-words about the clone. A local source's folder is still made, since `--out`
-may lie in it (`tests/test_adopt_plan_out_checked.py`), and still refused
-where it cannot be made (`tests/test_adopt_work_dir_refusal.py`); it is no
-longer probed.
+no `gh` before them (a passed `--tracking-branch` is asked of git alone, with
+an empty temporary directory as its `--git-dir`, which `plan` removes). An
+`org/repo` source is held to its derived id, `--org` and `--project` before
+the clone, and its unwritable folder is refused in words about the clone. A
+local source's folder is still made, since `--out` may lie in it
+(`tests/test_adopt_plan_out_checked.py`), and still refused where it cannot
+be made (`tests/test_adopt_work_dir_refusal.py`); it is no longer probed.
 
 Not every refusal a flag decides is asked first. Still after the source is
-read, for an `org/repo` source as for a local one: `naming-unclassified` and
-`naming-role-mismatch`, an empty `--elected-by` (the manifest validator's
-`adopt-manifest-invalid`) and a missing `--path-policy` (`yaml-unreadable`).
+read, for an `org/repo` source as for a local one, among them:
+`naming-unclassified` and `naming-role-mismatch`, an empty `--elected-by`
+(the manifest validator's `adopt-manifest-invalid`) and a missing
+`--path-policy` (`yaml-unreadable`).
 
 The runs here are given a fresh `TMPDIR`, a `GIT_TRACE` file that any `git`
 process would write, a `--work-dir` that is not there, and a `gh` that logs
@@ -240,9 +241,10 @@ def test_a_refusal_the_flags_alone_decide_is_asked_before_anything_exists(
 
 def test_the_branch_a_tracking_branch_names_is_asked_before_the_source(
         harness, source):
-    """`--tracking-branch a..b` is git's to refuse, and is asked of a
-    throwaway repository in the temp folder, never of the source: the only
-    `git` that ran is `check-ref-format`, and it removed what it made."""
+    """`--tracking-branch a..b` is git's to refuse, and is asked of git
+    with an empty temporary directory as its `--git-dir`, never of the
+    source: the only `git` that ran is `check-ref-format`, and the
+    directory was removed."""
     for given in (REPOSITORY, str(source)):
         result = harness.plan(given, "--tracking-branch", "a..b", work=True)
         assert result.returncode == 2, result.stdout + result.stderr