* [PATCH] pathspec: give better message for submodule related pathspec error
@ 2016-12-28 0:05 Stefan Beller
2016-12-28 5:58 ` Jeff King
2016-12-28 18:13 ` [PATCH] " Brandon Williams
0 siblings, 2 replies; 5+ messages in thread
From: Stefan Beller @ 2016-12-28 0:05 UTC (permalink / raw)
To: bmwill; +Cc: git, Stefan Beller
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.
For now just improve the user visible error message.
[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
Signed-off-by: Stefan Beller <sbeller@google.com>
---
If you were following the mailing list closely today, you may sense
that I am cleaning up stalled branches. :)
I think such a hot fix is warranted given how often we had reports
on the mailing list.
Thanks,
Stefan
pathspec.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..d522f43331 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,11 @@ 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)
+ die (_("Path leads inside submodule '%s', but the submodule "
+ "was not recognized, i.e. not initialized or deleted"),
+ ce->name);
return magic;
}
--
2.11.0.196.gee862f456e.dirty
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pathspec: give better message for submodule related pathspec error
2016-12-28 0:05 [PATCH] pathspec: give better message for submodule related pathspec error Stefan Beller
@ 2016-12-28 5:58 ` Jeff King
2016-12-28 17:17 ` [PATCHv2] " Stefan Beller
2016-12-28 18:13 ` [PATCH] " Brandon Williams
1 sibling, 1 reply; 5+ messages in thread
From: Jeff King @ 2016-12-28 5:58 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git
On Tue, Dec 27, 2016 at 04:05:59PM -0800, Stefan Beller wrote:
> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1].
If people are running into it, it definitely should not be an assert,
nor a die("BUG"). It should be a regular die(), which your patch does.
So this is definitely a good step, even if the ultimate goal may be to
handle the case more gracefully (I say that without having even read the
background, or knowing what the right handling would be).
But...
> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..d522f43331 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,11 @@ 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)
> + die (_("Path leads inside submodule '%s', but the submodule "
> + "was not recognized, i.e. not initialized or deleted"),
> + ce->name);
Don't you need to flip the logic here? An assert() triggers when the
condition is not true, but an "if" does the opposite. So "assert(X)"
should always become "if (!X) die(...)".
-Peff
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCHv2] pathspec: give better message for submodule related pathspec error
2016-12-28 5:58 ` Jeff King
@ 2016-12-28 17:17 ` Stefan Beller
2016-12-28 18:15 ` Brandon Williams
0 siblings, 1 reply; 5+ messages in thread
From: Stefan Beller @ 2016-12-28 17:17 UTC (permalink / raw)
To: peff; +Cc: git, bmwill, Stefan Beller
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.
For now just improve the user visible error message.
[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
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Peff wrote:
> Don't you need to flip the logic here? An assert() triggers when the
> condition is not true, but an "if" does the opposite. So "assert(X)"
> should always become "if (!X) die(...)".
Duh! and it should compile as well.
Thanks,
Stefan
pathspec.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..4724d522f2 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,11 @@ 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)
+ die (_("Path leads inside submodule '%s', but the submodule "
+ "was not recognized, i.e. not initialized or deleted"),
+ item->original);
return magic;
}
--
2.11.0.196.gee862f456e.dirty
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCHv2] pathspec: give better message for submodule related pathspec error
2016-12-28 17:17 ` [PATCHv2] " Stefan Beller
@ 2016-12-28 18:15 ` Brandon Williams
0 siblings, 0 replies; 5+ messages in thread
From: Brandon Williams @ 2016-12-28 18:15 UTC (permalink / raw)
To: Stefan Beller; +Cc: peff, git
On 12/28, 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.
>
> For now just improve the user visible error message.
>
> [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
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Peff wrote:
> > Don't you need to flip the logic here? An assert() triggers when the
> > condition is not true, but an "if" does the opposite. So "assert(X)"
> > should always become "if (!X) die(...)".
>
> Duh! and it should compile as well.
>
> Thanks,
> Stefan
>
> pathspec.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..4724d522f2 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,11 @@ 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)
> + die (_("Path leads inside submodule '%s', but the submodule "
> + "was not recognized, i.e. not initialized or deleted"),
> + item->original);
> return magic;
> }
Turns out I should comment on the most recent version of the patch :P
This looks better to me. (It resolves the issue with using a variable
not in scope).
--
Brandon Williams
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pathspec: give better message for submodule related pathspec error
2016-12-28 0:05 [PATCH] pathspec: give better message for submodule related pathspec error Stefan Beller
2016-12-28 5:58 ` Jeff King
@ 2016-12-28 18:13 ` Brandon Williams
1 sibling, 0 replies; 5+ messages in thread
From: Brandon Williams @ 2016-12-28 18:13 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
On 12/27, 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.
>
> For now just improve the user visible error message.
>
> [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
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> If you were following the mailing list closely today, you may sense
> that I am cleaning up stalled branches. :)
>
> I think such a hot fix is warranted given how often we had reports
> on the mailing list.
>
> Thanks,
> Stefan
>
> pathspec.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..d522f43331 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,11 @@ 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)
> + die (_("Path leads inside submodule '%s', but the submodule "
> + "was not recognized, i.e. not initialized or deleted"),
> + ce->name);
> return magic;
I haven't been following everything on the list these past couple days,
but are we sure this is caused by submodules? Also variable 'ce'
shouldn't be in scope here.
--
Brandon Williams
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-12-28 18:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-28 0:05 [PATCH] pathspec: give better message for submodule related pathspec error Stefan Beller
2016-12-28 5:58 ` Jeff King
2016-12-28 17:17 ` [PATCHv2] " Stefan Beller
2016-12-28 18:15 ` Brandon Williams
2016-12-28 18:13 ` [PATCH] " Brandon Williams
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.