Skip to content
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.

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

Add the argument --depth 1 to git clone for some tools missing it #178

Merged
merged 3 commits into from
Jul 6, 2023

Conversation

gbe
Copy link
Contributor

@gbe gbe commented Jun 28, 2023

After reviewing the PR #177, I noticed that the argument --depth 1 was missing when cloning CrackMapExec.

Turns out, there were other tools missing it (command below was run against the dev branch):

grep -rn clone * 2> /dev/null | grep git | grep -v depth | wc -l
20

To avoid any merge conflict, this PR must be merged after #177.
Indeed, I reproduced in this PR some of the changes brought by #177 and I added the modification to the comment I suggested.

@gbe
Copy link
Contributor Author

gbe commented Jun 28, 2023

@QU35T-code commented on my review in #177 and said that CME is a special case preventing the --depth 1 flag.

I better understand the user experience issue this change would introduce after reading https://medium.com/@thriving_chiffon_gnu_714/exploring-git-clone-depth-828eb4bac6c4

As a consequence, I'll revert the change for CME, and will add a comment above the clone line to explain why --depth 1 cannot be added here.

Is there any other tool covered by this exception ?

@Dramelac
Copy link
Member

@QU35T-code commented on my review in #177 and said that CME is a special case preventing the --depth 1 flag.

I think there's been some confusion or misunderstanding.
There's no particular reason not to do depth 1 on CME.

Git must be present to allow them to do git pulls if needed, but as long as depth 1 doesn't block this operation, it should be fine.

For operations that can generate errors and harm the user experience, why not think about adding quick fix methods to the exegol documentation to easily unshallow the repository (for example).

@QU35T-code
Copy link
Member

Yes sorry @gbe, actually it won't cause any issues, you can add it !

@gbe
Copy link
Contributor Author

gbe commented Jun 28, 2023

No worries @QU35T-code.

In that case, I leave the PR as it is for review.

Copy link
Member

@QU35T-code QU35T-code left a comment

Choose a reason for hiding this comment

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

Let's run the workflow to see if there is a problem with tools

sources/install/package_ad.sh Outdated Show resolved Hide resolved
@gbe
Copy link
Contributor Author

gbe commented Jul 4, 2023

Looks like the commit to remove a single line comment broke the CI/CD.
Is there something I can do to solve this?

@Dramelac
Copy link
Member

Dramelac commented Jul 5, 2023

Looks like the commit to remove a single line comment broke the CI/CD. Is there something I can do to solve this?

Not your fault, the error is raised because another tool (DonPAPI) had an update and the installation function no longer working, this error is not related to your changes.

@Dramelac
Copy link
Member

Dramelac commented Jul 5, 2023

However, your PR have some conflict with the official branch, can you merge the official merge in yours to fix the conflict ?

Thank you

@QU35T-code QU35T-code added the enhancement New feature or request label Jul 5, 2023
@gbe
Copy link
Contributor Author

gbe commented Jul 5, 2023

New commit.
It should merge fine now.

@Dramelac Dramelac merged commit 89d3055 into ThePorgs:dev Jul 6, 2023
@gbe gbe deleted the addGitDepth branch July 25, 2023 13:00
@ShutdownRepo ShutdownRepo mentioned this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants