On Sun, 11 Mar 2007, Junio C Hamano wrote:
> >
> > "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;
}
/*
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html