Patchwork [1,of,4,mergedriver] debugmergestate: explain why we create mergestate objects directly

login
register
mail settings
Submitter Siddharth Agarwal
Date Nov. 18, 2015, 6:41 a.m.
Message ID <2869fd1811666940c049.1447828896@dev666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/11468/
State Accepted
Delegated to: Martin von Zweigbergk
Headers show

Comments

Siddharth Agarwal - Nov. 18, 2015, 6:41 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1447798950 28800
#      Tue Nov 17 14:22:30 2015 -0800
# Node ID 2869fd1811666940c049c171ee074c0020e776eb
# Parent  9be8f04c78c211b27b05370e469c883d2f5a8568
# Available At http://42.netv6.net/sid0-wip/hg/
#              hg pull http://42.netv6.net/sid0-wip/hg/ -r 2869fd181166
debugmergestate: explain why we create mergestate objects directly

This shouldn't generally be done, but in this case it's fine because
debugmergestate needs to be privy to mergestate internals anyway.
Martin von Zweigbergk - Nov. 18, 2015, 6:56 a.m.
On Tue, Nov 17, 2015 at 10:42 PM Siddharth Agarwal <sid0@fb.com> wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1447798950 28800
> #      Tue Nov 17 14:22:30 2015 -0800
> # Node ID 2869fd1811666940c049c171ee074c0020e776eb
> # Parent  9be8f04c78c211b27b05370e469c883d2f5a8568
> # Available At http://42.netv6.net/sid0-wip/hg/
> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r 2869fd181166
> debugmergestate: explain why we create mergestate objects directly
>
> This shouldn't generally be done, but in this case it's fine because
> debugmergestate needs to be privy to mergestate internals anyway.
>

This explains why it's okay to do it, but why is it not okay to call read()
instead? I guess read() is the alternative here that you want to avoid. The
comment in the patch body explains even less about the reason for avoiding
it. I don't mind fixing up in flight if you provide a text


>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -2556,6 +2556,8 @@ def debugmergestate(ui, repo, *args):
>                  ui.write(('unrecognized entry: %s\t%s\n')
>                           % (rtype, record.replace('\0', '\t')))
>
> +    # avoid parsing the merge state -- we shouldn't be doing this, but
> this is
> +    # OK since this command is pretty low-level
>      ms = mergemod.mergestate(repo)
>
>      # sort so that reasonable information is on top
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Siddharth Agarwal - Nov. 18, 2015, 6:07 p.m.
On 11/17/15 22:56, Martin von Zweigbergk wrote:
> This explains why it's okay to do it, but why is it not okay to call 
> read() instead? I guess read() is the alternative here that you want 
> to avoid. The comment in the patch body explains even less about the 
> reason for avoiding it. I don't mind fixing up in flight if you 
> provide a text

Yeah, sorry, meant to add that before sending but forgot.

How about this for text?

We would normally use the read() constructur, but in this case it's fine
because
- we implement our own reading layer, so the extra parsing done by
   read() is unnecessary
- read() can throw an exception for unsupported merge state records,
   but here we'd like to handle that separately
- debugmergestate needs to be privy to mergestate internals anyway
Martin von Zweigbergk - Nov. 18, 2015, 6:38 p.m.
On Wed, Nov 18, 2015 at 10:07 AM Siddharth Agarwal <sid@less-broken.com>
wrote:

> On 11/17/15 22:56, Martin von Zweigbergk wrote:
> > This explains why it's okay to do it, but why is it not okay to call
> > read() instead? I guess read() is the alternative here that you want
> > to avoid. The comment in the patch body explains even less about the
> > reason for avoiding it. I don't mind fixing up in flight if you
> > provide a text
>
> Yeah, sorry, meant to add that before sending but forgot.
>
> How about this for text?
>

Sounds good. I also updated the code comment to explain why read() would be
bad.


>
> We would normally use the read() constructur, but in this case it's fine
> because
> - we implement our own reading layer, so the extra parsing done by
>    read() is unnecessary
> - read() can throw an exception for unsupported merge state records,
>    but here we'd like to handle that separately
> - debugmergestate needs to be privy to mergestate internals anyway
>

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -2556,6 +2556,8 @@  def debugmergestate(ui, repo, *args):
                 ui.write(('unrecognized entry: %s\t%s\n')
                          % (rtype, record.replace('\0', '\t')))
 
+    # avoid parsing the merge state -- we shouldn't be doing this, but this is
+    # OK since this command is pretty low-level
     ms = mergemod.mergestate(repo)
 
     # sort so that reasonable information is on top