Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lakeFS allows merging a branch to itself #7824

Open
itaigilo opened this issue Jun 2, 2024 · 6 comments · May be fixed by #8402
Open

lakeFS allows merging a branch to itself #7824

itaigilo opened this issue Jun 2, 2024 · 6 comments · May be fixed by #8402
Labels
bug Something isn't working good first issue Good for newcomers P3

Comments

@itaigilo
Copy link
Contributor

itaigilo commented Jun 2, 2024

After recently allowing merge using the "allow_empty" or "force" flags (#7798), it allows users to merge a branch into itself.

For example -
Create branch 1, branch branch 2 from branch 1, and merge branch 2 into branch 1.

Previously it would result in a no changes error,
And now, when merging using the "allow_empty" or "force" flags, it's possible to complete this merge.

@itaigilo itaigilo added the bug Something isn't working label Jun 2, 2024
@itaiad200 itaiad200 added the good first issue Good for newcomers label Jun 2, 2024
@talSofer talSofer added the P3 label Jun 5, 2024
@nadavsteindler
Copy link
Contributor

Why is this a problem? You said "allow empty" so shouldn't it allow the action???

@itaigilo
Copy link
Contributor Author

Why is this a problem? You said "allow empty" so shouldn't it allow the action???

Since we're checking if the content of the two branches is different, adding the "allow-empty" flag introduces the option to merge branch_a branch_a --allow-empty.
Although it can technically work, it's something we'd rather prevent from happening (mainly to prevent user mistakes, I think).

@ItamarYuran ItamarYuran self-assigned this Sep 10, 2024
@ItamarYuran ItamarYuran linked a pull request Sep 10, 2024 that will close this issue
@ItamarYuran ItamarYuran removed their assignment Nov 6, 2024
@tkalir tkalir linked a pull request Nov 29, 2024 that will close this issue
@tkalir
Copy link

tkalir commented Nov 30, 2024

I have a question about desired output of lakectl when trying to merge a branch to itself.

Currently, my PR changes the server only, and the output of lakectl is:

update branch main: same branch validation error  
400 Bad Request

Should the "400 Bad Request" line appear? it seems to expose implementation details unnecessarily, especially since we already have a descriptive error message.

I checked the output in a similar case where the server returns an error response - trying to make an empty commit without the "allow empty" flag. In this case the output also shows the Bad Request line.

commit: no changes  
400 Bad Request

I believe the output should be consistent in both cases, but from the same reason I think this line should not appear on both.
What do you think?

@arielshaqed
Copy link
Contributor

Should the "400 Bad Request" line appear? it seems to expose implementation details unnecessarily, especially since we already have a descriptive error message.

Personally I believe that it should: unlike unformatted error text, an http code is an unambiguous specification.

If you disagree, we could discuss on Slack or on a separate issue.

@arielshaqed
Copy link
Contributor

I talked to @ozkatz , here's his suggestion: do what Git does in this situation. I'll check and update here.

@arielshaqed
Copy link
Contributor

What Git does

This is actually impossible on Git! Git fails any merge from an ancestor of the branch HEAD, with the message "Already up to date.". This includes merging the branch HEAD itself to the branch

For instance:

❯ git log
commit bcf5a09671562551797fa747c43d9f86e5b30aef (HEAD -> main)
Author: Ariel Shaqed (Scolnicov) <[email protected]>
Date:   Thu Dec 5 19:57:07 2024 +0200

[...]

commit b5aa1a1b17a8bfb4d55f37e1d8a0395fb96773d5
Author: Ariel Shaqed (Scolnicov) <[email protected]>
Date:   Thu Dec 5 19:45:40 2024 +0200

[...]
❯ git merge main~
Already up to date.
❯ git merge b5aa
Already up to date.
❯ git merge main
Already up to date.

Proposal

Do what Git does: forbid the merge if a commit is an ancestor of the branch head. This has multiple advantages:

  • Does what Git does.
  • Prevent merging a branch HEAD to itself.
  • Prevent other empty merges.
  • Continue to treat a merge as going from a commit to a branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers P3
Projects
None yet
7 participants