Re: [PATCHv1 1/1] git-p4: better error reporting when p4 fails

2018-06-05 Thread Eric Sunshine
On Tue, Jun 5, 2018 at 6:59 AM Luke Diamand  wrote:
> On 5 June 2018 at 11:49, Merland Romain  wrote:
> >> +# now check that we can actually talk to the server
> >> +global p4_access_checked
> >> +if not p4_access_checked:
> >> +p4_access_checked = True
> >> +p4_check_access()
> >> +
> >
> > Just switch the 2 lines 'p4_access_checked = True' and 'p4_check_access()'
> > It seems to me more logical
>
> Like this:
>
> +p4_check_access()
> +p4_access_checked = True
>
> You need to set p4_access_checked first so that it doesn't go and try
> to check the p4 access before running "p4 login -s", which would then
> get stuck forever.

Such subtlety may deserve an in-code comment.


Re: [PATCHv1 1/1] git-p4: better error reporting when p4 fails

2018-06-05 Thread Luke Diamand
On 5 June 2018 at 11:49, Merland Romain  wrote:
>
>> @@ -91,6 +93,13 @@ def p4_build_cmd(cmd):
>> real_cmd = ' '.join(real_cmd) + ' ' + cmd
>> else:
>> real_cmd += cmd
>> +
>> +# now check that we can actually talk to the server
>> +global p4_access_checked
>> +if not p4_access_checked:
>> +p4_access_checked = True
>> +p4_check_access()
>> +
>
> Just switch the 2 lines 'p4_access_checked = True' and 'p4_check_access()'
> It seems to me more logical

Like this:

+p4_check_access()
+p4_access_checked = True

You need to set p4_access_checked first so that it doesn't go and try
to check the p4 access before running "p4 login -s", which would then
get stuck forever.

>
> Romain