Patchwork record: --user/-u now works with record when ui.username not set (issue3857)

# HG changeset patch
# User Prasoon Shukla <prasoon92.iitr@gmail.com>
# Date 1386831356 -19800
#      Thu Dec 12 12:25:56 2013 +0530
# Node ID 4dd9140ab488f45a5ddc832f26f1f482eebc6cba
# Parent  1c92524c37cdd251c1a36b2da0fb4148b0e6ba09
Collapsed revision
* record: --user/-u now works with record when ui.username not set (issue3857)

The -u flag didn't work when ui.username was not set and resulted in an
abort message. This was fixed.
* merge: updated the patch to not overwrite config, changed the test

The previous version of the patch relied on overwriting the config to
set the username. That was changed. Also, an error with the test was
fixed and another test was added that explicitly checks the username of
the last commit.

On 12/11/2013 11:26 PM, Prasoon Shukla wrote:
> # HG changeset patch
> # User Prasoon Shukla <prasoon92.iitr@gmail.com>
> # Date 1386831356 -19800
> #      Thu Dec 12 12:25:56 2013 +0530
> # Node ID 4dd9140ab488f45a5ddc832f26f1f482eebc6cba
> # Parent  1c92524c37cdd251c1a36b2da0fb4148b0e6ba09
> Collapsed revision
> * record: --user/-u now works with record when ui.username not set (issue3857)
>
> The -u flag didn't work when ui.username was not set and resulted in an
> abort message. This was fixed.
> * merge: updated the patch to not overwrite config, changed the test
>
> The previous version of the patch relied on overwriting the config to
> set the username. That was changed. Also, an error with the test was
> fixed and another test was added that explicitly checks the username of
> the last commit.
>
> diff -r 1c92524c37cd -r 4dd9140ab488 hgext/record.py
> --- a/hgext/record.py	Sun Dec 01 21:24:48 2013 -0600
> +++ b/hgext/record.py	Thu Dec 12 12:25:56 2013 +0530
> @@ -502,7 +502,8 @@
>                            cmdsuggest)
>
>       # make sure username is set before going interactive
> -    ui.username()
> +    if not opts.get('user'):
> +        ui.username() # raise exception, username not provided

The patch itself look pretty good but you need to fix your description.

Please check this out: 
http://mercurial.selenic.com/wiki/ContributingChanges#Patch_descriptions

In particular:
- your summary line is wrong
- The description should not contains reference to the previous version 
of you patch. the previous version will not end up in history.

Consider using --flag V3 when sending the new version to make it clear 
that your new patches replaces the other ones.

>       def recordfunc(ui, repo, message, match, opts):
>           """This is generic record driver.
> diff -r 1c92524c37cd -r 4dd9140ab488 tests/test-record.t
> --- a/tests/test-record.t	Sun Dec 01 21:24:48 2013 -0600
> +++ b/tests/test-record.t	Thu Dec 12 12:25:56 2013 +0530
> @@ -1277,5 +1277,23 @@
>      c
>     +d
>
> +Test --user when ui.username not set
> +  $ unset HGUSER
> +  $ echo e >> subdir/f1
> +  $ hg record  --config ui.username= -d '8 0' --user xyz -m "user flag" <<EOF
> +  > y
> +  > y
> +  > EOF
> +  diff --git a/subdir/f1 b/subdir/f1
> +  1 hunks, 1 lines changed
> +  examine changes to 'subdir/f1'? [Ynesfdaq?]
> +  @@ -4,3 +4,4 @@
> +   b
> +   c
> +   d
> +  +e
> +  record this change to 'subdir/f1'? [Ynesfdaq?]
> +  $ hg log --template '{author|user}\n' -l 1
> +  xyz
>
>     $ cd ..

On 12/12/2013 12:12 AM, Pierre-Yves David wrote:
> On 12/11/2013 11:26 PM, Prasoon Shukla wrote:
>> # HG changeset patch
>> # User Prasoon Shukla <prasoon92.iitr@gmail.com>
>> # Date 1386831356 -19800
>> #      Thu Dec 12 12:25:56 2013 +0530
>> # Node ID 4dd9140ab488f45a5ddc832f26f1f482eebc6cba
>> # Parent  1c92524c37cdd251c1a36b2da0fb4148b0e6ba09
>> Collapsed revision
>> * record: --user/-u now works with record when ui.username not set 
>> (issue3857)
>>
>> The -u flag didn't work when ui.username was not set and resulted in an
>> abort message. This was fixed.
>> * merge: updated the patch to not overwrite config, changed the test
>>
>> The previous version of the patch relied on overwriting the config to
>> set the username. That was changed. Also, an error with the test was
>> fixed and another test was added that explicitly checks the username of
>> the last commit.
>>
>> diff -r 1c92524c37cd -r 4dd9140ab488 hgext/record.py
>> --- a/hgext/record.py    Sun Dec 01 21:24:48 2013 -0600
>> +++ b/hgext/record.py    Thu Dec 12 12:25:56 2013 +0530
>> @@ -502,7 +502,8 @@
>>                            cmdsuggest)
>>
>>       # make sure username is set before going interactive
>> -    ui.username()
>> +    if not opts.get('user'):
>> +        ui.username() # raise exception, username not provided
>
> The patch itself look pretty good but you need to fix your description.
>
> Please check this out: 
> http://mercurial.selenic.com/wiki/ContributingChanges#Patch_descriptions
>
> In particular:
> - your summary line is wrong
> - The description should not contains reference to the previous 
> version of you patch. the previous version will not end up in history.
>
> Consider using --flag V3 when sending the new version to make it clear 
> that your new patches replaces the other ones.
>
>>       def recordfunc(ui, repo, message, match, opts):
>>           """This is generic record driver.
>> diff -r 1c92524c37cd -r 4dd9140ab488 tests/test-record.t
>> --- a/tests/test-record.t    Sun Dec 01 21:24:48 2013 -0600
>> +++ b/tests/test-record.t    Thu Dec 12 12:25:56 2013 +0530
>> @@ -1277,5 +1277,23 @@
>>      c
>>     +d
>>
>> +Test --user when ui.username not set
>> +  $ unset HGUSER

Hum why do you need to unset HGUSER ? if you do, you may want to set it 
back avec your test case.

>> +  $ echo e >> subdir/f1
>> +  $ hg record  --config ui.username= -d '8 0' --user xyz -m "user 
>> flag" <<EOF
>> +  > y
>> +  > y
>> +  > EOF
>> +  diff --git a/subdir/f1 b/subdir/f1
>> +  1 hunks, 1 lines changed
>> +  examine changes to 'subdir/f1'? [Ynesfdaq?]
>> +  @@ -4,3 +4,4 @@
>> +   b
>> +   c
>> +   d
>> +  +e
>> +  record this change to 'subdir/f1'? [Ynesfdaq?]
>> +  $ hg log --template '{author|user}\n' -l 1
>> +  xyz

small snitch, the |user part is probably unneeded