diff_no_index() fails to read files

[hausdorf]

PROBLEM:

Calls to diff_no_index() fail even when provided with correct paths for files.

RESOURCES:

Code ran is here:

int main() {
    git_diffresults_conf *conf;
    git_repository *repo;
    git_repository_open(&repo, "");
    git_index *index;
    git_commit *commit1;
    git_commit *commit2;

    //git_diff(diffdata, commit1, repo);

    printf("MAIN\n");
    printf("%d\n", git_diff_no_index(&conf, "difftest_before", "difftest_after"));

    //git_diff_cached(diffdata, commit1, index);
    //git_diff_commits(diffdata, commit1, commit2);
}

ls gives us this:

a.out           difftest_after  difftest_before main.c          tests.c

tests.c is unrelated.

[basepi]

Referencing the title of the issue, how do you know they aren't read?

[basepi]

Also, which version of the function are you using, the one that uses @kyena's load_file() method, or the one that uses fileops methods?

[vimalloc]

Check the newest one on branch development-interface. I have confirmed that it is working on my machine (i pushed it up a few hours ago).

[vimalloc]

Also, the above code wouldn't print anything out, as the diff function for git_diff_no_index is commented out. If you add printf() in git_diff_no_index you can see that the files get successfully loaded into memory.

[hausdorf]

I am using the current version of your code.

[vimalloc]

Ah, you need to put the full path to the file, aka (/home/drsleep/code/libgit/.../difftestafter, etc).

[hausdorf]

They do not get loaded. The printfs fail at existence check.

[hausdorf]

Oh. I don't think that should be desired behavior.

[vimalloc]

I disagree there. One way or another we would have to get the full filepath to the file, and because this is supposed to be cross platform we would have to change the filepath differently to get that full filepath. It seems like that would be out of the scope of this function and should be the callers responsibility, at least in my opinion.

[vimalloc]

If we do want to do it the other way though it can totally be done. However for the time being I would like to get the rest of the git_diff stuff working, before we worry about that.

[hausdorf]

I disagree. As a caller of this library, I would want it to use the pwd by default. The filesystem stuff should be handled transparently.

[basepi]

There are some path prettification methods in fileops that might do the trick. On Apr 22, 2011 1:01 PM, "DrSleep" < reply@reply.github.com> wrote:

Oh. I don't think that should be desired behavior.

Reply to this email directly or view it on GitHub: https://github.com/crakdmirror/libgit2/issues/32#comment_1045092

[vimalloc]

That would definitely make this much simpler for us :)

[hausdorf]

GOOD NEWS: Looking at this closely, it seems that relative paths are actually covered by default.

BAD NEWS: There's still an error. The problem appears to be that this block always trips:

    /* Check if either given path is a directory */
    if(gitfo_isdir(filepath1) || gitfo_isdir(filepath2)) {
        printf("HSDF\n");
        result = GIT_EINVALIDPATH;
        goto cleanup;
    }

[vimalloc]

What branch are you getting that code from? In development-interface we actually removed the gitfo_isdir() all together.

For the above code, I was looking through it before I removed it, and i think it turned out that gitfo_isdir() returns -1 if the file isn't a directory, and 0 if it is a directory (backward imo), and thus if it wasn't a directory it returned -1 which evaluated to true.

[hausdorf]

Hmm. Conditionals seem to be wrong in several places. For example:

if(gitfo_exists(filepath1) || gitfo_exists(filepath2))

What's getting returned here is a 0 if it's correct and an error code if not. So it should be

if(gitfo_exists(filepath1) < 0 || gitfo_exists(filepath2) < 0)

That's a pretty minor error. This next one does not seem to be:

if(gitfo_isdir(filepath1) == 0 || gitfo_isdir(filepath2) == 0)

So what we want to do is throw an error if either of these is true. But if they're true, they don't return true, they return 0. Thus we need to check if either of them returns 0.

Please correct me if this is wrong. If it's not, we do need to correct all similar errors.

[vimalloc]

Read above comment. All of those have been replaced with gitfo_read_file() in our branch.

[hausdorf]

To answer your question, I am getting it from your current branch:

git log development..upstream/development-interface

returns nothing.

[vimalloc]

I don't think you have the most current version of our branch, or if you do it was improperly merged or something. See here:

https://github.com/crakdmirror/libgit2/blob/development-interface/src/diff.c#L14-52

[hausdorf]

The code comes specifically from here.

[basepi]

Uhhhhh, are you sure? I could swear that I put NOTs in front of those exists. The gitfo_isdir() calls don't have NOTs, because they shouldn't. Also, I didn't have the ==0 stuff on the gitfo_isdir() calls. Did you add those, @kyeana?

OK, I just went and checked the source, and none of those conditionals exist as you've stated them, as far as I can tell. Also, that's a weird pull command.

So it works just with the read, @kyeana? I see you've removed all my checks. Is that wise, removing all the exists checks and so forth?

On Fri, Apr 22, 2011 at 9:04 PM, DrSleep < reply@reply.github.com>wrote:

Hmm. Conditionals seem to be wrong in several places. For example:

if(gitfo_exists(filepath1) || gitfo_exists(filepath2))

What's getting returned here is a 0 if it's correct and an error code if not. So it should be

if(gitfo_exists(filepath1) < 0 || gitfo_exists(filepath2) < 0)

That's a pretty minor error. This next one does not seem to be:

if(gitfo_isdir(filepath1) == 0 || gitfo_isdir(filepath2) == 0)

So what we want to do is throw an error if either of these is true. But if they're true, they don't return true, they return 0. Thus we need to check if either of them returns 0.

