New integration branch

[Adam Spiers]

On Sun, Dec 4, 2011 at 9:11 PM, Joey Hess <joey at kitenet.net> wrote:
> 37617a63ec993b128f77a945a2020ec894c58eb1
>        loadconfig already uses %loaded to avoid reloading the same
>        config twice, so this extra check is not necessary, I think.

Ah yes, I missed that.  Still, for the cost of an extra line of code,
isn't it worth being explicit?  If it confused me, then presumably it
could confuse other developers in the future.

> a61c1450ff4b108af26e899a89a1d8ff49cab86c
>        I picked the bugfix part.
>
>        The warning message on missing chain files exposes an unclear
>        thing in mr; it will try to chain to directories even when their
>        repository has skip = true, which causes the warning to show
>        up unexpectedly (ie, here). I think it needs to be changed to
>        honor skip = true even if chain = true.

Ah OK, I'll look into that.

> b3b68137988e61be1a0f7d90caf05eabf7850f44
>        I developed a different fix this morning that shows correct
>        line numbers for both the mrconfig and the position in the
>        include, it's in my tree.

Yep, I saw that - very nice :)

> 135e0076c9a93cd0556b9b25ff355ad25546a78c
>        This makes "mr fetch" do a git fetch, but nothing for
>        the over DVCSes which can also do things like fetch, and
>        no documentation of it

I can add documentation.  However, although ISTR that `hg pull' is
akin to `git fetch', I don't know how it's done for the other
decentralised VCS.

> 9c87f2352214175de307efedb8fd93889a26afbc
>        Can you give an example of when this is needed?

I can't remember but I definitely saw it happen at least once :-/

> 602f26714254f3c65389b7665d15d1d5d0e227a9
>        mr is quite typically (I know, not by you) run
>        inside the repository. Which would leave the user
>        in an apparently empty directory after mr update if
>        an mr update deleted and remade the whole repository.
>
>        I don't like that; I don't think things in mr should be
>        deleting repositories in general; mr doesn't even delete
>        a repo that has deleted = true, it only warns the user about it.

Hmm, that's a fair point, although the only alternative is to change
the contents of the directory rather than the directory itself -
similarly to how `git checkout' does, for instance.  I'll see if I can
get around to doing that.  Perhaps some of the effort could be reused
for implementing download_diff (diff against the archive file).

> 650620d7b6661f9cc59b4adfb6a7d945240fe5c7
> f16e51cea8595afc92f3ab9230e3c5a44baed904
>        I've held off on these plugins since I think they
>        depend on 602f26714254f3c65389b7665d15d1d5d0e227a9

No, only the download plugin.  The stow plugin *never* writes to the
repository tree, it manages symlinks in an entirely different place
(typically $HOME).  Having said that, I just remembered that the stow
plugin depends on MR_NAME so that will have to wait too.

> cf3388f443b9d7afe6dc7d8a2159b45fb01ab4e4
>        This is a slow way to make machine-parsable info available --
>        the similar mr list takes 8 seconds here, since it has to run
>        169 shells. That's ok when you're just running mr, but I would
>        not like to use a command that depended on that information.

Sure, that's why I used a simple on-disk cache:

  https://github.com/aspiers/kitenet-mr/commit/b60acb2e767b91ca6d406198d7eea1b3f73ad2bf

It works fine.  I could get more sophisticated and allow per-user
configuration of the cache invalidation strategy, e.g. so that it
would automatically rebuild the cache when ~/.mrconfig et al. are
changed, but manual rebuilds aren't a great hardship.  In fact I could
even rebuild the cache every time mr runs!

>        If a machine-parseable list of repositories is needed,
>        I think it'd be better to have a perl function that emits
>        it in one go.

I don't see how that's possible without ignoring the `skip',
`deleted', and `include' parameters.

>        (Also, the patch references a MR_NAME that is not present in my
>        mr tree.)

Yeah :-(  I had to rebase about 20 times to separate out the patches
you are currently willing to consider, so mistakes were inevitable.

> 4cd2b59d0c66d71316dfc1d411a3e3da439643bc
>        I'm not quite sure of the point of this refactoring,

Legibility and modularity.  Longer functions makes unfamiliar code
harder to understand, and often give variables an unnecessarily wide
scope.  If I see this line of code within bootstrap():

    my $tmpconfig = download($url);

then

  (a) it's immediately obvious that $url contains config which is
      being downloaded and stored in a temporary file, so there's no
      need for the existing comment "Download the config file to a
      temporary location",

  (b) it makes it easy _and_optional_ for me to view how the download
      is implemented, and

  (c) it means that the scope of @curlargs and $curlstatus are clearly
      limited to the download, so I don't need to visually grep for
      them in bootstrap() post-download to check whether any of the
      subsequent code depends on them.

>        since the factored out download function has a lot
>        of bootstrap-specific stuff in it?

You mean the references to "bootstrap" in the die lines?  In any case,
this function could be reused with a minimal amount of refactoring in
necessary, but on this occasion, promoting potential code reuse was
less of a motivation than legibility and modularity.

> a64e990a37ceb5ce2b200645ebc0aabe67d3626e
>        cherry-picked
>
> aa3caf53a9cb35ee3d0e4173ed44e964c6b8b5ab
>        cherry-picked. Nice feature!

Glad you like it :)

Thanks for the review.  I'll have to go away now and do a whole load
more rebases and patch editing - I'll let you know when the new
for_joey branch is ready for another review.

Adam