From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff King Subject: Re: [RFC PATCH] git-add--interactive: manual hunk editing mode v2 Date: Thu, 5 Jun 2008 04:11:28 -0400 Message-ID: <20080605081126.GA16416@sigill.intra.peff.net> References: <200805232221.45406.trast@student.ethz.ch> <200806010241.51464.trast@student.ethz.ch> <20080605014618.GA27381@sigill.intra.peff.net> <200806050954.13244.trast@student.ethz.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: git@vger.kernel.org To: Thomas Rast X-From: git-owner@vger.kernel.org Thu Jun 05 10:12:53 2008 Return-path: Envelope-to: gcvg-git-2@gmane.org Received: from vger.kernel.org ([209.132.176.167]) by lo.gmane.org with esmtp (Exim 4.50) id 1K4AaZ-0008M1-Ee for gcvg-git-2@gmane.org; Thu, 05 Jun 2008 10:12:51 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751355AbYFEILg (ORCPT ); Thu, 5 Jun 2008 04:11:36 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751042AbYFEILe (ORCPT ); Thu, 5 Jun 2008 04:11:34 -0400 Received: from peff.net ([208.65.91.99]:3638 "EHLO peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751127AbYFEILd (ORCPT ); Thu, 5 Jun 2008 04:11:33 -0400 Received: (qmail 15920 invoked by uid 111); 5 Jun 2008 08:11:30 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) (smtp-auth username relayok, mechanism cram-md5) by peff.net (qpsmtpd/0.32) with ESMTP; Thu, 05 Jun 2008 04:11:30 -0400 Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Thu, 05 Jun 2008 04:11:28 -0400 Content-Disposition: inline In-Reply-To: <200806050954.13244.trast@student.ethz.ch> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: On Thu, Jun 05, 2008 at 09:53:54AM +0200, Thomas Rast wrote: > On the other hand it would be just as powerful. Manually splitting a > hunk is, in the general case, only possible in "my" scheme. However, > to make any difference, you later have to answer 'n' to some of the > sub-hunks. So in "your" scheme, you could just have deleted the lines > in question. Sorry, I've gotten lost in which is mine and which is yours. Yours is the original patch or this patch? Mine is the proposal that spawned this patch, or my comments on this patch? > > What about lines starting with characters besides -, +, space, or @? > > They will normally be ignored by diff. > > Diff doesn't really have a say in this, does it? And looking in > builtin-apply.c: Sorry, I meant to say "patch" here. But even so, I'm still wrong. Arbitrary text is OK between _diffs_, but not between _hunks_. And by definition, this format is hunks inside a single diff. > default: > if (apply_verbosely) > error("invalid start of line: '%c'", first); > return -1; Right, so we signal the end of diff, and any hunks afterwards are "patch fragment without header". > so it appears invalid lines are actually not ignored, but abort hunk > processing. While the error checking will be handled by apply > --check, I don't think it would be a good idea to silently drop all > other lines from the edit, as they probably indicate user error. Good point. There's really no reason for such lines to occur, so the best thing is probably to notice the situation and let the user re-edit. > On the other hand, this also shows that dropping empty lines is > wrong... Hmm. Yes, it looks like they are significant. Given that they are generated by a particular version of GNU diff, and that these should be "git diff"-generated patches, we aren't likely to encounter them. But probably we should just leave them as-is, and let "git apply --check" do what it will with them. > From a Perl POV, they probably _are_ horrible. I'm just not used to > the idioms, and tend to fall for semantic differences to Python as > well. Heh. The Perl style in git is a bit tricky; it is such a small portion of the code base, and there is not a real sense of ownership, so we seem to have several different styles. > Thank you for the very thorough review! I'll improve the patch > accordingly. Great. I think this is a worthwhile feature, so please keep at it. -Peff