Re: [PATCH v3] Improve gdb_tilde_expand logic

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-01-10 8:00 p.m., Lancelot SIX via Gdb-patches wrote:
> Before this patch, gdb_tilde_expand would use glob(3) in order to expand
> tilde at the begining of a path. This implementation has limitation when
> expanding a tilde leading path to a non existing file since glob fails to
> expand.
> 
> This patch proposes to use glob only to expand the tilde component of the
> path and leaves the rest of the path unchanged.
> 
> This patch is a followup to the following discution:
> https://sourceware.org/pipermail/gdb-patches/2021-January/174776.html
> 
> Before the patch:
> 
> 	gdb_tilde_expand("~") -> "/home/lsix"
> 	gdb_tilde_expand("~/a/c/b") -> error() is called
> 
> After the patch:
> 
> 	gdb_tilde_expand("~") -> "/home/lsix"
> 	gdb_tilde_expand("~/a/c/b") -> "/home/lsix/a/c/b"
> 
> Tested on x84_64 linux.
> 
> Since V1:
> 	* Unittests added thanks to inputs and skeleton provided by
> 	Simon Marchi.
> 	* My copyright assignment has been signed!
> 
> Since V2:
> 	* Fix coding style issuess and typos.
> 	* Use $HOME and $USER env variables to test the behavior of
> 	gdb_tilde_expand instead of testing the function against itself.
> 
> gdb/ChangeLog:
> 
> 	* Makefile.in (SELFTESTS_SRCS): Add
> 	unittests/gdb_tilde_expand-selftests.c.
> 	* unittests/gdb_tilde_expand-selftests.c: New file.
> 
> gdbsupport/ChangeLog:
> 
> 	* gdb_tilde_expand.cc (gdb_tilde_expand): Improve
> 	implementation.
> 	(gdb_tilde_expand_up): Delegate logic to gdb_tilde_expand.
> 	* gdb_tilde_expand.h (gdb_tilde_expand): Update description.
> ---
>  gdb/Makefile.in                            |  1 +
>  gdb/unittests/gdb_tilde_expand-selftests.c | 78 ++++++++++++++++++++++
>  gdbsupport/gdb_tilde_expand.cc             | 39 +++++++----
>  gdbsupport/gdb_tilde_expand.h              |  3 +-
>  4 files changed, 107 insertions(+), 14 deletions(-)
>  create mode 100644 gdb/unittests/gdb_tilde_expand-selftests.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 9267bea7be..c8979fe276 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -446,6 +446,7 @@ SELFTESTS_SRCS = \
>  	unittests/filtered_iterator-selftests.c \
>  	unittests/format_pieces-selftests.c \
>  	unittests/function-view-selftests.c \
> +	unittests/gdb_tilde_expand-selftests.c \
>  	unittests/gmp-utils-selftests.c \
>  	unittests/lookup_name_info-selftests.c \
>  	unittests/memory-map-selftests.c \
> diff --git a/gdb/unittests/gdb_tilde_expand-selftests.c b/gdb/unittests/gdb_tilde_expand-selftests.c
> new file mode 100644
> index 0000000000..d64b7c6384
> --- /dev/null
> +++ b/gdb/unittests/gdb_tilde_expand-selftests.c
> @@ -0,0 +1,78 @@
> +/* Self tests for gdb_tilde_expand
> +
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "gdbsupport/common-defs.h"
> +#include "gdbsupport/selftest.h"
> +#include <cstdlib>
> +
> +#include "gdbsupport/gdb_tilde_expand.h"
> +
> +namespace selftests {
> +namespace gdb_tilde_expand_tests {
> +
> +static void
> +do_test ()
> +{
> +  /* Home directory of the user running the test.  */
> +  const char *c_home = std::getenv ("HOME");
> +
> +  /* Skip the test if $HOME is not available in the environment.  */
> +  if (c_home == nullptr)
> +    return;
> +
> +  const std::string home (c_home);
> +
> +  /* Basic situation.  */
> +  SELF_CHECK (home == gdb_tilde_expand ("~"));
> +
> +  /* When given a path that begins by a tilde and refers to a file that
> +     does not exist, gdb_tilde expand must still be able to do the tilde
> +     expansion.  */
> +  SELF_CHECK (gdb_tilde_expand ("~/non/existent/directory")
> +              == home + "/non/existent/directory");
> +
> +  /* gdb_tilde_expand only expands tilde and does not try to do any other
> +     substitution.  */
> +  SELF_CHECK (gdb_tilde_expand ("~/*/a.out") == home + "/*/a.out");
> +
> +  /* gdb_tilde_expand does no modification to a non tilde leading path.  */
> +  SELF_CHECK (gdb_tilde_expand ("/some/abs/path") == "/some/abs/path");
> +  SELF_CHECK (gdb_tilde_expand ("relative/path") == "relative/path");
> +
> +  /* If $USER is available in the env variables, Check the '~user'
> +     expansion, otherwise skip the rest of the test.  */
> +  const char *c_user = std::getenv ("USER");
> +  if (c_user == nullptr)
> +    return;
> +
> +  const std::string user (c_user);
> +  SELF_CHECK (gdb_tilde_expand (("~" + user).c_str ()) == home);
> +  SELF_CHECK (gdb_tilde_expand (("~" + user + "/a/b").c_str ()) == home + "/a/b");

Thanks for the extensive testing.

One more thing we could test is a tilde with an unknown user after it
(gdb_tilde_expand ("~some-non-existent-user/foo/bar")).  The current behavior is
that an error is thrown by glob, so we could assert that an error is thrown.

> +  /* Look for the first dir separator (if any) and split d around it.  */
> +  const auto first_sep
> +    = std::find_if (d.cbegin (), d.cend(),
> +                    [](const char c) -> bool { return IS_DIR_SEPARATOR (c); });

We'd typically put a space before the parenthesis here, like we put one when defining
a standard function.

Simon