Re: Resolving pull request conflicts
Hi Ryan, - On 23 Feb, 2017, at 12:04, Ryan Schmidt ryandes...@macports.org wrote: > GitHub said the pull request had a conflict. (The PR is a version update, but > a > revbump was committed to the port in the meantime.) GitHub said I could > resolve > the conflict on the command line or in a web-based editor. There was a green > button inviting me to start resolving the conflict on the web. I clicked this > and was brought to a web-based text editor showing the Portfile in its > conflicted state. I edited the file, removing the conflict markers and > incorrect lines. I clicked the button saying I have now resolved the > conflicts. That's fine, but what GitHub did under the hood (without telling you), is attempting to merge the current state of the master branch with the PR, and generating a merge commit with your conflict resolution. We don't want merge commits, we want rebase + force push, but GitHub does not give you this option when resolving conflicts on the web interface, apparently. > Absolutely the history that appears in master should be "clean", meaning one > commit per logical change. But it is natural, is it not, that a pull request > might need refinement before being merged, to address feedback or in this case > conflicts, and that that refinement would come in the form of additional > commits? There are multiple approaches here. Some push new commits until they are satisfied and then squash, others prefer force-pushing until the history is exactly like they want it. Squashing would also merge changes that might make sense to have in separate commits and you cannot review the exact history you are going to merge, so I personally prefer rebase + force push. In your case, though, you added a merge commit, which might possibly have been preserved by the rebase merge method and would have ended up in the master history. HTH, -- Clemens Lang
Re: Resolving pull request conflicts
On 23 February 2017 at 11:19, Ryan Schmidt wrote: > GitHub invited me to edit the file on the web to resolve the conflict, so I > did. I was then asked by the PR author not to do that. To avoid confusion about my earlier replies: the only part that's in my opinion non-desirable is ending up with lots and lots of merge commits in master. Nothing else. (If merge is required as an intermediate step to help people achieve the goal faster and more efficient, I don't see a problem. Before or once it ends up in master, linear history would be preferable.) Mojca
Re: Resolving pull request conflicts
On 23 February 2017 at 12:04, Ryan Schmidt wrote: > > GitHub said the pull request had a conflict. (The PR is a version update, but > a revbump was committed to the port in the meantime.) GitHub said I could > resolve the conflict on the command line or in a web-based editor. There was > a green button inviting me to start resolving the conflict on the web. I > clicked this and was brought to a web-based text editor showing the Portfile > in its conflicted state. I edited the file, removing the conflict markers and > incorrect lines. I clicked the button saying I have now resolved the > conflicts. This sounds like a totally reasonable way to do stuff. With lots of things that are still not clear to me (for one, I still don't see where all your commits are; I see a link posted by Zero King, but don't know how to actually reach that content from elewhere). Initially it looked like a merge commit somewhere in master. Then I found it in a branch that doesn't even seem to exist in macports/macports-ports. > Is it better to have a pull request that has conflicts which cannot be merged > without additional manual work, or one that has had those conflicts resolved? It's certainly better to have something that doesn't require additional manual work. (It would be even better if GitHub offered a way to perform certain operations online that it currently doesn't.) I'm currently not sure what exactly is involved in avoiding a merge before pushing things to master. If clicking on a green button on the web interface would get the job done without showing the merge commit, that would be great. (Maybe we need to test an an auxiliary repo.) >> If anything it's confusing to see 20 unrelated commits when >> it's about merging some trivial changes in an unrelated file. > > I'm not sure what you mean here. The only strange thing here is https://github.com/macports/macports-ports/commit/2f8d7fd90a9f0a66b6aa438c78128eec15c46d79 that was linked by Zero King. But I admit that it's not clear to me where that commit comes from (how to get to it from anywhere else). Did l2dy remove it from the Pull Request already and overwrite it with another one? >> We kind of decided that we'll try to keep our history linear & clean. > > Absolutely the history that appears in master should be "clean", meaning one > commit per logical change. But it is natural, is it not, that a pull request > might need refinement before being merged, to address feedback or in this > case conflicts, and that that refinement would come in the form of additional > commits? My understanding was that once it was deemed satisfactory such > multi-commit PRs would be squashed into a single commit before/during merging. Yes, that's the idea. Keeping committing to the pull request until it's ready and clean it before it goes to master. I would say that in many cases with have PRs that are ready to be committed (from the perspective of changes that they introduced), but not ready from the perspective of "clean commit messages" and having way too many commits with trivial bugfixes that should be cleaned up eventually. And in those cases there's still manual work needed to squash and reword commits and we often ask authors to do that, so that someone can eventually "click the green button". Again, I still don't quite understand where that merge commit comes from, but Zero King's complaint was probably that after your change the PR might have been in a similar "intermediate" state where there would be still manual work needed to get it directly to master. > (The "squash-and-merge" GitHub button that we haven't enabled yet.) If that works as expected, this could indeed solve the problem by making it a matter of a button click to fix the commits (provided one doesn't click the wrong button of course :). Mojca
Re: Resolving pull request conflicts
On Feb 23, 2017, at 04:46, Mojca Miklavec wrote:
>
> Dear Ryan,
>
> On 23 February 2017 at 11:19, Ryan Schmidt wrote:
>> Browsing pull requests, I found this one:
>>
>> https://github.com/macports/macports-ports/pull/305
>>
>> Another commit had happened to that port in the mean time, so the PR now had
>> conflicts and could not be applied as-is. GitHub invited me to edit the file
>> on the web to resolve the conflict, so I did. I was then asked by the PR
>> author not to do that. Can someone explain why resolving the conflict was
>> bad?
>
> It's not resolving the conflict that's bad.
>
> I guess it's just that the GitHub's user interface is anything but
> optimal when it comes to certain aspects.
>
> I'm still confused about what exactly you did to end up in that
> situation (but it's probably more clear to me now that you mentioned
> it; perhaps I should this on some test repository).
GitHub said the pull request had a conflict. (The PR is a version update, but a
revbump was committed to the port in the meantime.) GitHub said I could resolve
the conflict on the command line or in a web-based editor. There was a green
button inviting me to start resolving the conflict on the web. I clicked this
and was brought to a web-based text editor showing the Portfile in its
conflicted state. I edited the file, removing the conflict markers and
incorrect lines. I clicked the button saying I have now resolved the conflicts.
> I agree with Zero King that it makes more sense to keep the branches
> clean.
What does it mean to be "clean"?
Is it better to have a pull request that has conflicts which cannot be merged
without additional manual work, or one that has had those conflicts resolved?
> If anything it's confusing to see 20 unrelated commits when
> it's about merging some trivial changes in an unrelated file.
I'm not sure what you mean here.
> We kind of decided that we'll try to keep our history linear & clean.
Absolutely the history that appears in master should be "clean", meaning one
commit per logical change. But it is natural, is it not, that a pull request
might need refinement before being merged, to address feedback or in this case
conflicts, and that that refinement would come in the form of additional
commits? My understanding was that once it was deemed satisfactory such
multi-commit PRs would be squashed into a single commit before/during merging.
(The "squash-and-merge" GitHub button that we haven't enabled yet.)
> (I'm sure you still remember Andrea's 300 duplicated commits in a
> failed attempt to do so and probably agree that it makes sense to do
> our best to avoid repeating such type of commits?)
I do remember that occurrence, and not understanding how it happened.
> Ever since I discovered that GitHub won't use my email when I click on
> the merge button in pull requests, I never use the GitHub's GUI to do
> any operations like this one since I never know what it will do behind
> the scene.
I guess I'll just continue not to participate in anything other than the most
straightforward pull requests for now, since I still find the git command line
too complicated to understand.
> Here's what I do when I work on Pull Requests.
>
> 1.) First of all I made sure to add
>
>[pull]
>rebase = true
>
> to .git/config just in case that I would forget to call "--rebase"
> when I do "git pull". If that option is missing, one should always try
> to run "git pull origin master --rebase" on the local master branch to
> avoid ending up with weird merges.
>
> 2.) Here's what I do to fetch the changes locally and push them back:
>
> # set these three lines manually
> PR_N=123
> PR_USER=some-username
> PR_REMOTEBR=remote-branch-name
>
> PR_LOCALBR=pr${PR_N}-${PR_USER}-${PR_REMOTEBR}
>
> git fetch https://github.com/$PR_USER/macports-ports.git $PR_REMOTEBR
> git checkout -b $PR_LOCALBR FETCH_HEAD
> git rebase master
>
> ## then do whatever "magic" is needed
> ## very often using commands like
> # git rebase -i HEAD~4
> # git commit --amend [--date="..."]
>
> ## then check with
> # git log --graph
> ## to make sure that the history and commit messages are OK
>
> # then update the branch in PR first
> # (this is also important to make the PR marked as "merged" rather
> than "closed")
> git push -f https://github.com/$PR_USER/macports-ports.git
> $PR_LOCALBR:$PR_REMOTEBR
>
> # then check if the PR looks ok on the web just in case
>
> # and finally push the changes to master
> git push origin $PR_LOCALBR:master
>
>
> Maybe some software already does most of that in a more user-friendly way.
Thanks for trying to help. This is just really complicated to me and not
something I'm likely to do right now.
I find it remarkable that people find this easier than just downloading
patchfiles from Trac...
> I'm not sure about the proper way to resolve conflicts, but git offers
> some command line interface. When I was committing the "$Id" stuff, it
> was
Re: Resolving pull request conflicts
Dear Ryan,
On 23 February 2017 at 11:19, Ryan Schmidt wrote:
> Browsing pull requests, I found this one:
>
> https://github.com/macports/macports-ports/pull/305
>
> Another commit had happened to that port in the mean time, so the PR now had
> conflicts and could not be applied as-is. GitHub invited me to edit the file
> on the web to resolve the conflict, so I did. I was then asked by the PR
> author not to do that. Can someone explain why resolving the conflict was bad?
It's not resolving the conflict that's bad.
I guess it's just that the GitHub's user interface is anything but
optimal when it comes to certain aspects.
I'm still confused about what exactly you did to end up in that
situation (but it's probably more clear to me now that you mentioned
it; perhaps I should this on some test repository).
I agree with Zero King that it makes more sense to keep the branches
clean. If anything it's confusing to see 20 unrelated commits when
it's about merging some trivial changes in an unrelated file.
We kind of decided that we'll try to keep our history linear & clean.
(I'm sure you still remember Andrea's 300 duplicated commits in a
failed attempt to do so and probably agree that it makes sense to do
our best to avoid repeating such type of commits?)
Ever since I discovered that GitHub won't use my email when I click on
the merge button in pull requests, I never use the GitHub's GUI to do
any operations like this one since I never know what it will do behind
the scene.
Here's what I do when I work on Pull Requests.
1.) First of all I made sure to add
[pull]
rebase = true
to .git/config just in case that I would forget to call "--rebase"
when I do "git pull". If that option is missing, one should always try
to run "git pull origin master --rebase" on the local master branch to
avoid ending up with weird merges.
2.) Here's what I do to fetch the changes locally and push them back:
# set these three lines manually
PR_N=123
PR_USER=some-username
PR_REMOTEBR=remote-branch-name
PR_LOCALBR=pr${PR_N}-${PR_USER}-${PR_REMOTEBR}
git fetch https://github.com/$PR_USER/macports-ports.git $PR_REMOTEBR
git checkout -b $PR_LOCALBR FETCH_HEAD
git rebase master
## then do whatever "magic" is needed
## very often using commands like
# git rebase -i HEAD~4
# git commit --amend [--date="..."]
## then check with
# git log --graph
## to make sure that the history and commit messages are OK
# then update the branch in PR first
# (this is also important to make the PR marked as "merged" rather
than "closed")
git push -f https://github.com/$PR_USER/macports-ports.git
$PR_LOCALBR:$PR_REMOTEBR
# then check if the PR looks ok on the web just in case
# and finally push the changes to master
git push origin $PR_LOCALBR:master
Maybe some software already does most of that in a more user-friendly way.
I'm not sure about the proper way to resolve conflicts, but git offers
some command line interface. When I was committing the "$Id" stuff, it
was less work to start from scratch that to keep resolving conflicts.
And very often the commits are so simple that I simple go "from
scratch".
Mojca
Re: Resolving pull request conflicts
If you edit the file the change is then tagging against you. How I would resolve it is. 1) fetch the latest branch (either develop/master depending how git is being used) 2) merge latest branch into your pull request branch 2.1) I usually do this with --no-commit 2.2) fix merge comflicts 2.3) commit with git commit --no-edit 3) push your pull request branch again Not sure if this is how mac ports developers want to use git, realise mac ports are still only just moving over to git but that is how i've been doing pull request conflicts for other projects for 3-4 years. John On 23 February 2017 at 10:19, Ryan Schmidt wrote: > Browsing pull requests, I found this one: > > https://github.com/macports/macports-ports/pull/305 > > Another commit had happened to that port in the mean time, so the PR now had > conflicts and could not be applied as-is. GitHub invited me to edit the file > on the web to resolve the conflict, so I did. I was then asked by the PR > author not to do that. Can someone explain why resolving the conflict was bad? > >
Resolving pull request conflicts
Browsing pull requests, I found this one: https://github.com/macports/macports-ports/pull/305 Another commit had happened to that port in the mean time, so the PR now had conflicts and could not be applied as-is. GitHub invited me to edit the file on the web to resolve the conflict, so I did. I was then asked by the PR author not to do that. Can someone explain why resolving the conflict was bad?
