Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules
- From
John Keeping <john@keeping.me.uk>
- Date
- Sep 12, 2013, 20:21 UTC
- Message-ID
- <20130912202128.GB2582@serenity.lan>
- In-Reply-To
- <xmqqzjrhom85.fsf@gitster.dls.corp.google.com>
On Thu, Sep 12, 2013 at 12:48:10PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> John Keeping <john@keeping.me.uk> 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.
Show 51 quoted lines
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> > 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);
> > }