Re: [PATCH] PR-10034 Bad space handling in `set remote exec-file' command.

[Abhijit Halder <abhijit.k.halder@gmail.com>]

On Wed, Oct 5, 2011 at 9:48 AM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> On Tue, Oct 4, 2011 at 9:20 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Abhijit" == Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>>
>> Abhijit> In the `set remote exec-file' command if we provide space at the end
>> Abhijit> of the file-name, the space is not being chopped off and being
>> Abhijit> considered as part of file-name. This behavior is inconsistent across
>> Abhijit> similar set commands like `set logging file' etc. My patch will fix
>> Abhijit> that problem. Please review this patch and put your comments.
>>
>> I can't tell if you re-posted the same patch or if it has changes.
>> Please:
>>
>> * If you are sending a ping, just send a ping, as a followup to the
>>  patch being pinged, so that it threads properly in a threading mail
>>  reader.
>>
>> * If you are sending a new patch, again send it as a followup, and also
>>  indicate what changed and why.
>>
> Okay, I will follow this in future.
>>
>> This patch doesn't have a test case, but should.
>>
> Okay, I will write a test-case.
>> Abhijit>        case var_string_noescape:
>> Abhijit> -        if (arg == NULL)
>> Abhijit> -          arg = "";
>> Abhijit> -        if (*(char **) c->var != NULL)
>> Abhijit> -          xfree (*(char **) c->var);
>> Abhijit> -        *(char **) c->var = xstrdup (arg);
>> Abhijit> -        break;
>>
>> Why are you changing this case?
>>
> The idea was that the same code block is common between two adjacent
> cases and hence can be combined together.
>> Abhijit>        case var_optional_filename:
>> Abhijit>          if (arg == NULL)
>> Abhijit>            arg = "";
>> Abhijit> +        else
>> Abhijit> +          {
>> Abhijit> +            /* Clear trailing whitespace.  */
>> Abhijit> +            char *ptr = arg + strlen (arg) - 1;
>> Abhijit> +
>> Abhijit> +            while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
>> Abhijit> +              ptr--;
>> Abhijit> +            *(ptr + 1) = '\0';
>>
>> Why not use remove_trailing_whitespace?  You mentioned it in an earlier
>> note, it seems best to just use it from the start.
Since this same function has similar code block in several places used
for the purpose of removing trailing whiltespaces, I think now we
should go with the current approach of using this code block instead
of using remove_trailing_whitespace function. This may cause
confusion. Please comment on this.
>>
> Okay, I will do this change. I was just waiting for people's comment on this.
>>
>> What happens if the resulting string is empty?
>> At least for var_filename this should give an error.
>>
I did not make any change for var_filename in my patch. As in existing
code in the patch also the empty string will throw error.
> Okay.
>> Abhijit> @@ -419,7 +423,7 @@ cmd_show_list (struct cmd_list_element *
>> Abhijit>    for (; list != NULL; list = list->next)
>> Abhijit>      {
>> Abhijit>        /* If we find a prefix, run its list, prefixing our output by its
>> Abhijit> -         prefix (with "show " skipped).  */
>> Abhijit> +       prefix (with "show " skipped).  */
>> Abhijit>        if (list->prefixlist && !list->abbrev_flag)
>> Abhijit>        {
>> Abhijit>          struct cleanup *optionlist_chain
>>
>> This hunk looks like a gratuitous change.
>>
> This is a 8 space to tab conversion that we followed in rest of the
> code. If suggested I may revert back this change.
>>
>> Tom
>>
>
> Regards,
> Abhijit Halder
>