Skip to content

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

Merged
pks-t merged 1 commit intolibgit2:masterfrom
novalis:fix-example
Aug 1, 2019
Merged

Fix example checkout to forbid rather than require --#5184
pks-t merged 1 commit intolibgit2:masterfrom
novalis:fix-example

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.
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.

2 participants