obelisk icon indicating copy to clipboard operation
obelisk copied to clipboard

Add --preserve-symlinks option to thunk operations

Open lpsmith opened this issue 5 years ago • 1 comments

From the ob thunk pack --help documentation:

Usage: ob thunk pack THUNKDIRS... [--preserve-symlinks] [-f|--force]
                     ([--private] | [--public])
  Pack git checkout or unpacked thunk into thunk that points at the current
  branch's upstream

Available options:
  THUNKDIRS...             Paths to directories containing thunk data
  --preserve-symlinks      when a THUNKDIR is a symlink, overwrite the target of
                           the symlink, instead of overwriting the symlink
                           itself
  -f,--force               Force packing thunks even if there are branches not
                           pushed upstream, uncommitted changes, stashes. This
                           will cause changes that have not been pushed upstream
                           to be lost; use with care.
  --private                Mark thunks as pointing to a private repository
  --public                 Mark thunks as pointing to a public repository
  -h,--help                Show this help text

Though, it occurred to me this morning that there is another implementation with a different set of tradeoffs, namely, if a THUNKDIR is a symlink we resolve the symlink ourselves using the readlink(2) system call, which means we have to then check if the target is also a symlink and also detect symlink loops. Then we pass the resolved symlink as the target of the thunk operation. (Actually, in hindsight, one can get this behavior simply by ob thunk pack $(readlink -f THUNKDIR), though maybe this option is still worthwhile)

Advantages versus this MR as currently stands:

  • less impact on the implementation: we can do all of this inside runThunkCommand leaving everything else untouched
  • more efficient when thunk unpack'ing when the symlink points to another filesystem, as the current implementation creates a temp dir in same directory the symlink is, then uses renamePath to move the contents. (Actually, nevermind that: the renamePath implementation we typically use doesn't support moving files across devices, so this doesn't work at all in this case)

Disadvantages

  • this implementation only needs write permissions inside the symlink target: whereas given the current implementation, resolving the symlink beforehand would also require write permissions to the target's parent directory.

Would also be nice to codify this behavior in the test suite.

I have:

  • [x] Based work on latest develop branch
  • [x] Looked for lint in my changes with hlint . (lint found code you did not write can be left alone)
  • [x] Run the test suite: $(nix-build -A selftest --no-out-link)
  • [ ] (Optional) Run CI tests locally: nix-build release.nix -A build.x86_64-linux --no-out-link (or x86_64-darwin on macOS)

lpsmith avatar Jul 10 '20 15:07 lpsmith

@ali-abrar Should be moved to the nix-thunk repo ?

mankyKitty avatar Apr 22 '22 04:04 mankyKitty