mergiraf/mergiraf
mergiraf/mergiraf
9
213
Fork 45
Code Issues 51 Pull requests 7 Releases 19 Activity Actions

fix: The solve command preserves revision names in conflicts #74

Merged
wetneb merged 7 commits from wetneb/mergiraf:solve_keeps_revision_names into main 2024-12-06 19:49:33 +01:00
Conversation 13 Commits 7 Files changed 6 +147 -25
wetneb commented 2024-12-03 16:05:36 +01:00
Owner
Copy link

For #37. It does not solve the issue entirely, there is more to do before closing it.

For #37. It does not solve the issue entirely, there is more to do before closing it.
wetneb force-pushed solve_keeps_revision_names from acdbede88e to 21bc76e977 2024-12-03 23:08:09 +01:00 Compare
wetneb force-pushed solve_keeps_revision_names from 21bc76e977 to 6ae73906ab 2024-12-03 23:09:30 +01:00 Compare
ugur-a reviewed 2024-12-04 06:06:19 +01:00
src/parsed_merge.rs Outdated
@ -347,1 +381,4 @@
}
/// Update display settings by taking revision names from merge (if there are any conflicts)
pub fn add_revision_names<'a>(&'a self, settings: &DisplaySettings<'a>) -> DisplaySettings<'a> {
ugur-a commented 2024-12-04 06:06:19 +01:00
Contributor
Copy link

Since settings get either cloned or dereferenced, I think we might as well take them by value here.

Since `settings` get either cloned or dereferenced, I think we might as well take them by value here.
ugur-a requested changes 2024-12-04 06:25:54 +01:00
Dismissed
ugur-a left a comment
Contributor
Copy link

This looks great! I actually started something very similar to this, but didn't end up submitting a PR. I think this should be good to go after the one change I've proposed

This looks great! I actually started something very similar to this, but didn't end up submitting a PR. I think this should be good to go after the one change I've proposed
wetneb commented 2024-12-04 09:34:33 +01:00
Author
Owner
Copy link

Sure! It does add a few .clone() calls then, unless you'd make more changes to the callers of this method to avoid that?

Sure! It does add a few `.clone()` calls then, unless you'd make more changes to the callers of this method to avoid that?
ugur-a commented 2024-12-04 09:53:59 +01:00
Contributor
Copy link

Oh, you're right. Though I think those are actually fine in this case, since the only thing that needs to be cloned are the Cows, which are of the Borrowed variant in the tests, and clone is free for those (see implementation).

I'm away from the computer and can't check right now, but would it be possible to change resolve_merge to take settings by value, as you said?
I guess this just repeats your question back. I'll take a look at it when I can.

Oh, you're right. Though I think those are actually fine in this case, since the only thing that needs to be cloned are the `Cow`s, which are of the `Borrowed` variant in the tests, and `clone` is free for those (see [implementation](https://doc.rust-lang.org/src/alloc/borrow.rs.html#193)). ~~I'm away from the computer and can't check right now, but would it be possible to change `resolve_merge` to take `settings` by value, as you said?~~ I guess this just repeats your question back. I'll take a look at it when I can.
wetneb commented 2024-12-04 10:05:20 +01:00
Author
Owner
Copy link

From what I can tell it would just displace the clone() to resolve_merge_cascading.

From what I can tell it would just displace the `clone()` to `resolve_merge_cascading`.
ugur-a commented 2024-12-04 18:49:36 +01:00
Contributor
Copy link

I think I figured it out! Unfortunately I can't open a PR on your fork, so please see my branch: the current HEAD. It removes the second commit from this PR, but the first one is left unchanged.

I think I figured it out! Unfortunately I can't open a PR on your fork, so please see my branch: [the current HEAD](https://codeberg.org/ugur-a/mergiraf/commit/49e3fae8a7e82b2ce5b6c9fd3e2ac56eb89e6a94). It removes the second commit from this PR, but the first one is left unchanged.
wetneb force-pushed solve_keeps_revision_names from 46989ce879 to 49e3fae8a7 2024-12-04 19:29:30 +01:00 Compare
wetneb commented 2024-12-04 19:30:32 +01:00
Author
Owner
Copy link

I think it makes sense to move the method to DisplaySettings indeed, and make it mutate an instance feels sensible too. Thank you! Weird that you can't open a PR on my fork, it looks like it's enabled - what problem did you encounter exactly? I was able to make rvalyi/mergiraf#1 earlier…

I think it makes sense to move the method to DisplaySettings indeed, and make it mutate an instance feels sensible too. Thank you! Weird that you can't open a PR on my fork, it looks like it's enabled - what problem did you encounter exactly? I was able to make https://codeberg.org/rvalyi/mergiraf/pulls/1 earlier…
ugur-a commented 2024-12-06 07:28:07 +01:00
Contributor
Copy link

You're welcome:)

rvalyi/mergiraf#1

Coincidentally, I also wanted to open a PR there (to submit a resolution of the merge conflict), but couldn't -- the "New pull request" button was greyed out. And it's the same with wetneb/mergiraf. I guess you can open PRs under PRs coming from a fork of a project you own precisely because you are the owner.

You're welcome:) > rvalyi/mergiraf#1 Coincidentally, I also wanted to open a PR there (to submit a resolution of the merge conflict), but couldn't -- the "New pull request" button was greyed out. And it's the same with wetneb/mergiraf. I guess _you_ can open PRs under PRs coming from a fork of a project you own precisely because you are the owner.
ugur-a approved these changes 2024-12-06 07:29:55 +01:00
ugur-a commented 2024-12-06 07:34:26 +01:00
Contributor
Copy link

I would squash-commit this, since the latter commits are just various fixes in the first one. But I'll leave it to you in case you don't want to squash. Generally, I think the history/contents of the patch set is already captured well enough in the PR, so it's okay to have the final commit itself be a squashed one

I would squash-commit this, since the latter commits are just various fixes in the first one. But I'll leave it to you in case you don't want to squash. Generally, I think the history/contents of the patch set is already captured well enough in the PR, so it's okay to have the final commit itself be a squashed one
ugur-a commented 2024-12-06 09:08:06 +01:00
Contributor
Copy link

Oh, I've just noticed that my second commit (with lint fixes) has nothing to do with this PR, and should instead be a part of #81. So please do merge that one first.

Oh, I've just noticed that my second commit (with lint fixes) has nothing to do with this PR, and should instead be a part of #81. So please do merge that one first.
wetneb commented 2024-12-06 19:02:54 +01:00
Author
Owner
Copy link

Coincidentally, I also wanted to open a PR there (to submit a resolution of the merge conflict), but couldn't -- the "New pull request" button was greyed out. And it's the same with wetneb/mergiraf. I guess you can open PRs under PRs coming from a fork of a project you own precisely because you are the owner.

Weird, I don't see why privileges on this repo should have anything to do with the ability to open a PR on a fork of it… And even weirder if it requires ownership, because you already have write access. But it seems that you are right, I can reproduce this behaviour on other Codeberg projects. I have opened an issue for it: forgejo/forgejo#6185

Would you like to become an owner ("publisher" in terms of the governance model)? Beyond serving as a work-around to this problem, I would be comfortable with that, given the amazing participation you have made so far :)

