Skip to content

Conversation

@Lestropie
Copy link
Member

@Lestropie Lestropie commented Jul 24, 2025

Just couple of little snags discovered while building up the next-gen FBA pipeline.


I was about to start doing a git bisect to try to figure out why on dev for C++ commands, there is often, preceding the appearance of a progress bar, an extra erroneous progress bar with no message that immediately shows 100% completion. Eg.:

~/src/mrtrix3/build_debug/bin/mrconvert mask.nii deleteme.mif -force
mrconvert: [WARNING] existing output files will be overwritten
mrconvert: [100%] 
mrconvert: [100%] copying from "mask.nii" to "deleteme.mif"

Line 3 shouldn't be there. It does not occur if -nthreads 0 is used.

But from my first backtrace, it's almost certainly due to #2755 copying rather than moving something. @daljit46 Could you take that one on?

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie Lestropie force-pushed the dev_terminal_fixes branch from 18b91ec to ff02cd2 Compare July 24, 2025 12:55
@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@daljit46
Copy link
Member

daljit46 commented Jul 25, 2025

But from my first backtrace, it's almost certainly due to #2755 copying rather than moving something. @daljit46 Could you take that one on?

This issue stems from an incorrect move constructor for the ProgressBar. Since the class has a private const bool show member variable, the default move constructor issues a copy of the variable.
I think what we should do here is provide the ProgressBar class with a custom move constructor that ensures the original moved-from instance has show set to false. This requires making the show member non-const (generally speaking, I think we should avoid const member variables in a class). @jdtournier any thoughts?

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie
Copy link
Member Author

@daljit46 Makes sense to me, I think we just go with that.

@jdtournier
Copy link
Member

I think what we should do here is provide the ProgressBar class with a custom move constructor that ensures the original moved-from instance has show set to false. This requires making the show member non-const (generally speaking, I think we should avoid const member variables in a class). @jdtournier any thoughts?

Sounds spot on to me too 👍

Implements move constructors and assignment operators for
the ProgressBar class.
Also moves existing constructor implementation in cpp file.
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie
Copy link
Member Author

@MRtrix3/mrtrix3-devs: I'm using this PR to describe what I believe is the current situation with respect to erroneous force pushes.

All PRs on GitHub that are based on dev are still showing as containing a huge number of commits. This I believe to be misleading on the GitHub side, and will either resolve itself in time or can be resolved explicitly.

The discussion history of this PR claims that there have been two force pushes to target branch dev:

Screenshot from 2025-08-27 11-55-41 Screenshot from 2025-08-27 11-56-06

This is however incongruent with the current tip of dev as reported by GitHub itself (70031c3):

Screenshot from 2025-08-27 11-58-19

This I believe to be the "correct" current tip of dev. I would however appreciate external verification on that one from anyone with a clone that hasn't been synced within the last 24 hours.

I believe that in my mess yesterday, I have done a third force push of dev to revert it to 70031c3, but for whatever reason that is not showing up in the PRs that target dev.

To test this, I hit the GitHub "Update branch" button on this PR to explicitly load in the latest dev. That appears as dff5e72:

Screenshot from 2025-08-27 12-07-52

, which shows as a clean merge:

Screenshot from 2025-08-27 12-09-07

Importantly, it shows as its parents dff5e72e, which was the previous tip of this branch, and 70031c3, which I believe to be the correct tip for dev:

Screenshot from 2025-08-27 12-07-01

I therefore believe that as soon as a PR targeting dev is merged (#3101 would make sense, as that should be followed by #3149, which is required to pass CI), all PRs targeting dev should be refreshed on GitHub. If that doesn't happen, then an explicit update of each of those branches---as exemplified here---should resolve.

@Lestropie
Copy link
Member Author

Example of current misleading state of PRs (#3125):

Screenshot from 2025-08-27 12-18-09

@daljit46
Copy link
Member

This I believe to be the "correct" current tip of dev. I would however appreciate external verification on that one from anyone with a clone that hasn't been synced within the last 24 hours.

I only verified until a couple of commits before HEAD (as my local clone of dev is outdated by a couple of weeks), but this looks correct to me. I think you could also double-check with something like git rev-parse "origin/dev@{2025-08-26 00:00}" on your local clone.
If you don't want to go through the hassle of updating every PR that is out of sync with dev, it should be fairly straightforward to use the GitHub CLI to update all open PRs (rather than doing a local merge + push).

@Lestropie
Copy link
Member Author

Lestropie commented Aug 28, 2025

OK cool, not seen that rev-parse usage before.

git rev-parse "origin/dev@{2025-08-26 14:00}"
af5daaa7edae2c25e6c2e7d03af608fd86e90397
git rev-parse "origin/dev@{2025-08-26 15:00}"
70031c348fcefb16b534e342a9f87a5d1a343c94

That corresponds to #3153.

@daljit46
Copy link
Member

daljit46 commented Aug 28, 2025

It seems like the changes I made in 2b42c34 have been discarded. We could probably cherry-pick that commit again.

@Lestropie
Copy link
Member Author

There's a good chance that any work that anyone else had pushed to a branch on GitHub after the most recent git fetch that I had performed on my own system will have been lost.

It is however likely that this is flagging that there's still more fundamental issues. 2b42c34 shows as its parent dda9fd2, which is a copy of what's currently appearing in this PR as d21d986.

I expect more likely what's happening is that a lot of PRs have been moved to point to rebased commits. When I force-pushed dev back to 70031c3, which is where it should be, that resulted in all PRs showing massive changes, because those feature branches are now pointing to unexpectedly rebased versions. An explicit merge resolves because there's no actual differences in code content, but it's erroneous duplication nevertheless.

I'm hoping that on my local clone, I can use the rev-parse usage shown above to query what the tip of each branch individually used to be, and force push each individually back to where it should be.

@Lestropie
Copy link
Member Author

It seems like the changes I made in 2b42c34 have been discarded. We could probably cherry-pick that commit again.

Correction: Original commit was in fact 0d0ef7b; 2b42c34 is an erroneous rebase of such.

I've figured out how to use the GitHub REST CLI to query event history. I think I can go through the list of branches, and for each, find the first erroneous force push, and revert the branch back to its tip before that force push occurred.

@Lestropie Lestropie marked this pull request as ready for review September 15, 2025 05:55
@Lestropie Lestropie merged commit b3fbda8 into dev Sep 15, 2025
16 of 17 checks passed
@Lestropie Lestropie deleted the dev_terminal_fixes branch September 15, 2025 06:14
Lestropie added a commit that referenced this pull request Sep 29, 2025
- Re-generate command documentation.
- Resolve additions to CLI with augmentations to Python CLI in #2678.
- Resolve some poorly formed code logic identified by pylint.
- Improve progressbar appearance when not printing to a terminal, consistent with #3137.
- Fix export of optimal gradient table.
- Increase tolerance on default test: because a greater spectrum of gradient table alterations are now possible, it is more probable that a table that is not precisely equivalent to the unmodified table will be selected due to chance, but this should not lead to test failure given the stochasticity of the command and the relatively small number of streamlines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants