13 ms·

The way I've noticed people (accidentally) "hiding code" is by making changes during the conflict resolution stage of a git merge. This often happens after cod

by pkamb 9y ago

The way I've noticed people (accidentally) "hiding code" is by making changes during the conflict resolution stage of a git merge. This often happens after code review, and there's no oversight of the conflict resolutions. If the merge commit does get looked at, there will be potentially hundreds of lines and files of automated diffs within which the unrelated new code is hidden.

WorldMaker 9y ago
Too many experiences like that I make merge commits a required part of a code review. If a branch needs conflict resolution in order to merge, it needs that merge in that branch, and as part of the code review of that branch. (Some of the best conflict resolution is sometimes new code in light of the changes in the other branch, and so of course should be reviewed.) If a merge commit diff is showing too much information, that's more often than not a tooling problem. Most tools provide ways to filter out the "automated stuff" and focus only on the conflict resolutions. If they don't, they are deficient (and if there are too many conflict resolutions to easily review, that may be a sign of a larger process problem). (GitHub has gotten much better at merge commit views in the last couple of years, as one anecdotal bullet point.)
- seanwilson 9y ago
  > Too many experiences like that I make merge commits a required part of a code review. How can you do this practically though? In the turnaround time to do the code review and make the changes, a new conflict might have been introduced into the branch being targeted by the merge. I think merging e.g. the master branch into your branch before code review is a good idea though as when the review is complete there's going to be less or no conflicts merging into master.
  - WorldMaker 9y ago
    That's also a tooling issue I believe. GitHub, again, as easy example, has gotten pretty good at "these are the commits since your last review" incremental reviews, so if you complete a code review and need conflicts to be fixed before it can merge, you expect a merge commit to be made in that branch and should just need to review that merge commit. I've not recently seen an issue with a PR cycling through a lot of merge commits to keep up with the branch it is targeting, but in almost every case where I've seen that there was some other process decision in the way (two people working on the exact same feature simultaneously) and at that point it was an architectural, "political", or "social" problem to fix first, outside of the code review process. Your mileage may vary, of course.
stinos 9y ago
Isn't this sort of solved by rebasing first instead of just merging? (well, and some rules in place which tells people 'we only accept properly rebased branches for review). Then the conflicts are resolved in the branch and there's a clear view of what actually changed in the diff, before anything is effectively merged into the main branch. Or maybe I'm missing something here.
- Mithaldu 9y ago
  It's absolutely resolved that way. Rebase should be the weapon of choice in all cases, except those where it would be prohibitively expensive. People are too enamored with the SVN ways though, or have even made up rationalizations about fictional concepts such as "historical purity". So you have a ton of people merging instead, producing code repositories that look worse than the worst perl code i've touched.
  - lemoncucumber 9y ago
    Conversely, people get too enamored of rebasing as a way to solve the problem of merging in lots of junky commits. There are lots of ways to rewrite history and clean it up. Rebasing is just one of them, and it's one that can force you to spend more time resolving conflicts than you otherwise might have to (since you have to replay all of your commits and resolve any conflicts caused by each one individually). There's no reason why rebasing is a better way to clean up your history than squash merging (or even -- carefully -- using git reset).
    - Mithaldu 9y ago
      I already said that it can be too expensive, but thanks for playing. <3 Also, a squash merge is not a merge. It's just another form of rebase. And if you mention reset as an alternative to cherry pick and squash you should also mention --soft.
      - lemoncucumber 9y ago
        My point was that many git users don't understand git all that well, and I think that encouraging everyone to rebase is a bad idea. If you're going to promote a particular workflow as the the one that "should be the weapon of choice," I think the workflow of merge-master-into-feature-branch, squash-merge-feature-branch-into-master is a more user friendly alternative, since then the cases where rebasing is expensive never even come up. And yes, you're right that I was remiss not to mention the --soft part. The git reset workflow is one that I would definitely not encourage people to do if they don't understand git well.
deckar01 9y ago
GitLab makes it really easy to see what changed between commits while reviewing a merge request. Parts of the diff that are identical to the upstream branch are automatically ignored, and you can view a diff between of last two pushes. https://docs.gitlab.com/ce/user/project/merge_requests/versions.html https://docs.gitlab.com/ce/user/project/merge_requests/versi... Edit: GitHub has similar functionality, but it doesn't know how to diff force pushes, so you would end up having to review the full diff again to ensure integrity.
- WorldMaker 9y ago
  > Edit: GitHub has similar functionality, but it doesn't know how to diff force pushes, so you would end up having to review the full diff again to ensure integrity. I realize this is a religious war I'm stepping into, but I fall on the side of "if you are doing a force push of a branch already in code review you are doing something wrong". Once commits are out in the open and in review between multiple developers, that is shared code history that encodes important information about the code review process itself. (I make obvious exceptions for things like accidental PII disclosure or similar bone-headed mistakes, but the longer a branch has been in code review, the less willing I am to allow it to be force pushed.) Of course, to each their own and your mileage may vary, but that comment about diffing force pushes in a merge/pull request sent a shiver down my spine, regardless of whether or not the tool supports it.
  - closeparen 9y ago
    Phabricator is really nice here. The history of the code review process is maintained by Phab itself, you can amend all you want against an inflight code review, and upon landing it all becomes a single commit in master with a link to the code review in the commit message.
  - __david__ 9y ago
    You’d hate our team—we force push to master (not just feature branches) all the time. It seems like anarchy at first but it’s not really that hard to deal with. Our situation is a bit unique though, and I doubt it would scale well beyond a small team.
