[PATCH 0 of 1] --graph option for in/out/log (was: gin/gout ...)
dirkjan at ochtman.nl
Thu Nov 27 06:32:17 CST 2008
On Thu, Nov 27, 2008 at 13:17, Alpár Jüttner <alpar at cs.elte.hu> wrote:
>> 2. incoming --graph won't work with remote repositories (ssh or http).
> Strange. It works fine for me.
Yes, it turns out it wasn't as bad as I thought. Since we mostly use
"other" to refer to a remote repository directly, I got a bit confused
and didn't notice that you redefined it to be a bundlerepo. Still,
you're referencing bundlerepo.dirstate, which doesn't make a whole lot
of sense. I'm contemplating the patch at
to make this more obvious.
> Yes, this was asked before by Peter. My point was however, that
> graphabledag()s are very short function, it is quite embedded into the
> parent functions (use their variables, which should be explicitly passed
> if we wanted to extract it.), and a future extension would require
> different versions of graphabledag() anyway.
Explicitly passing is good, because it makes dependencies clearer.
Having less code duplication makes code much easier to maintain. I
have a patch for this at
but I won't push it if there aren't any tests (and it needs a fixup
for bundlerepo.dirstate). For future extensions, making a separate
function makes it easier to distinguish anyway (and it's not really a
good enough reason to leave bad code now).
>> 4. The coding style is pretty broken (a=b assignment, lots of new underscores).
> I'm sorry, I do not really understand what do you mean. Could you point
> out some examples of them, so that I can fix?
Please review http://www.selenic.com/mercurial/wiki/index.cgi/BasicCodingStyle.
You defined functions like check_unsupported_flags() and
incoming_revs(). You also have lines like
"revdict[repo.changectx(n).rev()]=True". While we aren't always very
strict about the underscores, not having the spaces around '=' is kind
of bad. Also, repo.changectx(n) should be written as repo[n] in all
>> 5. Use of dict where a set would be better/more intuitive.
> Yes, you are right. Here again, the above future extension was in my
> mind when I chose dict. Namely, I want to implement an option to also
> show the base changesets of the incoming/outgoing ones, using e.g. 'X'
> in place of 'o' or '@'. So, I would use this dict to indicate the status
> of the changesets.
That's somewhat reasonable, but I think it's still better to make it a
set for now. You could have added a comment saying why it was a dict.
Sorry if all this sounds nit-picky, but I just recently spent a lot of
time streamlining this code, so seeing all these things introduced
makes me a little grumpy.
Thanks for working on this!
More information about the Mercurial-devel