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: Thu, 12 Sep 2013 21:21:28 +0100 Message-ID: <20130912202128.GB2582@serenity.lan> References: <7c478c19da6ee3322ca87e77a90358a30178c286.1379013786.git.john@keeping.me.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: git@vger.kernel.org, Duy Nguyen , Jens Lehmann , Johannes Sixt To: Junio C Hamano X-From: git-owner@vger.kernel.org Thu Sep 12 22:21:44 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 1VKDOZ-00037v-Pp for gcvg-git-2@plane.gmane.org; Thu, 12 Sep 2013 22:21:44 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756831Ab3ILUVj (ORCPT ); Thu, 12 Sep 2013 16:21:39 -0400 Received: from coyote.aluminati.org ([72.9.247.114]:59803 "EHLO coyote.aluminati.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754247Ab3ILUVj (ORCPT ); Thu, 12 Sep 2013 16:21:39 -0400 Received: from localhost (localhost [127.0.0.1]) by coyote.aluminati.org (Postfix) with ESMTP id 6756419800B; Thu, 12 Sep 2013 21:21:38 +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 y466co9-ZruT; Thu, 12 Sep 2013 21:21:38 +0100 (BST) Received: from serenity.lan (banza.aluminati.org [10.0.7.182]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by coyote.aluminati.org (Postfix) with ESMTPSA id 97F90198007; Thu, 12 Sep 2013 21:21:30 +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 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 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. > > Signed-off-by: John Keeping > > --- > > pathspec.c | 27 +++++++++++++++++---------- > > 1 file changed, 17 insertions(+), 10 deletions(-) > > > > diff --git a/pathspec.c b/pathspec.c > > index 7c6963b..11b031a 100644 > > --- a/pathspec.c > > +++ b/pathspec.c > > @@ -251,12 +251,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item, > > item->len = strlen(item->match); > > item->prefix = prefixlen; > > > > - if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) && > > - (item->len >= 1 && is_dir_sep(item->match[item->len - 1])) && > > - (i = cache_name_pos(item->match, item->len - 1)) >= 0 && > > - S_ISGITLINK(active_cache[i]->ce_mode)) { > > - item->len--; > > - match[item->len] = '\0'; > > + if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) { > > + size_t pathlen = item->len; > > + while (pathlen > 0 && is_dir_sep(item->match[pathlen - 1])) > > + pathlen--; > > + > > + if ((i = cache_name_pos(item->match, pathlen)) >= 0 && > > + S_ISGITLINK(active_cache[i]->ce_mode)) { > > + item->len = pathlen; > > + match[item->len] = '\0'; > > + } > > } > > > > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE) > > @@ -271,11 +275,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item, > > !is_dir_sep(match[ce_len]) || > > memcmp(ce->name, match, ce_len)) > > continue; > > - if (item->len == ce_len + 1) { > > - /* strip trailing slash */ > > + > > + while (item->len > 0 && is_dir_sep(match[item->len - 1])) > > item->len--; > > - match[item->len] = '\0'; > > - } else > > + > > + /* strip trailing slash */ > > + match[item->len] = '\0'; > > + > > + if (item->len != ce_len) > > die (_("Pathspec '%s' is in submodule '%.*s'"), > > elt, ce_len, ce->name); > > }