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 flow #52

Open
Keith-CY opened this issue May 31, 2023 · 9 comments
Open

Git flow #52

Keith-CY opened this issue May 31, 2023 · 9 comments

Comments

@Keith-CY
Copy link
Member

我建议后续新开的大型分支的工作流从 基于 merge 引入依赖分支的改动 改为优先使用 基于 rebase 引入依赖分支的改动 。
这样可以很大程度的降低引入其他分支的改动时的心智负担,并让依赖分支的作者可以更容易的接手大型分支的 rebase 操作。
目前已有的大型分支就仍然保持原来的工作流,因为现在已经不太切换了。

举个例子,如果是 rebase 工作流的一个大型分支 A,要 rebase 到 eslint + prettier 的改动分支 B 上。
只需要在 A 执行 rebase B,然后在冲突的 commit 上全部应用 A 的改动,然后为每个新 commit 执行一遍 eslint + prettier 即可,不需要管具体的冲突内容。
或者在 B 执行 rebase A,然后 B 的几个小 commit 不会有太多冲突,手动处理掉,把冲突最多的那两个 eslint、prettier 执行的 commit 全部信任 A(或者直接 drop 然后重建)然后再执行一遍 eslint + prettier 来让改动正确。
最后通过 range-diff 来确认改动是否都符合预期,比如 git range-diff @{u}$(git rev-list --count @ ^upstream/develop)..@{u} @$(git rev-list --count @ ^upstream/develop)..@。

Proposed by @WhiteMinds

@Keith-CY
Copy link
Member Author

Keith-CY commented May 31, 2023

@WhiteMinds
Copy link

WhiteMinds commented Jun 27, 2023

My proposal has no impact on Git flow (branch model), it simply encourages team members to use rebase as much as possible to handle conflicts and reduce the creation of merge commits.

