zaquestion / lab

Lab wraps Git or Hub, making it simple to clone, fork, and interact with repositories on GitLab
https://zaquestion.github.io/lab
Creative Commons Zero v1.0 Universal
1.11k stars 102 forks source link

cmd/mr_checkout: check existing remotes before creating one #786

Closed bmeneg closed 2 years ago

bmeneg commented 2 years ago

In case tacking is enabled lab mr checkout -t, a new remote is created with the name of the MR author's name without checking if the already available remotes matches the one from which the MR was checked-out. A particular case where it happens constantly is in the branching development model: developers use a single repo to push their work as branches.

This patch change the code behavor by first checking if any of the existent remotes has the same path as the source MR project before creating a new remote. If no remote is found, then create a new one with the MR author's name.

Signed-off-by: Bruno Meneguele bmeneg@redhat.com

Related: #774

bmeneg commented 2 years ago

@claytonrcarter @fmueller, can you guys take a look in this?

fmuellner commented 2 years ago

I haven't tested the change (yet), but the code looks correct to me

codecov[bot] commented 2 years ago

Codecov Report

Merging #786 (c944c2e) into master (036ff5e) will increase coverage by 0.22%. The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #786      +/-   ##
==========================================
+ Coverage   54.89%   55.12%   +0.22%     
==========================================
  Files          77       77              
  Lines        5614     5631      +17     
==========================================
+ Hits         3082     3104      +22     
+ Misses       2251     2248       -3     
+ Partials      281      279       -2     
Impacted Files Coverage Δ
cmd/mr_checkout.go 81.96% <86.84%> (+7.49%) :arrow_up:
internal/git/git.go 56.80% <88.88%> (+1.87%) :arrow_up:
cmd/label_list.go 89.18% <0.00%> (+0.95%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7368ada...c944c2e. Read the comment docs.

fmuellner commented 2 years ago

I haven't tested the change (yet),

Now I have, works like a charm :-)

fmuellner commented 2 years ago

OK, so maybe I was too quick.

I just tried to check out a MR where the branch is on the upstream remote ("wip/exalm/foo" on "origin"), but then ended up with a tracked branch from a random remote that was created after the branch appeared (i.e. "wip/exalm/foo" on "someUserName")

claytonrcarter commented 2 years ago

I love the idea of this, and it makes much more sense than #779. But there's a but...

It doesn't seem to work from subdirectories. Doing so results (for me) in 2022/02/04 13:13:57 ERROR: mr_checkout.go:80: repository does not exist, which is coming from the call to gogit.PlainOpen(".") in internal/git/git.go:243 via the call to git.Remotes() on line 78 of this PR.

After switching to the root of my repo, this worked like a charm, though. I had expected it to work from any where w/i the repo.

bmeneg commented 2 years ago

OK, so maybe I was too quick.

I just tried to check out a MR where the branch is on the upstream remote ("wip/exalm/foo" on "origin"), but then ended up with a tracked branch from a random remote that was created after the branch appeared (i.e. "wip/exalm/foo" on "someUserName")

@fmuellner, I was working in a -r <remote> flag for the checkout yesterday and noticed something was indeed off with the code indeed, I couldn't get an additional test to pass due to something off in the refspec being used. I'll check it and post the fix in this same PR.

bmeneg commented 2 years ago

I love the idea of this, and it makes much more hence than #779. But there's a but...

It doesn't seem to work from subdirectories. Doing results (for me) in 2022/02/04 13:13:57 ERROR: mr_checkout.go:80: repository does not exist, which is coming from the call to gogit.PlainOpen(".") in internal/git/git.go:243 via the call to git.Remotes() on line 78 of this PR.

After switching to the root of my repo, this worked like a charm, though. I had expected it to work from any where w/i the repo.

Oh, ok. that's unexpected to me too. I'll check it!

bmeneg commented 2 years ago

@claytonrcarter, this last commit I pushed should solve the issue you're seeing of not being able to run 'lab mr checkout' from within repo subdirs. Would you mind testing it again with the latest update?

For the another issue I should have something for tomorrow.

claytonrcarter commented 2 years ago

Yes! It works perfectly. Thank you!

claytonrcarter commented 2 years ago

Oh, BTW the help text for mr checkout will need an update for this, too. It currently reads "set checked out branch to track mr author remote branch, adds remote if needed", but that's only 1/2 of the story now.

bmeneg commented 2 years ago

Oh, BTW the help text for mr checkout will need an update for this, too. It currently reads "set checked out branch to track mr author remote branch, adds remote if needed", but that's only 1/2 of the story now.

Oh yes! I had it updated in a PR I'm planning to send about enabling --remote <name> next :). But the msg should be changed with the commits we already have, agreed. I'll update it.

claytonrcarter commented 2 years ago

Thank you! This looks good. I didn't test out git.RemoteBranches, but it makes sense from a read through.

bmeneg commented 2 years ago

@fmuellner, I wasn't able to reproduce the behavior you described. Can you provide the command you ran and also the remote URLs just to check how the git.PathWithNamespace(remote) == project.PathWithNamespace() is not matching?

I fixed a portion of the code for another issue and also improved the MR lookup. Can you give it a try to make sure your error wasn't also solved by the changes?

fmuellner commented 2 years ago

just to check how the git.PathWithNamespace(remote) == project.PathWithNamespace() is not matching?

So on further inspection, the matching did work. The remotes that were picked instead of origin were created for branches on origin using the MR author; in other words, they were the result of the issue this PR is fixing.

Can you give it a try to make sure your error wasn't also solved by the changes?

I now get this:

~/src/gnome-shell (main=)$ git fetch 
From https://gitlab.gnome.org/GNOME/gnome-shell
 * [new branch]          wip/snwh/calendar-popover-fixes -> origin/wip/snwh/calendar-popover-fixes
 * [new branch]          wip/snwh/icon-resources         -> origin/wip/snwh/icon-resources
~/src/gnome-shell (main=)$ lab mr checkout 2161
2022/02/07 23:10:59 ERROR: mr_checkout.go:118: remote reference refs/remotes/origin/wip/snwh/calendar-popover-fixes already exists
fmuellner commented 2 years ago
lab mr checkout 2161

Ah, I should note that the --track option enabled via the config file.

bmeneg commented 2 years ago

If you fetch it before using lab mr checkout the remote ref will be already in place, requiring you to use --force to override it. What happens if you don't git fetch it first, but let lab fetch it for you? And also, using lab mr checkout --force.

fmuellner commented 2 years ago

Both work, thanks.