examples: ls-files: add ls-files to list paths in the index by cjhoward92 · Pull Request #4380 · libgit2/libgit2

cjhoward92 · 2017-10-18T04:13:20Z

Added an example to mimic git ls-files using the libgit2 library. It also supports the --error-unmatch parameter to determine if the specified paths are in the index. This will hopefully be a useful example for new libgit2 users.

pks-t

Hi @cjhoward92! I always think it worthwhile to have more examples, as a lot of people prefer to learn by example instead by documentation, so I welcome this PR. Thanks a lot :)

There are a few things which could be improved. Most things are not actually game breaking but just small nits, but you've got the --error-unmatch thing wrong. Right now you decide whether to print the complete index or by path simply by inspecting opts.error_unmatch, which is wrong. Instead, we want to print the complete index when no files were in by the user, otherwise we want to list just what the user provided. error_unmatch is then used to decide, in the second case where we have a list of paths from the user`, whether we want to give an error when he has listed a non-existing path.

So if you correct that behaviour and the other nits I'd be happy to merge :)

pks-t · 2017-10-20T13:08:52Z

@@ -3,7 +3,7 @@
 CC = gcc


We have a makefile here? I'll create a PR and delete that.

pks-t · 2017-10-20T13:10:30Z

+
+#define MAX_FILES 64
+
+typedef struct ls_options {


I think we usually avoid giving the struct itself a name if it's already typedef'd.

pks-t · 2017-10-20T13:12:04Z

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


You never check whether you overflow that array. Furthermore, I'd propose to just use an array, that should be much simpler. Just have a look at "array.h" :)

pks-t · 2017-10-20T13:12:23Z

+
+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);


static declarations are missing here. You could also consider to reorder functions to avoid these declarations, but I'm rather indifferent to that.

pks-t · 2017-10-20T13:12:55Z

+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[]) {


We prefer to have a newline between function head and the opening brace.

pks-t · 2017-10-20T13:28:40Z

+}
+
+/* Print a usage message for the program. */
+static void usage(const char *message, const char *arg)


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.

pks-t · 2017-10-20T13:31:05Z

+				opts->files[opts->file_count++] = a;
+			} else { 
+				parsing_files = 1;
+			}


No need for braces here.

pks-t · 2017-10-20T13:32:05Z