simmons 9y ago
That is scary. I think it would be a nifty feature for a code review tool to show only the conflict resolution for merge commits. (Although I suppose it would need knowledge of exactly which automated merge method/options were used to create the pre-CR merge, for any given commit with multiple parents.)
pkamb 9y ago
What I personally do is commit the merge as is, with all the <<< === >>> conflict markers. Then, in the next couple commits, fix each conflict via a normal commit that mixes and matches "mine" and "theirs" as needed + deletes the markers. Each resolution commit is independent of the giant merge commit, and can be separately diff'ed and code reviewed via the github PR or any other tool. Also easier IMO to make the complicated resolution you want, or reset back to a previous resolution attempt without messing up the entire merge. The drawback is that some commits won't compile, but compilable mid-branch commits are not something that I view as a goal.
- jsteemann 9y ago
  Non-compiling commits make bisecting harder, when the goal is to find the commit that broke the build or a specific test. IMHO having to skip over non-compiling merge commits only makes bisecting take more time, and it may also "pollute" the test pipelines if all commits are going to be built. Additionally, merging and fixing the merge in separate commits makes it much hard to revert a merge later, in case it turned out that the merge introduced something unwanted). So I will always recommend doing the merge and fixing the merge in the same commit. Ideally this produces a squash commit that can easily be reverted later when needed. This makes the merge process more time-consuming, but it can help to keep the builds more stable, and to more easily track which commit (merge) introduced a particular problem.
  - pkamb 9y ago
    > makes it much hard to revert a merge later I don't think this would apply, as I'd be merging from `dev` > `feature`, resolving conflicts, and then doing a single clean no-conflict merge back from `feature` > `dev`. That's the only commit you'd need to revert to undo the merge. (or on a new, third, `merge` branch, if you don't want to pollute the `feature` branch with commits from `dev`) I admit the non-compiling commits might be a problem in some workflows, but `git bisect skip` is the solution to the only problem I've ever had with it.
    - jsteemann 9y ago
herpderperator 9y ago
That's why I say "you have conflicts" and have them resolve the conflict before I do the review. GitHub, at least, correctly diffs against master after the conflicts are resolved so you don't see any of the merged code (if it's the same as on the master branch), only the actual changes. Doesn't that solve the problem?

by pkamb 9y ago

WorldMaker 9y ago

Too many experiences like that I make merge commits a required part of a code review. If a branch needs conflict resolution in order to merge, it needs that merge in that branch, and as part of the code review of that branch. (Some of the best conflict resolution is sometimes new code in light of the changes in the other branch, and so of course should be reviewed.) If a merge commit diff is showing too much information, that's more often than not a tooling problem. Most tools provide ways to filter out the "automated stuff" and focus only on the conflict resolutions. If they don't, they are deficient (and if there are too many conflict resolutions to easily review, that may be a sign of a larger process problem). (GitHub has gotten much better at merge commit views in the last couple of years, as one anecdotal bullet point.)

seanwilson 9y ago
> Too many experiences like that I make merge commits a required part of a code review. How can you do this practically though? In the turnaround time to do the code review and make the changes, a new conflict might have been introduced into the branch being targeted by the merge. I think merging e.g. the master branch into your branch before code review is a good idea though as when the review is complete there's going to be less or no conflicts merging into master.
- WorldMaker 9y ago
  That's also a tooling issue I believe. GitHub, again, as easy example, has gotten pretty good at "these are the commits since your last review" incremental reviews, so if you complete a code review and need conflicts to be fixed before it can merge, you expect a merge commit to be made in that branch and should just need to review that merge commit. I've not recently seen an issue with a PR cycling through a lot of merge commits to keep up with the branch it is targeting, but in almost every case where I've seen that there was some other process decision in the way (two people working on the exact same feature simultaneously) and at that point it was an architectural, "political", or "social" problem to fix first, outside of the code review process. Your mileage may vary, of course.

stinos 9y ago

Isn't this sort of solved by rebasing first instead of just merging? (well, and some rules in place which tells people 'we only accept properly rebased branches for review). Then the conflicts are resolved in the branch and there's a clear view of what actually changed in the diff, before anything is effectively merged into the main branch. Or maybe I'm missing something here.

Mithaldu 9y ago
It's absolutely resolved that way. Rebase should be the weapon of choice in all cases, except those where it would be prohibitively expensive. People are too enamored with the SVN ways though, or have even made up rationalizations about fictional concepts such as "historical purity". So you have a ton of people merging instead, producing code repositories that look worse than the worst perl code i've touched.
- lemoncucumber 9y ago
  Conversely, people get too enamored of rebasing as a way to solve the problem of merging in lots of junky commits. There are lots of ways to rewrite history and clean it up. Rebasing is just one of them, and it's one that can force you to spend more time resolving conflicts than you otherwise might have to (since you have to replay all of your commits and resolve any conflicts caused by each one individually). There's no reason why rebasing is a better way to clean up your history than squash merging (or even -- carefully -- using git reset).
  - Mithaldu 9y ago
    I already said that it can be too expensive, but thanks for playing. <3 Also, a squash merge is not a merge. It's just another form of rebase. And if you mention reset as an alternative to cherry pick and squash you should also mention --soft.
    - lemoncucumber 9y ago
      My point was that many git users don't understand git all that well, and I think that encouraging everyone to rebase is a bad idea. If you're going to promote a particular workflow as the the one that "should be the weapon of choice," I think the workflow of merge-master-into-feature-branch, squash-merge-feature-branch-into-master is a more user friendly alternative, since then the cases where rebasing is expensive never even come up. And yes, you're right that I was remiss not to mention the --soft part. The git reset workflow is one that I would definitely not encourage people to do if they don't understand git well.

deckar01 9y ago

GitLab makes it really easy to see what changed between commits while reviewing a merge request. Parts of the diff that are identical to the upstream branch are automatically ignored, and you can view a diff between of last two pushes. https://docs.gitlab.com/ce/user/project/merge_requests/versions.html https://docs.gitlab.com/ce/user/project/merge_requests/versi... Edit: GitHub has similar functionality, but it doesn't know how to diff force pushes, so you would end up having to review the full diff again to ensure integrity.

WorldMaker 9y ago
> Edit: GitHub has similar functionality, but it doesn't know how to diff force pushes, so you would end up having to review the full diff again to ensure integrity. I realize this is a religious war I'm stepping into, but I fall on the side of "if you are doing a force push of a branch already in code review you are doing something wrong". Once commits are out in the open and in review between multiple developers, that is shared code history that encodes important information about the code review process itself. (I make obvious exceptions for things like accidental PII disclosure or similar bone-headed mistakes, but the longer a branch has been in code review, the less willing I am to allow it to be force pushed.) Of course, to each their own and your mileage may vary, but that comment about diffing force pushes in a merge/pull request sent a shiver down my spine, regardless of whether or not the tool supports it.
- closeparen 9y ago
  Phabricator is really nice here. The history of the code review process is maintained by Phab itself, you can amend all you want against an inflight code review, and upon landing it all becomes a single commit in master with a link to the code review in the commit message.
- __david__ 9y ago
  You’d hate our team—we force push to master (not just feature branches) all the time. It seems like anarchy at first but it’s not really that hard to deal with. Our situation is a bit unique though, and I doubt it would scale well beyond a small team.

simmons 9y ago

That is scary. I think it would be a nifty feature for a code review tool to show only the conflict resolution for merge commits. (Although I suppose it would need knowledge of exactly which automated merge method/options were used to create the pre-CR merge, for any given commit with multiple parents.)

pkamb 9y ago

What I personally do is commit the merge as is, with all the <<< === >>> conflict markers. Then, in the next couple commits, fix each conflict via a normal commit that mixes and matches "mine" and "theirs" as needed + deletes the markers. Each resolution commit is independent of the giant merge commit, and can be separately diff'ed and code reviewed via the github PR or any other tool. Also easier IMO to make the complicated resolution you want, or reset back to a previous resolution attempt without messing up the entire merge. The drawback is that some commits won't compile, but compilable mid-branch commits are not something that I view as a goal.

jsteemann 9y ago
Non-compiling commits make bisecting harder, when the goal is to find the commit that broke the build or a specific test. IMHO having to skip over non-compiling merge commits only makes bisecting take more time, and it may also "pollute" the test pipelines if all commits are going to be built. Additionally, merging and fixing the merge in separate commits makes it much hard to revert a merge later, in case it turned out that the merge introduced something unwanted). So I will always recommend doing the merge and fixing the merge in the same commit. Ideally this produces a squash commit that can easily be reverted later when needed. This makes the merge process more time-consuming, but it can help to keep the builds more stable, and to more easily track which commit (merge) introduced a particular problem.
- pkamb 9y ago
  > makes it much hard to revert a merge later I don't think this would apply, as I'd be merging from `dev` > `feature`, resolving conflicts, and then doing a single clean no-conflict merge back from `feature` > `dev`. That's the only commit you'd need to revert to undo the merge. (or on a new, third, `merge` branch, if you don't want to pollute the `feature` branch with commits from `dev`) I admit the non-compiling commits might be a problem in some workflows, but `git bisect skip` is the solution to the only problem I've ever had with it.
  - jsteemann 9y ago

herpderperator 9y ago

That's why I say "you have conflicts" and have them resolve the conflict before I do the review. GitHub, at least, correctly diffs against master after the conflicts are resolved so you don't see any of the merged code (if it's the same as on the master branch), only the actual changes. Doesn't that solve the problem?