On Mon, Sep 24, 2018 at 11:20:34PM +0200, SZEDER Gábor wrote: > > Would we perhaps want to give the user a hint that the object is not > > really missing, but rather that we're not in a repository? E.g., > > something like: > > > > if (!have_git_dir()) > > return strbuf_addf_ret(err, -1, "format specifier requires a > > repository"); > > if (oid_object_info_extended(...)) > > return ...; > > > > ? > > I think it makes sense. > > I wanted to preserve the error message, because the description of > '--sort=<key>' in 'Documentation/git-ls-remote.txt' explicitly > mentions it, and I added the condition at this place because I didn't > want to duplicate the construction of the error message.
Ah, I didn't realize we actually documented that. And perhaps it is more consistent, too: you'd get different results from running "ls-remote" outside a repository versus one that just doesn't have the objects from the other side. > However, if we go for a more informative error message, then wouldn't > it be better to add this condition in populate_value() before it even > calls get_object()? Then we could also add the problematic format > specifier to the error message (I think, but didn't actually check), > just in case someone specified multiple sort keys. Yeah, that probably would be a better place. Though your response also has made me think that maybe just sticking with the "missing object" response is reasonable. I don't have a strong opinion between the two. -Peff