+			if (parsing_files) {
+				opts->files[opts->file_count++] = a;
+			} else { 
+				parsing_files = 1;


To me this looks like you're actually skipping the first file? Or am I misinterpreting that?

pks-t · 2017-10-20T13:33:06Z

+	}
+}
+
+int print_error_unmatch(ls_options *opts, git_index *index) {


I'd rename that to e.g. print_paths. You could then have a second function print_index when no paths are given.

pks-t · 2017-10-20T13:34:36Z

+		if (!entry) {
+			printf("error: pathspec '%s' did not match any file(s) known to git.\n", path);
+			printf("Did you forget to 'git add'?\n");
+			return -1;


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

cjhoward92 · 2017-10-20T14:38:06Z

@pks-t Thanks a ton for reviewing this! I will be happy to make these changes, hopefully tonight, but maybe tomorrow. I really appreciate you taking the time to look over this and being patient with me! I am pretty new to C, but I would really like to become an active contributor to libgit2, so I will work hard to get you some quality PR's.

Thanks again!

cjhoward92 · 2017-10-23T19:56:04Z

@pks-t This should be mostly good to go. Not 100% sure how to use the array.h effectively, though. I get a segfault when I have multiple files. I tried finding some good examples in the code base but could not. Do you have a suggestion or examples you could point me to? I know I am not supposed to reallocate the array per file, but none of the code I found was really clear on how I add an item to the array. Thanks for your time!

cjhoward92 · 2017-10-26T02:20:59Z

All updated. I figured out how to use git_array_t with char * as the element. Thanks for the pointers!

cjhoward92 · 2017-10-26T02:39:00Z

I was wondering if I should squash this?

cjhoward92 · 2017-11-01T20:31:18Z

Any chance anyone will take a look at this?

pks-t · 2017-11-03T20:16:27Z

I'll find some time next week.

pks-t

There's just one functional issue here, the rest is about style, so we're mostly there. Thanks for fixing up and working on this!

pks-t · 2017-11-06T09:52:37Z

 tag
 for-each-ref
 describe
+ls-files


I've deleted this file on libgit2's master now, as it should not be required anymore. You'll have to rebase your PR to fix that conflict. unfortunately.

pks-t · 2017-11-06T09:52:53Z

-APPS = general showindex diff rev-list cat-file status log rev-parse init blame tag remote
+APPS = general showindex diff rev-list cat-file status log rev-parse init blame tag remote ls-files
 APPS += for-each-ref
 APPS += describe


Same here, I've also deleted that file on master.

pks-t · 2017-11-06T09:55:12Z

+ * This includes staged and committed files, but unstaged files will not display.
+ *
+ * This currently supports:
+ * 	- The --error-unmatch paramter with the same output as the git cli


Small nit: there's one space too much here. I'd also put that whole supported/unsupported thing a little bit shorter: "This currently only supports default behaviour and the --error-unmatch option."

pks-t · 2017-11-06T09:58:34Z

+static int parse_options(ls_options *opts, int argc, char *argv[])
+{
+	int parsing_files = 0;
+	struct args_info args = ARGS_INFO_INIT;


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]; ... }

pks-t · 2017-11-06T09:59:36Z

+
+			file = git_array_alloc(opts->files);
+			GITERR_CHECK_ALLOC(file);
+			*file = a;


pks-t · 2017-11-06T10:00:58Z

+
+		/* if it doesn't start with a '-' or is after the '--' then it is a file */
+		if (a[0] != '-') {
+			parsing_files = 1;


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.

pks-t · 2017-11-06T10:02:14Z

+			*file = a;
+		} else if (!strcmp(a, "--")) {
+			parsing_files = 1;
+		} else if (!strcmp(a, "--error-unmatch") && !parsing_files) {


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

pks-t · 2017-11-06T10:03:06Z

+		entry = git_index_get_bypath(index, path, GIT_INDEX_STAGE_NORMAL);
+		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");


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

pks-t · 2017-11-06T10:06:02Z

+	if (git_array_size(opts.files) > 0) {
+		error = print_paths(&opts, index);
+		goto cleanup;
+	}


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.

pks-t · 2017-11-06T10:06:45Z

+	}
+
+cleanup:
+	/* free our allocated resources */


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

cjhoward92 · 2017-11-14T04:08:33Z

@pks-t This is all updated, but I am not too sure what is causing the windows builds to break.

ethomson · 2017-11-14T09:54:59Z

Regarding the Windows builds: git_array_alloc calls git__reallocarray. We're building with the CRT debugging functions on Windows, which ends up calling git__crtdbg__reallocarray.

However, I think that we're not linking the examples with the CRT debugging libraries. Ugh. Not sure how to fix this offhand.

cjhoward92 · 2017-11-14T17:52:07Z

Ok, I will check it out when I get a moment. Thanks for the explanation!

cjhoward92 · 2017-12-18T19:18:48Z

Would updating the CMakeLists for the examples to statically link the CRT debugging libraries work? I'm not super familiar with how CMake works to really know. All I found was the flags to link when running the main build process. I don't really understand why this fails on AppVeyor but works on my machine either, unless I installed something that exposes the CRT libraries on my includes path.

ethomson · 2017-12-19T15:24:58Z

AppVeyor is explicitly turning on the CRT memory debugging functionality, with -DDEBUG_POOL. I suspect that this should fail if you enable the same functionality?

In any case, the ideal way to fix this would be to not use git_array_alloc. It's not public API, it's private API, so we should not use it in examples. Anybody reading (or more likely, copy/pasting) the example code will not be able to build it since we do not expose that macro publicly.

ethomson · 2017-12-19T15:30:28Z

I see why you want to use git_array_alloc instead of trying to manage a dynamic array yourself... If it were me, I'd just create a static array of char * and hold a separate counter to the number of arguments that you increment. You can easily make this char *files[1024] and that's practically way more than enough. It could be 16, tbqh. This is a sample, not production code, and it should be readable and easy to use not perfect.

I do think that we should make sure that we do not crash if a user enters 17 filenames though. :grin

cjhoward92 · 2017-12-19T15:33:58Z

Thanks @ethomson! I'll verify -DDEBUG_POOL does not work then make those changes and see if compiling with -DDEBUG_POOL works. Hopefully it's as simple as that.

cjhoward92 · 2017-12-20T14:08:03Z

Looks like there is only an error with some of the credentials stuff now, might be some network error. I think they should be good to go, though.

cjhoward92 · 2018-01-04T19:26:17Z

bump

pks-t

This should be the last round of comments now. The example looks very nice by now, but there's two issues with C99-comments and the missing error when the files array is too small. I only added the other comments because there's already some stuff that needs fixing up, so they'd be nice to have, too.

pks-t · 2018-01-05T07:52:54Z

+
+			// watch for overflows (just in case)
+			if (opts->file_count == 1024) {
+				break;


You probably shouldn't break here but print an error and return an error code.

pks-t · 2018-01-05T07:53:12Z

+		if (a[0] != '-' || parsing_files) {
+			parsing_files = 1;
+
+			// watch for overflows (just in case)


C++-style comments are not allowed

pks-t · 2018-01-05T07:53:31Z

+	int file_count;
+} ls_options;
+
+/* Print a usage message for the program. */


This comment does not provide any useful information

pks-t · 2018-01-05T07:55:27Z

+static int print_paths(ls_options *opts, git_index *index)
+{
+	int i;
+	const git_index_entry *entry;


This could also be scoped to the loop

pks-t · 2018-01-05T07:58:05Z

+			entry = git_index_get_byindex(index, i);
+			printf("%s\n", entry->path);
+		}
+	}


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.

cjhoward92 · 2018-01-06T18:28:53Z

@pks-t Hopefully this is updated in the way you recommended. Let me know what you think! Thanks for all of the feedback!

pks-t

Your updates look perfect, thanks for it! I just tried it locally and found a bug in it such that all command line arguments are printed out, whether they are found or not. Everything else looks good to me.

pks-t · 2018-01-09T14:13:26Z

+			/* watch for overflows (just in case) */
+			if (opts->file_count == 1024) {
+				fprintf(stderr, "ls-files can only support 1024 files at this time.\n");
+				return -1;


pks-t · 2018-01-09T14:26:12Z

+			return -1;
+		}
+
+		printf("%s\n", path);


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; }

