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

get_change_id: Add diagnostic output #11

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

dancysoft
Copy link
Collaborator

If get_change_id doesn't find a Change-Id, include the commit message in the error message so that the user can identify the offending commit. It would be even better if it mentioned the commit hash but this is a step in the right direction.

If get_change_id doesn't find a Change-Id, include the commit message
in the error message so that the user can identify the offending
commit.  It would be even better if it mentioned the commit hash but
this is a step in the right direction.
@yaoyuannnn
Copy link
Owner

Thanks for the contribution! Looks like I have some flaky tests in the CI for a while. I couldn't root cause it last time I tried to look into it. Will get to that soon.

@dancysoft
Copy link
Collaborator Author

Thanks for the contribution! Looks like I have some flaky tests in the CI for a while. I couldn't root cause it last time I tried to look into it. Will get to that soon.

Thanks!

By the way you should be aware that your tool is being considered for regular use at Wikimedia Foundation (for gitlab.wikimedia.org), hence the recent batch of PRs from me and @thcipriani. This is the ticket where it is being discussed: https://phabricator.wikimedia.org/T300819. Your thoughts on the subject are welcome!

@yaoyuannnn
Copy link
Owner

Thanks for the contribution! Looks like I have some flaky tests in the CI for a while. I couldn't root cause it last time I tried to look into it. Will get to that soon.

Thanks!

By the way you should be aware that your tool is being considered for regular use at Wikimedia Foundation (for gitlab.wikimedia.org), hence the recent batch of PRs from me and @thcipriani. This is the ticket where it is being discussed: https://phabricator.wikimedia.org/T300819. Your thoughts on the subject are welcome!

Wow, thanks for letting me know @dancysoft @thcipriani ! It's kinda cool that this little tool caught this much attention. I'm very accustomed to the Gerrit-style stacked patches and believe having that will lead to a cleaner commit history and in turn make people write better code. But I also see the benefit of the GitLab style where multiple commits exist in an MR. For example, people sometimes may want to dig a hole with a draft MR and mess around a little and get early feedback, though I'd still prefer they split commits into separate MRs after things are ironed out. So this is really about what you want; I see my colleagues using GerritLab in one project while otherwise sticking with GitLab.

And therefore, I've been thinking if we can combine these two styles in GerritLab, specifically, allow multiple commits in an MR while still having the stacked MR hierarchy. My idea was having a "git review -i" command to let the user decide how to group the commits. -i means interactive, similar to the same flag in git rebase -i. So a file like the following would show up after a git review -i (assuming I have 5 commits ahead of the remote):

pick 1568d commit 0.
pick 6bb03 commit 1.
pick 8abc3 commit 2.
pick d4568 commit 3.
pick 89b03 commit 4.

# Review commits using GerritLab.
#
# Commands:
# p, pick <commit> = use commit for an individual MR.
# c, combine <commit> = combine commit to be in the same MR as the previous commit.

The above will create 5 MRs by default. The user can then edit this file, similar to git rebase -i, to group commits into a single MR by using the combine command. For example, the following will create 3 MRs, with commit 2 and commit 3 combined into the same MR as commit1.

pick 1568d commit 0.
pick 6bb03 commit 1.
combine 8abc3 commit 2.
combine d4568 commit 3.
pick 89b03 commit 4.

# Review commits using GerritLab.
#
# Commands:
# p, pick <commit> = use commit for an individual MR.
# c, combine <commit> = combine commit to be in the same MR as the previous commit.

Does this sound interesting or perhaps it's sort of over-engineering?

@dancysoft
Copy link
Collaborator Author

And therefore, I've been thinking if we can combine these two styles in GerritLab, specifically, allow multiple commits in an MR while still having the stacked MR hierarchy. My idea was having a "git review -i" command to let the user decide how to group the commits. -i means interactive, similar to the same flag in git rebase -i. So a file like the following would show up after a git review -i (assuming I have 5 commits ahead of the remote):

pick 1568d commit 0.
pick 6bb03 commit 1.
pick 8abc3 commit 2.
pick d4568 commit 3.
pick 89b03 commit 4.

# Review commits using GerritLab.
#
# Commands:
# p, pick <commit> = use commit for an individual MR.
# c, combine <commit> = combine commit to be in the same MR as the previous commit.

The above will create 5 MRs by default. The user can then edit this file, similar to git rebase -i, to group commits into a single MR by using the combine command. For example, the following will create 3 MRs, with commit 2 and commit 3 combined into the same MR as commit1.

pick 1568d commit 0.
pick 6bb03 commit 1.
combine 8abc3 commit 2.
combine d4568 commit 3.
pick 89b03 commit 4.

# Review commits using GerritLab.
#
# Commands:
# p, pick <commit> = use commit for an individual MR.
# c, combine <commit> = combine commit to be in the same MR as the previous commit.

Does this sound interesting or perhaps it's sort of over-engineering?

I love this idea and there have been many times where I could have used a feature like this!

@yaoyuannnn
Copy link
Owner

I just reran the tests. Looks like they now are passing. I will get to it over the weekend.

Also created an issue #16 for the new feature we discussed. Will start to do it as well :)

@dancysoft dancysoft merged commit b07f522 into yaoyuannnn:main Aug 21, 2023
@dancysoft dancysoft deleted the review/get_change_id_diags branch August 21, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants