Hi Phillip,
On Fri, 17 Aug 2018, Phillip Wood wrote:
> On 10/08/2018 17:51, Alban Gruin wrote:
>
> > +{
> > + const char *quiet = getenv("GIT_QUIET");
> > +
> > + if (head_name)
> > + write_file(rebase_path_head_name(), "%s\n", head_name);
>
> write_file() can call die() which isn't encouraged for code in libgit.
> I'm not sure how much it matters in this case. Rewriting all these as
>
> if (head_name && write_message(onto, strlen(onto), rebase_path_onto(),
> 1))
> return -1;
>
> is a bit tedious. An alternative would be it leave it for now and in the
> longer term move this function (and the ones above which I've just
> noticed also call write_file()) to in builtin/rebase.c (assuming that
> builtin/rebase--interactive.c and builtin/rebase.c get merged once
> they're finalized - I'm not sure if there is a plan for that or not.)
This came up in the review, and Alban said exactly what you did.
I then even dragged Peff into the discussion, as it was his idea to change
`write_file()` from returning an `int` to returning a `void` (instead of
libifying the function so that it would not `die()` in error cases and
`return 0` otherwise):
https://github.com/git/git/pull/518#discussion_r200606997
Christian Couder (one of Alban's mentors) then even jumped in and *agreed*
that libifying code "could be seen as unnecessary code churn and
rejected."
In light of these two respected community members suggesting to Alban to
go and not give a flying fish about proper error handling, I have to admit
that I am sympathetic to Alban simply using `write_file()` as-is.
I do agree with you, of course, that the over-use of `die()` in our code
base is a pretty bad thing.
But that's neither Alban's fault, nor should he be punished for the advice
he has been given.
In short: I agree with you that `write_file()` should be libified
properly, and I would suggest not to burden Alban with this (Alban, of
course you should feel free to work on this if this is something you care
about, too).
Ciao,
Dscho
