GNOME Bugzilla – Bug 702259
No diff for first commit of every repository
Last modified: 2013-06-21 13:04:32 UTC
Created attachment 246800 [details] Screenshot of Gitg unable to show diff for first commit of a git repository I am unable to see diff of only the first commit of every git repository. Tested this behaviour on gtk+ repository and a test repository (screenshot included here).
Created attachment 246801 [details] Screenshot of Gitg unable to show diff for first commit of gtk+ repository
Created attachment 246984 [details] [review] Fix for loading diff of first commit, bug 702259 Please review, thanks.
Review of attachment 246984 [details] [review]: Some small code style nitpicks inline. The patch seems correct, however the way the issue is fixed makes the logic hard to follow. It doesn't seem natural to iterate over numparents + 1, and then special case parents() > 0. Maybe you could instead factor out the code block that computes the diff, and then to a higherlevel check on commits that don't have parents. ::: libgitg/gitg-commit.vala @@ +154,2 @@ { + Ggit.Tree parent_tree = null; Pleas add a newline after this @@ +155,3 @@ + Ggit.Tree parent_tree = null; + if (parents.size() > 0) + parent_tree = parents.get(0).get_tree(); Please always use curly braces around the body of if statements
Created attachment 247342 [details] [review] Fix diff view for first commit, bug 702259 Simplified the logic :) Please review, thank you.
Created attachment 247343 [details] [review] Fix diff view for first commit, bug 702259 I noticed the indenting was a bit off in the previous patch and the errors in the terminal (including a warning in 'make' process) when gitg was run needed a bit of explanation. make[2]: Entering directory `/home/sindhu/code/gitg/libgitg' VALAC libgitg_1_0_la_vala.stamp gitg-commit.vala:179.40-179.43: warning: Argument 2: Cannot pass null to non-null parameter type null, ^^^^ Compilation succeeded - 1 warning(s) CC libgitg_1_0_la-gitg-commit.lo CCLD libgitg-1.0.la ** (gitg:30300): CRITICAL **: _ggit_native_get: assertion 'GGIT_IS_NATIVE (self)' failed Please refer to [1] This thing checks for something to not be NULL again. So even though nacho's commits[2][3] "allows" NULL to be passed, a lot of the older code still prevents accepting NULL as an argument The simplest explanation is: Whatever arguments are passed to libgit2-glib functions, currently it calls "_ggit_native_get" on all of the arguments when NULL is passed to _ggit_native_get, it throws a critical error, and returns another NULL. So if you call diff_tree_to_tree (old_tree, NULL), before it would throw a critical error because "tree_to_tree" could not accept NULL. [1] https://git.gnome.org/browse/libgit2-glib/tree/libgit2-glib/ggit-native.c?id=8a95d39e9098af4a4d18f47abfc4420a596b4acc#n101 [2] https://git.gnome.org/browse/libgit2-glib/commit/?id=204fc976f8d2d2f227335ce4d67ee26b711a5c5a [3] https://git.gnome.org/browse/libgit2-glib/commit/?id=f91b4f626bd8fcdacb11e19ea7a39dc6f1d6dc5e
Created attachment 247365 [details] [review] Fix diff view for first commit, bug 702259 Added space around '=='.
(In reply to comment #5) > gitg-commit.vala:179.40-179.43: warning: Argument 2: Cannot pass null to > non-null parameter type > null, This is most likely caused by somehow vala not picking up on allow-none. Check if the vapi marks the arguments correctly as nullable > This thing checks for something to not be NULL again. So even though nacho's > commits[2][3] "allows" NULL to be passed, a lot of the older code still > prevents accepting NULL as an argument > > The simplest explanation is: Whatever arguments are passed to libgit2-glib > functions, currently it calls "_ggit_native_get" on all of the arguments when > NULL is passed to _ggit_native_get, it throws a critical error, and returns > another NULL. So if you call diff_tree_to_tree (old_tree, NULL), before it > would throw a critical error because "tree_to_tree" could not accept NULL. I don't think so. Just look at the code in libgit2-glib. It only calls _ggit_native_get when it's not NULL... There is no older code here. It's more likely that because vala thinks it's not allowed to pass a null value, it gives it some kind of bogus argument instead, which then is not a valid GgitNative... > > [1] > https://git.gnome.org/browse/libgit2-glib/tree/libgit2-glib/ggit-native.c?id=8a95d39e9098af4a4d18f47abfc4420a596b4acc#n101 > [2] > https://git.gnome.org/browse/libgit2-glib/commit/?id=204fc976f8d2d2f227335ce4d67ee26b711a5c5a > [3] > https://git.gnome.org/browse/libgit2-glib/commit/?id=f91b4f626bd8fcdacb11e19ea7a39dc6f1d6dc5e
Review of attachment 247365 [details] [review]: See comment inline. ::: libgitg/gitg-commit.vala @@ +174,3 @@ } + + if (parents.size() == 0) I think we should check this on the if from above: if (parents.size() == 0) { } else if (i == 0) { } else { }
Created attachment 247426 [details] [review] Fix diff view for first commit, bug 702259 Patch now checks for parents.size()==0 right at the begining of building the diff thus one less check. Please review, thanks.
Review of attachment 247426 [details] [review]: Looks good to me.
Looks fine, but is the null problem solved or not yet?
Pushed to master in efac068 Available at: https://git.gnome.org/browse/gitg/commit/?id=efac068cfea66ecdc5bdea36e5f5792dead2cf74 Thank you for the reviews. Jesse, the errors I speak about in comment 5 were not solved, leaving bug open.
No errors for me, I guess you need to rebuild libgit2-glib
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.