pks-t · 2018-01-09T14:26:29Z

+	if ((error = git_repository_index(&index, repo)) < 0)
+		goto cleanup;
+
+	error = print_paths(&opts, index);


Yup, this looks like a very clean main function now :)

cjhoward92 · 2018-03-27T14:28:27Z

Wow, sorry this took so long to get done! I have been really busy lately. Thanks a ton for all of the review you have done! I hope I finally have it clean enough to get merged!

cjhoward92 · 2018-04-27T15:07:49Z

Any more review on this? It looks like the test failures and build issues were an internet connectivity issue perhaps. I wonder if restarting the build on Travis would fix that. Thanks!

pks-t

Looks good to me. There are some really minor nits, but I don't really care for them. Some quick testing showed no issues. Waiting for a few days if there's more feedback, otherwise I'm going to merge

pks-t · 2018-04-30T10:11:52Z

+
+typedef struct {
+	int error_unmatch;
+	char * files[1024];


* goes with the variable name, so this should rather be char *files[1024];.

pks-t · 2018-04-30T10:12:23Z

+	size_t i;
+	const git_index_entry *entry;
+
+	/* if there are not files explicitly listed by the user print all entries in the index */


"not" should be a "no"

pks-t · 2018-04-30T10:12:35Z

+
+		for (i = 0; i < entry_count; i++) {
+			entry = git_index_get_byindex(index, i);
+			printf("%s\n", entry->path);


puts(entry->path) would be preferred

pks-t · 2018-04-30T10:12:56Z

+		const char *path = opts->files[i];
+
+		if ((entry = git_index_get_bypath(index, path, GIT_INDEX_STAGE_NORMAL)) != NULL) {
+			printf("%s\n", path);


puts instead of printf

cjhoward92 · 2018-05-02T14:56:04Z

I was in here so I fixed those final issues you had anyway! Thanks again!

pks-t · 2018-05-04T14:20:47Z

Thanks a lot, @cjhoward92!

pks-t requested changes Oct 20, 2017

View reviewed changes

cjhoward92 force-pushed the examples/ls-files branch from f04a187 to 859ddde Compare October 23, 2017 04:30

pks-t requested changes Nov 6, 2017

View reviewed changes

cjhoward92 force-pushed the examples/ls-files branch from 0b255eb to d3f9aef Compare November 14, 2017 03:35

cjhoward92 force-pushed the examples/ls-files branch from e97d063 to 8c61dea Compare December 18, 2017 19:01

pks-t requested changes Jan 5, 2018

View reviewed changes

cjhoward92 force-pushed the examples/ls-files branch from a42087c to 02e8598 Compare January 6, 2018 18:27

pks-t requested changes Jan 9, 2018

View reviewed changes

Carson Howard and others added 6 commits March 27, 2018 07:18

examples: ls-files: list files

30e919c

examples: ls-files: show current files in index

991fc08

examples: ls-files: handle --error-unmatch

52d83dd

examples: ls-files: fix style and refactor

cd39273

examples: ls-files: fix compile error

d2f99e0

examples: ls-files: build file list with array

37cbc3e

Carson Howard added 6 commits March 27, 2018 07:18

examples: ls-files: use git_array_t to handle files

3f64a9d

examples: ls-files: address PR and style

e866116

examples: ls-files: fix build error

ee14465

test: ls-files: remove dependency on git_array

7d07941

examples: ls-files: update print_paths to print all cases

29ca3f3

examples: ls-files: print entry path only when entry is found

d7394c3

cjhoward92 force-pushed the examples/ls-files branch from 02e8598 to d7394c3 Compare March 27, 2018 14:27

pks-t approved these changes Apr 30, 2018

View reviewed changes

tests: ls-files: use puts instead of printf and fix typos

8aa437e

pks-t merged commit 0c6f631 into libgit2:master May 4, 2018

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Uh oh!

Conversation

cjhoward92 commented Oct 18, 2017

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjhoward92 commented Oct 20, 2017

Uh oh!

cjhoward92 commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjhoward92 commented Oct 26, 2017

Uh oh!

cjhoward92 commented Oct 26, 2017

Uh oh!

cjhoward92 commented Nov 1, 2017

Uh oh!

pks-t commented Nov 3, 2017

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjhoward92 commented Nov 14, 2017

Uh oh!

ethomson commented Nov 14, 2017

Uh oh!

cjhoward92 commented Nov 14, 2017

Uh oh!

cjhoward92 commented Dec 18, 2017

Uh oh!

ethomson commented Dec 19, 2017

Uh oh!

ethomson commented Dec 19, 2017

Uh oh!

cjhoward92 commented Dec 19, 2017

Uh oh!

cjhoward92 commented Dec 20, 2017

Uh oh!

cjhoward92 commented Jan 4, 2018

cjhoward92 commented Oct 23, 2017 •

edited

Loading