Re: [PATCH 2/2] optimize set_shared_perm()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2013, 21:20 UTC
Message-ID: <7vli95nbs4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201303251657.57222.tboegi@web.de>

Torsten Bögershausen <tboegi@web.de> writes:

Show 18 quoted lines

>   optimize set_shared_perm() in path.c:
>
>   - sometimes the chown() function is called even when not needed.
>     (This can be provoced by running t1301, and adding some debug code)
>
>     Save a chmod from 400 to 400, or from 600->600 on these files:
>     .git/info/refs+
>     .git/objects/info/packs+
>
>     Save chmod on directories from 2770 to 2770:
>     .git/refs
>     .git/refs/heads
>     .git/refs/tags
>
>   - as all callers use mode == 0 when caling set_shared_perm(),
>     the function can be simplified.
>   - all callers use the macro adjust_shared_perm(path) from cache.h
>     Convert adjust_shared_perm() from a macro into a function prototype

The last two points can become a separate "preparation" step. The result would be easier to read.

Your updated adjust_shared_perm() does not begin with:

	if (!shared_repository)
        	return 0;

as the original, but it always first calls to get_st_mode_bits() which makes a call to lstat(2).

That smells like a huge regression for !shared_repository case, unless you have updated the existing callers of adjust_shared_perm() not to call it when !shared_repository.

Show 18 quoted lines

> diff --git a/path.c b/path.c
> index 2fdccc2..4bc918a 100644
> --- a/path.c
> +++ b/path.c
> @@ -1,14 +1,5 @@
>  /*
> - * I'm tired of doing "vsnprintf()" etc just to open a
> - * file, so here's a "return static buffer with printf"
> - * interface for paths.
> - *
> - * It's obviously not thread-safe. Sue me. But it's quite
> - * useful for doing things like
> - *
> - *   f = open(mkpath("%s/%s.git", base, name), O_RDONLY);
> - *
> - * which is what it's designed for.
> + * Different utilitiy functions for path and path names
>   */

Removing the stale "I'm tired of" is good; you do not have to move it anywhere. A single-liner

	/* Utilities for paths and pathnames */

should suffice.

Will discard this step (but will keep PATCH 1/2).