Re: [PATCH] commit: Remove backward goto in read_craft_line()

Jonathan Nieder [off-list ref] writes:

quoted

@@ -155,6 +151,11 @@ struct commit_graft *read_graft_line(char *buf, int len)
 			goto bad_graft_data;
 	}
 	return graft;
+
+bad_graft_data:
+	error("bad graft data: %s", buf);

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

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:

Jonathan Nieder [off-list ref] writes:

quoted

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

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?

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

diff --git a/.gitattributes b/.gitattributes
index 5e98806..5888a53 100644
--- a/.gitattributes
+++ b/.gitattributes

@@ -1,3 +1,4 @@
 * whitespace=!indent,trail,space
 *.[ch] whitespace=indent,trail,space
+*.[ch] diff=cpp
 *.sh whitespace=indent,trail,space

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

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 [off-list ref]:

Hi Ralf,

Ralf Thielow wrote:

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.

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