[darcs-devel] nolinks argument

Mon Jul 9 10:27:45 PDT 2007

Replying to a couple of threads regarding --nolinks here:

* Yes, I agree that copy_repo_patches is probably the important place
to implement the --nolinks decision.

* I would prefer not splitting out the uses and having two different
copying routines at the base level though (although I don't feel
strongly about this).  My reasons:

   + I think applying --nolinks to cases where it's not needed
(temporary fetches) is not harmful in any way except a (small?)
performance penalty.  In fact, using --nolinks implies that the user
is less concerned about performance than other issues (as long
as the performance reduction isn't egregious).

   + I dislike having two ways of doing things.  Sometimes it's
appropriate, but I'm not sure that the distinction is strong enough
in this case to justify separate functions.  The disadvantage of
two different ways is that the programmer has to choose (if they
are even aware that there's a choice).

    + It still requires passing the [DarcsFlag] options down through
all the intervening code to tell the link-possible code what to do.
You're analysis is correct: most of the changes just involved getting
[DarcsFlag] passed down deep enough, which caused the "splatter" effect
you noted below.  Perhaps there would be less splattering with two
different functions... if folks feel strongly enough I can investigate
this, but I couldn't tell you off the top of my head.

    + Having two different functions would require a more intrusive
change in the darcs internals to implement... providing me a much
greater opportunity to break things.  :-)

* Regarding your auto-detection thread: I could see usefulness in only
making links for files you own, but it still wouldn't solve the
dev/release policy scenario I described, and I think there's still
usefulness in allowing links to other people's repos. It might be useful
though, either changing the --nolinks flag to --links= {yes,owner,no}
and/or some sort of darcs prefs or environment variable.  If there's
enough interest, I could look into implementing this but I'd like more
feedback on how people would like it to be indicated, and I'd like to
implement it as a separate, additional patch following the --nolinks
patches (assuming they are accepted).

* Julius: sorry to be over exhuberant in defense of --nolinks, and yes
you are quite right to call me on the ill-expressed bad Bob
scenario.  :-)

* Eric, I think you are also correct in that only darcs get is actually
affected by --nolinks.  This seems to be because push/pull will read
the patches into memory, perform various commutation analysis and
whatnot, and then write them as new files to the output repository.  I
wasn't 100% sure on this analysis, and decided to define --nolinks as
valid arguments for push/pull (a) just in case, (b) because darcs
users are unlikely to know these internals to know if linking applies,
so better to be consistent from the user perspective, and (c) this
seems like a good candidate for eventual performance enhancement
(getting push/pull to actually do links rather than rewriting files).

OK, enough blathering on my part.  Please let me know if you'd like me
to refactor --nolinks to use two separate low-level copy routines, and
also if (and how) you'd like me to look at linking only owner files (or
anything else you'd like me to change for this patch).

Regards,
  Kevin

On Mon, 9 Jul 2007 00:57:47 +0200, "Eric Y. Kow"
<eric.kow at gmail.com> wrote:

> Hi Kevin, all
>
> > Mon Jun 11 21:15:16 MST 2007  Kevin Quick <quick at sparq.org>
> >   * Provide [DarcsFlag] command-line options to copyLocal
> >
> > Wed Jun 13 12:37:42 MST 2007  Kevin Quick <quick at sparq.org>
> >   * Added --nolinks option to request actual copies instead of hard-links for files.
>
> I was a :wq away from accepting this patch, when something occurred to
> me.  The feature does seem useful, and I am just wondering if we could
> implement it with a somewhat lighter touch.
>
> If I'm reading the code right, the hard-link code is used for at least
> two things:
>
>  * fetchFile/gzFetchFile - links/copy/downloads a file into a temporary
>      directory
>
>  * copy_repo_patches - copying the patch files
>
> Now, would you agree that only the latter is important?  (Anyway,
> linking here seems unlikely for the former, as /tmp is usually on a
> separate partition).  If that is the case, maybe we could get the same
> benefits by having two versions of copyFileOrUrl, one of which supports
> NoLinks.
>
> The patch Kevin made is pretty straightforward stuff, just modify
> copyLocal to track [DarcsFlag].  The only annoying thing is that as a
> consequence, a lot of unrelated functions (see below) are getting
> splattered with the extra argument by their association with copyLocal
>
> Perhaps having *two* copyFileOrUrl functions would be a lesser evil than
> splattering all those poor little functions (*) What do you (all) think?
>
> Also, having done some small tests and ls -i _darcs/patches, I get the
> impression that this flag would only be relevant for darcs get.  Please
> feel correct me on this!  I haven't stared hard enough at the code to
> say so for sure.
>
> (*) To be honest, my hesitation wrt splattering is basically visceral
>
> Splattered functions
> --------------------
> If my grepping is right...
>
> copyFileOrUrl
> copyLocal
> createPartialsPristineDirectoryTree
> createPristineDirectoryTree
> createPristineDirectoryTree
> fetchFilePS
> fetchFileUsingCache
> get_checkpoint_sloppily
> get_whole_repo_patches
> gzFetchFilePS
> identifyRepoFormat
> lazily_read_repo
> optimizeInventory
> read_checkpoints
> read_checkpoints
> read_repo
> read_repo_private
> remove_from_checkpoint_inventory
> tags_cmd
> withRecorded
> write_checkpoint
> write_checkpoint_patch
> write_either_inventory
> write_inventory
> write_recorded_checkpoint
> write_tentative_inventory
>
> --
> Eric Kow                     http://www.loria.fr/~kow
> PGP Key ID: 08AC04F9         Merci de corriger mon français.
>

--
--
Kevin Quick
quick at org after sparq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.osuosl.org/pipermail/darcs-devel/attachments/20070709/6e7660bb/attachment-0001.pgp