Johan Herland [off-list ref] writes:
On Saturday 18 June 2011, Yann Dirson wrote:
quoted
Signed-off-by: Yann Dirson <redacted>
Please mention in the commit message that the commit merely replaces
write_or_die()/int fd with the corresponding stdio functionality, and that
there is no (intended) change in behavior. It was not apparent from your
commit message that you had not made any other changes.
Otherwise the patch looks OK.
I had an impression that you would lose a lot of error checking, unless
you are careful, if you go from write_or_die() to stdio.
On Monday 20 June 2011, Junio C Hamano wrote:
Johan Herland [off-list ref] writes:
quoted
On Saturday 18 June 2011, Yann Dirson wrote:
quoted
Signed-off-by: Yann Dirson <redacted>
Please mention in the commit message that the commit merely replaces
write_or_die()/int fd with the corresponding stdio functionality, and
that there is no (intended) change in behavior. It was not apparent
from your commit message that you had not made any other changes.
Otherwise the patch looks OK.
I had an impression that you would lose a lot of error checking, unless
you are careful, if you go from write_or_die() to stdio.
Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one
needs to check the return value, and handle errors accordingly.
An alternative solution would be to drop this patch, and instead use
strbuf_addf() to get the format printing functionality needed in PATCH 3/6.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
On Mon, Jun 20, 2011 at 09:41:54AM +0200, Johan Herland wrote:
On Monday 20 June 2011, Junio C Hamano wrote:
quoted
Johan Herland [off-list ref] writes:
quoted
On Saturday 18 June 2011, Yann Dirson wrote:
quoted
Signed-off-by: Yann Dirson <redacted>
Please mention in the commit message that the commit merely replaces
write_or_die()/int fd with the corresponding stdio functionality, and
that there is no (intended) change in behavior. It was not apparent
from your commit message that you had not made any other changes.
Otherwise the patch looks OK.
I had an impression that you would lose a lot of error checking, unless
you are careful, if you go from write_or_die() to stdio.
Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one
needs to check the return value, and handle errors accordingly.
It appears I based my code on buildin/commit.c from 1.7.4.1 - I just
did not realize that this part changed much in between with 098d0e0e.
I'll look into that.
An alternative solution would be to drop this patch, and instead use
strbuf_addf() to get the format printing functionality needed in PATCH 3/6.
I have thought about that, but that will make the i18n process for the
template much more awkward - and we probably don't want to reimplement
stdio formatting for strbuf.
--
Yann.
On Mon, Jun 20, 2011 at 08:48:42PM +0200, Yann Dirson wrote:
On Mon, Jun 20, 2011 at 09:41:54AM +0200, Johan Herland wrote:
quoted
On Monday 20 June 2011, Junio C Hamano wrote:
quoted
Johan Herland [off-list ref] writes:
quoted
On Saturday 18 June 2011, Yann Dirson wrote:
quoted
Signed-off-by: Yann Dirson <redacted>
Please mention in the commit message that the commit merely replaces
write_or_die()/int fd with the corresponding stdio functionality, and
that there is no (intended) change in behavior. It was not apparent
from your commit message that you had not made any other changes.
Otherwise the patch looks OK.
I had an impression that you would lose a lot of error checking, unless
you are careful, if you go from write_or_die() to stdio.
Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one
needs to check the return value, and handle errors accordingly.
It appears I based my code on buildin/commit.c from 1.7.4.1 - I just
did not realize that this part changed much in between with 098d0e0e.
I'll look into that.
Hm. So now builtin/commit.c heavily relies on status_printf_*. Those
do not do much more return-checking on fprintf() than the previous
code - but at least they provide a single point where such
return-checking can be inserted, which is already better.
Now, those require a wt_status struct... but AFAICT, it only uses the
FILE* inside. This seems a bit annoying for the purpose of reusing
the #-prefixing and line-folding mechanism in builtin/notes.c. Would
replacing those funcs with FILE*-based ones - let's say,
status_printf_*_fp() - and wrappers with the current names in
wt-status.h be seen as a good idea ?
quoted
An alternative solution would be to drop this patch, and instead use
strbuf_addf() to get the format printing functionality needed in PATCH 3/6.
I have thought about that, but that will make the i18n process for the
template much more awkward - and we probably don't want to reimplement
stdio formatting for strbuf.
... and for that matter, it looks like status_printf* provide us with
all that's needed.
--
Yann