Patchwork [1,of,2,v3] context: stop setting None for modified or added nodes

login
register
mail settings
Submitter Augie Fackler
Date Dec. 15, 2014, 7:14 p.m.
Message ID <b7772da447df306ddfad.1418670865@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/7118/
State Superseded
Delegated to: Pierre-Yves David
Headers show

Comments

Augie Fackler - Dec. 15, 2014, 7:14 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1418416194 18000
#      Fri Dec 12 15:29:54 2014 -0500
# Node ID b7772da447df306ddfad854d7acd6b72bb5d5534
# Parent  65c854f92d6ba8861414ff3191182fba28777a82
context: stop setting None for modified or added nodes

Instead use a modified nullid, so that we can identify modified or
added nodes correctly when using manifest.diff().

Thanks to Martin von Zweigbergk for catching that we have to update
_buildstatus as well. That part eluded my debugging for some time.
Pierre-Yves David - Dec. 17, 2014, 12:32 a.m.
On 12/15/2014 11:14 AM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1418416194 18000
> #      Fri Dec 12 15:29:54 2014 -0500
> # Node ID b7772da447df306ddfad854d7acd6b72bb5d5534
> # Parent  65c854f92d6ba8861414ff3191182fba28777a82
> context: stop setting None for modified or added nodes
>
> Instead use a modified nullid, so that we can identify modified or
> added nodes correctly when using manifest.diff().
>
> Thanks to Martin von Zweigbergk for catching that we have to update
> _buildstatus as well. That part eluded my debugging for some time.

So, as far as I understand that patch:

- None was a magic value (in _manifestmatch)
- None is used by something else for another magic value (in manifest.diff)
- Having them both equal creates issue, so we change the value of the 
_manifestmatch
- The content of the new magic value is absolution arbitrary.

If I got this write:

- We should improve the documentation around the initialization and 
usage of this new magic value.
- Using nullid confused me a bit, if you have no specific good semantic 
to use nullid we should probably use something else. (not sure what our 
constraint are)
Augie Fackler - Dec. 17, 2014, 1:20 a.m.
On Dec 16, 2014, at 7:32 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:

> 
> 
> On 12/15/2014 11:14 AM, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1418416194 18000
>> #      Fri Dec 12 15:29:54 2014 -0500
>> # Node ID b7772da447df306ddfad854d7acd6b72bb5d5534
>> # Parent  65c854f92d6ba8861414ff3191182fba28777a82
>> context: stop setting None for modified or added nodes
>> 
>> Instead use a modified nullid, so that we can identify modified or
>> added nodes correctly when using manifest.diff().
>> 
>> Thanks to Martin von Zweigbergk for catching that we have to update
>> _buildstatus as well. That part eluded my debugging for some time.
> 
> So, as far as I understand that patch:
> 
> - None was a magic value (in _manifestmatch)
> - None is used by something else for another magic value (in manifest.diff)
> - Having them both equal creates issue, so we change the value of the _manifestmatch
> - The content of the new magic value is absolution arbitrary.

Yup.

> 
> If I got this write:
> 
> - We should improve the documentation around the initialization and usage of this new magic value.

Probably.

> - Using nullid confused me a bit, if you have no specific good semantic to use nullid we should probably use something else. (not sure what our constraint are)

The constraints are “needs to look mostly like a nodeid, but not be a nodeid”. Other parts of the code use 21-byte strings (by +=ing a ‘a’ or ‘m’ or ‘+’ onto the string), so my work on moving the manifest into a C extension already allow for that, which made nullid+something appealing. Does that explain enough?

(I’ll wait on a resend until we talk this bit out.)

