Skip to content

examples: ls-files: add ls-files to list paths in the index #4380

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

Merged
pks-t merged 13 commits into from
May 4, 2018
Merged

examples: ls-files: add ls-files to list paths in the index #4380

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
30e919c
examples: ls-files: list files
Oct 16, 2017
991fc08
examples: ls-files: show current files in index
Oct 17, 2017
52d83dd
examples: ls-files: handle --error-unmatch
Oct 18, 2017
cd39273
examples: ls-files: fix style and refactor
cjhoward92 Oct 23, 2017
d2f99e0
examples: ls-files: fix compile error
cjhoward92 Oct 23, 2017
37cbc3e
examples: ls-files: build file list with array
Oct 23, 2017
3f64a9d
examples: ls-files: use git_array_t to handle files
Oct 26, 2017
e866116
examples: ls-files: address PR and style
Nov 14, 2017
ee14465
examples: ls-files: fix build error
Nov 14, 2017
7d07941
test: ls-files: remove dependency on git_array
Dec 19, 2017
29ca3f3
examples: ls-files: update print_paths to print all cases
Jan 6, 2018
d7394c3
examples: ls-files: print entry path only when entry is found
Mar 27, 2018
8aa437e
tests: ls-files: use puts instead of printf and fix typos
May 2, 2018
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
Prev Previous commit
Next Next commit
examples: ls-files: fix style and refactor
  • Loading branch information
@cjhoward92
cjhoward92 authored and Carson Howard committed Mar 27, 2018
commit cd39273dbb62d9abf16a08ea4e86efe17ae22b41
135 changes: 65 additions & 70 deletions examples/ls-files.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/

#include "common.h"
#include "array.h"

/**
* This example demonstrates the libgit2 index APIs to roughly
Expand All @@ -31,66 +32,12 @@
*
*/

#define MAX_FILES 64

typedef struct ls_options {
typedef struct {
int error_unmatch;
char *files[MAX_FILES];
char **files;
int file_count;
} ls_options;

static void usage(const char *message, const char *arg);
void parse_options(ls_options *opts, int argc, char *argv[]);
int print_error_unmatch(ls_options *opts, git_index *index);

int main(int argc, char *argv[]) {
ls_options opts;
git_repository *repo;
git_index *index;
const git_index_entry *entry;
size_t entry_count;
size_t i = 0;
int error;

parse_options(&opts, argc, argv);

/* we need to initialize libgit2 */
git_libgit2_init();

/* we need to open the repo */
if ((error = git_repository_open_ext(&repo, ".", 0, NULL)) != 0)
goto cleanup;

/* we need to load the repo's index */
if ((error = git_repository_index(&index, repo)) != 0)
goto cleanup;

/* if the error_unmatch flag is set, we need to print it differently */
if (opts.error_unmatch) {
error = print_error_unmatch(&opts, index);
goto cleanup;
}

/* we need to know how many entries exist in the index */
entry_count = git_index_entrycount(index);

/* loop through the entries by index and display their pathes */
for (i = 0; i < entry_count; i++) {
entry = git_index_get_byindex(index, i);
printf("%s\n", entry->path);
}

cleanup:
/* free our allocated resources */
git_index_free(index);
git_repository_free(repo);

/* we need to shutdown libgit2 */
git_libgit2_shutdown();

return error;
}

/* Print a usage message for the program. */
Copy link
Copy Markdown
Member

@pks-t pks-t Jan 5, 2018

Choose a reason for hiding this comment

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

This comment does not provide any useful information

static void usage(const char *message, const char *arg)
Copy link
Copy Markdown
Member

@pks-t pks-t Oct 20, 2017

Choose a reason for hiding this comment

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

This here is a strange signature, I'd have expected varargs. But as you've simply taken that from other examples, I guess it's fine. Probably it would also be a bit overkill as there is no need for that right now.

{
Expand All @@ -102,13 +49,13 @@ static void usage(const char *message, const char *arg)
exit(1);
}

void parse_options(ls_options *opts, int argc, char *argv[]) {
static void parse_options(ls_options *opts, int argc, char *argv[])
{
int parsing_files = 0;
struct args_info args = ARGS_INFO_INIT;
Copy link
Copy Markdown
Member

@pks-t pks-t Nov 6, 2017

Choose a reason for hiding this comment

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

This is not really being used, is it? You could instead just loop over argc/argv[]:

int i;
for (i = 1; i < argc; i++) {
    char *arg = argv[i];
    ...
}


git_array_t(char *) files = GIT_ARRAY_INIT;

memset(opts, 0, sizeof(ls_options));
opts->error_unmatch = 0;
opts->file_count = 0;

if (argc < 2)
return;
Expand All @@ -117,22 +64,25 @@ void parse_options(ls_options *opts, int argc, char *argv[]) {
char *a = argv[args.pos];

/* if it doesn't start with a '-' or is after the '--' then it is a file */
if (a[0] != '-' || !strcmp(a, "--")) {
if (parsing_files) {
opts->files[opts->file_count++] = a;
} else {
parsing_files = 1;
}
} else if (!strcmp(a, "--error-unmatch")) {
opts->error_unmatch = 1;
if (a[0] != '-') {
parsing_files = 1;
Copy link
Copy Markdown
Member

@pks-t pks-t Nov 6, 2017

Choose a reason for hiding this comment

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

You set this, but you don't use that value to distinguish files/non-files. It should probably be

if (a[0] != '-' || parsing_files) {
    ...
}

So as soon as we find the first non-argument, we treat all remaining arguments as files.


opts->files = git_array_alloc(files);
GITERR_CHECK_ALLOC(opts->files);

opts->files[opts->file_count++] = a;
} else if (!strcmp(a, "--")) {
parsing_files = 1;
} else if (!strcmp(a, "--error-unmatch") && !parsing_files) {
Copy link
Copy Markdown
Member

@pks-t pks-t Nov 6, 2017

Choose a reason for hiding this comment

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

If you change the first condition as proposed, there's no need for the !parsing_files anymore.

opts->error_unmatch = 1;
} else {
usage("Unsupported argument", a);
}
}
}

int print_error_unmatch(ls_options *opts, git_index *index) {
static int print_paths(ls_options *opts, git_index *index)
{
int i;
const git_index_entry *entry;
Copy link
Copy Markdown
Member

@pks-t pks-t Jan 5, 2018

Choose a reason for hiding this comment

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

This could also be scoped to the loop


Expand All @@ -141,13 +91,58 @@ int print_error_unmatch(ls_options *opts, git_index *index) {
const char *path = opts->files[i];

entry = git_index_get_bypath(index, path, GIT_INDEX_STAGE_NORMAL);
if (!entry) {
if (!entry && opts->error_unmatch) {
printf("error: pathspec '%s' did not match any file(s) known to git.\n", path);
printf("Did you forget to 'git add'?\n");
Copy link
Copy Markdown
Member

@pks-t pks-t Nov 6, 2017

Choose a reason for hiding this comment

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

These should probably be fprintf(stderr, ...) to print to stderr instead.

return -1;
Copy link
Copy Markdown
Member

@pks-t pks-t Oct 20, 2017

Choose a reason for hiding this comment

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

This error should in fact only trigger if (!entry && opts->error_unmatch).

}

printf("%s\n", path);
Copy link
Copy Markdown
Member

@pks-t pks-t Jan 9, 2018

Choose a reason for hiding this comment

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

I just tried the code locally as I wanted to merge it, but I found this bug here. You're inconditionally printing path whether it is found or not. But in fact, you only want to print it if entry != NULL. So something like the following:

if ((entry = git_index_get_bypath(...)) != NULL) {
    printf("%s\n", path);
} else if (opts->error_unmatch) {
    fprintf(stderr, ...);
    return -1;
}

}

return 0;
}

int main(int argc, char *argv[])
{
ls_options opts;
git_repository *repo;
git_index *index;
const git_index_entry *entry;
size_t entry_count;
size_t i = 0;
int error;

parse_options(&opts, argc, argv);

git_libgit2_init();

if ((error = git_repository_open_ext(&repo, ".", 0, NULL)) != 0)
goto cleanup;

if ((error = git_repository_index(&index, repo)) != 0)
goto cleanup;

/* if there are files explicitly listed by the user, we need to treat this command differently */
if (opts.file_count > 0) {
error = print_paths(&opts, index);
goto cleanup;
}
Copy link
Copy Markdown
Member

@pks-t pks-t Nov 6, 2017

Choose a reason for hiding this comment

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

I would simply put the rest into an else branch. Like that, you don't need that goto cleanup anymore and it becomes a bit more obvious.


/* we need to know how many entries exist in the index */
entry_count = git_index_entrycount(index);

/* loop through the entries by index and display their pathes */
for (i = 0; i < entry_count; i++) {
entry = git_index_get_byindex(index, i);
printf("%s\n", entry->path);
}
Copy link
Copy Markdown
Member

@pks-t pks-t Jan 5, 2018

Choose a reason for hiding this comment

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

Just a small style issue: I do find it a bit funny that some printing logic is in print_paths and some is not. Maybe put the complete priting logic into print_paths and destinguish cases by opts.file_count inside of that function, then? That would make the main function a bit nicer to read.


cleanup:
/* free our allocated resources */
Copy link
Copy Markdown
Member

@pks-t pks-t Nov 6, 2017

Choose a reason for hiding this comment

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

I think there's no need to state the obvious here ;) Same for the two previous comments.

git_index_free(index);
git_repository_free(repo);
git_libgit2_shutdown();

return error;
}