8000 negative refspec `^refs` not yet implemented · Issue #6741 · libgit2/libgit2 · GitHub
[go: up one dir, main page]
Skip to content

negative refspec ^refs not yet implemented #6741

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

Jump to bottom
Closed
bryango opened this issue Feb 18, 2024 · 9 comments
Closed

negative refspec ^refs not yet implemented #6741

bryango opened this issue Feb 18, 2024 · 9 comments
Labels
feature git compatibility v.next

Comments

@bryango
Copy link
bryango commented Feb 18, 2024 •
edited
Loading

It seems that negative refspec in the form of ^refs is not yet implemented in libgit2. See:

Reproduction steps

Perform a fetch with a negative refspec in some application. I am using https://github.com/arxanas/git-branchless/tree/v0.8.0.
Observe the error:

$ git-branchless submit @
branchless: running command: git fetch origin refs/heads/checkpoint-build-improve
Pseudo-terminal will not be allocated because stdin is not a terminal.
From github.com:bryango/nixpkgs
 * branch                      checkpoint-build-improve -> FETCH_HEAD
The application panicked (crashed).
Message:  A fatal error occurred: 
   0: could not find upstream branch for branch with name 'checkpoint-build-improve': '^refs/heads/rapidfuzz-opt-numpy' is not a valid refspec.; class=Invalid (3)
   1: '^refs/heads/rapidfuzz-opt-numpy' is not a valid refspec.; class=Invalid (3)

Location: git-branchless-submit/src/branch_forge.rs:155
Location: git-branchless/src/commands/mod.rs:235

The full error is specific to git-branchless, but the not a valid refspec part seems to come from libgit2.

Expected behavior

Understand negative refspec in the form of ^refs.

Actual behavior

Do not understand negative refspec in the form of ^refs.

Version of libgit2 (release number or SHA1)

libgit2-sys 0.15.2+1.6.4

I believe this is the rust binding of libgit2 v1.6.4.

Operating system(s) tested

$ uname -a
Linux 6.6.8-2-MANJARO #1 SMP PREEMPT_DYNAMIC Thu Dec 21 16:21:45 UTC 2023 x86_64 GNU/Linux
@Caleb-T-Owens Caleb-T-Owens mentioned this issue Sep 23, 2024
Closed
@ryan-ph ryan-ph mentioned this issue Nov 12, 2024
Closed
@ryan-ph
Copy link
Contributor
ryan-ph commented Nov 27, 2024 •
edited
Loading

Is this issue up for grabs for a first time contributor? I took a look and I believe parsing the refspec will attempt to normalize the name

libgit2/src/libgit2/refs.c

Line 902 in 07c77d4

int git_reference__normalize_name(

Which errors because ^ is not marked as a valid character

libgit2/src/libgit2/refs.c

Lines 820 to 836 in 07c77d4

static int is_valid_ref_char(char ch)
{
if ((unsigned) ch <= ' ')
return 0;
switch (ch) {
case '~':
case '^':
case ':':
case '\\':
case '?':
case '[':
return 0;
default:
return 1;
}
}

I guess we'd need to update the is_valid_ref_char() function and then likely this conditional check here at the end of git_reference__normalize_name() as well as updating and adding test cases for negative ref specs

libgit2/src/libgit2/refs.c

Lines 1001 to 1009 in 07c77d4

if ((segments_count == 1 ) &&
!(flags & GIT_REFERENCE_FORMAT_REFSPEC_SHORTHAND) &&
!(is_all_caps_and_underscore(name, (size_t)segment_len) ||
((flags & GIT_REFERENCE_FORMAT_REFSPEC_PATTERN) && !strcmp("*", name))))
goto cleanup;
if ((segments_count > 1)
&& (is_all_caps_and_underscore(name, strchr(name, '/') - name)))
goto cleanup;

In terms of just getting libgit2 to successfully parse the refspec, I guess this is all that we would need to change, but I'm not sure what else we'd need to update to get the rest of the library to handle negative ref specs correctly.

edit (2024-11-27):
Took a bit more of a look and it seems in terms of functionality, we'd need to update:

  • git_refspec_src_matches() to handle negative ref specs

    libgit2/src/libgit2/refspec.c

    Lines 228 to 234 in c86842f

    int git_refspec_src_matches(const git_refspec *refspec, const char *refname)
    {
    if (refspec == NULL || refspec->src == NULL)
    return false;
    return (wildmatch(refspec->src, refname, 0) == 0);
    }
  • git_remote__matching_refspec() to handle the case where the given refname matches a negative refspec

    libgit2/src/libgit2/remote.c

    Lines 2628 to 2642 in c86842f

    git_refspec *git_remote__matching_refspec(git_remote *remote, const char *refname)
    {
    git_refspec *spec;
    size_t i;
    git_vector_foreach(&remote->active_refspecs, i, spec) {
    if (spec->push)
    continue;
    if (git_refspec_src_matches(spec, refname))
    return spec;
    }
    return NULL;
    }

I'm likely missing some other places, but would be happy to start if this is open for contribution.

@ethomson
Copy link
Member

This is 💯 open for contribution. Thanks for digging in!

@ryan-ph
Copy link
Contributor
ryan-ph commented Nov 28, 2024

I've opened #6951 to handle being able to normalize negative refspecs, but it does not yet handle updating any of the actions that use refspecs (e.g. fetch) to respect how negative refspecs should behave yet. I am hoping to merge this fix to ref normalization and then handle each action as separate PRs, but if you think the PR should contain the full negative refspec behavior, please let me know.

I wasn't quite sure what to do with the changelog as there doesn't seem to be any entries for v1.8.3 or v1.8.4.

This was referenced Dec 16, 2024
Merged
Merged
@ryan-ph
Copy link
Contributor
ryan-ph commented Dec 16, 2024

I think remote fetching is the only place that makes active use of the negative refspecs. If there are other places that use it and need to be updated, please let me know and I can open up a separate PR for them.

@ethomson
Copy link
Member
ethomson commented Jan 2, 2025

I think that @ryan-ph has taken care of these issues; closing this. Please open a new issue if there are additional issues that we've missed!

@ethomson ethomson closed this as completed Jan 2, 2025
@skoulik
Copy link
skoulik commented Feb 19, 2025

Good day @ethomson, @ryan-ph,

I was reading through the history as I've just faced the ^refs parse issue with TortoiseGit, which is based on libgit2.

I am a bit confused with the outcome. The ticked is closed, but is the issue properly resolved? I mean, the normalization will not bark anymore, but do the negative refspecs actually work when fetching from remote? If not, I believe this should be reopened...

@ryan-ph
Copy link
Contributor
ryan-ph commented Feb 21, 2025

@skoulik this should be resolved. Have you run into the case where it is not respecting your negative refspec?

@skoulik
Copy link
skoulik commented Feb 24, 2025 •
edited
Loading

@skoulik this should be resolved. Have you run into the case where it is not respecting your negative refspec?

Hi @ryan-ph
It is hard to say. The maintainers of TortoiseGit did not pick up the fixed version of libgit2 yet. I've tried to hack the distribution myself by replacing the dll, to no avail unfortunately. So, the answer to your question is "don't know", hence asking...

@ryan-ph
Copy link
Contributor
ryan-ph commented Feb 26, 2025

I’ve been using this with the latest build of git-branchless and have not run into issues with it incorrectly pulling branches matched by the negative refspec. Without a reproducible bug, it’s hard to say what issue you’re running into is, especially since it’s for a tool that I do not know and cannot use. You’re more than welcome to read the attached PRs and see if there was a case that was missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature git compatibility v.next
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ethomson @skoulik @ryan-ph @bryango
0