* git branch -D doesn't work with deleted worktree
From: Roland Illig @ 2017-01-05 10:06 UTC (permalink / raw)
To: git@vger.kernel.org
Git 2.11.0 gives a wrong error message after the following commands:
$ git init
$ echo hello >file
$ git add file
$ git commit -m "message"
$ git worktree add ../worktree
$ rm -rf ../worktree
$ git br -D worktree
error: Cannot delete branch 'worktree' checked out at '../worktree'
Since ../worktree has been deleted, there cannot be anything checked out at that location.
In my opinion, deleting the branch should just work. Especially since I used the -D option and the "git worktree" documentation says "When you are done with a linked working tree you can simply delete it."
Regards,
Roland
^ permalink raw reply
* Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`
From: Lars Schneider @ 2017-01-05 10:05 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, git, 마누엘,
Junio C Hamano
In-Reply-To: <20170104080852.bmlmtzxhjx4qt74f@sigill.intra.peff.net>
> On 04 Jan 2017, at 09:08, Jeff King <peff@peff.net> wrote:
>
> On Mon, Jan 02, 2017 at 05:03:57PM +0100, Johannes Schindelin wrote:
>
>> From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de>
>>
>> The `user-manual.txt` is designed as a `book` but the `Makefile` wants
>> to build it as an `article`. This seems to be a problem when building
>> the documentation with `asciidoctor`. Furthermore the parts *Git
>> Glossary* and *Appendix B* had no subsections which is not allowed when
>> building with `asciidoctor`. So lets add a *dummy* section.
>
> The git-scm.com site uses asciidoctor, too, and I think I have seen some
> oddness with the rendering though. So in general I am in favor of making
> things work under both asciidoc and asciidoctor.
I am not familiar with both tools but it sounds to me as if "asciidoctor"
is kind of the "lowest common denominator". Is this true? If yes, would it
make sense to switch TravisCI [1] to asciidocter if this change gets merged?
- Lars
[1] https://github.com/git/git/blob/master/.travis.yml#L48
^ permalink raw reply
* Re: RFC: --force-with-lease default behaviour
From: G. Sylvie Davies @ 2017-01-05 6:52 UTC (permalink / raw)
To: git
In-Reply-To: <CAAj3zPz-jMVoxNTRZ0iR1ZPTFh873gEo33QjynBE1vaHsMmg3A@mail.gmail.com>
On Wed, Jan 4, 2017 at 6:34 PM, G. Sylvie Davies <sylvie@bit-booster.com> wrote:
> Right now the default variant does this:
>
>> --force-with-lease alone, without specifying the details, will protect all remote refs that are going to be updated by requiring their current value to be the same as the remote-tracking branch we have for them.
>
> The problem is people sometimes run "git fetch". And so "git push
> --force-with-lease" is going to do the push even if the local version
> is stale.
>
> Instead I think the default behavior should require that the remote
> ref's current value be equal to the merge-base of the local-branch and
> remote-tracking-branch.
>
> Here's an example (password is "test" for the push):
>
> git clone http://test@vm.bit-booster.com/bitbucket/scm/bb/a.git
> cd a
> git checkout bugfix/TKT-123
> git reset --hard HEAD~1 (to simulate situation where local is stale,
> but remote is up to date)
>
> At this point "git push --force-with-lease" is going to work. But I
> think it shouldn't. (Note: I use push.default = simple).
>
> Here's how I think it should work:
>
> git push --force-with-lease=bugfix/TKT-123:$(git merge-base HEAD
> origin/bugfix/TKT-123)
> To http://vm.bit-booster.com/bitbucket/scm/bb/a.git
> ! [rejected] bugfix/TKT-123 -> bugfix/TKT-123 (stale info)
>
>
> For now I'm happy with this alias:
>
> git config --global alias.please '!sh -c "git push
> --force-with-lease=$(git rev-parse --abbrev-ref HEAD):$(git merge-base
> HEAD @{u})"'
>
Nevermind! I realize this essentially removes the "--force" and
turns it into the original non-forced "fast-forwardable" only style
push. [BLUSH!]
I wonder if there's anything one could do to help those who type "git
fetch" and still want to enjoy "--force-with-lease"...
> But I'd like to put together a patch if people are interested in a
> tweak like this to the --force-with-lease default behaviour. I
> haven't written much C in my life, but thought this might make a good
> force-myself-to-learn-C exercise.
>
>
> - Sylvie Davies
>
> ps. I never thought about the fetch problem with --force-with-lease
> until reading https://developer.atlassian.com/blog/2015/04/force-with-lease/
> and https://buddyreno.me/git-please-a182f28efeb5#.s291gh5jn , so
> thanks to them!
^ permalink raw reply
* RFC: --force-with-lease default behaviour
From: G. Sylvie Davies @ 2017-01-05 2:34 UTC (permalink / raw)
To: git
Right now the default variant does this:
> --force-with-lease alone, without specifying the details, will protect all remote refs that are going to be updated by requiring their current value to be the same as the remote-tracking branch we have for them.
The problem is people sometimes run "git fetch". And so "git push
--force-with-lease" is going to do the push even if the local version
is stale.
Instead I think the default behavior should require that the remote
ref's current value be equal to the merge-base of the local-branch and
remote-tracking-branch.
Here's an example (password is "test" for the push):
git clone http://test@vm.bit-booster.com/bitbucket/scm/bb/a.git
cd a
git checkout bugfix/TKT-123
git reset --hard HEAD~1 (to simulate situation where local is stale,
but remote is up to date)
At this point "git push --force-with-lease" is going to work. But I
think it shouldn't. (Note: I use push.default = simple).
Here's how I think it should work:
git push --force-with-lease=bugfix/TKT-123:$(git merge-base HEAD
origin/bugfix/TKT-123)
To http://vm.bit-booster.com/bitbucket/scm/bb/a.git
! [rejected] bugfix/TKT-123 -> bugfix/TKT-123 (stale info)
For now I'm happy with this alias:
git config --global alias.please '!sh -c "git push
--force-with-lease=$(git rev-parse --abbrev-ref HEAD):$(git merge-base
HEAD @{u})"'
But I'd like to put together a patch if people are interested in a
tweak like this to the --force-with-lease default behaviour. I
haven't written much C in my life, but thought this might make a good
force-myself-to-learn-C exercise.
- Sylvie Davies
ps. I never thought about the fetch problem with --force-with-lease
until reading https://developer.atlassian.com/blog/2015/04/force-with-lease/
and https://buddyreno.me/git-please-a182f28efeb5#.s291gh5jn , so
thanks to them!
^ permalink raw reply
* Re: [PATCHv5] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-04 23:28 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <20170104231605.GB68241@google.com>
> I haven't taken a through look at this patch but I think you may want to
> base it off of 'origin/bw/pathspec-cleanup' series as the changes made in this
> patch now conflict with that series.
eh right, I forgot to mention this in the notes, it requires
sb/submodule-embed-gitdir as well, so I'll have to figure that out.
>
> Also I still don't really think this solves the problem of telling the
> user what is wrong, which is that the submodule's gitdir is gone.
>
The "git dir gone" is not a big deal IMHO as a deinitialized submodule
is perfectly fine (e.g. not initialized). The errors as I tested in Gerrit,
a superproject that contains submodules in plugins/* :
: gerrit/plugins/cookbook-plugin$ git add .
fatal: Pathspec '.' is in submodule 'plugins/cookbook-plugin'
: gerrit/plugins/cookbook-plugin$ cd ..
: gerrit/plugins$ git add cookbook-plugin/a
fatal: Pathspec 'cookbook-plugin/a' is in submodule
'plugins/cookbook-plugin'
: gerrit/plugins$ git add cookbook-plugin/.
: gerrit/plugins$ git add cookbook-plugin/./.
: gerrit/plugins$
I think that is perfect behavior for now, as it reliably detects
(a) the submodule being there and (b) if you are in there, no
matter if there is a .git dir or not.
The same error coming up if the submodule is initialized and valid, e.g.
: gerrit/plugins$ git submodule update --init cookbook-plugin
: gerrit/plugins$ git add cookbook-plugin/a/.
fatal: Pathspec 'cookbook-plugin/a/.' is in submodule
'plugins/cookbook-plugin'
So I think this is pretty much exactly what we want for now:
* if PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set
we keep the behavior as is and do the expensive thing
* if the caller wants to use path inside of a submodule no matter
the git dir of the submodule, then set the CHEAP flag instead
* in case of the assert (that I originally wanted to fix), we fall back to the
EXPENSIVE thing reporting the error message that we already reported
in such cases.
TL;DR: I was rather asking about the code being a viable;
by now I am convinced this is the correct behavior. ;)
^ permalink raw reply
* Re: [PATCHv5] pathspec: give better message for submodule related pathspec error
From: Brandon Williams @ 2017-01-04 23:16 UTC (permalink / raw)
To: Stefan Beller; +Cc: peff, git, gitster
In-Reply-To: <20170104231027.7065-1-sbeller@google.com>
On 01/04, Stefan Beller wrote:
> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1]. The usual response from the mailing
> list is link to old discussions[2], and acknowledging the problem
> stating it is known.
>
> This patch accomplishes two things:
>
> 1. Switch assert() to die("BUG") to give a more readable message.
>
> 2. Take one of the cases where we hit a BUG and turn it into a normal
> "there was something wrong with the input" message.
>
> This assertion triggered for cases where there wasn't a programming
> bug, but just bogus input. In particular, if the user asks for a
> pathspec that is inside a submodule, we shouldn't assert() or
> die("BUG"); we should tell the user their request is bogus.
>
> The only reason we did not check for it, is the expensive nature
> of such a check, so callers avoid setting the flag
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due
> to bogus input, the expense of cpu cycles spent outweighs the user
> wondering what went wrong, so run that check unconditionally before
> dying with a more generic error message.
>
> In case we found out that the path points inside a submodule, but the
> caller did not ask for PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, we
> should not silently fix the pathspec to just point at the submodule,
> as that would confuse callers.
>
> To make this happen, specifically the second part, move the check for
> being inside a submodule into a function and call it either when
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set or when we are in the
> buggy case to give a better error message.
>
> Note: There is this one special case ("git -C submodule add .") in which
> we call check_inside_submodule_expensive two times, once for checking
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and once in the code path
> handling the buggy user input. For this to work correctly we need to adapt
> the conditions in the check for being inside the submodule to account for
> the prior run to have taken effect.
>
> [1] https://www.google.com/search?q=item-%3Enowildcard_len
> [2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
> https://www.spinics.net/lists/git/msg249473.html
>
> Helped-by: Jeff King <peff@peff.net>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> This is just rerolling the second patch of that "make the assert go away",
> asking for opinions again.
>
> I agree with Brandon that pathspec code is not the ideal place to
> check for issues with submodules. However we should give the best error
> message possible for the user, so running this diagnosis is fine by me.
>
> Thanks,
> Stefan
>
> pathspec.c | 63 +++++++++++++++++++++++++++-------------
> t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++
> 2 files changed, 76 insertions(+), 20 deletions(-)
> create mode 100755 t/t6134-pathspec-in-submodule.sh
>
> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..41e0dac1df 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -88,6 +88,34 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
> strbuf_addf(sb, ",prefix:%d)", prefixlen);
> }
>
> +static void check_inside_submodule_expensive(struct pathspec_item *item,
> + char *match,
> + const char *elt, int die_inside)
> +{
> + int i;
> + for (i = 0; i < active_nr; i++) {
> + struct cache_entry *ce = active_cache[i];
> + int ce_len = ce_namelen(ce);
> +
> + if (!S_ISGITLINK(ce->ce_mode))
> + continue;
> +
> + if (item->len < ce_len ||
> + !(match[ce_len] == '/' || match[ce_len] == '\0') ||
> + memcmp(ce->name, match, ce_len))
> + continue;
> +
> + if (item->len != ce_len + 1 || die_inside)
> + die (_("Pathspec '%s' is in submodule '%.*s'"),
> + elt, ce_len, ce->name);
> +
> + /* strip trailing slash */
> + item->len--;
> + match[item->len] = '\0';
> + break;
> + }
> +}
> +
> /*
> * Take an element of a pathspec and check for magic signatures.
> * Append the result to the prefix. Return the magic bitmap.
> @@ -273,24 +301,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> }
>
> if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> - for (i = 0; i < active_nr; i++) {
> - struct cache_entry *ce = active_cache[i];
> - int ce_len = ce_namelen(ce);
> -
> - if (!S_ISGITLINK(ce->ce_mode))
> - continue;
> -
> - if (item->len <= ce_len || match[ce_len] != '/' ||
> - memcmp(ce->name, match, ce_len))
> - continue;
> - if (item->len == ce_len + 1) {
> - /* strip trailing slash */
> - item->len--;
> - match[item->len] = '\0';
> - } else
> - die (_("Pathspec '%s' is in submodule '%.*s'"),
> - elt, ce_len, ce->name);
> - }
> + check_inside_submodule_expensive(item, match, elt, 0);
>
> if (magic & PATHSPEC_LITERAL)
> item->nowildcard_len = item->len;
> @@ -313,8 +324,20 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> }
>
> /* sanity checks, pathspec matchers assume these are sane */
> - assert(item->nowildcard_len <= item->len &&
> - item->prefix <= item->len);
> + if (item->nowildcard_len > item->len ||
> + item->prefix > item->len) {
> + /*
> + * We know something is fishy and we're going to die
> + * anyway, so we don't care about following operation
> + * to be expensive, despite the caller not asking for
> + * an expensive submodule check. The potential expensive
> + * operation here reduces the users head scratching
> + * greatly, though.
> + */
> + check_inside_submodule_expensive(item, match, elt, 1);
> + die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
> + }
> +
> return magic;
> }
>
> diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
> new file mode 100755
> index 0000000000..2900d8d06e
> --- /dev/null
> +++ b/t/t6134-pathspec-in-submodule.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +
> +test_description='test case exclude pathspec'
> +
> +TEST_CREATE_SUBMODULE=yes
> +. ./test-lib.sh
> +
> +test_expect_success 'setup a submodule' '
> + git submodule add ./pretzel.bare sub &&
> + git commit -a -m "add submodule" &&
> + git submodule deinit --all
> +'
> +
> +cat <<EOF >expect
> +fatal: Pathspec 'sub/a' is in submodule 'sub'
> +EOF
> +
> +test_expect_success 'error message for path inside submodule' '
> + echo a >sub/a &&
> + test_must_fail git add sub/a 2>actual &&
> + test_cmp expect actual
> +'
> +
> +cat <<EOF >expect
> +fatal: Pathspec '.' is in submodule 'sub'
> +EOF
> +
> +test_expect_success 'error message for path inside submodule from within submodule' '
> + test_must_fail git -C sub add . 2>actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
I haven't taken a through look at this patch but I think you may want to
base it off of 'origin/bw/pathspec-cleanup' series as the changes made in this
patch now conflict with that series.
Also I still don't really think this solves the problem of telling the
user what is wrong, which is that the submodule's gitdir is gone.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-04 23:10 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams
In-Reply-To: <20170104075506.sa5oa5bheykswkwn@sigill.intra.peff.net>
On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@peff.net> wrote:
> As this last bit is quoted from me, I won't deny that it's brilliant as
> usual.
obviously. :)
>
> But as this commit message needs to stand on its own, rather than as part of a
> larger discussion thread, it might be worth expanding "one of the cases"
> here. And talking about what's happening to the other cases.
>
> Like:
>
> This assertion triggered for cases where there wasn't a programming
> bug, but just bogus input. In particular, if the user asks for a
> pathspec that is inside a submodule, we shouldn't assert() or
> die("BUG"); we should tell the user their request is bogus.
alt. cont'd:
We already would do that if PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE
is set, but we had to ask for this examination via a flag, because
it is expensive. At this point in code we know there is bogus input,
so all we would do is error out. For that case we can assume that the cost
of the expensive search is negligible compared to the users head scratching
that follows.
(This will appear in the patch I am about to send out)
^ permalink raw reply
* [PATCHv5] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-04 23:10 UTC (permalink / raw)
To: peff; +Cc: git, bmwill, gitster, Stefan Beller
In-Reply-To: <20170104205346.GE69227@google.com>
Every once in a while someone complains to the mailing list to have
run into this weird assertion[1]. The usual response from the mailing
list is link to old discussions[2], and acknowledging the problem
stating it is known.
This patch accomplishes two things:
1. Switch assert() to die("BUG") to give a more readable message.
2. Take one of the cases where we hit a BUG and turn it into a normal
"there was something wrong with the input" message.
This assertion triggered for cases where there wasn't a programming
bug, but just bogus input. In particular, if the user asks for a
pathspec that is inside a submodule, we shouldn't assert() or
die("BUG"); we should tell the user their request is bogus.
The only reason we did not check for it, is the expensive nature
of such a check, so callers avoid setting the flag
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due
to bogus input, the expense of cpu cycles spent outweighs the user
wondering what went wrong, so run that check unconditionally before
dying with a more generic error message.
In case we found out that the path points inside a submodule, but the
caller did not ask for PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, we
should not silently fix the pathspec to just point at the submodule,
as that would confuse callers.
To make this happen, specifically the second part, move the check for
being inside a submodule into a function and call it either when
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set or when we are in the
buggy case to give a better error message.
Note: There is this one special case ("git -C submodule add .") in which
we call check_inside_submodule_expensive two times, once for checking
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and once in the code path
handling the buggy user input. For this to work correctly we need to adapt
the conditions in the check for being inside the submodule to account for
the prior run to have taken effect.
[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
https://www.spinics.net/lists/git/msg249473.html
Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This is just rerolling the second patch of that "make the assert go away",
asking for opinions again.
I agree with Brandon that pathspec code is not the ideal place to
check for issues with submodules. However we should give the best error
message possible for the user, so running this diagnosis is fine by me.
Thanks,
Stefan
pathspec.c | 63 +++++++++++++++++++++++++++-------------
t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++
2 files changed, 76 insertions(+), 20 deletions(-)
create mode 100755 t/t6134-pathspec-in-submodule.sh
diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..41e0dac1df 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -88,6 +88,34 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
strbuf_addf(sb, ",prefix:%d)", prefixlen);
}
+static void check_inside_submodule_expensive(struct pathspec_item *item,
+ char *match,
+ const char *elt, int die_inside)
+{
+ int i;
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ int ce_len = ce_namelen(ce);
+
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
+
+ if (item->len < ce_len ||
+ !(match[ce_len] == '/' || match[ce_len] == '\0') ||
+ memcmp(ce->name, match, ce_len))
+ continue;
+
+ if (item->len != ce_len + 1 || die_inside)
+ die (_("Pathspec '%s' is in submodule '%.*s'"),
+ elt, ce_len, ce->name);
+
+ /* strip trailing slash */
+ item->len--;
+ match[item->len] = '\0';
+ break;
+ }
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -273,24 +301,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
- for (i = 0; i < active_nr; i++) {
- struct cache_entry *ce = active_cache[i];
- int ce_len = ce_namelen(ce);
-
- if (!S_ISGITLINK(ce->ce_mode))
- continue;
-
- if (item->len <= ce_len || match[ce_len] != '/' ||
- memcmp(ce->name, match, ce_len))
- continue;
- if (item->len == ce_len + 1) {
- /* strip trailing slash */
- item->len--;
- match[item->len] = '\0';
- } else
- die (_("Pathspec '%s' is in submodule '%.*s'"),
- elt, ce_len, ce->name);
- }
+ check_inside_submodule_expensive(item, match, elt, 0);
if (magic & PATHSPEC_LITERAL)
item->nowildcard_len = item->len;
@@ -313,8 +324,20 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
/* sanity checks, pathspec matchers assume these are sane */
- assert(item->nowildcard_len <= item->len &&
- item->prefix <= item->len);
+ if (item->nowildcard_len > item->len ||
+ item->prefix > item->len) {
+ /*
+ * We know something is fishy and we're going to die
+ * anyway, so we don't care about following operation
+ * to be expensive, despite the caller not asking for
+ * an expensive submodule check. The potential expensive
+ * operation here reduces the users head scratching
+ * greatly, though.
+ */
+ check_inside_submodule_expensive(item, match, elt, 1);
+ die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+ }
+
return magic;
}
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 0000000000..2900d8d06e
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+TEST_CREATE_SUBMODULE=yes
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+ git submodule add ./pretzel.bare sub &&
+ git commit -a -m "add submodule" &&
+ git submodule deinit --all
+'
+
+cat <<EOF >expect
+fatal: Pathspec 'sub/a' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule' '
+ echo a >sub/a &&
+ test_must_fail git add sub/a 2>actual &&
+ test_cmp expect actual
+'
+
+cat <<EOF >expect
+fatal: Pathspec '.' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule from within submodule' '
+ test_must_fail git -C sub add . 2>actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.11.0.rc2.32.gdde9519.dirty
^ permalink raw reply related
* Re: [PATCH v4 0/5] road to reentrant real_path
From: Brandon Williams @ 2017-01-04 21:55 UTC (permalink / raw)
To: Jacob Keller
Cc: Stefan Beller, Jeff King, Torsten Bögershausen,
git@vger.kernel.org, Junio C Hamano, Ramsay Jones, Johannes Sixt,
Duy Nguyen, Lars Schneider
In-Reply-To: <CA+P7+xp+j1ajPLjE-RukSmp33_bRkD7J65X-++frkYd9LWLSkQ@mail.gmail.com>
On 01/04, Jacob Keller wrote:
> On Wed, Jan 4, 2017 at 10:22 AM, Stefan Beller <sbeller@google.com> wrote:
> > On Wed, Jan 4, 2017 at 10:13 AM, Brandon Williams <bmwill@google.com> wrote:
> >> On 01/04, Jeff King wrote:
> >>> On Wed, Jan 04, 2017 at 07:56:02AM +0100, Torsten Bögershausen wrote:
> >>>
> >>> > On 04.01.17 01:48, Jeff King wrote:
> >>> > > On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
> >>> > >
> >>> > >> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
> >>> > >> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
> >>> > >
> >>> > > Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
> >>> > > what all other similar functions will be using.
> >>> > >
> >>> > > The only problem was that we were redefining the macro. So maybe:
> >>> > >
> >>> > > #ifndef MAXSYMLINKS
> >>> > > #define MAXSYMLINKS 5
> >>> > > #endif
> >>> > >
> >>> > > would be a good solution?
> >>> > Why 5 ? (looking at the 20..30 below)
> >>> > And why 5 on one system and e.g. on my Mac OS
> >>> > #define MAXSYMLINKS 32
> >>>
> >>> I mentioned "5" because that is the current value of MAXDEPTH. I do
> >>> think it would be reasonable to bump it to something higher.
> >>>
> >>> > Would the same value value for all Git installations on all platforms make sense?
> >>> > #define GITMAXSYMLINKS 20
> >>>
> >>> I think it's probably more important to match the rest of the OS, so
> >>> that open("foo") and realpath("foo") behave similarly on the same
> >>> system. Though I think even that isn't always possible, as the limit is
> >>> dynamic on some systems.
> >>>
> >>> I think the idea of the 20-30 range is that it's small enough to catch
> >>> an infinite loop quickly, and large enough that nobody will ever hit it
> >>> in practice. :)
> >>
> >> I agree that we should have similar guarantees as the OS provides,
> >> especially if the OS already has MAXSYMLINKS defined. What then, should
> >> the fall back value be if the OS doesn't have this defined? 5 like we
> >> have done historically, or something around the 20-30 range as Torsten
> >> suggests?
> >
> > As a fallback I'd rather go for a larger number than too small.
> > The reason for the existence is just to break an infinite loop
> > (and report an error? which the current code doesn't quite do,
> > but your series actually does).
> >
> > If the number is too large, then it takes a bit longer to generate the error
> > message, but the error path is no big deal w.r.t. performance, so it's fine
> > for it taking a bit longer.
> >
> > If the number is too low, then we may hinder people from getting actual
> > work done, (i.e. they have to figure out what the problem is and recompile
> > git), so I'd think a larger number is not harmful. So 32?
> >
>
> I think I agree as well.
>
> Thanks,
> Jake
>
> >>
> >> --
> >> Brandon Williams
That's two people for 32. I'll use that as the fallback and resend the
series.
--
Brandon Williams
^ permalink raw reply
* [PATCH v5 1/5] real_path: resolve symlinks by hand
From: Brandon Williams @ 2017-01-04 22:01 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds, larsxschneider
In-Reply-To: <20170104220124.145808-1-bmwill@google.com>
The current implementation of real_path uses chdir() in order to resolve
symlinks. Unfortunately this isn't thread-safe as chdir() affects a
process as a whole and not just an individual thread. Instead perform
the symlink resolution by hand so that the calls to chdir() can be
removed, making real_path one step closer to being reentrant.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
abspath.c | 194 ++++++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 133 insertions(+), 61 deletions(-)
diff --git a/abspath.c b/abspath.c
index 2825de859..629201e48 100644
--- a/abspath.c
+++ b/abspath.c
@@ -11,8 +11,47 @@ int is_directory(const char *path)
return (!stat(path, &st) && S_ISDIR(st.st_mode));
}
+/* removes the last path component from 'path' except if 'path' is root */
+static void strip_last_component(struct strbuf *path)
+{
+ size_t offset = offset_1st_component(path->buf);
+ size_t len = path->len;
+
+ /* Find start of the last component */
+ while (offset < len && !is_dir_sep(path->buf[len - 1]))
+ len--;
+ /* Skip sequences of multiple path-separators */
+ while (offset < len && is_dir_sep(path->buf[len - 1]))
+ len--;
+
+ strbuf_setlen(path, len);
+}
+
+/* get (and remove) the next component in 'remaining' and place it in 'next' */
+static void get_next_component(struct strbuf *next, struct strbuf *remaining)
+{
+ char *start = NULL;
+ char *end = NULL;
+
+ strbuf_reset(next);
+
+ /* look for the next component */
+ /* Skip sequences of multiple path-separators */
+ for (start = remaining->buf; is_dir_sep(*start); start++)
+ ; /* nothing */
+ /* Find end of the path component */
+ for (end = start; *end && !is_dir_sep(*end); end++)
+ ; /* nothing */
+
+ strbuf_add(next, start, end - start);
+ /* remove the component from 'remaining' */
+ strbuf_remove(remaining, 0, end - remaining->buf);
+}
+
/* We allow "recursive" symbolic links. Only within reason, though. */
-#define MAXDEPTH 5
+#ifndef MAXSYMLINKS
+#define MAXSYMLINKS 32
+#endif
/*
* Return the real path (i.e., absolute path, with symlinks resolved
@@ -21,7 +60,6 @@ int is_directory(const char *path)
* absolute_path().) The return value is a pointer to a static
* buffer.
*
- * The input and all intermediate paths must be shorter than MAX_PATH.
* The directory part of path (i.e., everything up to the last
* dir_sep) must denote a valid, existing directory, but the last
* component need not exist. If die_on_error is set, then die with an
@@ -33,22 +71,16 @@ int is_directory(const char *path)
*/
static const char *real_path_internal(const char *path, int die_on_error)
{
- static struct strbuf sb = STRBUF_INIT;
+ static struct strbuf resolved = STRBUF_INIT;
+ struct strbuf remaining = STRBUF_INIT;
+ struct strbuf next = STRBUF_INIT;
+ struct strbuf symlink = STRBUF_INIT;
char *retval = NULL;
-
- /*
- * If we have to temporarily chdir(), store the original CWD
- * here so that we can chdir() back to it at the end of the
- * function:
- */
- struct strbuf cwd = STRBUF_INIT;
-
- int depth = MAXDEPTH;
- char *last_elem = NULL;
+ int num_symlinks = 0;
struct stat st;
/* We've already done it */
- if (path == sb.buf)
+ if (path == resolved.buf)
return path;
if (!*path) {
@@ -58,74 +90,114 @@ static const char *real_path_internal(const char *path, int die_on_error)
goto error_out;
}
- strbuf_reset(&sb);
- strbuf_addstr(&sb, path);
-
- while (depth--) {
- if (!is_directory(sb.buf)) {
- char *last_slash = find_last_dir_sep(sb.buf);
- if (last_slash) {
- last_elem = xstrdup(last_slash + 1);
- strbuf_setlen(&sb, last_slash - sb.buf + 1);
- } else {
- last_elem = xmemdupz(sb.buf, sb.len);
- strbuf_reset(&sb);
- }
+ strbuf_reset(&resolved);
+
+ if (is_absolute_path(path)) {
+ /* absolute path; start with only root as being resolved */
+ int offset = offset_1st_component(path);
+ strbuf_add(&resolved, path, offset);
+ strbuf_addstr(&remaining, path + offset);
+ } else {
+ /* relative path; can use CWD as the initial resolved path */
+ if (strbuf_getcwd(&resolved)) {
+ if (die_on_error)
+ die_errno("unable to get current working directory");
+ else
+ goto error_out;
}
+ strbuf_addstr(&remaining, path);
+ }
- if (sb.len) {
- if (!cwd.len && strbuf_getcwd(&cwd)) {
+ /* Iterate over the remaining path components */
+ while (remaining.len > 0) {
+ get_next_component(&next, &remaining);
+
+ if (next.len == 0) {
+ continue; /* empty component */
+ } else if (next.len == 1 && !strcmp(next.buf, ".")) {
+ continue; /* '.' component */
+ } else if (next.len == 2 && !strcmp(next.buf, "..")) {
+ /* '..' component; strip the last path component */
+ strip_last_component(&resolved);
+ continue;
+ }
+
+ /* append the next component and resolve resultant path */
+ if (!is_dir_sep(resolved.buf[resolved.len - 1]))
+ strbuf_addch(&resolved, '/');
+ strbuf_addbuf(&resolved, &next);
+
+ if (lstat(resolved.buf, &st)) {
+ /* error out unless this was the last component */
+ if (errno != ENOENT || remaining.len) {
if (die_on_error)
- die_errno("Could not get current working directory");
+ die_errno("Invalid path '%s'",
+ resolved.buf);
else
goto error_out;
}
+ } else if (S_ISLNK(st.st_mode)) {
+ ssize_t len;
+ strbuf_reset(&symlink);
+
+ if (num_symlinks++ > MAXSYMLINKS) {
+ errno = ELOOP;
- if (chdir(sb.buf)) {
if (die_on_error)
- die_errno("Could not switch to '%s'",
- sb.buf);
+ die("More than %d nested symlinks "
+ "on path '%s'", MAXSYMLINKS, path);
else
goto error_out;
}
- }
- if (strbuf_getcwd(&sb)) {
- if (die_on_error)
- die_errno("Could not get current working directory");
- else
- goto error_out;
- }
- if (last_elem) {
- if (sb.len && !is_dir_sep(sb.buf[sb.len - 1]))
- strbuf_addch(&sb, '/');
- strbuf_addstr(&sb, last_elem);
- free(last_elem);
- last_elem = NULL;
- }
-
- if (!lstat(sb.buf, &st) && S_ISLNK(st.st_mode)) {
- struct strbuf next_sb = STRBUF_INIT;
- ssize_t len = strbuf_readlink(&next_sb, sb.buf, 0);
+ len = strbuf_readlink(&symlink, resolved.buf,
+ st.st_size);
if (len < 0) {
if (die_on_error)
die_errno("Invalid symlink '%s'",
- sb.buf);
+ resolved.buf);
else
goto error_out;
}
- strbuf_swap(&sb, &next_sb);
- strbuf_release(&next_sb);
- } else
- break;
+
+ if (is_absolute_path(symlink.buf)) {
+ /* absolute symlink; set resolved to root */
+ int offset = offset_1st_component(symlink.buf);
+ strbuf_reset(&resolved);
+ strbuf_add(&resolved, symlink.buf, offset);
+ strbuf_remove(&symlink, 0, offset);
+ } else {
+ /*
+ * relative symlink
+ * strip off the last component since it will
+ * be replaced with the contents of the symlink
+ */
+ strip_last_component(&resolved);
+ }
+
+ /*
+ * if there are still remaining components to resolve
+ * then append them to symlink
+ */
+ if (remaining.len) {
+ strbuf_addch(&symlink, '/');
+ strbuf_addbuf(&symlink, &remaining);
+ }
+
+ /*
+ * use the symlink as the remaining components that
+ * need to be resloved
+ */
+ strbuf_swap(&symlink, &remaining);
+ }
}
- retval = sb.buf;
+ retval = resolved.buf;
+
error_out:
- free(last_elem);
- if (cwd.len && chdir(cwd.buf))
- die_errno("Could not change back to '%s'", cwd.buf);
- strbuf_release(&cwd);
+ strbuf_release(&remaining);
+ strbuf_release(&next);
+ strbuf_release(&symlink);
return retval;
}
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 2/5] real_path: convert real_path_internal to strbuf_realpath
From: Brandon Williams @ 2017-01-04 22:01 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds, larsxschneider
In-Reply-To: <20170104220124.145808-1-bmwill@google.com>
Change the name of real_path_internal to strbuf_realpath. In addition
push the static strbuf up to its callers and instead take as a
parameter a pointer to a strbuf to use for the final result.
This change makes strbuf_realpath reentrant.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
abspath.c | 53 +++++++++++++++++++++++++----------------------------
cache.h | 2 ++
2 files changed, 27 insertions(+), 28 deletions(-)
diff --git a/abspath.c b/abspath.c
index 629201e48..a200d4220 100644
--- a/abspath.c
+++ b/abspath.c
@@ -57,21 +57,17 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)
* Return the real path (i.e., absolute path, with symlinks resolved
* and extra slashes removed) equivalent to the specified path. (If
* you want an absolute path but don't mind links, use
- * absolute_path().) The return value is a pointer to a static
- * buffer.
+ * absolute_path().) Places the resolved realpath in the provided strbuf.
*
* The directory part of path (i.e., everything up to the last
* dir_sep) must denote a valid, existing directory, but the last
* component need not exist. If die_on_error is set, then die with an
* informative error message if there is a problem. Otherwise, return
* NULL on errors (without generating any output).
- *
- * If path is our buffer, then return path, as it's already what the
- * user wants.
*/
-static const char *real_path_internal(const char *path, int die_on_error)
+char *strbuf_realpath(struct strbuf *resolved, const char *path,
+ int die_on_error)
{
- static struct strbuf resolved = STRBUF_INIT;
struct strbuf remaining = STRBUF_INIT;
struct strbuf next = STRBUF_INIT;
struct strbuf symlink = STRBUF_INIT;
@@ -79,10 +75,6 @@ static const char *real_path_internal(const char *path, int die_on_error)
int num_symlinks = 0;
struct stat st;
- /* We've already done it */
- if (path == resolved.buf)
- return path;
-
if (!*path) {
if (die_on_error)
die("The empty string is not a valid path");
@@ -90,16 +82,16 @@ static const char *real_path_internal(const char *path, int die_on_error)
goto error_out;
}
- strbuf_reset(&resolved);
+ strbuf_reset(resolved);
if (is_absolute_path(path)) {
/* absolute path; start with only root as being resolved */
int offset = offset_1st_component(path);
- strbuf_add(&resolved, path, offset);
+ strbuf_add(resolved, path, offset);
strbuf_addstr(&remaining, path + offset);
} else {
/* relative path; can use CWD as the initial resolved path */
- if (strbuf_getcwd(&resolved)) {
+ if (strbuf_getcwd(resolved)) {
if (die_on_error)
die_errno("unable to get current working directory");
else
@@ -118,21 +110,21 @@ static const char *real_path_internal(const char *path, int die_on_error)
continue; /* '.' component */
} else if (next.len == 2 && !strcmp(next.buf, "..")) {
/* '..' component; strip the last path component */
- strip_last_component(&resolved);
+ strip_last_component(resolved);
continue;
}
/* append the next component and resolve resultant path */
- if (!is_dir_sep(resolved.buf[resolved.len - 1]))
- strbuf_addch(&resolved, '/');
- strbuf_addbuf(&resolved, &next);
+ if (!is_dir_sep(resolved->buf[resolved->len - 1]))
+ strbuf_addch(resolved, '/');
+ strbuf_addbuf(resolved, &next);
- if (lstat(resolved.buf, &st)) {
+ if (lstat(resolved->buf, &st)) {
/* error out unless this was the last component */
if (errno != ENOENT || remaining.len) {
if (die_on_error)
die_errno("Invalid path '%s'",
- resolved.buf);
+ resolved->buf);
else
goto error_out;
}
@@ -150,12 +142,12 @@ static const char *real_path_internal(const char *path, int die_on_error)
goto error_out;
}
- len = strbuf_readlink(&symlink, resolved.buf,
+ len = strbuf_readlink(&symlink, resolved->buf,
st.st_size);
if (len < 0) {
if (die_on_error)
die_errno("Invalid symlink '%s'",
- resolved.buf);
+ resolved->buf);
else
goto error_out;
}
@@ -163,8 +155,8 @@ static const char *real_path_internal(const char *path, int die_on_error)
if (is_absolute_path(symlink.buf)) {
/* absolute symlink; set resolved to root */
int offset = offset_1st_component(symlink.buf);
- strbuf_reset(&resolved);
- strbuf_add(&resolved, symlink.buf, offset);
+ strbuf_reset(resolved);
+ strbuf_add(resolved, symlink.buf, offset);
strbuf_remove(&symlink, 0, offset);
} else {
/*
@@ -172,7 +164,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
* strip off the last component since it will
* be replaced with the contents of the symlink
*/
- strip_last_component(&resolved);
+ strip_last_component(resolved);
}
/*
@@ -192,24 +184,29 @@ static const char *real_path_internal(const char *path, int die_on_error)
}
}
- retval = resolved.buf;
+ retval = resolved->buf;
error_out:
strbuf_release(&remaining);
strbuf_release(&next);
strbuf_release(&symlink);
+ if (!retval)
+ strbuf_reset(resolved);
+
return retval;
}
const char *real_path(const char *path)
{
- return real_path_internal(path, 1);
+ static struct strbuf realpath = STRBUF_INIT;
+ return strbuf_realpath(&realpath, path, 1);
}
const char *real_path_if_valid(const char *path)
{
- return real_path_internal(path, 0);
+ static struct strbuf realpath = STRBUF_INIT;
+ return strbuf_realpath(&realpath, path, 0);
}
/*
diff --git a/cache.h b/cache.h
index a50a61a19..7a8129403 100644
--- a/cache.h
+++ b/cache.h
@@ -1064,6 +1064,8 @@ static inline int is_absolute_path(const char *path)
return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
}
int is_directory(const char *);
+char *strbuf_realpath(struct strbuf *resolved, const char *path,
+ int die_on_error);
const char *real_path(const char *path);
const char *real_path_if_valid(const char *path);
const char *absolute_path(const char *path);
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 5/5] real_path: canonicalize directory separators in root parts
From: Brandon Williams @ 2017-01-04 22:01 UTC (permalink / raw)
To: git
Cc: Johannes Sixt, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, pclouds, larsxschneider
In-Reply-To: <20170104220124.145808-1-bmwill@google.com>
From: Johannes Sixt <j6t@kdbg.org>
When an absolute path is resolved, resolution begins at the first path
component after the root part. The root part is just copied verbatim,
because it must not be inspected for symbolic links. For POSIX paths,
this is just the initial slash, but on Windows, the root part has the
forms c:\ or \\server\share. We do want to canonicalize the back-slashes
in the root part because these parts are compared to the result of
getcwd(), which does return a fully canonicalized path.
Factor out a helper that splits off the root part, and have it
canonicalize the copied part.
This change was prompted because t1504-ceiling-dirs.sh caught a breakage
in GIT_CEILING_DIRECTORIES handling on Windows.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
abspath.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/abspath.c b/abspath.c
index 72f716f80..fce40fddc 100644
--- a/abspath.c
+++ b/abspath.c
@@ -48,6 +48,19 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)
strbuf_remove(remaining, 0, end - remaining->buf);
}
+/* copies root part from remaining to resolved, canonicalizing it on the way */
+static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
+{
+ int offset = offset_1st_component(remaining->buf);
+
+ strbuf_reset(resolved);
+ strbuf_add(resolved, remaining->buf, offset);
+#ifdef GIT_WINDOWS_NATIVE
+ convert_slashes(resolved->buf);
+#endif
+ strbuf_remove(remaining, 0, offset);
+}
+
/* We allow "recursive" symbolic links. Only within reason, though. */
#ifndef MAXSYMLINKS
#define MAXSYMLINKS 32
@@ -82,14 +95,10 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
goto error_out;
}
- strbuf_reset(resolved);
+ strbuf_addstr(&remaining, path);
+ get_root_part(resolved, &remaining);
- if (is_absolute_path(path)) {
- /* absolute path; start with only root as being resolved */
- int offset = offset_1st_component(path);
- strbuf_add(resolved, path, offset);
- strbuf_addstr(&remaining, path + offset);
- } else {
+ if (!resolved->len) {
/* relative path; can use CWD as the initial resolved path */
if (strbuf_getcwd(resolved)) {
if (die_on_error)
@@ -97,7 +106,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
else
goto error_out;
}
- strbuf_addstr(&remaining, path);
}
/* Iterate over the remaining path components */
@@ -154,10 +162,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
if (is_absolute_path(symlink.buf)) {
/* absolute symlink; set resolved to root */
- int offset = offset_1st_component(symlink.buf);
- strbuf_reset(resolved);
- strbuf_add(resolved, symlink.buf, offset);
- strbuf_remove(&symlink, 0, offset);
+ get_root_part(resolved, &symlink);
} else {
/*
* relative symlink
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 4/5] real_path: have callers use real_pathdup and strbuf_realpath
From: Brandon Williams @ 2017-01-04 22:01 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds, larsxschneider
In-Reply-To: <20170104220124.145808-1-bmwill@google.com>
Migrate callers of real_path() who duplicate the return value to use
real_pathdup or strbuf_realpath.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/init-db.c | 6 +++---
environment.c | 2 +-
setup.c | 13 ++++++++-----
sha1_file.c | 2 +-
submodule.c | 2 +-
transport.c | 2 +-
worktree.c | 2 +-
7 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 2399b97d9..76d68fad0 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
{
int reinit;
int exist_ok = flags & INIT_DB_EXIST_OK;
- char *original_git_dir = xstrdup(real_path(git_dir));
+ char *original_git_dir = real_pathdup(git_dir);
if (real_git_dir) {
struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
if (real_git_dir && !is_absolute_path(real_git_dir))
- real_git_dir = xstrdup(real_path(real_git_dir));
+ real_git_dir = real_pathdup(real_git_dir);
if (argc == 1) {
int mkdir_tried = 0;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
const char *git_dir_parent = strrchr(git_dir, '/');
if (git_dir_parent) {
char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
- git_work_tree_cfg = xstrdup(real_path(rel));
+ git_work_tree_cfg = real_pathdup(rel);
free(rel);
}
if (!git_work_tree_cfg)
diff --git a/environment.c b/environment.c
index 0935ec696..9b943d2d5 100644
--- a/environment.c
+++ b/environment.c
@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)
return;
}
git_work_tree_initialized = 1;
- work_tree = xstrdup(real_path(new_work_tree));
+ work_tree = real_pathdup(new_work_tree);
}
const char *get_git_work_tree(void)
diff --git a/setup.c b/setup.c
index fe572b82c..1b534a750 100644
--- a/setup.c
+++ b/setup.c
@@ -256,8 +256,10 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
strbuf_addbuf(&path, &data);
strbuf_addstr(sb, real_path(path.buf));
ret = 1;
- } else
+ } else {
strbuf_addstr(sb, gitdir);
+ }
+
strbuf_release(&data);
strbuf_release(&path);
return ret;
@@ -692,7 +694,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
if (offset != cwd->len && !is_absolute_path(gitdir))
- gitdir = xstrdup(real_path(gitdir));
+ gitdir = real_pathdup(gitdir);
if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
@@ -800,11 +802,12 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
/* Keep entry but do not canonicalize it */
return 1;
} else {
- const char *real_path = real_path_if_valid(ceil);
- if (!real_path)
+ char *real_path = real_pathdup(ceil);
+ if (!real_path) {
return 0;
+ }
free(item->string);
- item->string = xstrdup(real_path);
+ item->string = real_path;
return 1;
}
}
diff --git a/sha1_file.c b/sha1_file.c
index 9c86d1924..6a03cc393 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -291,7 +291,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
struct strbuf pathbuf = STRBUF_INIT;
if (!is_absolute_path(entry) && relative_base) {
- strbuf_addstr(&pathbuf, real_path(relative_base));
+ strbuf_realpath(&pathbuf, relative_base, 1);
strbuf_addch(&pathbuf, '/');
}
strbuf_addstr(&pathbuf, entry);
diff --git a/submodule.c b/submodule.c
index ece17315d..55819ba9c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1301,7 +1301,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
{
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
- const char *real_work_tree = xstrdup(real_path(work_tree));
+ const char *real_work_tree = real_pathdup(work_tree);
/* Update gitfile */
strbuf_addf(&file_name, "%s/.git", work_tree);
diff --git a/transport.c b/transport.c
index 04e5d6623..a3b98f198 100644
--- a/transport.c
+++ b/transport.c
@@ -1146,7 +1146,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,
const struct ref *extra;
struct alternate_refs_data *cb = data;
- other = xstrdup(real_path(e->path));
+ other = real_pathdup(e->path);
len = strlen(other);
while (other[len-1] == '/')
diff --git a/worktree.c b/worktree.c
index eb6121263..edf14daf9 100644
--- a/worktree.c
+++ b/worktree.c
@@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,
return wt;
arg = prefix_filename(prefix, strlen(prefix), arg);
- path = xstrdup(real_path(arg));
+ path = real_pathdup(arg);
for (; *list; list++)
if (!fspathcmp(path, real_path((*list)->path)))
break;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 3/5] real_path: create real_pathdup
From: Brandon Williams @ 2017-01-04 22:01 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds, larsxschneider
In-Reply-To: <20170104220124.145808-1-bmwill@google.com>
Create real_pathdup which returns a caller owned string of the resolved
realpath based on the provide path.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
abspath.c | 13 +++++++++++++
cache.h | 1 +
2 files changed, 14 insertions(+)
diff --git a/abspath.c b/abspath.c
index a200d4220..72f716f80 100644
--- a/abspath.c
+++ b/abspath.c
@@ -209,6 +209,19 @@ const char *real_path_if_valid(const char *path)
return strbuf_realpath(&realpath, path, 0);
}
+char *real_pathdup(const char *path)
+{
+ struct strbuf realpath = STRBUF_INIT;
+ char *retval = NULL;
+
+ if (strbuf_realpath(&realpath, path, 0))
+ retval = strbuf_detach(&realpath, NULL);
+
+ strbuf_release(&realpath);
+
+ return retval;
+}
+
/*
* Use this to get an absolute path from a relative one. If you want
* to resolve links, you should use real_path.
diff --git a/cache.h b/cache.h
index 7a8129403..e12a5d912 100644
--- a/cache.h
+++ b/cache.h
@@ -1068,6 +1068,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
int die_on_error);
const char *real_path(const char *path);
const char *real_path_if_valid(const char *path);
+char *real_pathdup(const char *path);
const char *absolute_path(const char *path);
const char *remove_leading_path(const char *in, const char *prefix);
const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 0/5] road to reentrant real_path
From: Brandon Williams @ 2017-01-04 22:01 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds, larsxschneider
In-Reply-To: <20170103190923.11882-1-bmwill@google.com>
changes in v5:
* set errno to ELOOP when MAXSYMLINKS is exceded.
* revert to use MAXSYMLINKS instead of MAXDEPTH.
* If the OS hasn't defined MAXSYMLINKS, use a fallback value of 32.
Brandon Williams (4):
real_path: resolve symlinks by hand
real_path: convert real_path_internal to strbuf_realpath
real_path: create real_pathdup
real_path: have callers use real_pathdup and strbuf_realpath
Johannes Sixt (1):
real_path: canonicalize directory separators in root parts
abspath.c | 231 +++++++++++++++++++++++++++++++++++++-----------------
builtin/init-db.c | 6 +-
cache.h | 3 +
environment.c | 2 +-
setup.c | 13 +--
sha1_file.c | 2 +-
submodule.c | 2 +-
transport.c | 2 +-
worktree.c | 2 +-
9 files changed, 178 insertions(+), 85 deletions(-)
--- interdiff with v4
diff --git a/abspath.c b/abspath.c
index 3562d17bf..fce40fddc 100644
--- a/abspath.c
+++ b/abspath.c
@@ -62,7 +62,9 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
}
/* We allow "recursive" symbolic links. Only within reason, though. */
-#define MAXDEPTH 5
+#ifndef MAXSYMLINKS
+#define MAXSYMLINKS 32
+#endif
/*
* Return the real path (i.e., absolute path, with symlinks resolved
@@ -138,10 +140,12 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
ssize_t len;
strbuf_reset(&symlink);
- if (num_symlinks++ > MAXDEPTH) {
+ if (num_symlinks++ > MAXSYMLINKS) {
+ errno = ELOOP;
+
if (die_on_error)
die("More than %d nested symlinks "
- "on path '%s'", MAXDEPTH, path);
+ "on path '%s'", MAXSYMLINKS, path);
else
goto error_out;
}
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* Re: [PATCH v4 0/5] road to reentrant real_path
From: Jacob Keller @ 2017-01-04 21:46 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, Jeff King, Torsten Bögershausen,
git@vger.kernel.org, Junio C Hamano, Ramsay Jones, Johannes Sixt,
Duy Nguyen, Lars Schneider
In-Reply-To: <CAGZ79kbdNuGe038Wb9OR1SKq-XYtsPrLsn6XueO6zsKKGFYiNg@mail.gmail.com>
On Wed, Jan 4, 2017 at 10:22 AM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Jan 4, 2017 at 10:13 AM, Brandon Williams <bmwill@google.com> wrote:
>> On 01/04, Jeff King wrote:
>>> On Wed, Jan 04, 2017 at 07:56:02AM +0100, Torsten Bögershausen wrote:
>>>
>>> > On 04.01.17 01:48, Jeff King wrote:
>>> > > On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
>>> > >
>>> > >> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
>>> > >> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
>>> > >
>>> > > Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
>>> > > what all other similar functions will be using.
>>> > >
>>> > > The only problem was that we were redefining the macro. So maybe:
>>> > >
>>> > > #ifndef MAXSYMLINKS
>>> > > #define MAXSYMLINKS 5
>>> > > #endif
>>> > >
>>> > > would be a good solution?
>>> > Why 5 ? (looking at the 20..30 below)
>>> > And why 5 on one system and e.g. on my Mac OS
>>> > #define MAXSYMLINKS 32
>>>
>>> I mentioned "5" because that is the current value of MAXDEPTH. I do
>>> think it would be reasonable to bump it to something higher.
>>>
>>> > Would the same value value for all Git installations on all platforms make sense?
>>> > #define GITMAXSYMLINKS 20
>>>
>>> I think it's probably more important to match the rest of the OS, so
>>> that open("foo") and realpath("foo") behave similarly on the same
>>> system. Though I think even that isn't always possible, as the limit is
>>> dynamic on some systems.
>>>
>>> I think the idea of the 20-30 range is that it's small enough to catch
>>> an infinite loop quickly, and large enough that nobody will ever hit it
>>> in practice. :)
>>
>> I agree that we should have similar guarantees as the OS provides,
>> especially if the OS already has MAXSYMLINKS defined. What then, should
>> the fall back value be if the OS doesn't have this defined? 5 like we
>> have done historically, or something around the 20-30 range as Torsten
>> suggests?
>
> As a fallback I'd rather go for a larger number than too small.
> The reason for the existence is just to break an infinite loop
> (and report an error? which the current code doesn't quite do,
> but your series actually does).
>
> If the number is too large, then it takes a bit longer to generate the error
> message, but the error path is no big deal w.r.t. performance, so it's fine
> for it taking a bit longer.
>
> If the number is too low, then we may hinder people from getting actual
> work done, (i.e. they have to figure out what the problem is and recompile
> git), so I'd think a larger number is not harmful. So 32?
>
I think I agree as well.
Thanks,
Jake
>>
>> --
>> Brandon Williams
^ permalink raw reply
* Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error
From: Brandon Williams @ 2017-01-04 20:53 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jeff King, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <CAGZ79kY3oVfn-xH4RQR9jMoKxoQUtF5HezY9HPUZGbx9KP-S5Q@mail.gmail.com>
On 01/04, Stefan Beller wrote:
> On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@peff.net> wrote:
> > But as this commit message needs to stand on its own, rather than as part of a
> > larger discussion thread, it might be worth expanding "one of the cases"
> > here. And talking about what's happening to the other cases.
> >
> > Like:
> >
> > This assertion triggered for cases where there wasn't a programming
> > bug, but just bogus input. In particular, if the user asks for a
> > pathspec that is inside a submodule, we shouldn't assert() or
> > die("BUG"); we should tell the user their request is bogus.
> >
> > We'll retain the assertion for non-submodule cases, though. We don't
> > know of any cases that would trigger this, but it _would_ be
> > indicative of a programming error, and we should catch it here.
>
> makes sense.
>
> >
> > or something. Writing the first paragraph made me wonder if a better
> > solution, though, would be to catch and complain about this case
> > earlier. IOW, this _is_ a programming bug, because we're violating some
> > assumption of the pathspec code. And whatever is putting that item into
> > the pathspec list is what should be fixed.
> >
> > I haven't looked closely enough to have a real opinion on that, though.
>
> Well I think you get different behavior with different flags enabled, i.e.
> the test provided is a cornercase (as "git add ." in the submodule should
> not yell at us IF PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE
> were set, in my understanding of the code, so maybe the test rather adds
> a ./file/with/characters inside the submodule directory)
>
> I think a valid long term vision would be to have
>
> $ git -C submodule add file
> $ echo $?
> 0
>
> to behave the same as
>
> $ git add submodule/file
> advice/hint: adding file inside of a submodule
> $ echo $?
> 0
> $ git -c submodule.iKnowWhatIDo add submodule/anotherfile
> $ echo $?
> 0
>
> Brandon, who is refactoring the pathspec stuff currently may have
> an opinion if we could catch it earlier and still have beautiful code.
>
> Thanks,
> Stefan
>
> > Given the discussion, this comment seems funny now. Who cares about
> > "historically"? It should probably be something like:
> >
> > /*
> > * This case can be triggered by the user pointing us to a pathspec
> > * inside a submodule, which is an input error. Detect that here
> > * and complain, but fallback in the non-submodule case to a BUG,
> > * as we have no idea what would trigger that.
> > */
>
> Makes sense.
>
> >
> > -Peff
So there are really two different things going on in the pathspec code
with regards to submodules.
The case that this series is trying to solve is not because the user
provided a pathspec into a submodule, but rather they are executing in
the context of the submodule with bogus state. Typically this bogus
state has something to do with the submodule's .gitdir being blown away
(like in the last test (3/3) added in this patch). Because the
submodule doesn't have a .gitdir, it searches backward in the directory
hierarchy for a .gitdir and it happens to find the superproject's gitdir
and uses that as its own .gitdir. When this happens test 3/3 catches
that assert with the prefix being "sub/" and match being "sub" (since
the submodule slash was removed). The condition doesn't trigger when
you supply a pathspec of "./b/a" assuming you have a file 'a' in
directory 'b' inside the submodule, since the prefix would still be
"sub/" while the match string would be "sub/b/a". Coincidentally the
check that PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE does, does in fact
catch it (if using say the 'git add' command).
This leads me into the second case. If
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set, then any pathspec which
decends into a submodule will indeed be caught and cause and error (as
was happens in test 2/3 in this patch).
So in my opinion, the assert at the end of constructing a
pathspec object probably isn't the best place for determining if the
submodule's gitdir has been destroyed and instead it has fallen back to
its parent's gitdir. A check for something like this should happen much
sooner.
There are cases where it is advantages to be able to supply a pathspec
into a submodule without it erroring out (git grep --recurse-submodules
is one example). So right now the current method for not allowing a
pathspec into a submodule is to pass the STRIP_SUBMODULE_SLASH_EXPENSIVE
flag when creating the pathspec object.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 2/4] t7610: make tests more independent and debuggable
From: Stefan Beller @ 2017-01-04 20:27 UTC (permalink / raw)
To: Richard Hansen; +Cc: git@vger.kernel.org, David Aguilar, Johannes Sixt
In-Reply-To: <20170104005042.51530-3-hansenr@google.com>
On Tue, Jan 3, 2017 at 4:50 PM, Richard Hansen <hansenr@google.com> wrote:
> If a test fails it might leave the repository in a strange state. Add
> 'git reset --hard' at the beginning of each test to increase the odds
> of passing when an earlier test fails.
So each test is cleaning up the previous test, which *may* confuse
a reader ("how is the reset --hard relevant for this test? Oooh it's
just a cleanup").
We could put it another way by having each test itself make clean
up after itself via
test_when_finished "git reset --hard" &&
..
at the beginning of each test.
This would produce the same order of operations, i.e. a
reset run between each test, but semantically tells the reader
that the reset is part of the current test cleaning up after itself,
as "reset" is operation for this particular test to cleanup.
Does that make sense?
>
> Also use test-specific branches to avoid interfering with later tests
> and to make the tests easier to debug.
That sounds great!
Though in the code I only spot one occurrence for
+ git checkout -b test$test_count branch1 &&
so maybe that could be part of the first patch in the series?
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-04 18:46 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams
In-Reply-To: <20170104075506.sa5oa5bheykswkwn@sigill.intra.peff.net>
On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@peff.net> wrote:
> But as this commit message needs to stand on its own, rather than as part of a
> larger discussion thread, it might be worth expanding "one of the cases"
> here. And talking about what's happening to the other cases.
>
> Like:
>
> This assertion triggered for cases where there wasn't a programming
> bug, but just bogus input. In particular, if the user asks for a
> pathspec that is inside a submodule, we shouldn't assert() or
> die("BUG"); we should tell the user their request is bogus.
>
> We'll retain the assertion for non-submodule cases, though. We don't
> know of any cases that would trigger this, but it _would_ be
> indicative of a programming error, and we should catch it here.
makes sense.
>
> or something. Writing the first paragraph made me wonder if a better
> solution, though, would be to catch and complain about this case
> earlier. IOW, this _is_ a programming bug, because we're violating some
> assumption of the pathspec code. And whatever is putting that item into
> the pathspec list is what should be fixed.
>
> I haven't looked closely enough to have a real opinion on that, though.
Well I think you get different behavior with different flags enabled, i.e.
the test provided is a cornercase (as "git add ." in the submodule should
not yell at us IF PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE
were set, in my understanding of the code, so maybe the test rather adds
a ./file/with/characters inside the submodule directory)
I think a valid long term vision would be to have
$ git -C submodule add file
$ echo $?
0
to behave the same as
$ git add submodule/file
advice/hint: adding file inside of a submodule
$ echo $?
0
$ git -c submodule.iKnowWhatIDo add submodule/anotherfile
$ echo $?
0
Brandon, who is refactoring the pathspec stuff currently may have
an opinion if we could catch it earlier and still have beautiful code.
Thanks,
Stefan
> Given the discussion, this comment seems funny now. Who cares about
> "historically"? It should probably be something like:
>
> /*
> * This case can be triggered by the user pointing us to a pathspec
> * inside a submodule, which is an input error. Detect that here
> * and complain, but fallback in the non-submodule case to a BUG,
> * as we have no idea what would trigger that.
> */
Makes sense.
>
> -Peff
^ permalink raw reply
* Re: [PATCH v4 0/5] road to reentrant real_path
From: Stefan Beller @ 2017-01-04 18:22 UTC (permalink / raw)
To: Brandon Williams
Cc: Jeff King, Torsten Bögershausen, git@vger.kernel.org,
Jacob Keller, Junio C Hamano, Ramsay Jones, Johannes Sixt,
Duy Nguyen, Lars Schneider
In-Reply-To: <20170104181318.GC69227@google.com>
On Wed, Jan 4, 2017 at 10:13 AM, Brandon Williams <bmwill@google.com> wrote:
> On 01/04, Jeff King wrote:
>> On Wed, Jan 04, 2017 at 07:56:02AM +0100, Torsten Bögershausen wrote:
>>
>> > On 04.01.17 01:48, Jeff King wrote:
>> > > On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
>> > >
>> > >> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
>> > >> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
>> > >
>> > > Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
>> > > what all other similar functions will be using.
>> > >
>> > > The only problem was that we were redefining the macro. So maybe:
>> > >
>> > > #ifndef MAXSYMLINKS
>> > > #define MAXSYMLINKS 5
>> > > #endif
>> > >
>> > > would be a good solution?
>> > Why 5 ? (looking at the 20..30 below)
>> > And why 5 on one system and e.g. on my Mac OS
>> > #define MAXSYMLINKS 32
>>
>> I mentioned "5" because that is the current value of MAXDEPTH. I do
>> think it would be reasonable to bump it to something higher.
>>
>> > Would the same value value for all Git installations on all platforms make sense?
>> > #define GITMAXSYMLINKS 20
>>
>> I think it's probably more important to match the rest of the OS, so
>> that open("foo") and realpath("foo") behave similarly on the same
>> system. Though I think even that isn't always possible, as the limit is
>> dynamic on some systems.
>>
>> I think the idea of the 20-30 range is that it's small enough to catch
>> an infinite loop quickly, and large enough that nobody will ever hit it
>> in practice. :)
>
> I agree that we should have similar guarantees as the OS provides,
> especially if the OS already has MAXSYMLINKS defined. What then, should
> the fall back value be if the OS doesn't have this defined? 5 like we
> have done historically, or something around the 20-30 range as Torsten
> suggests?
As a fallback I'd rather go for a larger number than too small.
The reason for the existence is just to break an infinite loop
(and report an error? which the current code doesn't quite do,
but your series actually does).
If the number is too large, then it takes a bit longer to generate the error
message, but the error path is no big deal w.r.t. performance, so it's fine
for it taking a bit longer.
If the number is too low, then we may hinder people from getting actual
work done, (i.e. they have to figure out what the problem is and recompile
git), so I'd think a larger number is not harmful. So 32?
>
> --
> Brandon Williams
^ permalink raw reply
* Re: [PATCH v4 4/5] real_path: have callers use real_pathdup and strbuf_realpath
From: Brandon Williams @ 2017-01-04 18:14 UTC (permalink / raw)
To: Jacob Keller
Cc: Git mailing list, Stefan Beller, Jeff King, Junio C Hamano,
Ramsay Jones, Torsten Bögershausen, Johannes Sixt,
Duy Nguyen, Lars Schneider
In-Reply-To: <CA+P7+xrgt+aTF4ibJ139=WihwHwG_m01bjAaF5-VW=Rk8u1ykA@mail.gmail.com>
On 01/03, Jacob Keller wrote:
> On Tue, Jan 3, 2017 at 11:09 AM, Brandon Williams <bmwill@google.com> wrote:
> > Migrate callers of real_path() who duplicate the retern value to use
> > real_pathdup or strbuf_realpath.
>
> Nit: s/retern/return
Thanks for catching that, I'll fix that in the next reroll.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()
From: Stefan Beller @ 2017-01-04 18:09 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git@vger.kernel.org, Junio C Hamano, David Aguilar,
Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <alpine.DEB.2.20.1701032231400.3469@virtualbox>
On Tue, Jan 3, 2017 at 1:33 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> This patch was originally only to appease Coverity, but it actually *does*
> plug a very real memory leak: previously, *every* call to git_exec_path()
> *possibly* returned a newly-malloc()ed buffer. Now, the first call will
> store that pointer in a static variable and reuse it later.
>
> Could you maybe help me with improving the commit message?
As someone not familiar with that area of code, this explained it
enough for me to understand, so maybe:
exec_cmd: do not leak via git_exec_path
Every call to git_exec_path() possibly returned a newly-malloc()ed
buffer. Now, the first call will allocate the buffer and subsequent
calls return a pointer to it, which then prevents leaking memory
on each call.
The return value of a "const char *" hints to the caller, that the memory
is not owned by the caller, do we need to be explicit there (i.e. a comment
declaring the memory ownership? Probably not.)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v4 0/5] road to reentrant real_path
From: Brandon Williams @ 2017-01-04 18:13 UTC (permalink / raw)
To: Jeff King
Cc: Torsten Bögershausen, git, sbeller, jacob.keller, gitster,
ramsay, j6t, pclouds, larsxschneider
In-Reply-To: <20170104070107.huse2a6thz737epv@sigill.intra.peff.net>
On 01/04, Jeff King wrote:
> On Wed, Jan 04, 2017 at 07:56:02AM +0100, Torsten Bögershausen wrote:
>
> > On 04.01.17 01:48, Jeff King wrote:
> > > On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
> > >
> > >> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
> > >> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
> > >
> > > Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
> > > what all other similar functions will be using.
> > >
> > > The only problem was that we were redefining the macro. So maybe:
> > >
> > > #ifndef MAXSYMLINKS
> > > #define MAXSYMLINKS 5
> > > #endif
> > >
> > > would be a good solution?
> > Why 5 ? (looking at the 20..30 below)
> > And why 5 on one system and e.g. on my Mac OS
> > #define MAXSYMLINKS 32
>
> I mentioned "5" because that is the current value of MAXDEPTH. I do
> think it would be reasonable to bump it to something higher.
>
> > Would the same value value for all Git installations on all platforms make sense?
> > #define GITMAXSYMLINKS 20
>
> I think it's probably more important to match the rest of the OS, so
> that open("foo") and realpath("foo") behave similarly on the same
> system. Though I think even that isn't always possible, as the limit is
> dynamic on some systems.
>
> I think the idea of the 20-30 range is that it's small enough to catch
> an infinite loop quickly, and large enough that nobody will ever hit it
> in practice. :)
I agree that we should have similar guarantees as the OS provides,
especially if the OS already has MAXSYMLINKS defined. What then, should
the fall back value be if the OS doesn't have this defined? 5 like we
have done historically, or something around the 20-30 range as Torsten
suggests?
--
Brandon Williams
^ permalink raw reply
* [PATCH v5 06/16] pathspec: copy and free owned memory
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-bmwill@google.com>
The 'original' string entry in a pathspec_item is only duplicated some
of the time, instead always make a copy of the original and take
ownership of the memory.
Since both 'match' and 'original' string entries in a pathspec_item are
owned by the pathspec struct, they need to be freed when clearing the
pathspec struct (in 'clear_pathspec()') and duplicated when copying the
pathspec struct (in 'copy_pathspec()').
Also change the type of 'match' and 'original' to 'char *' in order to
more explicitly show the ownership of the memory.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 23 +++++++++++++++++++----
pathspec.h | 4 ++--
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 1f918cbae..b8faa8f46 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -259,8 +259,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
strbuf_addstr(&sb, match);
item->original = strbuf_detach(&sb, NULL);
- } else
- item->original = elt;
+ } else {
+ item->original = xstrdup(elt);
+ }
item->len = strlen(item->match);
item->prefix = prefixlen;
@@ -388,8 +389,8 @@ void parse_pathspec(struct pathspec *pathspec,
die("BUG: PATHSPEC_PREFER_CWD requires arguments");
pathspec->items = item = xcalloc(1, sizeof(*item));
- item->match = prefix;
- item->original = prefix;
+ item->match = xstrdup(prefix);
+ item->original = xstrdup(prefix);
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
pathspec->nr = 1;
@@ -453,13 +454,27 @@ void parse_pathspec(struct pathspec *pathspec,
void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
{
+ int i;
+
*dst = *src;
ALLOC_ARRAY(dst->items, dst->nr);
COPY_ARRAY(dst->items, src->items, dst->nr);
+
+ for (i = 0; i < dst->nr; i++) {
+ dst->items[i].match = xstrdup(src->items[i].match);
+ dst->items[i].original = xstrdup(src->items[i].original);
+ }
}
void clear_pathspec(struct pathspec *pathspec)
{
+ int i;
+
+ for (i = 0; i < pathspec->nr; i++) {
+ free(pathspec->items[i].match);
+ free(pathspec->items[i].original);
+ }
free(pathspec->items);
pathspec->items = NULL;
+ pathspec->nr = 0;
}
diff --git a/pathspec.h b/pathspec.h
index 70a592e91..49fd823dd 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -25,8 +25,8 @@ struct pathspec {
unsigned magic;
int max_depth;
struct pathspec_item {
- const char *match;
- const char *original;
+ char *match;
+ char *original;
unsigned magic;
int len, prefix;
int nowildcard_len;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 05/16] pathspec: remove the deprecated get_pathspec function
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-bmwill@google.com>
Now that all callers of the old 'get_pathspec' interface have been
migrated to use the new pathspec struct interface it can be removed
from the codebase.
Since there are no more users of the '_raw' field in the pathspec struct
it can also be removed. This patch also removes the old functionality
of modifying the const char **argv array that was passed into
parse_pathspec. Instead the constructed 'match' string (which is a
pathspec element with the prefix prepended) is only stored in its
corresponding pathspec_item entry.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/technical/api-setup.txt | 2 --
cache.h | 1 -
pathspec.c | 42 +++--------------------------------
pathspec.h | 1 -
4 files changed, 3 insertions(+), 43 deletions(-)
diff --git a/Documentation/technical/api-setup.txt b/Documentation/technical/api-setup.txt
index 540e45568..eb1fa9853 100644
--- a/Documentation/technical/api-setup.txt
+++ b/Documentation/technical/api-setup.txt
@@ -27,8 +27,6 @@ parse_pathspec(). This function takes several arguments:
- prefix and args come from cmd_* functions
-get_pathspec() is obsolete and should never be used in new code.
-
parse_pathspec() helps catch unsupported features and reject them
politely. At a lower level, different pathspec-related functions may
not support the same set of features. Such pathspec-sensitive
diff --git a/cache.h b/cache.h
index a50a61a19..0f80e01bd 100644
--- a/cache.h
+++ b/cache.h
@@ -514,7 +514,6 @@ extern void set_git_work_tree(const char *tree);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
-extern const char **get_pathspec(const char *prefix, const char **pathspec);
extern void setup_work_tree(void);
extern const char *setup_git_directory_gently(int *);
extern const char *setup_git_directory(void);
diff --git a/pathspec.c b/pathspec.c
index 22ca74a12..1f918cbae 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -103,7 +103,7 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
*/
static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned *p_short_magic,
- const char **raw, unsigned flags,
+ unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
{
@@ -240,7 +240,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
- *raw = item->match = match;
+ item->match = match;
/*
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
@@ -381,8 +381,6 @@ void parse_pathspec(struct pathspec *pathspec,
/* No arguments with prefix -> prefix pathspec */
if (!entry) {
- static const char *raw[2];
-
if (flags & PATHSPEC_PREFER_FULL)
return;
@@ -394,10 +392,7 @@ void parse_pathspec(struct pathspec *pathspec,
item->original = prefix;
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
- raw[0] = prefix;
- raw[1] = NULL;
pathspec->nr = 1;
- pathspec->_raw = raw;
return;
}
@@ -415,7 +410,6 @@ void parse_pathspec(struct pathspec *pathspec,
pathspec->nr = n;
ALLOC_ARRAY(pathspec->items, n);
item = pathspec->items;
- pathspec->_raw = argv;
prefixlen = prefix ? strlen(prefix) : 0;
for (i = 0; i < n; i++) {
@@ -423,7 +417,7 @@ void parse_pathspec(struct pathspec *pathspec,
entry = argv[i];
item[i].magic = prefix_pathspec(item + i, &short_magic,
- argv + i, flags,
+ flags,
prefix, prefixlen, entry);
if ((flags & PATHSPEC_LITERAL_PATH) &&
!(magic_mask & PATHSPEC_LITERAL))
@@ -457,36 +451,6 @@ void parse_pathspec(struct pathspec *pathspec,
}
}
-/*
- * N.B. get_pathspec() is deprecated in favor of the "struct pathspec"
- * based interface - see pathspec.c:parse_pathspec().
- *
- * Arguments:
- * - prefix - a path relative to the root of the working tree
- * - pathspec - a list of paths underneath the prefix path
- *
- * Iterates over pathspec, prepending each path with prefix,
- * and return the resulting list.
- *
- * If pathspec is empty, return a singleton list containing prefix.
- *
- * If pathspec and prefix are both empty, return an empty list.
- *
- * This is typically used by built-in commands such as add.c, in order
- * to normalize argv arguments provided to the built-in into a list of
- * paths to process, all relative to the root of the working tree.
- */
-const char **get_pathspec(const char *prefix, const char **pathspec)
-{
- struct pathspec ps;
- parse_pathspec(&ps,
- PATHSPEC_ALL_MAGIC &
- ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
- PATHSPEC_PREFER_CWD,
- prefix, pathspec);
- return ps._raw;
-}
-
void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
{
*dst = *src;
diff --git a/pathspec.h b/pathspec.h
index 59809e479..70a592e91 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -19,7 +19,6 @@
#define PATHSPEC_ONESTAR 1 /* the pathspec pattern satisfies GFNM_ONESTAR */
struct pathspec {
- const char **_raw; /* get_pathspec() result, not freed by clear_pathspec() */
int nr;
unsigned int has_wildcard:1;
unsigned int recursive:1;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox