Re: Dead code or bug in gpg_agent.c

2012-10-10 Thread Hyrum K Wright 
On Tue, Oct 9, 2012 at 4:34 AM, Daniel Shahaf d...@daniel.shahaf.name wrote:
 Hyrum K Wright wrote on Mon, Oct 08, 2012 at 21:51:02 -0400:
 Poking around subversion/libsvn_subr/gpg_agent.c I found this snippet
 (at around line 325):

 [[[
   /* Send DISPLAY to the gpg-agent daemon. */
   display = getenv(DISPLAY);
   if (display != NULL)
 {
   request = apr_psprintf(pool, OPTION display=%s\n, display);
   if (!send_option(sd, buffer, BUFFER_SIZE, display, display, pool))
 ...

 I don't know enough about what's going on here.  Is the first time the
 variable is set supposed to be combined with the second, or is it just
 a superfluous assignment which we can safely remove?


 Remove it.  The definition of send_option() recomputes REQUEST and sends
 it over the socket --- the copy here is not needed.

r1396534

-Hyrum

Re: Dead code or bug in gpg_agent.c

2012-10-09 Thread Daniel Shahaf 
Hyrum K Wright wrote on Mon, Oct 08, 2012 at 21:51:02 -0400:
 Poking around subversion/libsvn_subr/gpg_agent.c I found this snippet
 (at around line 325):
 
 [[[
   /* Send DISPLAY to the gpg-agent daemon. */
   display = getenv(DISPLAY);
   if (display != NULL)
 {
   request = apr_psprintf(pool, OPTION display=%s\n, display);
   if (!send_option(sd, buffer, BUFFER_SIZE, display, display, pool))
...
 
 I don't know enough about what's going on here.  Is the first time the
 variable is set supposed to be combined with the second, or is it just
 a superfluous assignment which we can safely remove?
 

Remove it.  The definition of send_option() recomputes REQUEST and sends
it over the socket --- the copy here is not needed.

 -Hyrum

Dead code or bug in gpg_agent.c

2012-10-08 Thread Hyrum K Wright 
Poking around subversion/libsvn_subr/gpg_agent.c I found this snippet
(at around line 325):

[[[
  /* Send DISPLAY to the gpg-agent daemon. */
  display = getenv(DISPLAY);
  if (display != NULL)
{
  request = apr_psprintf(pool, OPTION display=%s\n, display);
  if (!send_option(sd, buffer, BUFFER_SIZE, display, display, pool))
{
  close(sd);
  return SVN_NO_ERROR;
}
}
]]]

Among other things, it looks to be setting the request variable.
However, that variable is never read before it is reset a few lines
later:

[[[
  request = apr_psprintf(pool,
 GET_PASSPHRASE --data %s--repeat=1 
 %s X %s %s\n,
 non_interactive ? --no-ask  : ,
 cache_id,
 escape_blanks(password_prompt),
 escape_blanks(realm_prompt));
]]]

I don't know enough about what's going on here.  Is the first time the
variable is set supposed to be combined with the second, or is it just
a superfluous assignment which we can safely remove?

-Hyrum