Compare buffers in diff example

[albfan]

Adding compare buffers feature in diff example

This tries to mimic git diff --no-index

echo hello > file1
echo bye > file2
git diff --no-index file1 file2
diff --git 1/file1 2/file2
index 5c1b14949..b023018ca 100644
--- 1/file1
+++ 2/file2
@@ -1 +1 @@
-hello
+bye

Right now this patch does:

$ ./diff --files --text --color
buf: diff --git a/file1 b/file2
index ce01362..b023018 100644
--- a/file1
+++ b/file2
@@ -1 +1 @@
-hello
+bye
diff --git a/file1 b/file2
index ce01362..b023018 100644
--- a/file1
+++ b/file2
@@ -1 +1 @@
-hello
+bye

Actual problems:

• Read real files
• Do not detect buffers as binary files
• Use --no-index as parameter
• Check two files are passed and they exists

[albfan]

Not sure if there is any better code to load an entire file into a char* (or void*):

https://codereview.stackexchange.com/q/163424

[neithernut]

mmap (or rather the portability-interface provided by libgit2)?

[ethomson]

mmap (or rather the portability-interface provided by libgit2)?

Let's not add an example that uses that, it's private API and not available to consumers.

I think that the best example would be simplest, and avoiding mmap is definitely the simple route.

[albfan]

Read files is implemented right now:

$ echo hello > file1
$ echo bye > file2
$ ./lg2 diff --no-index file1 file2
diff --git a/file1 b/file2
index ce01362..b023018 100644
--- a/file1
+++ b/file2
@@ -1 +1 @@
-hello
+bye

@neithernut @ethomson Let me know any further change to do.

We plan to use this on https://gitlab.gnome.org/albfan/diferencia/issues/3 and integrate in libgit2-glib so anyone can diff files without being in a repo.

[tiennou]

A few minor nitpicks, but other that that LGTM. Thanks for working on this @albfan!

[tiennou]

Let's not add an example that uses that, it's private API and not available to consumers.

You do realize some of us are itching to run git's test suite against our "examples", right? Right 😉? Just to say that I'd like to revisit that. Not being able to access libgit2's stdlib makes writing examples painful when you're left longing for futils…

[neithernut]

Disclaimer: not a maintainer. Commenting nonetheless, since requested.

I only had a quick look, mainly at read_file() since that's the part were I meddled initially. Your indentation is partially inconsistent both with the rest of the code-base and in itself. As already pointed out, it's tabs equivalent to 8 spaces for this project.

[tiennou]

Another batch of review comments: there's still a few issues with the file reading function, a few more style issues, and a possible cleanup request (which you must feel free to disregard). I can't merge though, so it'll have to wait for someone who can.

[albfan]

We are on air! https://gitlab.gnome.org/GNOME/libgit2-glib/commit/d5f41735489ac31f389de66edaba7a2c55d67024

thanks!. If you want to avoid creating a patch to extract a diff to do this, I would be glad to help.

[pks-t]

On Tue, Jun 18, 2019 at 01:12:32PM -0700, Etienne Samson wrote: > Let's not add an example that uses that, it's private API and > not available to consumers. You do realize some of us are itching to run git's test suite against our "examples", right? Right 😉? Just to say that I'd like to revisit that. Not being able to access libgit2's stdlib makes writing examples painful when you're left longing for `futils`…

I agree with @ethomson, though. While it'd be nice to be able to run git.git's tests against libgit2, the most important goal of examples is to demonstrate how to use libgit2's API in another project. As such, we just cannot use our own internal APIs, as otherwise it may become much harder for others to simply lift out the code from our examples into their own codebase. I also want to avoid that we start using internal APIs by accident.

[pks-t]

On Wed, Jun 19, 2019 at 01:09:10AM -0700, Etienne Samson wrote: tiennou commented on this pull request. > check_lg2( - git_diff_tree_to_tree(&diff, repo, t1, t2, &o.diffopts), - "diff trees", NULL); - else if (o.cache != CACHE_NORMAL) { - if (!t1) - treeish_to_tree(&t1, repo, "HEAD"); - - if (o.cache == CACHE_NONE) + git_patch_from_buffers(&patch, file1_str, strlen(file1_str), o.treeish1, file2_str, strlen(file2_str), o.treeish2, &o.diffopts), + "patch buffers", NULL); + check_lg2( + git_patch_to_buf(&buf, patch), @pks-t Any opinions ? I'm not a fan of having `check_lg2` in example code in the first place (it makes the error handling brittle), and now it impacts readability (somewhat).

Will try to have a look tomorrow. No promise, though.

[albfan]

Just to mention, we are facing a problem when differing files with no newline at end. Seems patch is not parseable.

Is there any config or workaround? Compare buffers without need to convert to a patch would be nice. Right now we just always add newline at end if it not exists.

Let me know if I can rework any part to get this merged

[pks-t]

On Thu, Jun 27, 2019 at 04:07:01AM -0700, Alberto Fanjul wrote: Just to mention, we are facing a problem when differing files with no newline at end. Seems patch is not parseable. Is there any config or workaround? Compare buffers without need to convert to a patch would be nice. Right now we just always add newline at end if it not exists.

This sounds like a bug to me. Could you please create a bug for this so that we can handle it separately?

Let me know if I can rework any part to get this merged

I'll try to give feedback tomorrow :)

[albfan]

@pks-t check #5153

[pks-t]

Sorry, took me longer than expected to review this

[pks-t]

Thanks for your updates! Some more nits and then we should be mostly good to go.

[pks-t]

I've amended a commit to this PR to fix Win32 builds so that you don't have to handle this :)

[albfan]

@pks-t Nice to see you in action! haha

[albfan]

Still finding github PR hard to follow

Merging can be performed automatically once the requested changes are addressed.

Can't find anything else to do. Is there some "I approve this PR" check to press or something?

[neithernut]

One of the maintainers has to approve the changes (and then merge them). This may take a while for this project due to them being understaffed/limited in bandwidth. So there is nothing else to do except for waiting patiently.

[albfan]

@neithernut I was sure about that. All the process was really awesome and supportive. We will try to provide feedback on #5161, and use as patch meanwhile it is being reviewed.

[pks-t]

Nope, nothing you have to do now :) I'll wait with merging until #5158 has landed though, as you have uncovered a lifetime issue with how we handle parsed patches

[pks-t]

#5158 has been merged, so I've merged this now. Thanks a lot for your work!