Re: [PATCH 1/7] PR gdb/15224 "set history filename" to by immediately converted to absolute path

Pedro Alves
2013-05-08T15:46:58Z

On 05/07/2013 01:43 PM, mbilal wrote:
> please find updated patch .
>
> I made new 'set_history_filename' function to solve this problem

Please aim at posting self contained descriptions of patches, or
at least linking to them.

I happen to still recall the original issue, but I bet there are others
who don't. It's also very useful for future archaeology to find
the rationale for the patch close to the patch. Here's an example,
synthesized from <http://sourceware.org/ml/gdb-patches/2013-04/msg00066.html>:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
GDB resolves a relative .gdb_history path early on at init time, meaning,
the default history file written is the same that was
read, while "set history filename .gdb_history" resolves to
different history files at read and at write times if the user
changes directory in between, as seen in these examples:

$ unset GDBHISTFILE; gdb -ex "set history filename .gdb_history"
(gdb) show history filename
The filename in which to record the command history is "/tmp/.gdb_history".
(gdb) q

vs

$ cd /tmp
$ mkdir a
$ gdb -ex "set history filename .gdbhist"
(gdb) show history filename
The filename in which to record the command history is ".gdbhist".
(gdb) cd a
Working directory /tmp/a.
(gdb) q
$ cat a/.gdbhist
show history filename
cd a
q
$ ls .gdbhist a/.gdbhist
ls: cannot access .gdbhist: No such file or directory
a/.gdbhist
$
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Bonus points if that ends up in the commit log.

IIRC, a previous version had a test; what happened to it?
Did it cover the issue this patch proposes fixing?

> diff --git a/gdb/top.c b/gdb/top.c
> index 480b67e..20aecc9 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -48,6 +48,7 @@
> #include "interps.h"
> #include "observer.h"
> #include "maint.h"
> +#include "filenames.h"
>
> /* readline include files. */
> #include "readline/readline.h"
> @@ -1607,6 +1608,14 @@ set_verbose (char *args, int from_tty, struct cmd_list_element *c)
> }
> }
>
> +static void
> +set_history_filename (char *args, int from_tty, struct cmd_list_element *c)
> +{
> + if (!IS_ABSOLUTE_PATH (*(char **) c->var))
> + *(char **) c->var = concat (current_directory, "/", *(char **) c->var,
> + (char *)NULL);
> +}

Missing space before NULL. That's a lot of casting. We can just refer
to history_filename directly. It'd be very good to have a comment here with the
rationale for this -- there's one in init_history we can reuse. So:

if (!IS_ABSOLUTE_PATH (history_filename))
{
/* We include the current directory so that if the user changes
directories the file written will be the same as the one
that was read. */
history_filename = concat (current_directory, "/", history_filename,
(char *) NULL);
}

Also, a ChangeLog entry is missing.

Thanks,
--
Pedro Alves