Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules
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 '/'.. > 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. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules
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.
Thanks.
> 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);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules
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);
> > }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
