Linus Torvalds [off-list ref] writes:
On Sun, 11 Mar 2007, Jim Meyering wrote:
quoted
I like the idea of the new ':/<oneline prefix>' notation, and gave it
a try, but all I could get was a segfault. It was dereferencing a NULL
commit list. Fix below. With it, this example now works:
The fix is correct, but not complete.
quoted
- while ((commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {
+ while (list && (commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {
The old code was broken, but the new one isn't much better.
"pop_most_recent_commit()" simply doesn't work that way. It *never*
returns NULL. So having it as part of a while-loop was buggy to begin
with, and you fixed the test, but the thing is, it should just look like
while (list) {
struct commit *commit;
commit = pop_most_recent_commit(&list, ONELINE_SEEN);
..
and the "pop_most_recent_commit()" simply shouldn't be part of the
conditional at all.
That's what I did in my tentative commit based on Jim's patch
(except "commit" is also used to determine the return value from
the function).
On Sun, 11 Mar 2007, Junio C Hamano wrote:
quoted
"pop_most_recent_commit()" simply doesn't work that way. It *never*
returns NULL. So having it as part of a while-loop was buggy to begin
with, and you fixed the test, but the thing is, it should just look like
while (list) {
struct commit *commit;
commit = pop_most_recent_commit(&list, ONELINE_SEEN);
..
and the "pop_most_recent_commit()" simply shouldn't be part of the
conditional at all.
That's what I did in my tentative commit based on Jim's patch
(except "commit" is also used to determine the return value from
the function).
Your version is *also* buggy.
And the bug is exactly that return value.
You simply cannot do it that way. You do
...
commit = pop_most_recent_commit(&list, ONELINE_SEEN);
if (!commit)
break;
...
which is totally broken and pointless, and more importantly, *exactly*
because you use "commit" outside the loop, it's broken.
It will now return the last commit it ever saw, whether that commit
matches the buffer or not. Because "commit" will *never* be set to NULL
ever again after the initial assignment!
The fact is, if you don't do it the way I described:
while (list) {
struct commit *commit;
commit = pop_most_recent_commit(&list, ONELINE_SEEN);
...
you will almost certainly have a bug. Why? Because the return value of
"pop_most_recent_commit()" simply *makes*no*sense* outside the loop. It's
that simple.
Outside the loop, you have to have a *different* return value, namely the
one that matches. Which may be NULL. Something that
pop_most_recent_commit() simply never returns!
So I repeat my suggestion:
- either use the sequence above
- or change the semantics of "pop_most_recent_commit()" to return NULL if
the list is empty.
Considering that there's now been two totally broken versions of this
loop, with the exact same bug, I'm inclined to say that you should just
change "pop_most_recent_commit()" instead, and then change the loop back
to its original form of "while ((commit = pop_..) != NULL) {"
Otherwise, please apply something like this, which also fixes the return
value. We should return -1 on error!
We should add a test with
git log :/hjashjs
or similar. It's supposed to return an error ("fatal: ambiguous argument
':/hjashjs': unknown revision or path not in the working tree.")
Linus
[ Same return value rules as for the kernel and for system calls, either:
- 0/1 for "false/true", where _1_ is success, and 0 is "false".
- negative/0 for "error/no-error", where _0_ is success, and negative
is a failure.
but not some mixture with 1 being failure! ]
---diff --git a/sha1_name.c b/sha1_name.c
index 6b8b67b..40dd2d1 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -602,10 +602,10 @@ static int handle_one_ref(const char *path,
*/
#define ONELINE_SEEN (1u<<20)
-int get_sha1_oneline(const char *prefix, unsigned char *sha1)
+static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
{
struct commit_list *list = NULL, *backup = NULL, *l;
- struct commit *commit = NULL;
+ int retval = -1;
if (prefix[0] == '!') {
if (prefix[1] != '!')@@ -619,6 +619,7 @@ int get_sha1_oneline(const char *prefix, unsigned char *sha1)
commit_list_insert(l->item, &backup);
while (list) {
char *p;
+ struct commit *commit;
commit = pop_most_recent_commit(&list, ONELINE_SEEN);
if (!commit)@@ -628,13 +629,14 @@ int get_sha1_oneline(const char *prefix, unsigned char *sha1)
continue;
if (!prefixcmp(p + 2, prefix)) {
hashcpy(sha1, commit->object.sha1);
+ retval = 0;
break;
}
}
free_commit_list(list);
for (l = backup; l; l = l->next)
clear_commit_marks(l->item, ONELINE_SEEN);
- return commit == NULL;
+ return retval;
}
/*
On Mon, 12 Mar 2007, Linus Torvalds wrote:
Otherwise, please apply something like this, which also fixes the return
value. We should return -1 on error!
Btw, I didn't remove the
if (!commit)
break;
but I should have. It doesn't actually matter (since it will never
trigger), but it adds to the confusion around pop_most_recent_commit.
Linus