On Fri, May 30, 2014 at 2:59 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Ronnie Sahlberg <sahlberg@xxxxxxxxxx> writes: > >> read_ref_at has its own parsing of the reflog file for no really good reason >> so lets change this to use the existing reflog iterators. This removes one >> instance where we manually unmarshall the reflog file format. >> >> Log messages for errors are changed slightly. We no longer print the file >> name for the reflog, instead we refer to it as 'Log for ref <refname>'. >> This might be a minor useability regression, but I don't really think so, since >> experienced users would know where the log is anyway and inexperienced users >> would not know what to do about/how to repair 'Log ... has gap ...' anyway. >> >> Adapt the t1400 test to handle the cahnge in log messages. >> >> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx> >> --- >> refs.c | 217 ++++++++++++++++++++++++++++---------------------- >> t/t1400-update-ref.sh | 4 +- >> 2 files changed, 122 insertions(+), 99 deletions(-) > > After reading the log message "we are removing one redundant > implementation", I would have expected that such a change would > remove a lot more lines than it would add. Seeing the diffstat, I > have to wonder if the fact that we need more lines to reuse the API > is an indication that the reflog iterator API is overly cumbersome > to use, perhaps requiring too much boilerplate or something. Yeah. We simplify the code and make it reuse existing unmarshallers and make it easier to read, and the line count goes up :-( Most of the extra code is the due to the structure to hold all the data we need in the callbacks and is a bit less compact. That said, I think the new code is a little easier to read. The biggest value is that we reduce the number of reflog unmarshallers by one which will save work when we start storing reflogs in a different type of backend. > > The update in the error message may be a side-effect, but I think it > is a change in the good direction. > The update in error message is also to prepare for the possibility that we might get a different type of ref and reflog store in the future. So that we only generate log messages that refer to filenames in those places where we are actually accessing files directly. Thanks. I will resend the patch later with Eric's suggestions. ronnie sahlberg -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html