[RFC] Cleaning up die() error messages

Hello git list,

I've started working on cleaning up the various die() error messages found throughout git, and had a few thoughts along the way.

Currently, I've been adding missing program name prefixes, and quoting path names (e.g., "%s" -> "'%s'"), but before I go any further, I'm wondering if this is a) desired, or perhaps b) superfluous? This may be best discussed with along-side the patch - which'll follow shortly. ;)

Also, I got to thinking whether it might be an idea to use the following idiom in the code:

	[shell scripts]
	prog="`basename $0`"
	..
	foo || die "${prog}: foo failed"

	[C sources]
	static char *prog;
	..
	static inline void set_prog_name (char *argv0)
	{
		prog = strrchr(argv0, '/');
		if (prog)
			prog++;
		else
			prog = argv0;
	}
	..
	int main (int argc, char **argv)
	{
		set_prog_name(argv[0]);
		..
		if (!do_bar())
			die("%s: do_bar() failed", prog);
		..
	}

The idea behind this being that, if any of the git programs get renamed (again :) there won't be a need for s/git-foo/git-bar/g just to fix-up die() error messages, and it'll also shave a *bit* off of the size of the compiled binaries. ;)

(Of course, the C parts (`prog' and `set_prog_name()') would go into a header, and not in every single C source file. ;)

So, any thoughts/comments/flames? :)

Best, Elfyn

Elfyn McBratney <beu@gentoo.org> writes:

I do not have objections to either one, except I tend to prefer programs that tells its full path when erroring out, which helps me identify "Oops, my path was screwed up and I am not testing the right one" case.

One thing to keep in mind is how badly this C part might interact with the libification effort going on underwater. Since current code Smurf is working on is based on 0.99.6 and many small pieces need to be reviewed anyway, I am not so much worried about forward porting the changes. But some die()s that are in the parts that will be moved to the common library code would also want to use this prog global somehow.

But that would not be too much of a problem. Worst case, we force the library clients to do set_prog_name(), or initialize prog to "(unnamed)", or do both.

Hi, Junio C Hamano wrote:

Not too badly.

I've merged it up once already; will do that again soon.

I've also mostly succeeded in keeping the individual patches clean so that everything still builds (and verifies), so it might be easiest to just merge with it. ;-)

But we'll cross that bridge when we get to it.

IMHO, common library code should not be allowed to die. (Yes, that does imply replacing all the xmalloc() calls.)

My library effort has a buffer for the (first) error message. The caller can elect to suppress printing it, so that it can be formatted appropriately. Python, for instance, will wrap errors in an exception.

The way I structured it so far, a C program would do 

	git_env = git_env_new();
	die_if_null(git_env());
	git_env->print_error = 0;
[...]
	git_whatever(git_env, ...);
	if (git_env->error) {
		fprintf(stderr, "%s: doing whatever: %s\n",
			my_program_name, git_env->error);
		git_env_clear_error(git_env);
		goto whatever_bad_so_go_clean_up;
	}

Matthias Urlichs wrote:

The sane way to do this is probably to call an overridable git_die() function, which can be specified by the user to use longjmp(), to use exceptions, or do something else appropriately.

However, a much bigger problem is cleanup.

	-hpa

H. Peter Anvin wrote:

I thought about doing something like that, but ...

... exactly.

Matthias Urlichs wrote:

I thought about this, and probably the sanest way is to wrap malloc() with something that creates a linked list of allocations. If we abort, we can unwind the linked list and free all allocations.

	-hpa

"H. Peter Anvin" <hpa@zytor.com> writes:

You can free() the allocated memory with something like that, but it is likely that the aborted function would have already created some linked structure out of those memory blocks, and cleaning up that you would need the real exception handling wouldn't you?

Junio C Hamano wrote:

Not unless any of those linked structures can be reached from outside the git library.

	-hpa

"H. Peter Anvin" <hpa@zytor.com> writes:

OK. How about things like open file descriptors and mmap'ed regions then? That is:

    fd = open();
    xmalloc(); /* may die now */
    close(fd);

I think in the long run we would be better off if we properly dealt with the case where (x)malloc returns NULL. Of course, that would make libification a bigger task, so we do not have to do it from day one.

Hi,

H. Peter Anvin:

There already is a malloc library that does this, plus it can call cleanup for you -- there's more to cleaning up than freeing memory. :-/

Let's see if I can actually find it again.

On the other hand, I wonder if the overhead when managing data structures like that really offsets the additional work we'd need to do otherwise, which is simply checking a few more return values.

On Mon, 10 Oct 2005, Elfyn McBratney wrote:

Just put set_prog_name() next to die(), and have die() write "prog: " at the beginning if set_prog_name() got called. Assuming it's useful at all to show the name of the program, it's not going to be less useful if the error is actually found in a library function called by the program.

	-Daniel
*This .sig left intentionally blank*

On 10/10/05, Elfyn McBratney <beu@gentoo.org> wrote:

I'd also use readlink on /proc/self/exe by default (if set_prog_name _not_ called). It simplifies the code at least on linux, and makes possible very slow transition for other platforms. So you don't have to update each and every .c file containing "main[[:space:]]*(" ;)

Alex Riesen wrote:

It's really better just to put the stuff at the beginning of each main. 
  If that's too annoying, librarize main and rename your mains "git_main".

	-hpa