From: Shawn Bohrer <shawn.bohrer@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
git@vger.kernel.org, j.sixt@viscovery.net
Subject: Re: [PATCH] Fix off by one error in prep_exclude.
Date: Sun, 27 Jan 2008 18:34:04 -0600 [thread overview]
Message-ID: <20080128003404.GA18276@lintop> (raw)
In-Reply-To: <7v3asiyk2i.fsf@gitster.siamese.dyndns.org>
On Sun, Jan 27, 2008 at 02:34:13PM -0800, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> but doesn't address the fact that we probably should remove files that
> >> aren't a part of the repository at in the first place.
> >
> > I am sorry, but I cannot begin to see what this commit tries to
> > accomplish. Yes, sure, there is an off-by-one error, and your commit
> > message says how that was fixed. But I miss a description what usage it
> > would affect, i.e. when this bug triggers.
> >
> > I imagine that you would be as lost as me, reading that commit message 6
> > months from now, trying to understand why that change was made.
>
> Likewise. The message has somewhat to be desired...
Agreed I'll resend with a improved message.
> In "struct exclude_stack", prep_exclude() and excluded(), the
> convention for a path is to express the length of directory part
> including the trailing slash (e.g. "foo" and "bar/baz" will get
> baselen=0 and baselen=4 respectively).
>
> The variable current and parameter baselen follow that
> convention in the codepath the patch touches.
>
> else {
> cp = strchr(base + current + 1, '/');
> if (!cp)
> die("oops in prep_exclude");
> cp++;
> }
> stk->prev = dir->exclude_stack;
> stk->baselen = cp - base;
>
> is about coming up with the next value for current (which is
> taken from stk->baselen) to dig one more level.
>
> If base="foo/a/boo" and current=4 (i.e. we are looking at
> "foo/"), at the point, scanning from (base+current) as Shawn
> Bohrer's patch suggests means the scan begins at "a/boo" to find
> the next slash. The existing code skips one letter ('a') and
> starts scanning from "/boo".
>
> The only case this microoptimization makes difference is when an
> input is malformed and has double-slash (i.e. path component
> whose length is zero), like "foo//boo".
Good catch, I didn't think of this case but this indeed will cause
the same issue.
> Perhaps the "oops part of the issue Johannes found" had a caller
> that feeds such an incorrect input?
Nope the problem Johannes Sixt was having was that he mistakenly ran
git clean -n /*foo
Now that isn't what he meant to do, but I figured it might be possible
that someone has their whole filesystem in a git repository, or maybe
is using some sort of chroot on their repository. Your malformed
paths guess is probably much more likely to occur.
--
Shawn
next prev parent reply other threads:[~2008-01-28 0:36 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-01-23 15:14 git-clean buglet Johannes Sixt
2008-01-23 15:24 ` Johannes Sixt
2008-01-23 15:29 ` Johannes Schindelin
2008-01-23 15:40 ` Johannes Sixt
2008-01-27 19:55 ` [PATCH] Fix off by one error in prep_exclude Shawn Bohrer
2008-01-27 20:44 ` Johannes Schindelin
2008-01-27 21:15 ` Shawn Bohrer
2008-01-27 22:34 ` Junio C Hamano
2008-01-28 0:34 ` Shawn Bohrer [this message]
2008-01-28 0:37 ` Shawn Bohrer
2008-01-28 11:59 ` Johannes Schindelin
2008-01-28 12:04 ` Junio C Hamano
2008-01-28 2:52 ` Junio C Hamano
2008-01-28 7:12 ` Johannes Sixt
2008-01-28 8:46 ` Junio C Hamano
2008-01-28 9:05 ` Johannes Sixt
2008-01-28 9:22 ` Junio C Hamano
2008-01-28 12:33 ` [RFH/PATCH] prefix_path(): disallow absolute paths Johannes Schindelin
2008-01-28 15:05 ` [PATCH] " Johannes Schindelin
2008-01-29 1:23 ` [RFH/PATCH] " Junio C Hamano
2008-01-29 2:03 ` Junio C Hamano
2008-01-29 2:03 ` Junio C Hamano
2008-01-29 7:02 ` Junio C Hamano
2008-01-29 8:29 ` [PATCH] setup: sanitize absolute and funny paths in get_pathspec() Junio C Hamano
2008-02-01 4:07 ` [PATCH] Make blame accept absolute paths Robin Rosenberg
2008-02-01 4:34 ` [PATCH] More test cases for sanitized path names Robin Rosenberg
2008-02-01 7:17 ` Junio C Hamano
2008-02-01 9:10 ` Robin Rosenberg
2008-02-01 10:22 ` Junio C Hamano
2008-02-01 10:51 ` Junio C Hamano
2008-02-01 11:10 ` Junio C Hamano
2008-02-01 14:17 ` Robin Rosenberg
2008-02-01 17:45 ` Junio C Hamano
2008-02-01 9:16 ` Karl Hasselström
2008-02-01 9:50 ` [PATCH for post 1.5.4] Sane use of test_expect_failure Junio C Hamano
2008-02-02 10:06 ` [PATCH] " Junio C Hamano
2008-03-07 8:23 ` [PATCH] More test cases for sanitized path names Junio C Hamano
2008-03-07 15:24 ` Robin Rosenberg
2008-01-29 2:37 ` [RFH/PATCH] prefix_path(): disallow absolute paths Johannes Schindelin
2008-01-29 2:45 ` Junio C Hamano
2008-01-29 2:59 ` Johannes Schindelin
2008-01-29 7:20 ` Johannes Sixt
2008-01-29 7:28 ` Junio C Hamano
2008-01-29 7:43 ` Johannes Sixt
2008-01-29 8:31 ` Junio C Hamano
2008-01-29 21:53 ` しらいしななこ
2008-01-30 0:43 ` Junio C Hamano
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=20080128003404.GA18276@lintop \
--to=shawn.bohrer@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j.sixt@viscovery.net \
/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).