From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5826 invoked by alias); 8 May 2013 15:46:58 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 5814 invoked by uid 89); 8 May 2013 15:46:58 -0000 X-Spam-SWARE-Status: No, score=-8.4 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 08 May 2013 15:46:57 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r48Fkshg011192 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 8 May 2013 11:46:54 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r48FkrVs017266; Wed, 8 May 2013 11:46:53 -0400 Message-ID: <518A736C.8070009@redhat.com> Date: Wed, 08 May 2013 15:46:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: mbilal CC: gdb-patches@sourceware.org, jan.kratochvil@redhat.com Subject: Re: [PATCH 1/7] PR gdb/15224 "set history filename" to by immediately converted to absolute path References: <51877A32.1030503@codesourcery.com> <51877A99.4060503@codesourcery.com> <5188AA15.5010904@codesourcery.com> <5188F70A.1030908@codesourcery.com> In-Reply-To: <5188F70A.1030908@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00282.txt.bz2 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 : ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 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