Repository: pallets/click (python)

Issue:
Bug: `make_default_short_help` can return strings as long as `max_length + 3`
## The bug
The current algorithm doesn't take into account the length of "..." properly:

```python
from click.utils import make_default_short_help

out = make_default_short_help('1234 67890 xxxxxx', max_length=10)
print(out)
print('Length:', len(out))
```
Prints:
```
1234 67890...
Length: 13
```
Expected output: `1234...`

## Environment
- Click version: 7.2 and master

## Solution
I'm going to open a PR.

## Related issue (or: why this bug has never been reported)
The reason this bug has not been reported is because another "issue" compensates for it, making sure it never manifests itself in the terminal. The following code is from `Group.format_commands`:

https://github.com/pallets/click/blob/0dc6d74df5c984ff923f5d17e97bf94a21b3a3cd/src/click/core.py#L1501-L1508

`Group.format_commands` needs to know `col_spacing` to calculate the width of the 2nd column (i.e. the short help `limit`). But it: 
- doesn't set `col_spacing` in `formatter.write_dl` and, 
- it assumes that the formatter won't necessarily use the default `col_spacing` value (note that the only way to change it is to subclass `HelpFormatter` and override `write_dl`; not a very clean use of inheritance).

As a consequence, it makes the conservative choice of "allowing for 3 times the default `col_spacing`" subtracting 6 to `limit` (i.e. `max_length`). Because `col_spacing` will always be 2, the `limit` will always be 4 characters smaller than needed, compensating for the eventual excess of 3 characters due to the bug reported in this issue.

Let me know if I misunderstood something.

Pull request description:
Fix and refactor make_default_short_help (issue #1849)
Fixes #1849. 

I found the old code a little bit difficult to follow, so I rewrote it. As it written now, the code can also be easily generalized to an arbitrary overflow placeholder.

Checklist:

- [x] Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
- [x] **[Not relevant]** Add or update relevant docs, in the docs folder and in code.
- [x] Add an entry in `CHANGES.rst` summarizing the change and linking to the issue.
- [x] **[Not relevant]** Add `.. versionchanged::` entries in any relevant code docs.
- [x] Run `pre-commit` hooks and fix any issues.
- [x] Run `pytest` and `tox`, no tests failed.


Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 86942e5..d532792 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -210,6 +210,8 @@ Unreleased
     ``result_callback``. The old name is deprecated. :issue:`1160`
 -   Fix issues with ``CliRunner`` output when using ``echo_stdin=True``.
     :issue:`1101`
+-   Fix a bug of ``click.utils.make_default_short_help`` for which the
+    returned string could be as long as ``max_width + 3``. :issue:`1849`
 
 
 Version 7.1.2
diff --git a/src/click/utils.py b/src/click/utils.py
index 41ce001..114afcc 100644
--- a/src/click/utils.py
+++ b/src/click/utils.py
@@ -47,35 +47,54 @@ def make_str(value):
     return str(value)
 
 
-def make_default_short_help(help, max_length=45):
-    """Return a condensed version of help string."""
-    line_ending = help.find("\n\n")
-    if line_ending != -1:
-        help = help[:line_ending]
+def make_default_short_help(help: str, max_length: int = 45) -> str:
+    """Returns a condensed version of help string."""
+    # Consider only the first paragraph.
+    paragraph_end = help.find("\n\n")
+
+    if paragraph_end != -1:
+        help = help[:paragraph_end]
+
+    # Collapse newlines, tabs, and spaces.
     words = help.split()
-    total_length = 0
-    result = []
-    done = False
 
+    if not words:
+        return ""
+
+    # The first paragraph started with a "no rewrap" marker, ignore it.
     if words[0] == "\b":
         words = words[1:]
 
-    for word in words:
-        if word[-1:] == ".":
-            done = True
-        new_length = 1 + len(word) if result else len(word)
-        if total_length + new_length > max_length:
-            result.append("...")
-            done = True
-        else:
-            if result:
-                result.append(" ")
-            result.append(word)
-        if done:
+    total_length = 0
+    last_index = len(words) - 1
+
+    for i, word in enumerate(words):
+        total_length += len(word) + (i > 0)
+
+        if total_length > max_length:  # too long, truncate
             break
-        total_length += new_length
 
-    return "".join(result)
+        if word[-1] == ".":  # sentence end, truncate without "..."
+            return " ".join(words[: i + 1])
+
+        if total_length == max_length and i != last_index:
+            break  # not at sentence end, truncate with "..."
+    else:
+        return " ".join(words)  # no truncation needed
+
+    # Account for the length of the suffix.
+    total_length += len("...")
+
+    # remove words until the length is short enough
+    while i > 0:
+        total_length -= len(words[i]) + (i > 0)
+
+        if total_length <= max_length:
+            break
+
+        i -= 1
+
+    return " ".join(words[:i]) + "..."
 
 
 class LazyFile:


Test patch:
diff --git a/tests/test_utils.py b/tests/test_utils.py
index b77d74b..23b3709 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -444,3 +444,39 @@ def test_expand_args(monkeypatch):
     assert "setup.cfg" in click.utils._expand_args(["*.cfg"])
     assert os.path.join("docs", "conf.py") in click.utils._expand_args(["**/conf.py"])
     assert "*.not-found" in click.utils._expand_args(["*.not-found"])
+
+
+@pytest.mark.parametrize(
+    ("value", "max_length", "expect"),
+    [
+        pytest.param("", 10, "", id="empty"),
+        pytest.param("123 567 90", 10, "123 567 90", id="equal length, no dot"),
+        pytest.param("123 567 9. aaaa bbb", 10, "123 567 9.", id="sentence < max"),
+        pytest.param("123 567\n\n 9. aaaa bbb", 10, "123 567", id="paragraph < max"),
+        pytest.param("123 567 90123.", 10, "123 567...", id="truncate"),
+        pytest.param("123 5678 xxxxxx", 10, "123...", id="length includes suffix"),
+        pytest.param(
+            "token in ~/.netrc ciao ciao",
+            20,
+            "token in ~/.netrc...",
+            id="ignore dot in word",
+        ),
+    ],
+)
+@pytest.mark.parametrize(
+    "alter",
+    [
+        pytest.param(None, id=""),
+        pytest.param(
+            lambda text: "\n\b\n" + "  ".join(text.split(" ")) + "\n", id="no-wrap mark"
+        ),
+    ],
+)
+def test_make_default_short_help(value, max_length, alter, expect):
+    assert len(expect) <= max_length
+
+    if alter:
+        value = alter(value)
+
+    out = click.utils.make_default_short_help(value, max_length)
+    assert out == expect