[PATCH 0 of 1] --graph option for in/out/log (was: gin/gout ...)

Dirkjan Ochtman 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
http://hg.xavamedia.nl/mercurial/djc.mq/file/2a8ac3d8cfe5/bundle-dirstate#l1
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
http://hg.xavamedia.nl/mercurial/djc.mq/file/2a8ac3d8cfe5/graphdag#l1,
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
new code.

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

Cheers,

Dirkjan



More information about the Mercurial-devel mailing list