Skip to content

Conversation

@Fatpandac
Copy link
Contributor

@Fatpandac Fatpandac commented Jan 20, 2025

This Pull Request fixes/closes #2404.

It changes the following:

  • support choosing checkout branch method when status is not empty
  • add .editorconfig file

image

I followed the checklist:

  • I added unittests
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog
Copy link
Collaborator

@extrawurst extrawurst 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 looking into this! I left the first round of feedbacks

@Fatpandac Fatpandac requested a review from extrawurst January 29, 2025 04:08
Copy link

@noleptr noleptr left a comment

Choose a reason for hiding this comment

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

hi, apologies for the intrusion. i have come across this repo just now and have been poking around. i hope you don't mind but i've left a few comments and questions i have, but don't feel obligated to reply

@Fatpandac Fatpandac requested a review from noleptr February 19, 2025 10:10
@extrawurst
Copy link
Collaborator

@Fatpandac I compared our behavior when choosing stash and reapply with Forks and ours has one obvious difference that might (at least) confuse:
if you have changes stages and changes to the same file unstaged this will merge these into a single stash and it will have all changes unstaged after the procedure.

Copy link
Collaborator

@extrawurst extrawurst left a comment

Choose a reason for hiding this comment

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

We should either fix this and stash staged and unstaged separately or flat out error if we detect that this will happen. Wdyt?

Copy link
Collaborator

@extrawurst extrawurst left a comment

Choose a reason for hiding this comment

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

Considering the findings I would further argue lets move the logic to checkout with different options to asyncgit and lets cover them with proper unit tests that recreate the various scenarios and the expected results/errors we want to achieve. this is a fundamental enough feature to warrant that quality and resilience for future changes not breaking it

@extrawurst
Copy link
Collaborator

@Fatpandac if the stash staging and non-staged changes is gonna be hard (I assume so, I am not aware we have that machinery yet) then lets for now rip out the stashing option and support only Discard & Keep option that is largely handled by libgit2 and followup with the stashing variants in a followup PR?

@Fatpandac
Copy link
Contributor Author

@extrawurst I agree. I'll remove the KeepLocalChanges option in the next commit.

@Fatpandac Fatpandac force-pushed the feat/checkout_method branch from d8500df to 0db55e9 Compare November 2, 2025 03:28
@extrawurst
Copy link
Collaborator

@Fatpandac please resolve conflicts and let me know when this is ready for a new round of reviews

@Fatpandac
Copy link
Contributor Author

@extrawurst Could you please take a look at this comment? I’m not sure how to resolve it.

@Fatpandac Fatpandac requested a review from extrawurst November 15, 2025 07:17
@extrawurst
Copy link
Collaborator

@Fatpandac can you resolve the merge conflict, then we can merge

Copy link
Collaborator

@extrawurst extrawurst left a comment

Choose a reason for hiding this comment

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

LGTM

@extrawurst extrawurst enabled auto-merge (squash) November 28, 2025 20:08
auto-merge was automatically disabled November 28, 2025 20:27

Head branch was pushed to by a user without write access

@Fatpandac
Copy link
Contributor Author

Done

@extrawurst extrawurst merged commit 3082396 into gitui-org:master Nov 28, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

4 participants