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

feat: improve error messages when path is not found in commit #13

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

MingweiSamuel
Copy link
Contributor

No description provided.

@MingweiSamuel MingweiSamuel marked this pull request as draft January 26, 2024 01:05
@MingweiSamuel MingweiSamuel marked this pull request as ready for review January 26, 2024 01:23
@MingweiSamuel
Copy link
Contributor Author

Alt

                    if let Some(released_dir_id) = released_target
                        .object()?
                        .peel_to_kind(object::Kind::Tree)?
                        .into_tree()
                        .peel_to_entry(components)?
                        .map(|entry| entry.object_id())
                    {
                        (released_dir_id != current_dir_id).then_some(PackageChangeKind::ChangedOrNew)
                    } else {
                        eprintln!(
                                    "path '{}' in released_target commit `{}` must exist as it was supposedly released there. Was it moved to a different directory? Changelog may have missing entries.",
                                    dir, released_target
                                );
                        Some(PackageChangeKind::ChangedOrNew)
                    }

@MingweiSamuel
Copy link
Contributor Author

I guess moving a package completely obliterates the changelog, including the logs for old versions

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these improvements!

Yes, changelog tracking is very limited as it only tracks renames on the top-level of the repository, not deeper than that. This also prevents me from moving the gix-* crates into a subdirectory.

By now, better technology is available though meaning that full rename tracking is available in gix when diffing trees. It's expensive to compute, but in ein there is a query engine that calculates the information and stores it in an sqlite database, which can be updated incrementally.

If that would be brought into cargo-smart-release it could benefit from full rename tracking, which will be as fast or faster as the current implementation. It's work though, let's see when I get to it. If you feel inclined though, I am happy to mentor - the pieces for this already exist, so 'just' the integration of these remains here.

@Byron Byron merged commit b684fcb into Byron:main Jan 26, 2024
11 checks passed
@MingweiSamuel MingweiSamuel deleted the err-msg branch January 26, 2024 06:34
@MingweiSamuel
Copy link
Contributor Author

So yes I am interested in looking into that and have been reading through the source code. Wondering if a different strategy of looking at the workspace Cargo.toml file to see changed paths might simplify things

@Byron
Copy link
Owner

Byron commented Jan 26, 2024

That's great to hear.

I must recommend though to prefer small PRs over sweeping ones, as this tool is totally under-tested. This means in order to know if it works for me, I have to try publish gitoxide, which is all it really has to do for me, and it has to keep doing that. Thus, testing by hand will be required to merge PRs which takes more time and is more dangerous, so I am more anxious about certain changes, which impedes contributions that change core logic.

Of course, the best way to deal with this would be to build out the test suite before making any changes, but it's something I skimped on for a reason before.

In any case, I hope the ramblings above help at least a little :).

@MingweiSamuel
Copy link
Contributor Author

Is there a way to keep the old changelog? I see code for merging with the old. But when I run the bin it overwrites everything existing

@Byron
Copy link
Owner

Byron commented Jan 26, 2024

It does keep the previous changelog, and the worst I have seen is that it doesn't understand anything and keeps the old changelog as a trailer. But maybe there are cases when that doesn't work.

The merge logic is quite complex, and probably also under-tested compared to the sheer amount of possible inputs, so I wouldn't be surprised if it can do what you describe.

@MingweiSamuel
Copy link
Contributor Author

MingweiSamuel commented Jan 26, 2024

Ok, only the Commit Statistics and Commit Details are getting removed, it seems

Edit: seems they're not being parsed

Seems that it skips generated content. may be possible for me to manually modify the changelogs (so they get detected as user content) for my use case

Yup sweet, that works

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

Successfully merging this pull request may close these issues.

2 participants