Please correct me if this is wrong. If it's not, we do need to correct all similar errors.

Reply to this email directly or view it on GitHub: https://github.com/crakdmirror/libgit2/issues/32#comment_1046465

[basepi]

That's from development, @DrSleep, not development-interface. Every time you go back to github it resets you to the default branch, which is in this case development.

On Fri, Apr 22, 2011 at 9:10 PM, DrSleep < reply@reply.github.com>wrote:

The code comes specifically from here.

Reply to this email directly or view it on GitHub: https://github.com/crakdmirror/libgit2/issues/32#comment_1046476

[vimalloc]

@alex, that is development, not development interface

@colton, yes. I have tested it with a valid file, an invalid file, and a directory, Unless a valid file is passed in, an error is returned from that method (it takes care of all those tests for us)

[hausdorf]

That's not a pull command, that's a log command.

Also, development-ready code I assumed was actually in development.

So...

[basepi]

But yes, going back to what I said before, I don't know that it's wise to drop all of the exists checks and the isdir checks. I'd be interested to know your reasoning behind removing those. And I'm far too lazy to check your commit messages. =P

[vimalloc]

Ok... well we were just following protocol keeping code in our own branch. I had no plans to merge it into development until it had been reviewed by other people.

[vimalloc]

@colton, that method will return an error if something that isn't a valid filepath, thus the checks are already taken care of us in there, so there is no reason to check twice.

[hausdorf]

@kyeana You're right, don't check it twice. I saw that too.

But to answer your question, I thought Colton was reviewing your code. Is this wrong?

[vimalloc]

We are reviewing each others code, we just haven't gotten around to this portion of code yet. I changed a significant portion of it today.

[hausdorf]

@crakdmirror, @kyeana, There are pretty severe merge conflicts, as merge conflicts go. If it's development-ready, I do need that code, and would be really appreciative if you merged into development. If it's not, what do you need to get it dev-ready?

[basepi]

...well......all I know is what's in my development-interface branch. Which is the most up-to-date interface code, so I will almost always assume you're looking in that branch when we're talking about the interface code. @kyeana, you should use usernames, not first names, or leave off the @ completely, as the @ tags link to usernames, not real names. So @alex leads to some random dude with alex as his username. Additionally, are we sure that the error message is the same if the read fails? Will it return a GIT_EINVALIDPATH or whatever that error was? Because the advantage of all the checks is returning the correct error. In the case of those checks, it's always the invalid path error, whether it's a directory or not. But I assume that the read method will probably NOT return an invalid path error, but rather some sort of uninitialized file reference error or something. I guess I should go look at the gitfo_read() method directly.

[hausdorf]

@crakdmirror, it does return the correct error code. I've looked at it myself.

[alex]

I would appreciate that to a wonderful degree :)

[vimalloc]

@alex hahahahahaha, sorry, my bad mate :P @crakdmirror point taken :)

[hausdorf]

Ahahahahahahah. Awesome. Nice to meet you, @alex.

[basepi]

Hehe, forgot about the mentions as well. Sorry, mate. =)

@DrSleep, it looks like it just returns GIT_ERROR at each juncture if there's an error. Don't we want invalid path instead, or whatever?

[hausdorf]

@crakdmirror, I really have no idea what's going on with interface code. You or @kyeana should merge it, because you do.

As for the GIT_ERROR stuff, I dunno. I'll think about it. Probably.

[hausdorf]

I actually have no idea why I'm even getting merge conflicts. I haven't even committed to diff.c since you guys merged awhile back. No idea what's going on.

[vimalloc]

No worries, I'm taking care of it right now :)

On Fri, 22 Apr 2011 21:35:51 -0600, DrSleep
reply@reply.github.com
wrote:

I actually have no idea why I'm even getting merge conflicts. I haven't
even committed to diff.c since you guys merged awhile back. No idea
what's going on.

Using Opera's revolutionary e-mail client: http://www.opera.com/mail/

[hausdorf]

kisses landon

[vimalloc]

/blush

Its up. I tested it with git_diff() and that was working, but didn't check
with git_diff_no_index(); Give it a shot and let me know if there are any
problems.

Cheers

On Fri, 22 Apr 2011 21:39:43 -0600, DrSleep
reply@reply.github.com
wrote:

kisses landon

Using Opera's revolutionary e-mail client: http://www.opera.com/mail/

[hausdorf]

Nope, seems to be good work.

Also, again, a general, friendly reminder to watch your spaces. Not to belabor the point. You guys know I don't like bringing it up.

[basepi]

Admit it, you love bringing it up! =P JK, mate. Adding a highlight to my vimrc now.

On Fri, Apr 22, 2011 at 9:50 PM, DrSleep < reply@reply.github.com>wrote:

Nope, seems to be good work.

Also, again, a general, friendly reminder to watch your spaces. Not to belabor the point. You guys know I don't like bringing it up.

Reply to this email directly or view it on GitHub: https://github.com/crakdmirror/libgit2/issues/32#comment_1046550

[vimalloc]

I will do the same :P

[hausdorf]

Thank you so much. I reeeeeeeeally actually do hate bringing it up, because I hate confrontation.

[basepi]

Needs to happen sometimes, though. We need to be kicked in the pants and all that from time to time.

On Fri, Apr 22, 2011 at 9:54 PM, DrSleep < reply@reply.github.com>wrote:

Thank you so much. I reeeeeeeeally actually do hate bringing it up, because I hate confrontation.

Reply to this email directly or view it on GitHub: https://github.com/crakdmirror/libgit2/issues/32#comment_1046559