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

[Torsten Bögershausen]

On 29.03.13 22:20, Junio C Hamano wrote:
[snip]
 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.
 
Thanks for carefull review:
The achieved effect of the code is the same,
but the developer is to blame.

I send a new patch of 2/2 in a minute, splitted into 2 commits.

Highlights of part 2:
a) move if (!shared_repository) to the right place
b) Simplify calc_shared_perm() even more: Remove the variable int shared
c) Remove calc_shared_perm_dir(), the functionality is baked into
   adjust_shared_perm()

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Torsten Bögershausen tbo...@web.de writes:

   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.

 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).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html