Add notes on copying git repository #1296

nakabonne · 2020-12-21T03:51:00Z

What this PR does / why we need it:
On MacOS, BSD's cp command is installed by default. So the behavior is slightly different from GNU's one.
This PR will cause the build of the binary for Windows to fail. I don't mind add command_windows.go if it seems to be a problem.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2020-12-21T03:56:59Z

Code coverage for golang is 34.25%. This pull request decreases coverage by -0.00%.

File	Function	Base	Head	Diff
pkg/git/command_linux.go	copyCommand	`--`	`0.00%`	`+0.00%`

khanhtc1202 · 2020-12-21T04:14:32Z

pkg/git/command_darwin.go

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


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

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

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

Actually the trailing . is unneeded.

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?

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

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.

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

nakabonne · 2020-12-21T07:15:36Z

/hold

nakabonne · 2020-12-21T08:14:18Z

pkg/git/repo.go

 }

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


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

@nghialv If so, my usage is just evil.

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

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

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!

This reverts commit 2b17c9f.

pipecd-bot · 2020-12-22T06:11:28Z

Code coverage for golang is 34.25%. This pull request does not change code coverage.

nakabonne · 2020-12-22T06:12:05Z

Fixed done.

pipecd-bot · 2020-12-22T06:17:29Z

Code coverage for golang is 34.25%. This pull request does not change code coverage.

**What this PR does / why we need it**: Usage of copying repo became clear from the discussion on #1296 **Which issue(s) this PR fixes**: Fixes # **Does this PR introduce a user-facing change?**:  ```release-note NONE ``` This PR was merged by Kapetanios.

nakabonne · 2020-12-22T06:59:57Z

/hold cancel

pipecd-bot · 2020-12-22T08:00:53Z

This pull request is APPROVED by nghialv.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

Use OS-specific command to copy directory

2b17c9f

pipecd-bot added the release-note-none label Dec 21, 2020

pipecd-bot requested review from khanhtc1202 and nghialv December 21, 2020 03:51

pipecd-bot added the size/M label Dec 21, 2020

khanhtc1202 reviewed Dec 21, 2020

View reviewed changes

pipecd-bot added the do-not-merge/hold label Dec 21, 2020

nakabonne commented Dec 21, 2020

View reviewed changes

nakabonne added 2 commits December 22, 2020 15:00

Revert "Use OS-specific command to copy directory"

285bf29

This reverts commit 2b17c9f.

Add notes on copying git repository

440e557

pipecd-bot added size/XS and removed size/M labels Dec 22, 2020

nakabonne changed the title ~~Use OS-specific command to copy directory~~ Add notes on copying git repository Dec 22, 2020

Update notes

0580d94

nakabonne mentioned this pull request Dec 22, 2020

Ensure to give path that doesn’t exist for copying repo #1311

Merged

pipecd-bot removed the do-not-merge/hold label Dec 22, 2020

nghialv approved these changes Dec 22, 2020

View reviewed changes

pipecd-bot added the approved label Dec 22, 2020

pipecd-bot merged commit 3c3d2e5 into master Dec 22, 2020

pipecd-bot deleted the cp-os branch December 22, 2020 08:07

pipecd-bot mentioned this pull request Dec 22, 2020

Fix malformed summary of the quick sync case #1316

Merged

Add notes on copying git repository #1296

Add notes on copying git repository #1296

Uh oh!

Conversation

nakabonne commented Dec 21, 2020

Uh oh!

pipecd-bot commented Dec 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne commented Dec 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Dec 22, 2020

Uh oh!

nakabonne commented Dec 22, 2020

Uh oh!

pipecd-bot commented Dec 22, 2020

Uh oh!

nakabonne commented Dec 22, 2020

Uh oh!

pipecd-bot commented Dec 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

khanhtc1202 Dec 21, 2020 •

edited

Loading

nakabonne Dec 22, 2020 •

edited

Loading