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

[Pedro Alves <palves@redhat.com>]

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