Code review comment for ~sudipmuk/ubuntu/+source/zeitgeist:merge-lp2044300-noble

Revision history for this message
Utkarsh Gupta (utkarsh) wrote :

Hey there!

Thanks for raising this much, much appreciated. This is great as-is but here are some points that'd make it EVEN better:

- https://git.launchpad.net/~sudipmuk/ubuntu/+source/zeitgeist/commit/?id=95fd328c45f72fa0f3895900216ad0726e86139e -> it misses the DEP-3 headers. I am aware that you're just rebasing the new version and you're not really adding the patch itself. But whilst at it, if you could really add DEP-3 headers to a patch, that'd be EXCELLENT!

See: https://git.launchpad.net/~sudipmuk/ubuntu/+source/zeitgeist/commit/?id=a6cff2e5ac675b90cc574a26dc953f452aa9a00c for example. It has the DEP-3 headers and tells me why it's needed, some bug history, et al.

I generally look at it because sometimes we shouldn't really be merging stuff as-is and carry-forwarding the tech-debt but actively be looking at drop the delta wherever and whenever possible.

Now, OTOH, look at https://git.launchpad.net/~sudipmuk/ubuntu/+source/zeitgeist/commit/?id=e940d9763d0fba97dc8504ed382e01210a43f08d -> it gives me a quick description but doesn't tell me if it's coming from upstream or was it written by downstream, is there an upstream bug? a downstream bug? or? and I spend my time looking at things, et al. That's why DEP-3 headers are really useful.

- https://git.launchpad.net/~sudipmuk/ubuntu/+source/zeitgeist/commit/?id=2ab99989a22d2e07bfc28e5eae942bd8283c1aa7 -> did you verify if that's still needed? why? :)

So except for the above question and a general pointer (first point), it mostly looks good! :D

review: Needs Information

« Back to merge proposal