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

git lab creates new MRs instead of updating existing MRs #49

Closed
kostajh opened this issue Oct 4, 2023 · 3 comments
Closed

git lab creates new MRs instead of updating existing MRs #49

kostajh opened this issue Oct 4, 2023 · 3 comments

Comments

@kostajh
Copy link
Contributor

kostajh commented Oct 4, 2023

Steps to reproduce:

Expected:

  • MRs 97 and 98 are unchanged, MR 96 has an updated commit message and MR description

Actual:

  • New MRs are opened for all three commits.
@kostajh kostajh changed the title Not updating existing MRs git lab creates new MRs instead of updating existing MRs Oct 4, 2023
kostajh added a commit to kostajh/gerritlab that referenced this issue Oct 11, 2023
Why:

- It is better not to be surprised by the output of `git lab` (see issue
  yaoyuannnn#49)

What:

- Show the list of proposed MR updates/creations, and prompt the user to
  proceed
- [misc] set the _web_url property as "(new)" when printing info for an
  MR that does not yet exist

Fixes yaoyuannnn#50

Change-Id: I3a758a21ccd46930fe33eabecbec9c57b705b3aa
kostajh added a commit to kostajh/gerritlab that referenced this issue Oct 11, 2023
Why:

- It is better not to be surprised by the output of `git lab` (see issue
  yaoyuannnn#49)

What:

- Show the list of proposed MR updates/creations, and prompt the user to
  proceed
- [misc] set the _web_url property as "(new)" when printing info for an
  MR that does not yet exist

Fixes yaoyuannnn#50

Change-Id: I3a758a21ccd46930fe33eabecbec9c57b705b3aa
@kostajh
Copy link
Contributor Author

kostajh commented Oct 11, 2023

With the patch in #50 (and a few print() statements), I see this:

Commits to be reviewed:
* 90162e9c shell: Make scripts portable for macOS

Proceed? ([y]/n) y
{'macos-6b8f': <gerritlab.merge_request.MergeRequest object at 0x101d246a0>}
macos-6b8f-6b8f

Proposed MRs:

No existing MRs will be updated
MRs to create
* (new) shell: Make scripts portable for macOS
    macos-6b8f-6b8f -> main

The branch I'm on is macos-6b8f. That's als what I see at https://gitlab.wikimedia.org/repos/mediawiki/services/ipoid/-/merge_requests/122. But when running git lab, it looks like the source_branch property is incorrectly identified as macos-6b8f-6b8f, instead of macos-6b8f

kostajh added a commit to kostajh/gerritlab that referenced this issue Oct 11, 2023
Why:

- Running `git lab` on a commit that has an existing MR can result in
  gerritlab creating a new MR (yaoyuannnn#49). This occurs when the local branch
  is already in the gerritlab format (e.g. perhaps one ran `glab mr
  checkout` on a gerritlab-created MR).

What:

- Check if the local branch is already in the {branch}-{change-id}
  format, and if so, return that rather than returning an incorrect
  remote branch name

Fixes yaoyuannnn#49

Change-Id: Icb201f9cc07f930d8ad8dd4eb5bd6c467afa08f2
@kostajh
Copy link
Contributor Author

kostajh commented Oct 13, 2023

With the patch in #50 (and a few print() statements), I see this:

Commits to be reviewed:
* 90162e9c shell: Make scripts portable for macOS

Proceed? ([y]/n) y
{'macos-6b8f': <gerritlab.merge_request.MergeRequest object at 0x101d246a0>}
macos-6b8f-6b8f

Proposed MRs:

No existing MRs will be updated
MRs to create
* (new) shell: Make scripts portable for macOS
    macos-6b8f-6b8f -> main

The branch I'm on is macos-6b8f. That's als what I see at https://gitlab.wikimedia.org/repos/mediawiki/services/ipoid/-/merge_requests/122. But when running git lab, it looks like the source_branch property is incorrectly identified as macos-6b8f-6b8f, instead of macos-6b8f

Maybe branch name is the wrong thing to check, though. We have Change-Id which should be the source of proof for saying "Commit X belongs in MR Y". Should we switch to using that, rather than relying on branch name?

@dancysoft
Copy link
Collaborator

I think this is probably resolved as of #54. Note that upgrading to the latest Gerritlab code means that existing MRs with the old naming scheme will de-associated.

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 a pull request may close this issue.

2 participants