Never download anything when using the checkout command

[sinbad]

This was always the intention but the PointerSmudge functions would automatically do it if the local files were missing. Now they take a boolean argument to say whether to download or not, and the case of skipped smudge is dealt with by writing out the original pointer data and returning a known non-fatal error.

[technoweenie]

👍

I really don't like all the positional args though:

PointerSmudge(writer io.Writer, ptr *Pointer, workingfile string, download bool, cb CopyCallback)

I'd like to explore the functional options pattern. I don't think it's worth holding this PR up though. The fact that there are multiple functions wrapping PointerSmudge() means it won't be a trivial change.

I think something like this could be cool though:

type pointerSmudgeOption struct {
  WorkingFile string
  Download bool
  Callback CopyCallback
}

func SetWorkingFile(file string) func(o *pointerSmudgeOption) {
  o.WorkingFile = file
}

func SkipDownload(o *pointerSmudgeOption) {
  o.Download = false
}

func PointerSmudge(writer io.Writer, ptr *Pointer, options ...func(*pointerSmudgeOption) error {
  opt := &pointerSmudgeOption{Download: true} // set defaults
  for _, o := range options {
    o(opt)
  }

  ...
}

// Default: Downloads file with blank working file, and no callback
lfs.PointerSmudge(w, p)

// Set a working file
lfs.PointerSmudge(w, p, lfs.SetWorkingFile("foo/bar"))

// Set a working file and skip downloading
lfs.PointerSmudge(w, p, lfs.SetWorkingFile("foo/bar"), lfs.SkipDownload)

// Only skip downloading
lfs.PointerSmudge(w, p, lfs.SkipDownload)

I do like that, but after writing it all out, I don't think now is the time. The lfs package contains a lot of stuff. If multiple functions used the options pattern, we'd need to either prefix all of the option functions, or create some shared option type that's shared among the functions.

// yuck
lfs.PointerSmudge(w, p, lfs.PointerSmudgeSetWorkingFile("foo/bar"), lfs.PointerSmudgeSkipDownload)

[technoweenie]

I'm going to merge this, but if anyone has comments about the positional args, I'd love to hear it (@sinbad, @rubyist, @michael-k).

[rubyist]

I agree that the args list for that function is a little long now. I do like the functional options pattern, but I agree that the lfs package is a little too large for that to be an elegant solution. I don't think we should block this PR because of this, but I think the set of smudge functions could be better factored. We've added enough things on to the concept of Smudge now that we should revisit it soon.

[sinbad]

Go needs defaults and named params ;) The functional options pattern is clever but I'm not sure it's any simpler or intuitive, unless that becomes a really common approach. I quite like the simpler option of just defining function variants which take fewer parameters and default the rest on a cascade call through to one private function with either all the args or an options struct. Not as clever but everyone knows what it is immediately on looking at it.

[technoweenie]

I agree we can get away with some func variants since there aren't too many permutations to deal with here.