> Coincidentally, I also wanted to open a PR there (to submit a resolution of the merge conflict), but couldn't -- the "New pull request" button was greyed out. And it's the same with wetneb/mergiraf. I guess you can open PRs under PRs coming from a fork of a project you own precisely because you are the owner. Weird, I don't see why privileges on this repo should have anything to do with the ability to open a PR on a fork of it… And even weirder if it requires ownership, because you already have write access. But it seems that you are right, I can reproduce this behaviour on other Codeberg projects. I have opened an issue for it: https://codeberg.org/forgejo/forgejo/issues/6185 Would you like to become an owner ("publisher" in terms of the governance model)? Beyond serving as a work-around to this problem, I would be comfortable with that, given the amazing participation you have made so far :)
ugur-a commented 2024-12-06 19:12:36 +01:00
Contributor
Copy link

Weird, I don't see why privileges on this repo should have anything to do with the ability to open a PR on a fork of it… And even weirder if it requires ownership, because you already have write access. But it seems that you are right, I can reproduce this behaviour on other Codeberg projects. I have opened an issue for it: forgejo/forgejo#6185

It hadn't even occurred to me that this can be a bug! Thank you for taking your time to do this:)

Would you like to become an owner ("publisher" in terms of the governance model)? Beyond serving as a work-around to this problem, I would be comfortable with that, given the amazing participation you have made so far :)

That's very kind of you! I would be glad to:)

> Weird, I don't see why privileges on this repo should have anything to do with the ability to open a PR on a fork of it… And even weirder if it requires ownership, because you already have write access. But it seems that you are right, I can reproduce this behaviour on other Codeberg projects. I have opened an issue for it: forgejo/forgejo#6185 It hadn't even occurred to me that this can be a bug! Thank you for taking your time to do this:) > Would you like to become an owner ("publisher" in terms of the governance model)? Beyond serving as a work-around to this problem, I would be comfortable with that, given the amazing participation you have made so far :) That's very kind of you! I would be glad to:)
Merge branch 'main' into solve_keeps_revision_names
All checks were successful
/ rustfmt (pull_request) Successful in 23s
Details
/ test (pull_request) Successful in 48s
Details
aa5b303983
wetneb merged commit aa9ca593a1 into main 2024-12-06 19:49:33 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
ugur-a
Labels
Clear labels   
Compat/Breaking

Breaking change that won't be backward compatible

  
Kind
Bad merge
Examples where the merging algorithm doesn't work as well as it should

  
Kind
Bug
Something is not working

  
Kind
Documentation
Documentation changes

  
Kind
Enhancement
Improve existing functionality

  
Kind
Feature
New functionality

  
Kind
New language

  
Kind
Security
This is security issue

  
Kind
Testing
Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

No labels
Compat/Breaking
Kind
Bad merge
Kind
Bug
Kind
Documentation
Kind
Enhancement
Kind
Feature
Kind
New language
Kind
Security
Kind
Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: mergiraf/mergiraf#74
No description provided.
Delete branch "wetneb/mergiraf:solve_keeps_revision_names"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?