Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_git.py:remove_files does not react to git's fatal error if newsfragment is untracked by git #448

Open
fleetingbytes opened this issue Nov 5, 2022 · 11 comments

Comments

@fleetingbytes
Copy link

fleetingbytes commented Nov 5, 2022

I created some news fragments for towncrier to build me my new changelog and after that I inteded to commit the changes to the git repo. So the news fragments themselves were not tracked by git at the time of the changelog generation. When towncrier asked if it is ok to remove the news fragments I said yes and this was the result:

Is it okay if I remove those files? [Y/n]: y
fatal: pathspec '<REDACTED_PATH>\changelog.d\11.improved.md' did not match any files

This error should be expected and handled here

    if answer_yes or click.confirm("Is it okay if I remove those files?", default=True):
        call(["git", "rm", "--quiet"] + fragment_filenames)

it could look something like this:

    if answer_yes or click.confirm("Is it okay if I remove those files?", default=True):
        try:
            subprocess.run(["git", "rm", "--quiet"] + fragment_filenames, check=True)
        except subprocess.CalledProcessError as err:
            ... # handle git's errors here
@fleetingbytes fleetingbytes changed the title _git.py:remove_files throws a fatal error if newsfragment is untracked by git _git.py:remove_files does not react to git's fatal error if newsfragment is untracked by git Nov 5, 2022
@jat255
Copy link

jat255 commented Apr 14, 2023

I just ran across this as well. I didn't realize it was standard (actually, required) to commit the files via git first.

@Dreamsorcerer
Copy link

Another issue is backup files. I got this error after it tried to use a backup file that ended with ~. Ideally, such files should be ignored.

@adiroiban
Copy link
Member

Thanks @Dreamsorcerer for the comment. I think this needs a separate ticket.
A ticket dedicated to adding a default list of ignored files.
Maybe towncrier already ignores some file, but it needs to do more :)

It's important to gather info about what extensions or file formats are used by backup tools/text editor.


I think this might be related to #357

I think that this can be closed as we are now handling this error.

For now, towncrier needs to have the file commited to git... so this works as expected.

Also, in latest releses, a stacktrack is no longer presented.

$ echo "something" > /SNIP/towncrier/src/towncrier/newsfragments/4242.feature
$ towncrier build

SNIP/towncrier/src/towncrier/newsfragments/4242.feature
SNIP/towncrier/src/towncrier/newsfragments/540.bugfix
Is it okay if I remove those files? [Y/n]: y
Removing news fragments...
fatal: pathspec '/SNIP/towncrier/src/towncrier/newsfragments/4242.feature' did not match any files
Done!

I agree that it would be nice to have a better explanation... but that would be a feature.

I am closing this.

We can re-open if anyone thinks that this is still an issue.

Happy to review a PR in which the error message is better and expalins that files must be added to git before being handled by towncrier.

@adiroiban adiroiban closed this as not planned Won't fix, can't repro, duplicate, stale Oct 23, 2023
@Dreamsorcerer
Copy link

I think that this can be closed as we are now handling this error.

In your output, it doesn't look like it tried to remove the second file (540.bugfix).
I would only consider this issue resolved once it can successfully remove all valid files, rather than stopping as soon as it finds a single untracked file.

@adiroiban adiroiban reopened this Oct 23, 2023
@adiroiban
Copy link
Member

Agree. And it should not say Done! if in fact it failed :)

I don't know what is the best way to fix this.

I am ok with just ignoring files that are not managed by git... I don't know if there are any unwanted side-effects by ignoring them.

@Dreamsorcerer
Copy link

A quick glance at the code suggests it's just calling git rm. I don't see anything in the man page to suggest there's a way to make it work better. But, it could just be tweaked to call it once for each file, catching any errors.

Otherwise, with a bigger refactoring, you could consider interfacing with git more directly using gitpython.

@adiroiban
Copy link
Member

adiroiban commented Oct 23, 2023

well, towncrier is a development tool, and not an end user tool.

I expect that devs will know what they are doing

I think that using git rm is good enough. Happy not to add any extra deps.

If this is a big "nerve" for someone and they submit a PR for better error reporting, we can merge this.

For me, this is not a problem. I have all the files in git :)

I am not sure why a file like 11.improved.md is not in git in the first place.

@rbpatt2019
Copy link

Sorry to revive an old thread, but came across this thread while sanity checking my assumptions when I encountered this error. For me, it was pretty obvious that the issue was git remove on an untracked file. What surprised me is that no error was triggered, so I was unexpectedly left with a non-empty newsfragments directory. I think this should trigger an error of some sort that prevents successful completion.
@adiroiban - what are your thoughts? If it's welcomed, I'd be happy to contribute this

@Dreamsorcerer
Copy link

I think if you reread the previous 4 comments, it aligns with your expectation. We should attempt each file, rather than aborting on the first failure, and the message should not indicate that it completed successfully.

@adiroiban
Copy link
Member

Happy to review and approve a PR that fixes this.

As long as the PR has documentation and automated tests and the code is not a big mess, there should be no reason not to merge it.

Thanks

@rbpatt2019
Copy link

@Dreamsorcerer - sorry for the confusion, just wanted to make sure I was understanding the issue correctly.
@adiroiban - I'm happy to put together a PR! I've got work obligations till Friday but have some blue sky time then to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants