Patchwork [4,of,7] patch.difffeatureopts: add a feature for diff.git

login
register
mail settings
Submitter Siddharth Agarwal
Date Nov. 22, 2014, 1 a.m.
Message ID <959a08bde79e880c8ce3.1416618006@devbig136.prn2.facebook.com>
Download mbox | patch
Permalink /patch/6825/
State Superseded
Commit 27af986a332bb1c01d16ad8a71e0072532f4bf78
Headers show

Comments

Siddharth Agarwal - Nov. 22, 2014, 1 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1416359414 28800
#      Tue Nov 18 17:10:14 2014 -0800
# Node ID 959a08bde79e880c8ce3ed638a5e4f242db6d1a0
# Parent  ec03d9f0198eafe8933924bbeadd2586866eb9f7
patch.difffeatureopts: add a feature for diff.git

This deserves to be its own feature -- indeed, this is often the only feature
several commands care about.
Augie Fackler - Nov. 25, 2014, 3:36 p.m.
On Fri, Nov 21, 2014 at 05:00:06PM -0800, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1416359414 28800
> #      Tue Nov 18 17:10:14 2014 -0800
> # Node ID 959a08bde79e880c8ce3ed638a5e4f242db6d1a0
> # Parent  ec03d9f0198eafe8933924bbeadd2586866eb9f7
> patch.difffeatureopts: add a feature for diff.git
>
> This deserves to be its own feature -- indeed, this is often the only feature
> several commands care about.
>
> diff --git mercurial/patch.py mercurial/patch.py
> --- mercurial/patch.py
> +++ mercurial/patch.py
> @@ -1559,11 +1559,12 @@
>      pass
>
>  def diffallopts(ui, opts=None, untrusted=False, section='diff'):
> -    return difffeatureopts(ui, opts=opts, untrusted=untrusted, section=section)
> +    return difffeatureopts(ui, opts=opts, untrusted=untrusted, section=section,
> +                           git=True)
>
>  diffopts = diffallopts
>
> -def difffeatureopts(ui, opts=None, untrusted=False, section='diff'):
> +def difffeatureopts(ui, opts=None, untrusted=False, section='diff', git=False):

This function is begging for a docstring that describes what the
various options mean. In this change it's still kind of guessable, but
a couple of patches later it becomes pretty subtle.

>      def get(key, name=None, getter=ui.configbool, forceplain=None):
>          if opts:
>              v = opts.get(key)
> @@ -1575,7 +1576,6 @@
>
>      buildopts = {
>          'text': opts and opts.get('text'),
> -        'git': get('git'),
>          'nodates': get('nodates'),
>          'nobinary': get('nobinary'),
>          'noprefix': get('noprefix', forceplain=False),
> @@ -1586,6 +1586,9 @@
>          'context': get('unified', getter=ui.config),
>      }
>
> +    if git:
> +        buildopts['git'] = get('git')
> +
>      return mdiff.diffopts(**buildopts)
>
>  def diff(repo, node1=None, node2=None, match=None, changes=None, opts=None,
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Siddharth Agarwal - Nov. 25, 2014, 6:39 p.m.
On 11/25/2014 07:36 AM, Augie Fackler wrote:
> On Fri, Nov 21, 2014 at 05:00:06PM -0800, Siddharth Agarwal wrote:
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0@fb.com>
>> # Date 1416359414 28800
>> #      Tue Nov 18 17:10:14 2014 -0800
>> # Node ID 959a08bde79e880c8ce3ed638a5e4f242db6d1a0
>> # Parent  ec03d9f0198eafe8933924bbeadd2586866eb9f7
>> patch.difffeatureopts: add a feature for diff.git
>>
>> This deserves to be its own feature -- indeed, this is often the only feature
>> several commands care about.
>>
>> diff --git mercurial/patch.py mercurial/patch.py
>> --- mercurial/patch.py
>> +++ mercurial/patch.py
>> @@ -1559,11 +1559,12 @@
>>       pass
>>
>>   def diffallopts(ui, opts=None, untrusted=False, section='diff'):
>> -    return difffeatureopts(ui, opts=opts, untrusted=untrusted, section=section)
>> +    return difffeatureopts(ui, opts=opts, untrusted=untrusted, section=section,
>> +                           git=True)
>>
>>   diffopts = diffallopts
>>
>> -def difffeatureopts(ui, opts=None, untrusted=False, section='diff'):
>> +def difffeatureopts(ui, opts=None, untrusted=False, section='diff', git=False):
> This function is begging for a docstring that describes what the
> various options mean. In this change it's still kind of guessable, but
> a couple of patches later it becomes pretty subtle.

Would you prefer a V2 or a followup?
Augie Fackler - Nov. 25, 2014, 6:50 p.m.
On Tue, Nov 25, 2014 at 1:39 PM, Siddharth Agarwal <sid@less-broken.com> wrote:
> On 11/25/2014 07:36 AM, Augie Fackler wrote:
>>
>> On Fri, Nov 21, 2014 at 05:00:06PM -0800, Siddharth Agarwal wrote:
>>>
>>> # HG changeset patch
>>> # User Siddharth Agarwal <sid0@fb.com>
>>> # Date 1416359414 28800
>>> #      Tue Nov 18 17:10:14 2014 -0800
>>> # Node ID 959a08bde79e880c8ce3ed638a5e4f242db6d1a0
>>> # Parent  ec03d9f0198eafe8933924bbeadd2586866eb9f7
>>> patch.difffeatureopts: add a feature for diff.git
>>>
>>> This deserves to be its own feature -- indeed, this is often the only
>>> feature
>>> several commands care about.
>>>
>>> diff --git mercurial/patch.py mercurial/patch.py
>>> --- mercurial/patch.py
>>> +++ mercurial/patch.py
>>> @@ -1559,11 +1559,12 @@
>>>       pass
>>>
>>>   def diffallopts(ui, opts=None, untrusted=False, section='diff'):
>>> -    return difffeatureopts(ui, opts=opts, untrusted=untrusted,
>>> section=section)
>>> +    return difffeatureopts(ui, opts=opts, untrusted=untrusted,
>>> section=section,
>>> +                           git=True)
>>>
>>>   diffopts = diffallopts
>>>
>>> -def difffeatureopts(ui, opts=None, untrusted=False, section='diff'):
>>> +def difffeatureopts(ui, opts=None, untrusted=False, section='diff',
>>> git=False):
>>
>> This function is begging for a docstring that describes what the
>> various options mean. In this change it's still kind of guessable, but
>> a couple of patches later it becomes pretty subtle.
>
>
> Would you prefer a V2 or a followup?


A v2 would be a little easier for me at this point.

Patch

diff --git mercurial/patch.py mercurial/patch.py
--- mercurial/patch.py
+++ mercurial/patch.py
@@ -1559,11 +1559,12 @@ 
     pass
 
 def diffallopts(ui, opts=None, untrusted=False, section='diff'):
-    return difffeatureopts(ui, opts=opts, untrusted=untrusted, section=section)
+    return difffeatureopts(ui, opts=opts, untrusted=untrusted, section=section,
+                           git=True)
 
 diffopts = diffallopts
 
-def difffeatureopts(ui, opts=None, untrusted=False, section='diff'):
+def difffeatureopts(ui, opts=None, untrusted=False, section='diff', git=False):
     def get(key, name=None, getter=ui.configbool, forceplain=None):
         if opts:
             v = opts.get(key)
@@ -1575,7 +1576,6 @@ 
 
     buildopts = {
         'text': opts and opts.get('text'),
-        'git': get('git'),
         'nodates': get('nodates'),
         'nobinary': get('nobinary'),
         'noprefix': get('noprefix', forceplain=False),
@@ -1586,6 +1586,9 @@ 
         'context': get('unified', getter=ui.config),
     }
 
+    if git:
+        buildopts['git'] = get('git')
+
     return mdiff.diffopts(**buildopts)
 
 def diff(repo, node1=None, node2=None, match=None, changes=None, opts=None,