Skip to content

Conversation

@nkiesel
Copy link
Contributor

@nkiesel nkiesel commented Feb 15, 2024

Fixes #1133

Copy link
Collaborator

@hyperupcall hyperupcall left a comment

Choose a reason for hiding this comment

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

The code change itself LGTM, but I'm wondering if this is the right approach

To me, it seems the behavior with --proceed is what is expected when running the command. If there is 10 branches that should be deleted, I do not think it should stop on the first one, if the first one fails to delete, by default. Maybe the best approach is to just add || true to the git branch -D "$branch" because it is more intuitive? It would also match the behavior in git-delete-branch.

@spacewander what do you think?

@hyperupcall
Copy link
Collaborator

hyperupcall commented Feb 16, 2024

Oops, I didn't realize there was disussion about this in #1133. I've added the reference to PR description

@nkiesel
Copy link
Contributor Author

nkiesel commented Feb 17, 2024

Yes, I would have preferred making this the default behavior. However, I was afraid that this breaks compatibility, so opted for a non-default option. Alternative could be to also support an env variable which changes the default behavior.

@hyperupcall
Copy link
Collaborator

hyperupcall commented Feb 18, 2024

@nkiesel I agree, backwards compatability is important. In my opinion, it is OK because the fix restores the command behavior to what is expected when this command runs, especially as these commands are typically run interactively and the behavior and flags remain the same as similar commands like git-delete-{branch,merged-branches}.

I'm not going to block on this issue, the final verdict is up to @spacewander. We can always make a change later that will change the default behavior, which will both be backwards-compatible to, and obsolete, the --proceed flag. Perhaps these can be aggregated into a single change for a major release.

@spacewander
Copy link
Collaborator

Let's merge this one first 😄

@spacewander spacewander merged commit fea9667 into tj:master Feb 19, 2024
hyperupcall pushed a commit to hyperupcall/git-extras that referenced this pull request Feb 19, 2024
spacewander pushed a commit that referenced this pull request Feb 20, 2024
hyperupcall added a commit that referenced this pull request Oct 3, 2024
@norwd
Copy link

norwd commented Mar 26, 2025

@tj / @spacewander, I think this PR is missing from main (it's in master though), should this be reopened and merged to main?

See: main...master

@spacewander
Copy link
Collaborator

@norwd
Thanks for your nice catch. Let's open a new PR to the main branch.

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.

4 participants