Date: Sun, 26 Jan 2003 04:22:07 -0800 (PST) From: Doug Barton <DougB@FreeBSD.org> To: Matt Dillon <dillon@FreeBSD.org> Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/usr.sbin/mergemaster mergemaster.sh Message-ID: <20030126040430.Q26257@12-234-22-23.pyvrag.nggov.pbz> In-Reply-To: <200301251918.h0PJI6ld069132@freefall.freebsd.org> References: <200301251918.h0PJI6ld069132@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 25 Jan 2003, Matt Dillon wrote: > These changes are the ones that were discussed quite a while ago, > a few months ago I think. Generally speaking the end of that discussion > was positive towards the patch, but some people still had reservations > over not displaying the complete diff | pager initially. I was one of those people, but I don't think that I made my objection explicit. I'm not sure if that was a mistake or not, however I honestly didn't expect you to commit this change without passing it by me first. I've been very clear about my desire to review changes to mergemaster before they are committed. My preference is listed in MAINTAINERS, and the Makefile. I have the following technical problems with this patch as well: 1. You've redefined the meaning of the 'v' menu option. This is a _huge_ UI bozo no-no. 2. You've duplicated effort on the column width code, your stty commands are inefficient, and the style in general doesn't match the rest of the code (although you've duplicated some of the same mistakes others have made here, so that last one is forgivable). 3. The most disturbing part of your change is that it's not immediately apparent when there is a diff which is longer than DIFFROWS. Previously, if the user saw a page full of diff, then the mm menu at the bottom, they could safely assume that they'd seen all of the diff. You've redefined this behavior as well, and I'm not really comfortable with that. The one part of your patch that I can agree with is the concept of using 'clear' to start display of new messages. I'd be willing to incorporate that part if I can prove that this is a safe thing to do in all circumstances. The rest of your patch only applies to diffs which are shorter than $ROWS; and the meat of your changes are accomplished just by using 'clear'. Diffs that are longer are already handled by PAGER. So, at this point I'd really like you to back out this change. I'll work on a version that incorporates the 'clear,' and send it over to see if it meets your needs. Doug To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030126040430.Q26257>