@MrSubidubi

We generally discourage pinging maintainers unless there was a prior discussion/agreement about this. Feel free to educate me should that have been the case, but just as an FYI

I had the pairing session with Cole on this one, so this PR was created for the pairing session and I will adjust it based on the conclusion.

Appreciate the context, was obviously unaware of that.

@cole-miller
Alright, so I updated the branch as per our conclusion. But! It still does run the git for-each-ref --format ... command the second time and I can explain why (despite that we talked about a simple case -> if it failed the for-each-ref command the first time you just parse what you can and continue).

I investigated the original git source code here for the actual implementation of for-each-ref and how it decides whether or not it should load/parse the actual git object (from the .git/objects folder or things like packs/alternates)

If you look here at the "valid atoms" ("fields" in our terminology) you can see that objectname is SOURCE_OTHER andHEAD + refname are both SOURCE_NONE and everything else: parent, authorname, committerdate, contents, etc are SOURCE_OBJ (well, upstream is also SOURCE_NONE but problematic because of upstream:track, but let's not really dig into it).

There is a logic in which if it is SOURCE_OBJ it sets those fields here

if (*atom == '*')
     oi_deref.info.contentp = &oi_deref.content;
else
     oi.info.contentp = &oi.content;
}

And then it will decide based on those fields whether it should call get_object():

if (!memcmp(&oi.info, &empty, sizeof(empty)) &&
	    !memcmp(&oi_deref.info, &empty, sizeof(empty)))
		return 0;

So, those 3 fields does not require the same object-loading path: HEAD, refname, objectname. And by doing that we will make sure that we iterate on all refs because if we simply "collect what we can" we might end up being broken on the first branch and parse 0 branches.

If you still think we shouldn't do this second command I can remove it but IMO it makes the fix robust and is "safe enough" to use (as your original concern was "why it will not fail again if the first one failed" -> because it will not go into the same path in the git's code 😄 )

Thanks for the additional research @GoldStrikeArch. However, I still think it's better here not to call a fallback command. I think if a user has some git refs that are broken in this way (which seems rare), it's okay for the branch picker to be degraded, and not worth the complexity of a second command invocation to try to recover some of the refs we couldn't get before (especially because interacting with those refs might not work for the same reason we couldn't process them here). I think a better path forward would be to try to surface errors on this codepath better in the UI, so that people don't have to check their logs to see what went wrong. (And parsing whatever lines we can from the output of the existing command still seems reasonable too, since we can do that without much additional complexity.)

@cole-miller Alright, so I reverted the changes and now we are doing exactly that:

• running git for-each-ref --format... once and parse what we can (the parsing logic didn't change as it was already robust enough)
• and then show this error in "branch picker UI" if the status code was not 0 and there were any errors:

@GoldStrikeArch Thanks! I tweaked the UI slightly but this looks good otherwise--merging.