> 
> -- 
> Pierre-Yves David
Pierre-Yves David - Dec. 17, 2014, 1:34 a.m.
On 12/16/2014 05:20 PM, Augie Fackler wrote:
>
> On Dec 16, 2014, at 7:32 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>
>>
>>
>> On 12/15/2014 11:14 AM, Augie Fackler wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <augie@google.com>
>>> # Date 1418416194 18000
>>> #      Fri Dec 12 15:29:54 2014 -0500
>>> # Node ID b7772da447df306ddfad854d7acd6b72bb5d5534
>>> # Parent  65c854f92d6ba8861414ff3191182fba28777a82
>>> context: stop setting None for modified or added nodes
>>>
>>> Instead use a modified nullid, so that we can identify modified or
>>> added nodes correctly when using manifest.diff().
>>>
>>> Thanks to Martin von Zweigbergk for catching that we have to update
>>> _buildstatus as well. That part eluded my debugging for some time.
>>
>> So, as far as I understand that patch:
>>
>> - None was a magic value (in _manifestmatch)
>> - None is used by something else for another magic value (in manifest.diff)
>> - Having them both equal creates issue, so we change the value of the _manifestmatch
>> - The content of the new magic value is absolution arbitrary.
>
> Yup.
>
>>
>> If I got this write:
>>
>> - We should improve the documentation around the initialization and usage of this new magic value.
>
> Probably.

Certainly then, think about the amount of people who lost time figuring 
out the 21th char in node.

>> - Using nullid confused me a bit, if you have no specific good semantic to use nullid we should probably use something else. (not sure what our constraint are)
>
> The constraints are “needs to look mostly like a nodeid, but not be a nodeid”. Other parts of the code use 21-byte strings (by +=ing a ‘a’ or ‘m’ or ‘+’ onto the string), so my work on moving the manifest into a C extension already allow for that, which made nullid+something appealing. Does that explain enough?
>
> (I’ll wait on a resend until we talk this bit out.)

would 21 "!" works ? or something that say "Iamamodifiedoradded!". Not 
sure if it more sensible but would carry the "magic value" meaning 
around well.
Augie Fackler - Dec. 17, 2014, 1:38 a.m.
On Dec 16, 2014, at 8:34 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:

> 
> 
> On 12/16/2014 05:20 PM, Augie Fackler wrote:
>> 
>> On Dec 16, 2014, at 7:32 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>> 
>>> 
>>> 
>>> On 12/15/2014 11:14 AM, Augie Fackler wrote:
>>>> # HG changeset patch
>>>> # User Augie Fackler <augie@google.com>
>>>> # Date 1418416194 18000
>>>> #      Fri Dec 12 15:29:54 2014 -0500
>>>> # Node ID b7772da447df306ddfad854d7acd6b72bb5d5534
>>>> # Parent  65c854f92d6ba8861414ff3191182fba28777a82
>>>> context: stop setting None for modified or added nodes
>>>> 
>>>> Instead use a modified nullid, so that we can identify modified or
>>>> added nodes correctly when using manifest.diff().
>>>> 
>>>> Thanks to Martin von Zweigbergk for catching that we have to update
>>>> _buildstatus as well. That part eluded my debugging for some time.
>>> 
>>> So, as far as I understand that patch:
>>> 
>>> - None was a magic value (in _manifestmatch)
>>> - None is used by something else for another magic value (in manifest.diff)
>>> - Having them both equal creates issue, so we change the value of the _manifestmatch
>>> - The content of the new magic value is absolution arbitrary.
>> 
>> Yup.
>> 
>>> 
>>> If I got this write:
>>> 
>>> - We should improve the documentation around the initialization and usage of this new magic value.
>> 
>> Probably.
> 
> Certainly then, think about the amount of people who lost time figuring out the 21th char in node.
> 
>>> - Using nullid confused me a bit, if you have no specific good semantic to use nullid we should probably use something else. (not sure what our constraint are)
>> 
>> The constraints are “needs to look mostly like a nodeid, but not be a nodeid”. Other parts of the code use 21-byte strings (by +=ing a ‘a’ or ‘m’ or ‘+’ onto the string), so my work on moving the manifest into a C extension already allow for that, which made nullid+something appealing. Does that explain enough?
>> 
>> (I’ll wait on a resend until we talk this bit out.)
> 
> would 21 "!" works ? or something that say "Iamamodifiedoradded!". Not sure if it more sensible but would carry the "magic value" meaning around well.

# Phony node value to stand-in for new files in some uses of
# manifests. Manifests support 21-byte hashes for nodes which are
# dirty in the working copy.
_newnode = '!' * 21

how’s that?

