| 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
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 >
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
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