Re: [PATCH] config: do not ungetc EOF

From: Jeff King <peff@peff.net>
Date: Feb 5, 2015, 21:28 UTC
Message-ID: <20150205212847.GA2360@peff.net>
In-Reply-To: <xmqqr3u4jbyj.fsf@gitster.dls.corp.google.com>

On Thu, Feb 05, 2015 at 01:16:36PM -0800, Junio C Hamano wrote:

Show 19 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Feb 05, 2015 at 01:53:27AM -0500, Jeff King wrote:
> >
> >> I also notice that config_buf_ungetc does not actually ungetc the
> >> character we give it; it just rewinds one character in the stream. This
> >> is fine, because we always feed the last-retrieved character. I dunno if
> >> it is worth fixing (it also would have fixed this infinite loop, but for
> >> the wrong reason; we would have stuck "-1" back into the stream, and
> >> retrieved it on the next fgetc rather than the same '\r' over and over).
> >
> > Here's a patch to deal with that. I'm not sure if it's worth doing or
> > not.
> 
> I am not sure, either.  If this were to become stdio emulator over
> random in-core data used throughout the system, perhaps.
> 
> But in its current form it is tied to the implementation of config.c
> very strongly, so...

Yeah, that was my thinking, and why I have doubts. Maybe a comment would make more sense, like the patch below. I am also OK with just leaving it as-is.

-- >8 --
Subject: [PATCH] config_buf_ungetc: document quirks in a comment

Our config_buf_ungetc implements just enough for the config code to work. That's OK, but we would not want anyone to mistakenly move it elsewhere as a general purpose ungetc.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/config.c b/config.c
index 2c63099..089a94f 100644
--- a/config.c
+++ b/config.c
@@ -71,6 +71,12 @@ static int config_buf_fgetc(struct config_source *conf)
 	return EOF;
 }
 
+/*
+ * Note that this is not a real ungetc replacement. It only rewinds
+ * the position, and ignores the "c" parameter, rather than
+ * putting it into our (const) buffer. That's good enough for
+ * the callers here, though.
+ */
 static int config_buf_ungetc(int c, struct config_source *conf)
 {
 	if (conf->u.buf.pos > 0)

-- 
2.3.0.rc1.287.g761fd19