Add Repository::in_progress_operation() #382
Conversation
|
The obvious omission here is test coverage. I'm not sure what you had in mind. For my project I've been setting up repos as minimum viable cases and tarring them up so as to avoid depending on a specific version of git being installed locally. The tests I've used for this PR live here: |
There was a problem hiding this comment.
That's great work, I am loving it!
I also took a look at the tests and would love to see them ported like so. The git-testtools crate allows to do what you need. Note that it makes the following assumptions:
- The
gitbinary is recent enough - the
gitbinary is a stable interface to git.
With this in mind, you are able to get a Repository on the result of a shell script, where the following lines are a mandatory preamble.
#!/bin/bash
set -eu -o pipefail
git init -q
git config commit.gpgsign false
From there it should be possible to adapt the shell scripts in the test repository mentioned here and run the tests.
Thanks a lot, can't wait to get this merged.
|
By the way, I love the idea of freezing the script output into tar files and untarring them on the fly. This is something one could use to reduce the time tests take on CI especially on windows which is awkwardly slow in executing processes. It will probably take some work but I am inspired to find a way to support on-demand freezing of slow-running fixture shell scripts which would be picked up transparently if available, and which are tied to the version of the fixture that created them. I would, however, avoid putting these in to the repository and instead use |
(apologies for the CI churn)
1.) Install gnu-sed on the macOS instances 2.) Remember that which(1) sets a non-zero exit code if it doesn't find each item you've specified. In this case we're searching for 'gsed' and 'sed' in the hopes of finding at least one. The ultimate goal is to prefer gsed if it exists as systems with a BSD userland tend to install GNU tools with a 'g' prefix such that GNU sed is accessible as 'gsed'. GNU sed lets us easily (in a somewhat portable way) edit the second line of a sequence so that we can change one 'pick' to 'edit'. All while pretending to be an interactive text editor to make git happy.
|
Apologies for all the CI churn. I did, however, notice something interesting. The Windows test instance gave a reasonably descriptive error message upon failure but the Ubuntu test instance did not. This is all the Ubuntu test showed: In this case it should be the same failure on both platforms. |
|
These seem to be different errors though, maybe the shell on MacOS has trouble? MacOSWindows |
|
The root problem was that I didn't realize
My goal is to take a look at the rest of your feedback in the next few days. |
|
Thanks a lot for the summary! When reading this I thought I was happy that the whole fixture setup affair worked so well for me thus far. Regarding feedback, there isn't more except for the two minor changes that are probably fixed already (I yet have to take a look), and the notes about using the archive-technique in |
|
Thanks for the update. I took another look and noticed that for some reason GitHub is all hung up on something so won't run CI while displaying the commit list like this:
I assume it's the same for you, but not having CI is a bit of an issue. Maybe it's because of the conflict in In any case, please let me know once I should take a look again for a merge attempt. Thanks. |
|
Correct. The whole point of most of these commits is to ensure test coverage. I've merged the latest changes from |
|
To the other point, cross platform shell scripting is a bit of a black art and that's a large part of why I split out the generation of the repositories from the tests on my own projects. This PR isn't too bad in that regard, but it would be nice to find a more portable solution to feigning an interactive The other thing I've noticed is that if a fixture fails the artifacts are left in place. What are your thoughts on deleting or moving them out of the way? The issue I've had is that if I'm trying to do some quick and dirty A/B testing I will likely be using the same two variants (a.k.a. the same two CRC values) and have to manually remove the generated directory to ensure the fixture is run again. Alternatively what about using the fixture's mtime as part of the data to hash? |
I think the current default behaviour is confusing and undesirable. Instead it would be better to cleanup automatically while allowing to set some environment variable to keep the previous version to help with debugging (my previous default case :D). I understand that if some directory is there the next time it runs it happily takes the state of the previous failed run. You are very welcome to make these adjustments in this PR.
Is this question obsolete by the statement above? |
|
Not sure if it is appropriate for this function, as it is more expensive, but it would be very nice to also get progress information on the states (starship implementation). |
|
@Byron Yeah, if the repos will get cleaned up after each run then there's no need to worry about a different hash. However I think that's more appropriate for a different PR. @davidkna FWIW there was a bit of related discussion in #255. My use case is for a shell prompt as well and in fact this was the next feature on my list. I don't think getting the current status of the sequencer is all that expensive (read a couple files), but the bigger question is whether it's more appropriate to have the state enum contain that information or not. e.g. enum State {
…
// Should CherryPick and Revert use the Interactive suffix here?
CherryPickSequence { current_operation: usize, total_operations: usize },
RebaseInteractive { current_operation: usize, total_operations: usize },
RevertSequence { current_operation: usize, total_operations: usize },
}Or: struct SequencerState {
pub completed_operations: usize,
pub total_operations: usize,
// Commit currently subjected to an edit request, if any
pub edit_commit: Option<Id>,
// Other rebase state? See also:
// https://github.com/git/git/blob/master/sequencer.c
}
impl Repository {
pub fn sequencer_progress(&self) -> Option<SequencerState>;
}I don't have any particularly strong feelings either way. |
|
Apologies I just fat fingered that. |
|
@inferiorhumanorgans Thanks for the update! I don't have strong opinions on the API, either. I feel like the |
|
I think it depends on the intended use case. For something like a prompt generator the only important metadata is the number of operations (completed, total) so having that as part of the However @davidkna thoughts? |
|
Regarding the test failure, you could merge When seeing the conversation about the potential performance impact of the obtaining the status for everything, I realized that it's probably more in line with the existing API to make it more lazy. This can be achieved using a platform, like so: fn status(&self) -> Platform<'_> {…}
impl<'a> Platform<'a> {
fn rebase(&self) -> git_rebase::Status {…}
}
fn main() {
repo.status().rebase()
}An empty I think this means the enum can be replaced with smaller structures, which probably fit well into I hope this helps for the issues brought up here around efficiency and usability. |
git-rebase can be useful to hold status types for the rebase operation and anticipates that eventually this will be implemented using gitoxide. git-lfs anticipates its implementation and reserves the name.
|
I think the current implementation of Agreed on keeping the API lazy. My preference is to keep more detailed info separate from the The workflow could look something like this: match repo.in_progress_operation() {
None => {},
// Should RepositoryState be renamed to something like RepositoryOperation?
op @ RepositoryState::RebaseInteractive,
| op @ RepositoryState::CherryPickSequence,
| op @ RepositoryState::RevertSequence => {
let seq = repo.sequencer_state()?;
if let Some(id) = seq.commit_being_edited {
print_ps1_segment("{:?} editing: {}", op, id);
} else {
print_ps1_segment("{:?} {}/{}", op, seq.completed_operations, seq.total_operations);
}
}
op => print_ps1_segment("{:?}", op)
} |
|
Now I see, the status quo is tailor made for this use-case, and pretty good at that. Thanks for elaborating. Let me pivot back to the previous stance so we can merge as is once all tests have been ported/migrated. For context on the |
|
Yeah, this was intended to be a lightweight check for where a lot of details aren't needed. I think that the next logical step is to flesh out how to expose interactive rebase state (since that seems to have the most interest). |
|
I'd be happy if we could get this version tested by integrating all tests there are in the repository you created before and place all adjustments/changes in another PR, keeping the scope of this one smaller. |
|
Yeah I agree 100%. The get rebase status discussion belongs in a different issue. The tests I didn't carry over from https://github.com/inferiorhumanorgans/git-verification/ don't apply here. They are for validating branch information on empty repositories and for counting untracked files and are unrelated to in progress operations. However the following |
|
So I was going to punt on writing tests for the other states until I sat down to write a test to cover the I've never used the mailbox stuff so I'm not sure where to begin with that. |
I am happy about any test you contribute, so no need to force it with mailboxes :). Generally, it should be possible to create an email to send patches, save the mailbox format, and apply that while causing a conflict maybe. I only have done the mail-creation once in my life so unfortunately can't help more here either. The refactoring is done now and the additional tests could be submitted in a new PR. And: Thanks a lot for your contribution :)! |
This function is designed to mimic
libgit2'sRepository::state()function.