[RFC] Cleaning up die() error messages

[RFC] Cleaning up die() error messages

[Elfyn McBratney]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

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
Gentoo Developer/Perl Team Lead
beu/irc.freenode.net                            http://dev.gentoo.org/~beu/
+------------O.o--------------------- http://dev.gentoo.org/~beu/pubkey.asc

PGP Key ID: 0x69DF17AD
PGP Key Fingerprint:
  DBD3 B756 ED58 B1B4 47B9  B3BD 8D41 E597 69DF 17AD

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC] Cleaning up die() error messages

[Junio C Hamano]

Elfyn McBratney <beu@gentoo.org> writes:

> (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? :)

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.

Re: [RFC] Cleaning up die() error messages

[Daniel Barkalow]

On Mon, 10 Oct 2005, Elfyn McBratney wrote:

> 	[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);
> 		..
> 	}

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*

Re: [RFC] Cleaning up die() error messages

[Alex Riesen]

On 10/10/05, Elfyn McBratney <beu@gentoo.org> wrote:
>         int main (int argc, char **argv)
>         {
>                 set_prog_name(argv[0]);

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:]]*(" ;)

Re: [RFC] Cleaning up die() error messages

[H. Peter Anvin]

Alex Riesen wrote:
> On 10/10/05, Elfyn McBratney <beu@gentoo.org> wrote:
> 
>>        int main (int argc, char **argv)
>>        {
>>                set_prog_name(argv[0]);
> 
> 
> 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:]]*(" ;)

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

Re: [RFC] Cleaning up die() error messages

[Matthias Urlichs]

Hi, Junio C Hamano wrote:

> One thing to keep in mind is how badly this C part might
> interact with the libification effort going on underwater.

Not too badly.

> Since current code Smurf is working on is based on 0.99.6 and

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

> many small pieces need to be reviewed anyway, I am not so much
> worried about forward porting the changes.  

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.

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

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   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
A hundred mouths, a hundred tongues, And throats of brass, inspired with
iron lungs.
					-- Virgil

Re: [RFC] Cleaning up die() error messages

[H. Peter Anvin]

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

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

Re: [RFC] Cleaning up die() error messages

[Matthias Urlichs]

H. Peter Anvin 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.
> 
I thought about doing something like that, but ...

> However, a much bigger problem is cleanup.
> 
... exactly.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
BOFH excuse #282:

High altitude condensation from U.S.A.F prototype aircraft has contaminated
the primary subnet mask. Turn off your computer for 9 days to avoid
damaging it.

Re: [RFC] Cleaning up die() error messages

[H. Peter Anvin]

Matthias Urlichs wrote:
> 
> I thought about doing something like that, but ...
>>However, a much bigger problem is cleanup.
> 
> ... exactly.
> 

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

Re: [RFC] Cleaning up die() error messages

[Junio C Hamano]

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

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

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?

Re: [RFC] Cleaning up die() error messages

[H. Peter Anvin]

Junio C Hamano wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
>>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.
> 
> 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?
> 

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

	-hpa

Re: [RFC] Cleaning up die() error messages

[Junio C Hamano]

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

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

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.

Re: [RFC] Cleaning up die() error messages

[Matthias Urlichs]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

Hi,

H. Peter Anvin:
> >I thought about doing something like that, but ...
> >>However, a much bigger problem is cleanup.
> >
> >... exactly.
> 
> 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.
> 
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.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
BOFH excuse #381:

Robotic tape changer mistook operator's tie for a backup tape.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]