Repository: twisted/towncrier
Pull request #357 (2021-07-22 08:30:06)

Handle deletion of uncommitted news fragments
Before this commit, all the news fragments needed to be committed into git, or the fragments removal after building the news file would crash.

In my workflow, I add missing fragments before building the news file because I'm extracting author names from the git log, and towncrier crashes at the end of the build process.

codecov[bot]: # [Codecov](https://codecov.io/gh/twisted/towncrier/pull/357?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted) Report
> Merging [#357](https://codecov.io/gh/twisted/towncrier/pull/357?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted) (cb08709) into [master](https://codecov.io/gh/twisted/towncrier/commit/a0a34d3ee76acd424d69f46a240bbec513c39bb5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted) (a0a34d3) will **decrease** coverage by `6.05%`.
> The diff coverage is `90.00%`.

[![Impacted file tree graph](https://codecov.io/gh/twisted/towncrier/pull/357/graphs/tree.svg?width=650&height=150&src=pr&token=9npndnog2E&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted)](https://codecov.io/gh/twisted/towncrier/pull/357?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted)

```diff
@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
- Coverage   96.85%   90.80%   -6.06%     
==========================================
  Files          20       11       -9     
  Lines        1241      500     -741     
  Branches      120      101      -19     
==========================================
- Hits         1202      454     -748     
- Misses         20       24       +4     
- Partials       19       22       +3     
```


| [Impacted Files](https://codecov.io/gh/twisted/towncrier/pull/357?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted) | Coverage Δ | |
|---|---|---|
| [src/towncrier/\_git.py](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted#diff-c3JjL3Rvd25jcmllci9fZ2l0LnB5) | `75.86% <88.23%> (+9.19%)` | :arrow_up: |
| [src/towncrier/check.py](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted#diff-c3JjL3Rvd25jcmllci9jaGVjay5weQ==) | `93.87% <100.00%> (-0.57%)` | :arrow_down: |
| [src/towncrier/\_settings.py](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted#diff-c3JjL3Rvd25jcmllci9fc2V0dGluZ3MucHk=) | `89.02% <0.00%> (-7.32%)` | :arrow_down: |
| [src/towncrier/\_builder.py](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted#diff-c3JjL3Rvd25jcmllci9fYnVpbGRlci5weQ==) | `92.85% <0.00%> (-1.43%)` | :arrow_down: |
| [src/towncrier/test/test\_build.py](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted#diff-c3JjL3Rvd25jcmllci90ZXN0L3Rlc3RfYnVpbGQucHk=) | | |
| [src/towncrier/test/test\_write.py](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted#diff-c3JjL3Rvd25jcmllci90ZXN0L3Rlc3Rfd3JpdGUucHk=) | | |
| [src/towncrier/test/test\_check.py](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted#diff-c3JjL3Rvd25jcmllci90ZXN0L3Rlc3RfY2hlY2sucHk=) | | |
| [src/towncrier/\_\_main\_\_.py](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted#diff-c3JjL3Rvd25jcmllci9fX21haW5fXy5weQ==) | | |
| [src/towncrier/test/test\_create.py](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted#diff-c3JjL3Rvd25jcmllci90ZXN0L3Rlc3RfY3JlYXRlLnB5) | | |
| [src/towncrier/test/test\_settings.py](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted#diff-c3JjL3Rvd25jcmllci90ZXN0L3Rlc3Rfc2V0dGluZ3MucHk=) | | |
| ... and [3 more](https://codecov.io/gh/twisted/towncrier/pull/357/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/twisted/towncrier/pull/357?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/twisted/towncrier/pull/357?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted). Last update [a0a34d3...cb08709](https://codecov.io/gh/twisted/towncrier/pull/357?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=twisted).

jaraco: It looks like this issue has gotten a little stale, and since we haven't heard back from the contributor, I'm going to close it for now. If you'd like to revive the effort, feel free to ping this PR and we can re-open, or start a new one. Thanks.
abompard: Hey folks! Sorry I had totally forgotten about this PR. I'll try to find some time to finalize it, if it's still relevant.
abompard: Could you please reopen @jaraco ?
adiroiban: Thanks for the update.

Can you please fix the conflicts and update the docstring for `run` to describe it's current behaviour

We no longer support python 2.7
abompard: I'm more than happy to remove Python 2.7 remnants :-D
Alright, it's updated and tests should pass now.
abompard: I've made the changes, thanks a lot for your patience. FYI I do appreciate the in-depth reviews, even for minor style changes. I'm frequently on the other end of those ;-)
So thanks.
adiroiban: Thanks for the update. I will let Chris do the final review and approval for this PR.

@abompard Note that we merge with squash.
There is not much use in rewriting the history for the PR branch.
Rewriting/amending the commits on the PR make it harder to do a followup for a review.

Cheers
abompard: > @abompard Note that we merge with squash. There is not much use in rewriting the history for the PR branch. Rewriting/amending the commits on the PR make it harder to do a followup for a review.

Ah, sorry about that, I didn't know.

FYI, I've had this same issue many times (trying to review PRs that rebase/rewrite) and I ended up writing a small tool to help me with the workflow: https://pypi.org/project/git-pr-branch/. The main idea is that it will download the different revisions of a PR in different branches that you can diff, even if it's always 1 commit that's being rebased.
Maybe you'll find it useful for your workflow as well.
adiroiban: No problem.
I am looking after `towncrier` as my 4th job ... so I don't have much time for it.
This is why, using the embedded GitHub web tools makes it easier to review.

For most of the reviews, I don't checkout the code on my computer.
I am just doing a code review and hope the automated tests are doing their job.
adiroiban: needs-review

Patch:
diff --git a/docs/cli.rst b/docs/cli.rst
index 6adbb94..c804046 100644
--- a/docs/cli.rst
+++ b/docs/cli.rst
@@ -28,8 +28,9 @@ If there are no news fragments (including an empty fragments directory or a
 non-existent directory), a notice of "no significant changes" will be added to
 the news file.
 
-By default, the processed news fragments are removed using ``git``, which will
-also remove the fragments directory if now empty.
+By default, the processed news fragments are removed. For any fragments
+committed in your git repository, git rm will be used (which will also remove
+the fragments directory if now empty).
 
 .. option:: --draft
 
diff --git a/src/towncrier/_git.py b/src/towncrier/_git.py
index 94f82d6..ff3d544 100644
--- a/src/towncrier/_git.py
+++ b/src/towncrier/_git.py
@@ -5,12 +5,27 @@ from __future__ import annotations
 
 import os
 
-from subprocess import STDOUT, call, check_output
+from subprocess import STDOUT, CalledProcessError, call, check_output
 
 
 def remove_files(fragment_filenames: list[str]) -> None:
-    if fragment_filenames:
-        call(["git", "rm", "--quiet"] + fragment_filenames)
+    if not fragment_filenames:
+        return
+
+    # Filter out files that are unknown to git
+    try:
+        git_fragments = check_output(
+            ["git", "ls-files"] + fragment_filenames, encoding="utf-8"
+        ).split("\n")
+    except CalledProcessError:
+        # we may not be in a git repository
+        git_fragments = []
+
+    git_fragments = [os.path.abspath(f) for f in git_fragments if os.path.isfile(f)]
+    call(["git", "rm", "--quiet", "--force"] + git_fragments)
+    unknown_fragments = set(fragment_filenames) - set(git_fragments)
+    for unknown_fragment in unknown_fragments:
+        os.remove(unknown_fragment)
 
 
 def stage_newsfile(directory: str, filename: str) -> None:
diff --git a/src/towncrier/newsfragments/357.feature.rst b/src/towncrier/newsfragments/357.feature.rst
new file mode 100644
index 0000000..d459d5c
--- /dev/null
+++ b/src/towncrier/newsfragments/357.feature.rst
@@ -0,0 +1,1 @@
+``towncrier build`` now handles removing news fragments which are not part of the git repository. For example, uncommitted or unstaged files.


Test patch:
diff --git a/src/towncrier/test/test_build.py b/src/towncrier/test/test_build.py
index 5da7d82..5141fe3 100644
--- a/src/towncrier/test/test_build.py
+++ b/src/towncrier/test/test_build.py
@@ -6,6 +6,7 @@ import tempfile
 
 from datetime import date
 from pathlib import Path
+from subprocess import call
 from textwrap import dedent
 from unittest.mock import patch
 
@@ -747,8 +748,8 @@ class TestCli(TestCase):
                     "--yes",
                 ],
             )
-            # not git repository, manually remove fragment file
-            Path(f"newsfragments/{fragment_file}").unlink()
+            # Fragment files unknown to git are removed even without a git repo
+            assert not Path(f"newsfragments/{fragment_file}").exists()
             return result
 
         results = []
@@ -845,8 +846,8 @@ class TestCli(TestCase):
                 ],
                 catch_exceptions=False,
             )
-            # not git repository, manually remove fragment file
-            Path(f"newsfragments/{fragment_file}").unlink()
+            # Fragment files unknown to git are removed even without a git repo
+            assert not Path(f"newsfragments/{fragment_file}").exists()
             return result
 
         results = []
@@ -1530,3 +1531,55 @@ class TestCli(TestCase):
 
         self.assertEqual(0, result.exit_code, result.output)
         self.assertEqual(expected_output, result.output)
+
+    @with_git_project()
+    def test_uncommitted_files(self, runner, commit):
+        """
+        At build time, it will delete any fragment file regardless of its stage,
+        included files that are not part of the git reporsitory,
+        or are just staged or modified.
+        """
+        # 123 is committed, 124 is modified, 125 is just added, 126 is unknown
+
+        with open("foo/newsfragments/123.feature", "w") as f:
+            f.write("Adds levitation. File committed.")
+        with open("foo/newsfragments/124.feature", "w") as f:
+            f.write("Extends levitation. File modified in Git.")
+
+        commit()
+
+        with open("foo/newsfragments/125.feature", "w") as f:
+            f.write("Baz levitation. Staged file.")
+        with open("foo/newsfragments/126.feature", "w") as f:
+            f.write("Fix (literal) crash. File unknown to Git.")
+
+        with open("foo/newsfragments/124.feature", "a") as f:
+            f.write(" Extended for an hour.")
+        call(["git", "add", "foo/newsfragments/125.feature"])
+
+        result = runner.invoke(_main, ["--date", "01-01-2001", "--yes"])
+
+        self.assertEqual(0, result.exit_code)
+        for fragment in ("123", "124", "125", "126"):
+            self.assertFalse(os.path.isfile(f"foo/newsfragments/{fragment}.feature"))
+
+        path = "NEWS.rst"
+        self.assertTrue(os.path.isfile(path))
+        news_contents = open(path).read()
+        self.assertEqual(
+            news_contents,
+            dedent(
+                """\
+                Foo 1.2.3 (01-01-2001)
+                ======================
+
+                Features
+                --------
+
+                - Adds levitation. File committed. (#123)
+                - Extends levitation. File modified in Git. Extended for an hour. (#124)
+                - Baz levitation. Staged file. (#125)
+                - Fix (literal) crash. File unknown to Git. (#126)
+                """
+            ),
+        )