Below, I will list the reasons for doing this, rebase vs merge (focusing only on the (efficiency) differences in team collaboration, not on differences such as commit graph):

  1. After resolving conflicts, using rebase instead of merge can make it easier for other team members to read the changes caused by the pull request.

    1. Because merge creates a merge commit without changing the base of the entire pull request, team members will read the diff as old base vs new commits + merge commit (which may have a lot of changes that they shouldn't actually be concerned about).
      On the other hand, rebase is new base vs new commits, without any other unnecessary diffs, which allows team members to focus on the actual changes caused by the pull request.

    2. In cases where there are many commits but each commit's description or classification is not clear enough, or where later commits undo the changes made in earlier commits, multiple commits can actually hinder the efficiency of code review.
      In such cases, I would use git rebase -i $(git merge-base upstream/develop @) locally to combine them into a single commit for review. However, it's difficult to do this when there is a merge commit, because the changes made in the merge commit will also be included in the combined commit.

    3. When reviewing multiple commits in a pull request locally, if the pull request has undergone multiple merge operations, it can be difficult to locate the first few commits, which reduces the efficiency of the review.

  2. Using rebase can reduce the mental burden when resolving conflicts in many scenarios, but it may increase the workload slightly.

    1. Because rebase resolves conflicts commit by commit, each commit only has its own part of the conflict, which reduces the number of conflicts that need to be handled simultaneously, making it easier to think and check whether the conflicts have been resolved correctly.
      Additionally, after resolving conflicts, git range-diff can be used to verify which changes were made during conflict resolution.

    2. Merge only resolves conflicts between the merge target and the latest commit in the pull request, so it is faster, but this advantage is not very significant when there are few conflicts. When there are many conflicts, the increase in mental burden may increase the time it takes to resolve conflicts or introduce bugs during conflict resolution.

    3. When there are significant conflicts in some commits, rebase can refer to the changes in the original commit to independently resolve them or completely rewrite and regenerate them (similar to the automatic processing scenario of prettier --write). In contrast, merge can only handle all conflicts together.

  3. It can be difficult to organize commits using git rebase -i after using merge, and organizing commits is an important means of improving reviewer efficiency.

  4. In the case of commit -> merge -> commit, it can be difficult to undo the merge operation, while it is much easier with rebase.

Below are some commands that are helpful for a workflow based on rebase:

  1. git rebase --onto

    1. This is suitable for use when the history of the current base has changed, such as when the parent branch has done a force push operation, or when you want to change the parent branch to another version.

    2. For example, suppose I have a hotfix that needs to be switched from being based on the upstream/develop branch to being based on the upstream/master branch:
      git rebase --onto upstream/master $(git merge-base upstream/develop @)
      This command can be used.

  2. git rebase --rebase-merges

    1. Use this command when the commits in the current branch include a merge commit to ensure that the merge commit exists after the rebase.
  3. git rebase --strategy-option="rename-threshold=10"

    1. By changing the rename-threshold, file migration operations (such as refactoring) that occur on the newBase can be more easily matched, rather than deleting one file and adding a new file.
  4. git range-diff

    1. This command is used to compare the diffs of n commits in two pull requests. It can be used to compare the pull request before and after conflict resolution, so you can see what impact was actually caused during conflict resolution.

    2. For example, suppose I have a branch based on upstream/develop, and the previous changes have been pushed to origin (which can be referenced locally using @{u}). After resolving conflicts with rebase locally, use the following command to check the changes:
      git range-diff @{u}~$(git rev-list --count @ ^upstream/develop)..@{u} @~$(git rev-list --count @ ^upstream/develop)..@

Below are some suggestions for organizing commits:

  1. When organizing commits, differentiate them by functionality or implementation order. The changes in each commit should be related to each other and indispensable. Otherwise, consider splitting them into another commit.
    Avoid putting a large number of changes in a single commit, as this increases the mental burden and efficiency of reviewers in distinguishing whether each change is related and the scope of their impact.
    When there are too many changes or the mental burden is too great, it may cause the pull request to be postponed for review by the reviewer, or the reviewer may not be careful enough during review and overlook the possibility of bugs caused by the interaction between certain changes.

  2. When migrating files and refactoring, create a migration commit first, followed by a refactoring commit. This makes it easier for Git and reviewers to distinguish between a file migration and refactoring, rather than a file deletion and a file addition.

@yanguoyu
Copy link

yanguoyu commented Jun 28, 2023

An example of git rebase --rebase-merges https://yanhaijing.com/git/2020/09/23/git-rebase-merge/

@yanguoyu
Copy link

Then should we choose rebase and merge to merge PR?

@WhiteMinds
Copy link

Then should we choose rebase and merge to merge PR?

Since this does not affect our team's collaboration efficiency, we should consider whether to use rebase and merge from other aspects. For now, we can continue to use the previous merge PR solution.

@WhiteMinds
Copy link

My proposal has no impact on Git flow (branch model), it simply encourages team members to use rebase as much as possible to handle conflicts and reduce the creation of merge commits.

Below, I will list the reasons for doing this, rebase vs merge (focusing only on the (efficiency) differences in team collaboration, not on differences such as commit graph):

...

Based on my previous discussion, I suggest using rebase to resolve conflicts in newly created PR.

@Magickbase/developers Does anyone have any other suggestions?

@Keith-CY
Copy link
Member Author

Keith-CY commented Jul 2, 2023

My proposal has no impact on Git flow (branch model), it simply encourages team members to use rebase as much as possible to handle conflicts and reduce the creation of merge commits.

Below, I will list the reasons for doing this, rebase vs merge (focusing only on the (efficiency) differences in team collaboration, not on differences such as commit graph):

...

Based on my previous discussion, I suggest using rebase to resolve conflicts in newly created PR.

@Magickbase/developers Does anyone have any other suggestions?

I'll also suggest using rebase to resolve conflicts(not only resolving conflicts, but also catching up with the base branch) because it makes commit history clear. The commit history itself is the document of the PR to express author's purpose. Any merge develop into feature branch is a disturbance. I even have a plugin to dim this kind of commits
image

I'll only use merge develop into feature branch on GitHub Web UI when the branch is out of date and the author doesn't update it timely.

It's a passive strategy.

The ideal case is that authors will rebase their branches on the develop branch actively.

@homura
Copy link

homura commented Jul 4, 2023

I suggest that we first consider

  • what platform/tools of our workflow is
  • what purpose of our unified git flow with git rebase is

What's the current workflow

The diff view of GitHub is the most frequently used before a PR has been merged, and I think the diff view is more close to a "merge" workflow since it diff a whole PR instead of each commit.

Does rebase lower the mental load?

Maybe not. AFAIK, the most git user starts by the git merge instead of git rebase

Dose we have to support multiple version delivery?

Most likely not. The clear history graph that git rebase brings may not be very useful to us, which would increase development costs.

Is the additional merge commit a disturbance?

Maybe yes, maybe no. When the answer is

  • yes, for when we want to know how this PR/branch is advanced
  • no, for when we want to find an available fork from the history, the merge commit can be a search pattern to this

So what's the problem?

I think it is the not good enough arrangement. When a new PR that would change a lot, and there was another large PR working on in the PR list

  • if the new PR can be done automatically, like the mentioned eslint above
    • the new PR could wait for another large PR to merge first
    • or maybe we can change
  • if the new PR is another feature with a lot of changes
    • maybe we should consider how to split the task into smaller ones
    • or maybe we should git merge/rebase with main/develop branch frequently to avoid inattention due to the long processing time of a large diff at once

Therefore, I suggest that the merge method in a working PR or branch could follow personal habits, while the merge of PR to main should be handled as long as there is a unified rule, it doesn't matter if it is merge/squash/rebase

@WhiteMinds
Copy link

WhiteMinds commented Jul 4, 2023

I suggest that we first consider

  • what platform/tools of our workflow is

If you are referring to tools used during the review process, there should be no constraints. However, I believe that most developers in the frontend ecosystem use one of the following:

  1. Github diff view
  2. Github desktop
  3. VSCode diff view
  4. WebStorm diff view
  5. Vim / other IDE
  • what purpose of our unified git flow with git rebase is

The core purpose is to make it easier, faster, and more reassuring for reviewers to read PRs, which will improve the efficiency of merging PRs and increase the likelihood of errors being pointed out in PRs.

I believe that there are two important factors that contribute to the ability to quickly review PRs:

  1. Control the amount of content in the PR.
  2. Divide changes in the PR that do not affect each other through commits and organize the commits in order.
    Using merge to resolve conflicts mainly affects this part.

What's the current workflow

The diff view of GitHub is the most frequently used before a PR has been merged, and I think the diff view is more close to a "merge" workflow since it diff a whole PR instead of each commit.

I think this statement is basically correct and does not conflict with my views:

  1. Many reviewers use Github's diff view to compare overall changes, but they can also select the left commit in this view to view the changes in order. Similarly, even if the PR is pulled locally, it can also be reviewed by commits or as a whole.

  2. The overall diff changes are obviously closer to the result of the merge, but this obviously loses the review benefits brought by commits. Moreover, this will review the changes of unrelated merge commits again, which will waste the reviewer's time.

Does rebase lower the mental load?

Maybe not. AFAIK, the most git user starts by the git merge instead of git rebase

It's not about reducing the mental burden of the PR creator, but about reducing the mental burden of the reviewer, and the cost of doing so is completely worth it.

Dose we have to support multiple version delivery?

Most likely not. The clear history graph that git rebase brings may not be very useful to us, which would increase development costs.

Regarding the action of merging PRs, I also tend to merge rather than rebase + merge, but I haven't thought about it too much.

Is the additional merge commit a disturbance?

Maybe yes, maybe no. When the answer is

  • yes, for when we want to know how this PR/branch is advanced
  • no, for when we want to find an available fork from the history, the merge commit can be a search pattern to this

During the review, it is a distraction (the reason is stated in #52 (comment)), but when it becomes history, it may be an advantage, as I mentioned earlier.

Therefore, my suggestion is to use rebase when resolving conflicts, and either solution is acceptable when merging PRs, as they do not involve my core goal (improving the quality and efficiency of reviews).

So what's the problem?

I think it is the not good enough arrangement. When a new PR that would change a lot, and there was another large PR working on in the PR list

  • if the new PR can be done automatically, like the mentioned eslint above

    • the new PR could wait for another large PR to merge first
    • or maybe we can change

I don't think I understand the statement here.

  • if the new PR is another feature with a lot of changes
    • maybe we should consider how to split the task into smaller ones
    • or maybe we should git merge/rebase with main/develop branch frequently to avoid inattention due to the long processing time of a large diff at once

I agree with splitting PRs and merging them as soon as possible, but this does not conflict with the idea of using rebase to resolve conflicts. Both should be done simultaneously, as they will both improve the quality and efficiency of PR reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants