10000 remote: Handle fetching negative refspecs by ryan-ph · Pull Request #6962 · libgit2/libgit2 · GitHub
[go: up one dir, main page]

Skip to content

remote: Handle fetching negative refspecs #6962

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

Merged
merged 5 commits into from
Dec 24, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
9 changes: 9 additions & 0 deletions include/git2/refspec.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,15 @@ GIT_EXTERN(int) git_refspec_force(const git_refspec *refspec);
*/
GIT_EXTERN(git_direction) git_refspec_direction(const git_refspec *spec);

/**
* Check if a refspec's source descriptor matches a negative reference
*
* @param refspec the refspec
* @param refname the name of the reference to check
* @return 1 if the refspec matches, 0 otherwise
*/
GIT_EXTERN(int) git_refspec_src_matches_negative(const git_refspec *refspec, const char *refname);

/**
* Check if a refspec's source descriptor matches a reference
*
Expand Down
29 changes: 28 additions & 1 deletion src/libgit2/refspec.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ int git_refspec__parse(git_refspec *refspec, const char *input, bool is_fetch)
const char *lhs, *rhs;
int valid = 0;
unsigned int flags;
bool is_neg_refspec = false;

GIT_ASSERT_ARG(refspec);
GIT_ASSERT_ARG(input);
Expand All @@ -34,6 +35,9 @@ int git_refspec__parse(git_refspec *refspec, const char *input, bool is_fetch)
refspec->force = 1;
lhs++;
}
if (*lhs == '^') {
is_neg_refspec = true;
}

rhs = strrchr(lhs, ':');

Expand Down Expand Up @@ -62,7 +66,14 @@ int git_refspec__parse(git_refspec *refspec, const char *input, bool is_fetch)

llen = (rhs ? (size_t)(rhs - lhs - 1) : strlen(lhs));
if (1 <= llen && memchr(lhs, '*', llen)) {
if ((rhs && !is_glob) || (!rhs && is_fetch))
/*
* If the lefthand side contains a glob, then one of the following must be
* true, otherwise the spec is invalid
* 1) the rhs exists and also contains a glob
* 2) it is a negative refspec (i.e. no rhs)
* 3) the rhs doesn't exist and we're fetching
*/
if ((rhs && !is_glob) || (rhs && is_neg_refspec) || (!rhs && is_fetch && !is_neg_refspec))
goto invalid;
is_glob = 1;
} else if (rhs && is_glob)
Expand Down Expand Up @@ -225,6 +236,14 @@ int git_refspec_force(const git_refspec *refspec)
return refspec->force;
}

int git_refspec_src_matches_negative(const git_refspec *refspec, const char *refname)
{
if (refspec == NULL || refspec->src == NULL || !git_refspec_is_negative(refspec))
return false;

return (wildmatch(refspec->src + 1, refname, 0) == 0);
}

int git_refspec_src_matches(const git_refspec *refspec, const char *refname)
{
if (refspec == NULL || refspec->src == NULL)
Expand Down Expand Up @@ -340,6 +359,14 @@ int git_refspec_is_wildcard(const git_refspec *spec)
return (spec->src[strlen(spec->src) - 1] == '*');
}

int git_refspec_is_negative(const git_refspec *spec)
{
GIT_ASSERT_ARG(spec);
GIT_ASSERT_ARG(spec->src);

return (spec->src[0] == '^' && spec->dst == NULL);
}

git_direction git_refspec_direction(const git_refspec *spec)
{
GIT_ASSERT_ARG(spec);
Expand Down
8 changes: 8 additions & 0 deletions src/libgit2/refspec.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ int git_refspec__serialize(git_str *out, const git_refspec *refspec);
*/
int git_refspec_is_wildcard(const git_refspec *spec);

/**
* Determines if a refspec is a negative refspec.
*
* @param spec the refspec
* @return 1 if the refspec is a negative, 0 otherwise
*/
int git_refspec_is_negative(const git_refspec *spec);

/**
* DWIM `spec` with `refs` existing on the remote, append the dwim'ed
* result in `out`.
Expand Down
8 changes: 6 additions & 2 deletions src/libgit2/remote.c
Original file line number Diff line number Diff line change
Expand Up @@ -2628,17 +2628,21 @@ int git_remote_name_is_valid(int *valid, const char *remote_name)
git_refspec *git_remote__matching_refspec(git_remote *remote, const char *refname)
{
git_refspec *spec;
git_refspec *match = NULL;
size_t i;

git_vector_foreach(&remote->active_refspecs, i, spec) {
if (spec->push)
continue;

if (git_refspec_is_negative(spec) && git_refspec_src_matches_negative(spec, refname))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (git_refspec_is_negative(spec) && git_refspec_src_matches_negative(spec, refname))
if (git_refspec_src_matches_negative(spec, refname))

I think the is_negative check is superfluous, it looks like src_matches_negative does the check itself?

Copy link
Contributor Author
@ryan-ph ryan-ph Dec 24, 2024

Choose a reason for hiding this comment

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

Removed in 83994da

return NULL;

if (git_refspec_src_matches(spec, refname))
return spec;
match = spec;
Copy link
Member

Choose a reason for hiding this comment

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

Is this a subtle behavior change? Can there be multiple matches here? Previously, we exited the function as soon as we found a match, returning the first match; now, we would clobber the match, this returning the last match. 🤔

Copy link
Contributor Author
@ryan-ph ryan-ph Dec 24, 2024

Choose a reason for hiding this comment

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

Ah, right. Fixed in 6039d28

}

return NULL;
return match;
}

git_refspec *git_remote__matching_dst_refspec(git_remote *remote, const char *refname)
Expand Down
2 changes: 2 additions & 0 deletions tests/libgit2/refs/normalize.c
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,8 @@ void test_refs_normalize__negative_refspec_pattern(void)
{
ensure_refname_normalized(
GIT_REFERENCE_FORMAT_REFSPEC_PATTERN, "^foo/bar", "^foo/bar");
ensure_refname_normalized(
GIT_REFERENCE_FORMAT_REFSPEC_PATTERN, "^foo/bar/*", "^foo/bar/*");
ensure_refname_invalid(
GIT_REFERENCE_FORMAT_REFSPEC_PATTERN, "foo/^bar");
}
64 changes: 64 additions & 0 deletions tests/libgit2/remote/fetch.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@ static char* repo2_path;

static const char *REPO1_REFNAME = "refs/heads/main";
static const char *REPO2_REFNAME = "refs/remotes/repo1/main";
static const char *REPO1_UNDERSCORE_REFNAME = "refs/heads/_branch";
static const char *REPO2_UNDERSCORE_REFNAME = "refs/remotes/repo1/_branch";
static char *FORCE_FETCHSPEC = "+refs/heads/main:refs/remotes/repo1/main";
static char *NON_FORCE_FETCHSPEC = "refs/heads/main:refs/remotes/repo1/main";
static char *NEGATIVE_SPEC = "^refs/heads/_*";

void test_remote_fetch__initialize(void) {
git_config *c;
Expand Down Expand Up @@ -170,3 +173,64 @@ void test_remote_fetch__do_update_refs_if_not_descendant_and_force(void) {

git_reference_free(ref);
}

/**
* This checks that negative refspecs are respected when fetching. We create a
* repository with a '_' prefixed reference. A second repository is configured
* with a negative refspec to ignore any refs prefixed with '_' and fetch the
* first repository into the second.
*
* @param commit1id A pointer to an OID which will be populated with the first
* commit.
*/
static void do_fetch_repo_with_ref_matching_negative_refspec(git_oid *commit1id) {
/* create a commit in repo 1 and a reference to it with '_' prefix */
{
git_oid empty_tree_id;
git_tree *empty_tree;
git_signature *sig;
git_treebuilder *tb;
cl_git_pass(git_treebuilder_new(&tb, repo1, NULL));
cl_git_pass(git_treebuilder_write(&empty_tree_id, tb));
cl_git_pass(git_tree_lookup(&empty_tree, repo1, &empty_tree_id));
cl_git_pass(git_signature_default(&sig, repo1));
cl_git_pass(git_commit_create(commit1id, repo1, REPO1_UNDERSCORE_REFNAME, sig,
sig, NULL, "one", empty_tree, 0, NULL));

git_tree_free(empty_tree);
git_signature_free(sig);
git_treebuilder_free(tb);
}

/* fetch the remote with negative refspec for references prefixed with '_' */
{
char *refspec_strs = { NEGATIVE_SPEC };
git_strarray refspecs = {
.count = 1,
.strings = &refspec_strs,
};
Copy link
Member

Choose a reason for hiding this comment

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

We don't use designated initializers; we try to keep support for very ancient compilers so that people can do clever things.

Suggested change
git_strarray refspecs = {
.count = 1,
.strings = &refspec_strs,
};
git_strarray refspecs = { &refspec_strs, 1 };

Copy link
Contributor Author
@ryan-ph ryan-ph Dec 24, 2024

Choose a reason for hiding this comment

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

Addressed in 099e62c


git_remote *remote;

cl_git_pass(git_remote_create_anonymous(&remote, repo2,
git_repository_path(repo1)));
cl_git_pass(git_remote_fetch(remote, &refspecs, NULL, "some message"));

git_remote_free(remote);
}
}

void test_remote_fetch__skip_negative_refspec_match(void) {
git_oid commit1id;
git_reference *ref1;
git_reference *ref2;

do_fetch_repo_with_ref_matching_negative_refspec(&commit1id);

/* assert that the reference in exists in repo1 but not in repo2 */
cl_git_pass(git_reference_lookup(&ref1, repo1, REPO1_UNDERSCORE_REFNAME));
cl_assert_equal_b(git_reference_lookup(&ref2, repo2, REPO2_UNDERSCORE_REFNAME), GIT_ENOTFOUND);

git_reference_free(ref1);
git_reference_free(ref2);
}
Loading
0