> 
> -- 
> Pierre-Yves David
Pierre-Yves David - Dec. 17, 2014, 1:38 a.m.
On 12/16/2014 05:38 PM, Augie Fackler wrote:
>
> On Dec 16, 2014, at 8:34 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>
>>
>>
>> On 12/16/2014 05:20 PM, Augie Fackler wrote:
>>>
>>> On Dec 16, 2014, at 7:32 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>>
>>>>
>>>>
>>>> On 12/15/2014 11:14 AM, Augie Fackler wrote:
>>>>> # HG changeset patch
>>>>> # User Augie Fackler <augie@google.com>
>>>>> # Date 1418416194 18000
>>>>> #      Fri Dec 12 15:29:54 2014 -0500
>>>>> # Node ID b7772da447df306ddfad854d7acd6b72bb5d5534
>>>>> # Parent  65c854f92d6ba8861414ff3191182fba28777a82
>>>>> context: stop setting None for modified or added nodes
>>>>>
>>>>> Instead use a modified nullid, so that we can identify modified or
>>>>> added nodes correctly when using manifest.diff().
>>>>>
>>>>> Thanks to Martin von Zweigbergk for catching that we have to update
>>>>> _buildstatus as well. That part eluded my debugging for some time.
>>>>
>>>> So, as far as I understand that patch:
>>>>
>>>> - None was a magic value (in _manifestmatch)
>>>> - None is used by something else for another magic value (in manifest.diff)
>>>> - Having them both equal creates issue, so we change the value of the _manifestmatch
>>>> - The content of the new magic value is absolution arbitrary.
>>>
>>> Yup.
>>>
>>>>
>>>> If I got this write:
>>>>
>>>> - We should improve the documentation around the initialization and usage of this new magic value.
>>>
>>> Probably.
>>
>> Certainly then, think about the amount of people who lost time figuring out the 21th char in node.
>>
>>>> - Using nullid confused me a bit, if you have no specific good semantic to use nullid we should probably use something else. (not sure what our constraint are)
>>>
>>> The constraints are “needs to look mostly like a nodeid, but not be a nodeid”. Other parts of the code use 21-byte strings (by +=ing a ‘a’ or ‘m’ or ‘+’ onto the string), so my work on moving the manifest into a C extension already allow for that, which made nullid+something appealing. Does that explain enough?
>>>
>>> (I’ll wait on a resend until we talk this bit out.)
>>
>> would 21 "!" works ? or something that say "Iamamodifiedoradded!". Not sure if it more sensible but would carry the "magic value" meaning around well.
>
> # Phony node value to stand-in for new files in some uses of
> # manifests. Manifests support 21-byte hashes for nodes which are
> # dirty in the working copy.
> _newnode = '!' * 21
>
> how’s that?

Okay, get that pushed to crew.
Augie Fackler - Dec. 17, 2014, 1:42 a.m.
On Dec 16, 2014, at 8:38 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:

> 
> 
> On 12/16/2014 05:38 PM, Augie Fackler wrote:
>> 
>> # Phony node value to stand-in for new files in some uses of
>> # manifests. Manifests support 21-byte hashes for nodes which are
>> # dirty in the working copy.
>> _newnode = '!' * 21
>> 
>> how’s that?
> 
> Okay, get that pushed to crew.

Given my track record with this series, I’m mailing a v4.

> 
> -- 
> Pierre-Yves David

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -17,6 +17,8 @@  import revlog
 
 propertycache = util.propertycache
 
+_newnode = nullid + '!'
+
 class basectx(object):
     """A basectx object represents the common logic for its children:
     changectx: read-only context that is already present in the repo,
@@ -104,7 +106,7 @@  class basectx(object):
                 if (fn not in deletedset and
                     ((fn in withflags and mf1.flags(fn) != mf2.flags(fn)) or
                      (mf1[fn] != mf2node and
-                      (mf2node or self[fn].cmp(other[fn]))))):
+                      (mf2node != _newnode or self[fn].cmp(other[fn]))))):
                     modified.append(fn)
                 elif listclean:
                     clean.append(fn)
@@ -1387,7 +1389,7 @@  class workingctx(committablectx):
         """
         mf = self._repo['.']._manifestmatches(match, s)
         for f in s.modified + s.added:
-            mf[f] = None
+            mf[f] = _newnode
             mf.setflag(f, self.flags(f))
         for f in s.removed:
             if f in mf: