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

Fix example checkout to forbid rather than require -- #5184

Merged
merged 1 commit into from Aug 1, 2019

Conversation

novalis
Copy link
Contributor

@novalis novalis commented Jul 24, 2019

Make the example program for checkout follow git syntax, where "--" indicates a file. This was likely just a strcmp return value confusion.

Make the example program for checkout follow git syntax, where
"--" indicates a file.  This was likely just a strcmp return
value confusion.
@@ -195,7 +195,7 @@ int lg2_checkout(git_repository *repo, int argc, char **argv)
fprintf(stderr, "unhandled\n");
err = -1;
goto cleanup;
} else if (strcmp("--", args.argv[args.pos])) {
} else if (!strcmp("--", args.argv[args.pos])) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this definitely looks like an error to me. If you ask me, though, this block could be improved even further: else if (args.pos == args.argc - 1) { resolve_refish... } else { fprintf(stderr, "Only switching between branches is supported"); }. We only support checking out a ref anyway, so we could just assert that we received a single parameter only and, if so, unconditionally pass that to resolve_refish. Because as the code stands right now, we just ignore every parameter after the first one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure -- there's a ton of possible improvements here. I just happened to notice that while trying to investigate #5185 (which turned out to be only for merges). So I'm unlikely to make further changes right now. Feel free to merge or ignore.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough!

Copy link
Member

@pks-t pks-t left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @novalis!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants