You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "liurenjie1024 (via GitHub)" <gi...@apache.org> on 2024/08/14 13:14:20 UTC
[PR] doc: Add statement for contributors to avoid force push. [iceberg]
liurenjie1024 opened a new pull request, #10939:
URL: https://github.com/apache/iceberg/pull/10939
When reviews prs for contributors, I found that some contributors prefer to use force push to update pr. This makes it difficult for reviewers to start from last reviews, so I add a statement in contribution guide for new contributors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#discussion_r1719259458
##########
site/docs/contribute.md:
##########
@@ -44,6 +44,7 @@ The Iceberg community prefers to receive contributions as [Github pull requests]
* It's helpful to include a prefix in the summary that provides context to PR reviewers, such as `Build:`, `Docs:`, `Spark:`, `Flink:`, `Core:`, `API:`
* If a PR is related to an issue, adding `Closes #1234` in the PR description will automatically close the issue and helps keep the project clean
* If a PR is posted for visibility and isn't necessarily ready for review or merging, be sure to convert the PR to a draft
+* If a PR is already under review, please avoid force-pushing to the branch as it can make it difficult for reviewers to track changes. If you need to keep the PR up-to-date with the main branch, consider merge rather than rebase
Review Comment:
I meant when I use `git log -n` on the source branch after merge. `merge` would get the new commits from main at the top.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#discussion_r1719050896
##########
site/docs/contribute.md:
##########
@@ -44,6 +44,7 @@ The Iceberg community prefers to receive contributions as [Github pull requests]
* It's helpful to include a prefix in the summary that provides context to PR reviewers, such as `Build:`, `Docs:`, `Spark:`, `Flink:`, `Core:`, `API:`
* If a PR is related to an issue, adding `Closes #1234` in the PR description will automatically close the issue and helps keep the project clean
* If a PR is posted for visibility and isn't necessarily ready for review or merging, be sure to convert the PR to a draft
+* If a PR is already under review, please avoid force-pushing to the branch as it can make it difficult for reviewers to track changes. If you need to keep the PR up-to-date with the main branch, consider merge rather than rebase
Review Comment:
> If you need to keep the PR up-to-date with the main branch, consider merge rather than rebase
I have frequently used rebase and force push. from local branch, rebase is cleaner and easier to see the changes/commits in the PR source branch, as they are in the top. With merge, it can be difficult to find my changes with dozens or hundreds of commits merged in from main branch added on top.
trying to understand the difference btw merge and rebase in this case. can you share a PR where merge is used? want to see how it affect the look of the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#discussion_r1719692703
##########
site/docs/contribute.md:
##########
@@ -44,6 +44,7 @@ The Iceberg community prefers to receive contributions as [Github pull requests]
* It's helpful to include a prefix in the summary that provides context to PR reviewers, such as `Build:`, `Docs:`, `Spark:`, `Flink:`, `Core:`, `API:`
* If a PR is related to an issue, adding `Closes #1234` in the PR description will automatically close the issue and helps keep the project clean
* If a PR is posted for visibility and isn't necessarily ready for review or merging, be sure to convert the PR to a draft
+* If a PR is already under review, please avoid force-pushing to the branch as it can make it difficult for reviewers to track changes. If you need to keep the PR up-to-date with the main branch, consider merge rather than rebase
Review Comment:
I don't think the wording actually says what it intends to say. It's less about the force push and more about not amending/squashing commits, so that it's easier for a reviewer to see what review feedback has been applied
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#issuecomment-2467006691
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#discussion_r1720849209
##########
site/docs/contribute.md:
##########
@@ -44,6 +44,7 @@ The Iceberg community prefers to receive contributions as [Github pull requests]
* It's helpful to include a prefix in the summary that provides context to PR reviewers, such as `Build:`, `Docs:`, `Spark:`, `Flink:`, `Core:`, `API:`
* If a PR is related to an issue, adding `Closes #1234` in the PR description will automatically close the issue and helps keep the project clean
* If a PR is posted for visibility and isn't necessarily ready for review or merging, be sure to convert the PR to a draft
+* If a PR is already under review, please avoid force-pushing to the branch as it can make it difficult for reviewers to track changes. If you need to keep the PR up-to-date with the main branch, consider merge rather than rebase
Review Comment:
anyway, I am neural on this one. I generally don't see force push to be a problem. but agree that it is generally better not to amend earlier commits for the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "ndrluis (via GitHub)" <gi...@apache.org>.
ndrluis commented on PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#issuecomment-2289089290
In my workflow, I prefer to use rebase instead of merge, and I often force-push. However, I utilize the [fixup commit](https://jordanelver.co.uk/blog/2020/06/04/fixing-commits-with-git-commit-fixup-and-git-rebase-autosquash/) to reference the specific past commit I'm modifying, which makes it easier for the reviewer and helps keep the git history cleaner. In the end, I can run a git rebase --autosquash, but since we use squash, this is not a problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#issuecomment-2292614179
> I believe I may have caused some confusion. The problem with force pushing arises when someone fixes the requested changes using --amend. If the person uses a new commit, it’s not an issue. So, in my view, the recommendation should be to avoid using --amend for commits. Instead, create new commits with the suggested changes or use the fixup strategy.
I agree with this.
> For me, a pull request with a lot of "fix" commits and merge commits is much harder to review. It’s better to review the whole PR rather than trying to understand each fix commit, and the merge commits are just unnecessary noise.
In fact we don't need to review each fix commit, in github in you see changes from last view. Merge commits usually will not introduce much noise if you don't have much conflict with main branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#issuecomment-2481696670
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #10939: doc: Add statement for contributors to avoid force push.
URL: https://github.com/apache/iceberg/pull/10939
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#discussion_r1719212282
##########
site/docs/contribute.md:
##########
@@ -44,6 +44,7 @@ The Iceberg community prefers to receive contributions as [Github pull requests]
* It's helpful to include a prefix in the summary that provides context to PR reviewers, such as `Build:`, `Docs:`, `Spark:`, `Flink:`, `Core:`, `API:`
* If a PR is related to an issue, adding `Closes #1234` in the PR description will automatically close the issue and helps keep the project clean
* If a PR is posted for visibility and isn't necessarily ready for review or merging, be sure to convert the PR to a draft
+* If a PR is already under review, please avoid force-pushing to the branch as it can make it difficult for reviewers to track changes. If you need to keep the PR up-to-date with the main branch, consider merge rather than rebase
Review Comment:
The problem with rebase and force push is that reviewer can't pick up from last review.
> With merge, it can be difficult to find my changes with dozens or hundreds of commits merged in from main branch added on top.
Seems this is not the case, unless you have much conflicts with main branch. https://github.com/apache/iceberg-rust/pull/491 is an example of using merge and doesn't introduce commits from main.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#issuecomment-2292839168
I also often use force push myself. The main cases being:
1. CI catches errors between reviews - immediate fix, and easier to review
2. Need to rebase on small upstream changes - if the rebase is trivial, then typically it doesn't need reviewer attention, if big, then a full review is needed.
If there is an easy way to create reviewable merge commits, then the 2nd point could be avoided. Maybe adding a few example comments to the doc would help the developers.
I still think that the first point is a valid use case for the force pushes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "ndrluis (via GitHub)" <gi...@apache.org>.
ndrluis commented on PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#issuecomment-2291278718
> Sorry, I don't get how git commit --fixup + git rebase + git force push helps to make things easier for reviewer to pick up from last review?
I believe I may have caused some confusion. The problem with force pushing arises when someone fixes the requested changes using `--amend`. If the person uses a new commit, it’s not an issue. So, in my view, the recommendation should be to avoid using `--amend` for commits. Instead, create new commits with the suggested changes or use the fixup strategy.
For me, a pull request with a lot of "fix" commits and merge commits is much harder to review. It’s better to review the whole PR rather than trying to understand each fix commit, and the merge commits are just unnecessary noise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#issuecomment-2293295952
> I'm ok with this. Personally I amend + force push quite a bit but if folks find that style hard to review, I will aim for changing that. When reviewing I go through the files again and the targeted review comments rather than the diff between commits but of course recognize that everyone has their style.
I do agree with Amogh as I also amend and force push quite often and it hasn't been brought up as an issue, but I'm fine changing this and only push update commits if people think this is easier when reviewing.
Personally speaking, I do always review the entire diff and use the GH function to mark reviewed files as **Viewed**. If an update to that file has been pushed, then GH will properly show that there are new changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#discussion_r1719050896
##########
site/docs/contribute.md:
##########
@@ -44,6 +44,7 @@ The Iceberg community prefers to receive contributions as [Github pull requests]
* It's helpful to include a prefix in the summary that provides context to PR reviewers, such as `Build:`, `Docs:`, `Spark:`, `Flink:`, `Core:`, `API:`
* If a PR is related to an issue, adding `Closes #1234` in the PR description will automatically close the issue and helps keep the project clean
* If a PR is posted for visibility and isn't necessarily ready for review or merging, be sure to convert the PR to a draft
+* If a PR is already under review, please avoid force-pushing to the branch as it can make it difficult for reviewers to track changes. If you need to keep the PR up-to-date with the main branch, consider merge rather than rebase
Review Comment:
> If you need to keep the PR up-to-date with the main branch, consider merge rather than rebase
I have frequently used rebase and force push. from local branch, rebase is cleaner and easier to see the changes/commits in the PR source branch, as they are in the top.
trying to understand the difference btw merge and rebase in this case. can you share a PR where merge is used? want to see how it affect the look of the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] doc: Add statement for contributors to avoid force push. [iceberg]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #10939:
URL: https://github.com/apache/iceberg/pull/10939#issuecomment-2290809213
> In my workflow, I prefer to use rebase instead of merge, and I often force-push. However, I utilize the [fixup commit](https://jordanelver.co.uk/blog/2020/06/04/fixing-commits-with-git-commit-fixup-and-git-rebase-autosquash/) to reference the specific past commit I'm modifying, which makes it easier for the reviewer and helps keep the git history cleaner. In the end, I can run a git rebase --autosquash, but since we use squash, this is not a problem.
Sorry, I don't get how `git commit --fixup` + `git rebase` + `git force push` helps to make things easier for reviewer to pick up from last review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org