git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Thomas Rast <trast@student.ethz.ch>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Junio C Hamano <gitster@pobox.com>, <git@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH] common_prefix: be more careful about pathspec bounds
Date: Tue, 15 Jun 2010 10:16:16 +0200	[thread overview]
Message-ID: <201006151016.16492.trast@student.ethz.ch> (raw)
In-Reply-To: <825550ec93610c2d3c7dae7550729d96fc6cebbc.1276194169.git.trast@student.ethz.ch>

Thomas Rast wrote:
> Normally this won't be a problem, which is probably why nobody has
> noticed that this was broken since 2006.

Actually I just noticed it's not from back in 2006 when Linus wrote
this code, but from the following:

commit c7f34c180b7117cf60ad12a8b180eed33716e390
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date:   Mon Apr 23 10:21:25 2007 +0200

    dir.c(common_prefix): Fix two bugs
    
    The function common_prefix() is used to find the common subdirectory of
    a couple of pathnames. When checking if the next pathname matches up with
    the prefix, it incorrectly checked the whole path, not just the prefix
    (including the slash). Thus, the expensive part of the loop was executed
    always.
    
    The other bug is more serious: if the first and the last pathname in the
    list have a longer common prefix than the common prefix for _all_ pathnames
    in the list, the longer one would be chosen. This bug was probably hidden
    by the fact that bash's wildcard expansion sorts the results, and the code
    just so happens to work with sorted input.
[...]
--- a/dir.c
+++ b/dir.c
@@ -24,8 +24,9 @@ int common_prefix(const char **pathspec)
        prefix = slash - path + 1;
        while ((next = *++pathspec) != NULL) {
                int len = strlen(next);
-               if (len >= prefix && !memcmp(path, next, len))
+               if (len >= prefix && !memcmp(path, next, prefix))
                        continue;
+               len = prefix - 1;
                for (;;) {
                        if (!len)
                                return 0;

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

  reply	other threads:[~2010-06-15  8:17 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-06-10 18:24 [PATCH] common_prefix: be more careful about pathspec bounds Thomas Rast
2010-06-15  8:16 ` Thomas Rast [this message]
2010-06-15  9:05   ` Johannes Schindelin
2010-06-15 15:05 ` Junio C Hamano
2010-06-15 16:06   ` Junio C Hamano
2010-06-15 18:04     ` Thomas Rast
2010-06-15 22:12       ` Junio C Hamano
2010-06-15 23:02         ` [PATCH] common_prefix: simplify and fix scanning for prefixes Thomas Rast

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201006151016.16492.trast@student.ethz.ch \
    --to=trast@student.ethz.ch \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).