The goal of this issue is to collect any feedback on user's experiences with the new system for detecting changes in the diff. We of course also welcome any ideas, suggestions, or additional use cases for leveraging patch-id, and would encourage you to leave them on the project epic so they're not lost.
I was babysitting this MR, and I after I approved it, I needed to rebase. I used a slash command and afterwards, my approval was gone and I could not re-approve. Is this expected?
I was working with a GitLab Premiumcustomer on confidential ticket 501547. This case involved an indentation-only change that did not reset the patch ID. In cases like YAML and Python whitespace changes can be important; from the git-patch-id docs ignoring whitespace appears to be expected behavior; this could allow unapproved changes to Kubernetes manifests and/or Python files.
@duncan_harris Wouldn't something like a linter catch that the whitespace is now wrong? I don't think it would actually change the function of the code, but it would just be wrong? I guess I'd expect that to be caught via a pipeline failure at that point and then it wouldn't merge because the pipeline wouldn't pass.
@kerrizor Sorry, I don't remember exactly why we went with --stable, but I suppose we may have with it to only detect meaningful changes. I see the point that it could be troublesome for those cases. @pks-gitlab Do you have any thoughts on this?
Patch hunks can be reordered in a diff. A diff of two files would result in the same patch ID regardless of which of the two files comes first inside of the diff.
Whitespace is being ignored.
It's kind of dubious whether the first property really is required for our usecase. It may be when generating patch IDs across different Git versions, but the last time this changed was in Git 1.9. This is thus rather a theoretical concern.
The --verbatim flag actually implies --stable, so the first of the above bullet points will remain the same. The only change compared to --stable is that whitespace will not be stripped.
So from the technical point of view both would be fine. We can easily amend the RPC to take e.g. an enum as input that allows the caller to specify which of both modes to use, defaulting to --stable.
Regarding the UI/UX I'll defer to you to decide what's best, as I'm not qualified to judge here.
Thanks for the additional details here @pks-gitlab.
@dskim_gitlab@kerrizor I'm inclined to think we leave it as is for now and continue to evaluate. I'm still not sure on the whitespace bits here and whether or not that would have other unintended consequences.
From my perspective, for whitespace sensisitive languages like Python... that should be evaluated via CI anyway which should run on the new push. It shouldn't be something we're trying to catch with an approval.
I would say it's safer to err on the side of whitespace-sensitive exact matches.
I don't know if a vulnerability/exploit exists that only involves adding whitespace to some code, but I'm sure an attacker could come up with one eventually.
I would say it's safer to err on the side of whitespace-sensitive exact matches.
I think I'm in agreement with you, @cwoolley-gitlab. I can't think of an exploit for whitespace changes, but in a language or context where whitespace is meaningful then I think we want to encourage having its changes be reviewed. After all, that's sort of the story of this feature - detecting meaningful change.
@pks-gitlab I like being able to specify which mode to use, as that will let us feature flag it, and possibly do more with it later - I don't know what, but it leaves the door open for now. Should I open a new issue and we can hash out the details there?
@kerrizor If we used --verbatim it seems like that might reset more approvals since patch hunks wouldn't be allowed to move. For example, if I change lines 15-20 and then rebase and the rebase adds new lines like 5-10... then my changes will move down 5 lines and show up as a different patch-id now. I'm not sure that's what we want to happen here?
@phikai--verbatimdoes account for this and behaves the same as --stable in that regard. The only difference really only is how whitespace is being accounted for.
I agree that this is not obvious at all from git-patch-id(1), so I had to look this up in code myself.
Reposting this here from #216144 (closed): We (GitLab Ultimate customer) use Renovate Bot for dependency updates and have the "Prevent approvals by users who add commits" option enabled and require fast forward merges. I approved such an MR but it was behind main at that point and required a rebase. So I clicked "Rebase" in the MR UI. However, after that I was not able to merge the MR because I was now considered as a user who added commits. In my opinion I should still be able to merge because I did not make any changes. I read the thread closed above where compliance and auditing was discussed. My expectation as a user is that I should be able to merge it. Though, I don't fully understand/am sure if this is an issue from an audit/compliance perspective.