Compare buffers in diff example by albfan · Pull Request #5125 · libgit2/libgit2 · GitHub

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

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

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

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

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.

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.

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

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…

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.

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.

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.

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.

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.

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

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 :)

@pks-t check #5153

Sorry, took me longer than expected to review this

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

Consolidate all standard includes and defines into "common.h". This lets us avoid having to handle platform-specific things in multiple places.

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

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

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?

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.

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

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

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

albfan force-pushed the wip/albfan/diff_buffers branch 4 times, most recently from 61687c1 to 1a214d6 Compare June 18, 2019 19:27

albfan closed this Jun 18, 2019

albfan reopened this Jun 18, 2019

albfan force-pushed the wip/albfan/diff_buffers branch from 1a214d6 to 93de380 Compare June 18, 2019 19:35

tiennou requested changes Jun 18, 2019

View reviewed changes

neithernut reviewed Jun 18, 2019

View reviewed changes

Comment thread examples/diff.c Outdated

Comment thread examples/diff.c Outdated

Comment thread examples/diff.c Outdated

Comment thread examples/diff.c Outdated

albfan force-pushed the wip/albfan/diff_buffers branch 2 times, most recently from b82a02e to 64dbe94 Compare June 19, 2019 00:29

tiennou requested changes Jun 19, 2019

View reviewed changes

albfan force-pushed the wip/albfan/diff_buffers branch 2 times, most recently from 35dc4c3 to d08fd11 Compare June 19, 2019 10:06

tiennou reviewed Jun 20, 2019

View reviewed changes

Comment thread examples/diff.c Outdated

albfan force-pushed the wip/albfan/diff_buffers branch from d08fd11 to 4d8aec5 Compare June 20, 2019 10:54

albfan mentioned this pull request Jun 29, 2019

diff from buf from patch fails if any buffer has no newline at e #5153

Closed

pks-t requested changes Jul 4, 2019

View reviewed changes

Comment thread examples/common.c Outdated

Comment thread examples/common.h Outdated

Comment thread examples/diff.c

Comment thread examples/diff.c

albfan force-pushed the wip/albfan/diff_buffers branch from 4d8aec5 to 52d887a Compare July 4, 2019 08:53

albfan force-pushed the wip/albfan/diff_buffers branch from 52d887a to 3caab05 Compare July 4, 2019 09:08

pks-t requested changes Jul 5, 2019

View reviewed changes

Comment thread examples/common.c

Comment thread examples/common.c Outdated

Comment thread examples/diff.c

Comment thread examples/diff.c

pks-t mentioned this pull request Jul 5, 2019

patch_parse: do not depend on parsed buffer's lifetime #5158

Merged

albfan force-pushed the wip/albfan/diff_buffers branch 2 times, most recently from 93fdb01 to bae7597 Compare July 5, 2019 10:48

neithernut reviewed Jul 5, 2019

View reviewed changes

Comment thread examples/common.h Outdated

Compare buffers in diff example

3be09b6

albfan force-pushed the wip/albfan/diff_buffers branch from bae7597 to 3be09b6 Compare July 5, 2019 12:12

examples: consolidate includes into "common.h"

960d2a0

Consolidate all standard includes and defines into "common.h". This lets us avoid having to handle platform-specific things in multiple places.

pks-t approved these changes Jul 11, 2019

View reviewed changes

pks-t merged commit 56e7aaf into libgit2:master Aug 1, 2019

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

Compare buffers in diff example by albfan · Pull Request #5125 · libgit2/libgit2 · GitHub

Uh oh!

Compare buffers in diff example - #5125

Compare buffers in diff example#5125pks-t merged 2 commits intolibgit2:masterlibgit2/libgit2:masterfrom albfan:wip/albfan/diff_buffersalbfan/libgit2:wip/albfan/diff_buffersCopy head branch name to clipboard

Conversation

albfan commented Jun 18, 2019

Uh oh!

albfan commented Jun 18, 2019

Uh oh!

neithernut commented Jun 18, 2019

Uh oh!

ethomson commented Jun 18, 2019

Uh oh!

albfan commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiennou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tiennou commented Jun 18, 2019

Uh oh!

neithernut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tiennou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

albfan commented Jun 20, 2019

Uh oh!

Uh oh!

pks-t commented Jun 27, 2019 via email

Uh oh!

pks-t commented Jun 27, 2019 via email

Uh oh!

albfan commented Jun 27, 2019

Uh oh!

pks-t commented Jun 27, 2019 via email

Uh oh!

albfan commented Jun 29, 2019

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pks-t commented Jul 5, 2019

Uh oh!

albfan commented Jul 5, 2019

Uh oh!

albfan commented Jul 8, 2019

Compare buffers in diff example#5125
pks-t merged 2 commits into
libgit2:masterfrom
albfan:wip/albfan/diff_buffers

albfan commented Jun 18, 2019 •
edited
Loading