From owner-cvs-all Sun Jan 26 4:22:16 2003 Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 4C01637B401; Sun, 26 Jan 2003 04:22:13 -0800 (PST) Received: from 12-234-22-23.client.attbi.com (12-234-22-23.client.attbi.com [12.234.22.23]) by mx1.FreeBSD.org (Postfix) with ESMTP id 75FFB43ED8; Sun, 26 Jan 2003 04:22:12 -0800 (PST) (envelope-from DougB@FreeBSD.org) Received: from slave.gorean.org (0ekj0thsl7mgygub@slave.gorean.org [10.0.0.1]) by 12-234-22-23.client.attbi.com (8.12.6/8.12.6) with ESMTP id h0QCM7jD028349; Sun, 26 Jan 2003 04:22:12 -0800 (PST) (envelope-from DougB@FreeBSD.org) Date: Sun, 26 Jan 2003 04:22:07 -0800 (PST) From: Doug Barton To: Matt Dillon Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/usr.sbin/mergemaster mergemaster.sh In-Reply-To: <200301251918.h0PJI6ld069132@freefall.freebsd.org> Message-ID: <20030126040430.Q26257@12-234-22-23.pyvrag.nggov.pbz> References: <200301251918.h0PJI6ld069132@freefall.freebsd.org> Organization: http://www.FreeBSD.org/ X-message-flag: Outlook -- Not just for spreading viruses anymore! MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-cvs-all@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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