Skip to contentSkip to conversation

builtin/tag.c: return appropriate value when --points-at finds an empty list

PATCH In review George Papanikolaou 2 participants 3 messages Dec 11, 2017

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 builtin/tag.c: return appropriate value when --points-at finds an empty list

git tag --points-at can simply return if the given rev does not have any tags pointing to it. It's not a failure but it shouldn't return with 0 value.

---
 builtin/tag.c | 4 ++++
 1 file changed, 4 insertions(+)
builtin/tag.c +4 -0
index b38329b59..68b84db2a 100644
@@ -58,6 +58,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 		die(_("unable to parse format string"));
 	filter->with_commit_tag_algo = 1;
 	filter_refs(&array, filter, FILTER_REFS_TAGS);
+
+	if (array.nr == 0)
+		return -1;
+
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++)
Signature · 1 line
2.11.0
#1 lore raw
Quoted 3 lines · George Papanikolaou

git tag --points-at can simply return if the given rev does not have any tags pointing to it. It's not a failure but it shouldn't return with 0 value.

I disagree. I think the 0 return means "I completed successfully" and the empty output means "I didn't find any tags pointing to this object."

Changing the return value here could break a lot of scripts out in the wild, and I consider this to be an "API" compatibility that needs to stay as-is.

What are you using "--points-at" where you need a nonzero exit code instead of a different indicator?

Thanks,
-Stolee

#2 lore raw

I agree with what you're saying, just I thought this might be ultra-minor for API-breakage. To me, 0 doesn't necessarily mean "I didn't segfault". I lot of tools use ret-values to give information back. And that way it's much easier to just || the command to something else instead of [[ -z ]] in the script.

But I see what you're saying...
--
/ΓΠ

Quoted 19 lines · Derrick Stolee
Quoted 4 lines · George Papanikolaou

git tag --points-at can simply return if the given rev does not have any tags pointing to it. It's not a failure but it shouldn't return with 0 value.

I disagree. I think the 0 return means "I completed successfully" and the empty output means "I didn't find any tags pointing to this object."

Changing the return value here could break a lot of scripts out in the wild, and I consider this to be an "API" compatibility that needs to stay as-is.

What are you using "--points-at" where you need a nonzero exit code instead of a different indicator?

Thanks,
-Stolee