More concise this way and also supports N git args in the error message:

    args := []string{"clone", b.source}
    if b.target != "" {
        args = append(args, b.target)
    }
    if output, err := b.execCommand("git", args, dir); err != nil {
        return fmt.Errorf("failed to exec 'git %s': %s: %v", strings.Join(append(args[:1], args[1:len(args)]...), " "), output, err)
    }

Will this part fail if you specify a target? The current implementation assumes there will be exactly 1 directory (the repo name) in the volume.

if len(files) != 1 {
        return fmt.Errorf("unexpected directory contents: %v", files)
    }

I couldn't comment on that line in the diff because you didn't edit it.

This will fail when clone directory is '.'

Actually, see my comments here: #15421 (comment)

I'd like to update the code if we reached a agreement.

As per #15421

How urgent is this one small feature? I'm not deeply against adding it if
it solves real problems, but I'd rather focus energy on the git-sync
sidecar. Additionally, we need clarity on whether this is just "checkout
and rename" or whether "." is expected to be supported. Doing a checkout to
a subdir has one significant advantage - it can be updated atomically. This
means you could periodically sync to head of a branch, which seems like a
powerful feature to me.

If we support ".", that does not work.

On Wed, Oct 14, 2015 at 12:54 AM, Harry Zhang notifications@github.com
wrote:

In pkg/volume/git_repo/git_repo.go
#15533 (comment)
:

@@ -138,8 +140,15 @@ func (b *gitRepoVolumeBuilder) SetUpAt(dir string) error {
return err
}

• if output, err := b.execCommand("git", []string{"clone", b.source}, dir); err != nil {

•   return fmt.Errorf("failed to exec 'git clone %s': %s: %v", b.source, output, err)
  

• // If provided target directory
• if "" != b.target {

•   if output, err := b.execCommand("git", []string{"clone", b.source, b.target}, dir); err != nil {
  

•       return fmt.Errorf("failed to exec 'git clone %s %s': %s: %v", b.source, b.target, output, err)
  

•   }
  

• } else {

•   if output, err := b.execCommand("git", []string{"clone", b.source}, dir); err != nil {
  

•       return fmt.Errorf("failed to exec 'git clone %s': %s: %v", b.source, output, err)
  

•   }
  

  }

This will fail when clone directory is '.'

Actually, see my comments here: #15421 (comment)
#15421 (comment)

I'd like to update the code if we reached a agreement.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/15533/files#r41964131.

Better to use switch over if/else:

switch {
case b.target == "":
case b.target != "" && len(files) == 1:
case b.target != && len(files) != 1
}

len(b.revision) == 0 and b.target != "" are the same (in effect) but different implementations. They should be consistent. I've mostly seen (and used) empty string "" comparison instead of string length check.