Code review comment for lp:~parthm/bzr/300062-invalid-pattern-warnings

Revision history for this message
Parth Malwankar (parthm) wrote :

Well, we now have both solutions in place. The warnings based (this) and the error based[1] though the latter may need some cleanup and review.

My personal preference is towards an error based approach. I feel warnings are may be ok for operations like 'status' that don't change anything, but specifically for something like 'add' an error would probably be better so that the user doesn't miss the warning message and go ahead and do the commit. We saw The user could 'revert' but it may be a bit of a pain to do that, or worse, user may miss the message and end up with multiple commits after that containing large binary files :) The 'add' operation with warnings specifically reminds me of bug #549310 where whoami was guessing and I ended up with multiple commits with a bad username[2]. I would prefer a uniform error based approach as Robert suggested.
I don't expect the rollout of an error based approach to be a problem as bzr currently crashes on bad patterns so the existing ignore files will not have bad patterns in them.

That said, I think both warning and error based approach are an improvement over the current situation where bzr shows a stack trace which can scare users. It would be great if we can align on one of these after some more discussion and go ahead.

[1] https://code.edge.launchpad.net/~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern/+merge/26400
[2] https://bugs.launchpad.net/bzr/+bug/549310/comments/3

« Back to merge proposal