Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.

[Yann Dirson <ydirson@free.fr>]

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:
> > On Monday 20 June 2011, Junio C Hamano wrote:
> > > Johan Herland <johan@herland.net> writes:
> > > > On Saturday 18 June 2011, Yann Dirson wrote:
> > > >> Signed-off-by: Yann Dirson <ydirson@free.fr>
> > > > 
> > > > 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 ?


> > 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