From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Keeping Subject: Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules Date: Fri, 13 Sep 2013 09:48:34 +0100 Message-ID: <20130913084833.GD2582@serenity.lan> References: <7c478c19da6ee3322ca87e77a90358a30178c286.1379013786.git.john@keeping.me.uk> <20130912202128.GB2582@serenity.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Junio C Hamano , Git Mailing List , Jens Lehmann , Johannes Sixt To: Duy Nguyen X-From: git-owner@vger.kernel.org Fri Sep 13 10:48:53 2013 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1VKP3d-0001Qo-BS for gcvg-git-2@plane.gmane.org; Fri, 13 Sep 2013 10:48:53 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751830Ab3IMIsu (ORCPT ); Fri, 13 Sep 2013 04:48:50 -0400 Received: from coyote.aluminati.org ([72.9.247.114]:33130 "EHLO coyote.aluminati.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751487Ab3IMIsq (ORCPT ); Fri, 13 Sep 2013 04:48:46 -0400 Received: from localhost (localhost [127.0.0.1]) by coyote.aluminati.org (Postfix) with ESMTP id 7C1AF198002; Fri, 13 Sep 2013 09:48:45 +0100 (BST) X-Virus-Scanned: Debian amavisd-new at caracal.aluminati.org X-Spam-Flag: NO X-Spam-Score: -2.899 X-Spam-Level: X-Spam-Status: No, score=-2.899 tagged_above=-9999 required=6.31 tests=[ALL_TRUSTED=-1, BAYES_00=-1.9, URIBL_BLOCKED=0.001] autolearn=ham Received: from coyote.aluminati.org ([127.0.0.1]) by localhost (coyote.aluminati.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id olN0iTwCIl2b; Fri, 13 Sep 2013 09:48:44 +0100 (BST) Received: from pichi.aluminati.org (pichi.aluminati.org [10.0.16.50]) by coyote.aluminati.org (Postfix) with ESMTP id 7BC12606538; Fri, 13 Sep 2013 09:48:44 +0100 (BST) Received: from localhost (localhost [127.0.0.1]) by pichi.aluminati.org (Postfix) with ESMTP id 6E3D9161E4BF; Fri, 13 Sep 2013 09:48:44 +0100 (BST) X-Virus-Scanned: Debian amavisd-new at aluminati.org Received: from pichi.aluminati.org ([127.0.0.1]) by localhost (pichi.aluminati.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id MTUN+5kiWP5A; Fri, 13 Sep 2013 09:48:43 +0100 (BST) Received: from serenity.lan (tg1.aluminati.org [10.0.16.53]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by pichi.aluminati.org (Postfix) with ESMTPSA id 3A971161E189; Fri, 13 Sep 2013 09:48:35 +0100 (BST) Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: On Fri, Sep 13, 2013 at 08:28:24AM +0700, Duy Nguyen wrote: > On Fri, Sep 13, 2013 at 3:21 AM, John Keeping wrote: > > On Thu, Sep 12, 2013 at 12:48:10PM -0700, Junio C Hamano wrote: > >> John Keeping writes: > >> > >> > This allows us to replace the submodule path trailing slash removal in > >> > builtin/rm.c with the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to > >> > parse_pathspec() without changing the behaviour with respect to multiple > >> > trailing slashes. > >> > >> Where does prefix_pathspec()'s input, which could have an unwanted > >> trailing slash, come from? > >> > >> If it is read from some of our internal data structure and known to > >> have at most one, then this change makes me feel very uneasy to cope > >> with potentially sloppy end-user input and data generated by ourselves > >> with the same logic. It will allow our internal to be sloppy without > >> forcing us notice and fix that sloppiness. > >> > >> If it is coming from an end-user input, then I would not object to > >> the change, though. > > > > I added this in response to Duy's comment on v1 [1]. > > > > [1] http://article.gmane.org/gmane.comp.version-control.git/234548 > > Looks like I add more noise to this thread than anything valuable. > Yes, once argv goes through parse_pathspec it's normalized so we do > not need to strip consecutive slashes any more. I'm not entirely sure > if it also converts Windows '\\' to '/'.. If it goes through normalize_path_copy_len then it does. Junio, can you please drop the first two patches in this series? I can resend the final two unmodified if necessary, but I suspect it's easier for you to just drop the commits. > > Looking more closely, this does come from user input (via the argv > > passed into parse_pathspec) but does (some of the time) go through > > prefix_path_gently which calls normalize_path_copy_len. > > > > It's not immediately clear to me when prefix_pathspec goes through this > > particular code path, but I think we may be able to drop this (and the > > previous patch) without affecting the user.