Skip to content

Add notes on copying git repository #1296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pipecd-bot merged 4 commits into from
Dec 22, 2020
+2 −0
Merged

Add notes on copying git repository #1296

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b17c9f
Use OS-specific command to copy directory
nakabonne Dec 21, 2020
285bf29
Revert "Use OS-specific command to copy directory"
nakabonne Dec 22, 2020
440e557
Add notes on copying git repository
nakabonne Dec 22, 2020
0580d94
Update notes
nakabonne Dec 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/git/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ go_library(
name = "go_default_library",
srcs = [
"client.go",
"command_darwin.go",
"command_linux.go",
"commit.go",
"repo.go",
"ssh_config.go",
Expand Down
27 changes: 27 additions & 0 deletions pkg/git/command_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// +build darwin

// Copyright 2020 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package git

import (
"os/exec"
"strings"
)

func copyCommand(src, dst string) *exec.Cmd {
src = strings.TrimSuffix(src, "/")
return exec.Command("cp", "-rf", src+"/.", dst)

@khanhtc1202 khanhtc1202 Dec 21, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits, does the cp command not work without the . at the end of src path? 👀

@nghialv nghialv Dec 21, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too. Not sure why do we need this change?

@nakabonne nakabonne Dec 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MacOS's cp -r puts the directory you're copying into the destination directory.

❯ ls -a /var/folders/dn/4wqz0bq15x5_5zpxf1grd455pmlkzw/T/srcdir/
./             ../            .git/          Dockerfile     client/        client2/       go.mod         go.sum         main.go

❯ cp -rf /var/folders/dn/4wqz0bq15x5_5zpxf1grd455pmlkzw/T/srcdir /var/folders/dn/4wqz0bq15x5_5zpxf1grd455pmlkzw/T/dstdir

❯ ls -a /var/folders/dn/4wqz0bq15x5_5zpxf1grd455pmlkzw/T/dstdir
./                      ../                     srcdir/

We can copy only the files under the srcdir by keeping the trailing slash for srcdir:

❯ cp -rf /var/folders/dn/4wqz0bq15x5_5zpxf1grd455pmlkzw/T/srcdir/ /var/folders/dn/4wqz0bq15x5_5zpxf1grd455pmlkzw/T/dstdir

❯ ls -a /var/folders/dn/4wqz0bq15x5_5zpxf1grd455pmlkzw/T/dstdir
./             ../            .git/          Dockerfile     client/        client2/       go.mod         go.sum         main.go

@nakabonne nakabonne Dec 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the trailing . is unneeded.

@khanhtc1202 khanhtc1202 Dec 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, then this part is to ensure the last character of src path is / if I guess right? 🤔
besides, I checked on my macOS environment, src/path/ and src/path// have the same effect to the cp command 😅 I think we could simplify this logic here as just add the '/' character to the end of src path. How do you think about that?

@nakabonne nakabonne Dec 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I'll do so after gaining everyone's agreements.

@khanhtc1202 khanhtc1202 Dec 21, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked on my Linux environment, looks like the cp Linux command has the same behavior as macOS command, which means cp -r will copy the entry directory as a subdirectory of the dist path. If you want to just copy all subdirectories and files of the src directory to the dist path, you should add * to the end of the src path in case of using the Linux cp command.

@nakabonne nakabonne Dec 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it true? Seems like I need to look into it more.

}
25 changes: 25 additions & 0 deletions pkg/git/command_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// +build linux

// Copyright 2020 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package git

import (
"os/exec"
)

func copyCommand(src, dst string) *exec.Cmd {
return exec.Command("cp", "-rf", src, dst)
}
2 changes: 1 addition & 1 deletion pkg/git/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func (r *repo) GetClonedBranch() string {

// Copy does copying the repository to the given destination.
func (r *repo) Copy(dest string) (Repo, error) {

@nakabonne nakabonne Dec 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nghialv btw, perhaps was this method originally created for copying the repo root to the given dest as a subdirectory?

@nakabonne nakabonne Dec 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nghialv If so, my usage is just evil.

@nakabonne nakabonne Dec 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But even if so, it's not appropriate to give back Repo. Well, it's nothing!

@nghialv nghialv Dec 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Copy function copies all the data of the original repository into a new directory.
And it expects the destination directory was not created before passing to this function.
I am sorry for the lack of explanation. We should have at least a comment.

From the current situation, I think we have 2 options:

  • keep the current behavior and add a comment to explain that the dest directory must not be created before
  • require the user to ensure that the directory must be created before passing to this function and change the command to cp -rf src/* dest

@nakabonne nakabonne Dec 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One vote for the first option. In the second context, The behaviors are slightly different depending on the cp command implementation (mostly BSD and GNU). Actually cp -rf src/* dest doesn't copy the hidden files including .git/ and so on.

Let me keep the current behavior and just add a comment!

@nghialv nghialv Dec 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

cmd := exec.Command("cp", "-rf", r.dir, dest)
cmd := copyCommand(r.dir, dest)
out, err := cmd.CombinedOutput()
if err != nil {
return nil, formatCommandError(err, out)
Expand Down