patchcommit: Remove backward goto in read_craft_line()

In case of bad graft data which is determine on many places we go back to the first place of detection, so move it to the end of the function.

Signed-off-by: Ralf Thielow <ralf.thielow@googlemail.com>
---
 commit.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

Ralf Thielow wrote:

I like it.  Three tiny nits.

Perhaps the log message could be clarified like so:

 bad graft data is noticed in several places in read_graft_line and
 in each case we go back to the first site of detection.  Move the
 error handling to the end of the function for better readability.

Trailing whitespace. (You can avoid this in the future with "git diff --check".)

[...]

A space before the "bad_graft_data:" label would improve future diff --show-c-function output.

Except as noted above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks. ---

Jonathan Nieder <jrnieder@gmail.com> writes:

Hmm, I actually do not think we encourage that (nor we should).

    $ git grep -e '^ [a-z0-9]*:' -- '*.c' | wc -l
    23
    $ git grep -e '^[a-z0-9]*:' -- '*.c' | wc -l
    42

If "--show-c-function" output is the problem, perhaps we should know a bit better about what C function header looks like?

Junio C Hamano wrote:

Thanks for checking.  Yes, I think so.

	$ git grep --show-function strbuf_release -- http.c
	http.c=static int http_request(const char *url, void *result, int target, int options)
	http.c: strbuf_release(&buf);
	http.c=cleanup:
	http.c: strbuf_release(&tmpfile);
	http.c=int http_fetch_ref(const char *base, struct ref *ref)
	http.c: strbuf_release(&buffer);

The following gives me some joy.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---

In fact the "--show-c-function" output is the problem. But I think that a change can't be rejected because of another issue. The style of placing "goto"-statements, which leave a function to the end of that is used in many other projects. And I think it's very usefull.

2010/12/1 Jonathan Nieder <jrnieder@gmail.com>:

Hi Ralf,

Ralf Thielow wrote:

Thinking more about your patch reminds me of something that can be confusing to new contributors. Sometimes it is hard for a trivial patch to be accepted than one which makes a larger change, requires more time reviewing it, and has more potential for catastrophic breakage.

Why is that? Resistance to trivial changes is in my opinion a good thing and I'd like to emphasize that now, since my feeling about this patch is borderline.[1]

In this case, the patch is changing code like this:

	do something
	if (error) {
	error_case:
		report error;
		free resources;
		return -1;
	}
	do something else
	if (error)
		goto error_case;
	if (other error)
		goto error_case;
	return result;

into the more usual simulated exception handling:

	do something
	if (error)
		goto error_case;
	do something else
	if (error)
		goto error_case;
	...
	return result;
error_case:
	report error;
	free resources;
	return -1;

The latter is not really much clearer than the former, just less unusual. Changing the code means patches are less likely to apply to both the before and after. Changing the code requires time to review it and to explain why this kind of change is okay in this case but in other cases it wouldn't be.

So it would be much easier to like this if the change fixed a noticeable problem (in user-visible behavior, maintainability, or clarity).

Sorry for the mixed message. Partly I really _wanted_ to like this because it is in better taste than some of the trivial patches the git list has received before. As far as applying this one, I suppose I would not mind either way.

Anyway, I hope that makes the underlying principles a bit clearer.

Thanks for a useful example. Jonathan

[1] Linux has a "maintainer of trivial" that takes care of such patches while minimizing the damage to bystanders. Traditionally there were some pretty rigid guidelines for what patches in this area were accepted, which was probably a good idea. ftp://ftp.kernel.org/pub/linux/kernel/people/rusty/trivial/template-index.html

Ralf Thielow <ralf.thielow@googlemail.com> writes:

Well, I never said anything about rejection nor acceptance.

It is a good discipline to follow in general to place an exceptional case at the end if you jump out of the general flow. But because the affected function was so small, its readability doesn't benefit very much from the discipline (in other words, you knew about a good discipline, but applied it to a wrong function). The patch was small and obviously correct, so it will not hurt, but it will not make the world greatly a better place, either.

IOW, it was a "Meh" topic for me.

It was more important to discuss Jonathan's "leave SP at the beginning of a goto label to please --show-c-function" from the maintainer's point of view, as people may remember it as a rule and start sending patches that follow it, which I will need to deal with in the future. I do not think that one is a good rule.

Now that we have dealt with that more important business of letting people know that protecting goto labels with a leading SP is _not_ the rule ;-), I am happy with this discussion.

And often I forget about the original issue when a discussion reaches this stage of happiness. So thanks for reminding me.

As I said, even though I do not think the particular function you touched badly needs the discipline applied, it would not hurt, and it obviously is the right thing to do (iow, if the function were written from day one in a way your patch reorganized it, we would never accept a patch to change it into today's shape of jumping backwards). For one thing, it would remove an example from the codebase people can point at to make excuses when responding to a review of their patch that adds a backward goto to a much larger function.

Will queue after massaging the log message somewhat.  Thanks.

I remember about a peoble (not on this project) who sad that also a little change like this should includes in the sources because it's make the code better without fixing a bug or add a new feature. But in my experience these patches are not very popular. But sure, I understand your reasons, Junio. The statement from above is not even true.

2010/12/1 Junio C Hamano <gitster@pobox.com>: