From: Junio C Hamano <hidden> Date: 2016-06-15 22:43:37
Jeff King [off-list ref] writes:
On Tue, Oct 02, 2007 at 07:16:43PM +0100, Andy Parkins wrote:
quoted
Changed repeated use of the same constants for the ref paths to be
symbolic constants. I've defined them in refs.h
I've manually inspected the patch. Comments are below.
quoted
- if (prefixcmp(head, "refs/heads/"))
- die("HEAD not found below refs/heads!");
- head += 11;
+ if (prefixcmp(head, PATH_REFS_HEADS))
+ die("HEAD not found below " PATH_REFS_HEADS "!");
+ head += STRLEN_PATH_REFS_HEADS;
This slightly changes the message (extra "/"), but I don't think that is
a big deal...
die("HEAD not found below %.*%s!",
PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS-1)
quoted
- if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
+ if (snprintf(ref, sizeof(ref), PATH_REFS_TAGS"%s", *p)
I find the 'PATH_REFS_TAGS "%s"' (with a space) you used earlier a
little easier to read.
Even though we all know that PATH_REFS_* do not have any '%' in
them, it is somewhat unnerving to see such an opaque string in
the format specifier part of _any_printf() function. It just
makes you think twice, disrupting the flow of thoughts.
This applies to die() and friends as well; see my above rewrite.
To me, the valid reasons for this kind of rewrite are if:
- it makes typo harder to make and easier to spot
(e.g. "refs/head/");
- it makes miscount harder to make and easier to spot (e.g.
what is this magic constant 11? Is it strlen("refs/heads/")?);
- it makes reviewing the resulting code, and more importantly,
future patches on the resulting code, easier.
- it makes it easier for us to later revamp the strings
wholesale (e.g. "refs/heads/" => "refs/branches/").
- it saves us repeated instances of the same string constant;
using C literal string as values for PATH_REFS_HEADS would
not help and you would need (const char []) strings instead,
but the compiler may be clever enough to do so.
Unquestionably, this series helps on the first two counts.
It however actively hurts on the third count. These long
constants in CAPITAL_LETTERS_WITH_UNDERSCORE shout too loudly to
the eye, overwhelming the surrounding code. I wonder if we can
do anything about this point to resurrect the first two
benefits, which I like very much.
The forth is a myth we shouldn't care about. If we later would
want to change refs/heads to refs/branches, we would want to
rename PATH_REFS_HEADS to PATH_REFS_BRANCHES at the same time as
well, so the kind of rewrite this patch does does not buy us
anything there. More importantly, such a change would need to
be made in a backward compatible way (e.g. "if we have heads
then keep using them but in new repositories we favor
branches"), so it won't be straight token replacement anyway.
And the fifth do not apply to us. This matters only if we were
an embedded application on memory starved machine and string
constants are far smaller matter compared to the amount of other
data we use in-core.
From: Jeff King <hidden> Date: 2016-06-15 22:43:38
On Tue, Oct 02, 2007 at 12:47:59PM -0700, Junio C Hamano wrote:
- it makes typo harder to make and easier to spot
(e.g. "refs/head/");
- it makes miscount harder to make and easier to spot (e.g.
what is this magic constant 11? Is it strlen("refs/heads/")?);
- it makes reviewing the resulting code, and more importantly,
future patches on the resulting code, easier.
[...]
It however actively hurts on the third count. These long
Yes, I find some of the substitutions more readable, but some are a bit
less readable. The parts of the patch I found the _most_ improved are
the ones that get rid of a memcmp in favor of a prefixcmp (i.e.,
removing the count entirely).
Perhaps a better quest would be to eliminate all of those counts
entirely with code that is obviously correct. I think it is much more
readable to replace:
url = xmalloc(strlen(repo->base) + 64);
sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
with something like:
strbuf_init(&url);
strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
which has the same number of lines, but no magic numbers at all. Or
since most of the uses of things like PATH_OBJECTS are more or less the
same, maybe something like:
mkpath_object(&url, "pack/pack-%s.idx", hex);
i.e., rather than fiddling with string constants, wrap them
functionally.
constants in CAPITAL_LETTERS_WITH_UNDERSCORE shout too loudly to
Part of the problem is also that they're long. Perhaps REFS_HEADS, while
being less unique in the C namespace, would look better?
-Peff
From: Junio C Hamano <hidden> Date: 2016-06-15 22:43:38
Jeff King [off-list ref] writes:
Perhaps a better quest would be to eliminate all of those counts
entirely with code that is obviously correct. I think it is much more
readable to replace:
url = xmalloc(strlen(repo->base) + 64);
sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
with something like:
strbuf_init(&url);
strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
Ugh, this typically calls snprintf() twice doesn't it?
Ugh, this typically calls snprintf() twice doesn't it?
Yes, it probably does. However, I think it is considerably easier to
read and more maintainable. Are you "ugh"ing because of the performance
impact (which should be negligible unless this is in a tight loop) or
because of the portability problems associated with va_copy?
-Peff
Ugh, this typically calls snprintf() twice doesn't it?
Yes, it probably does. However, I think it is considerably easier to
read and more maintainable. Are you "ugh"ing because of the performance
impact (which should be negligible unless this is in a tight loop) or
because of the portability problems associated with va_copy?
I wonder, I wonder, if
strbuf_addstr(&url, repo->base);
strbuf_addstr(&url, "/objects/pack/pack-");
strbuf_addstr(&url, hex);
strbuf_addstr(&url, ".idx");
would make anybody else but me happy...
Ciao,
Dscho
From: Jeff King <hidden> Date: 2016-06-15 22:43:38
On Wed, Oct 03, 2007 at 05:05:15AM +0100, Johannes Schindelin wrote:
I wonder, I wonder, if
strbuf_addstr(&url, repo->base);
strbuf_addstr(&url, "/objects/pack/pack-");
strbuf_addstr(&url, hex);
strbuf_addstr(&url, ".idx");
would make anybody else but me happy...
I actually wrote that originally, and then switched to the formatted
version for readability. But I would be happy with that, as well, if we
are truly concerned about the cost of 2 snprintfs.
-Peff
From: Andy Parkins <hidden> Date: 2016-06-15 22:43:38
On Tuesday 2007 October 02, Jeff King wrote:
Perhaps a better quest would be to eliminate all of those counts
entirely with code that is obviously correct. I think it is much more
readable to replace:
I've got a patch replacing every appropriate memcmp() with prefixcmp(), but it
goes on top of this one, so wanted to get this through review to save
constantly spamming the list with the same patch slightly modified because of
changes in a different patch.
I've not been following the strbuf() changes, so have missed the appearance of
these handy new functions. They would appear to be an improvement for cases
just like this.
quoted
constants in CAPITAL_LETTERS_WITH_UNDERSCORE shout too loudly to
Part of the problem is also that they're long. Perhaps REFS_HEADS, while
being less unique in the C namespace, would look better?
I completely agree with the length and loudness concerns, but my worry was
polluting the namespace while maintaining some sort of rationality between
PATH_REFS_HEADS and STRLEN_PATH_REFS_HEADS. My reasoning was that
"refs/heads" -> PATH_REFS_HEADS
is only three extra characters, and
strlen("refs/heads/") -> STRLEN_PATH_REFS_HEADS
is only one extra character.
However I have no strong feelings about changing them.
Andy
--
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com
Ugh, this typically calls snprintf() twice doesn't it?
Yes, it probably does. However, I think it is considerably easier to
read and more maintainable. Are you "ugh"ing because of the performance
impact (which should be negligible unless this is in a tight loop) or
because of the portability problems associated with va_copy?
I wonder, I wonder, if
strbuf_addstr(&url, repo->base);
strbuf_addstr(&url, "/objects/pack/pack-");
strbuf_addstr(&url, hex);
strbuf_addstr(&url, ".idx");
would make anybody else but me happy...
strbuf_addstr_many(&url, repo->base, "/objects/pack/pack-", hex, ".idx", NULL);
is what I'd prefer. It's not overly complicated, requires no *printf(), and doesn't
introduce any new portability issues (va_arg() is C89).
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231