From: René Scharfe <hidden> Date: 2016-06-15 23:01:46
Instead of using strbuf to create a message string in case a path is
too long for our fixed-size buffer, replace that buffer with a strbuf
and thus get rid of the limitation.
Signed-off-by: Rene Scharfe <redacted>
---
sha1_file.c | 41 +++++++++++++++--------------------------
1 file changed, 15 insertions(+), 26 deletions(-)
@@ -1177,48 +1177,36 @@ static void report_pack_garbage(struct string_list *list)staticvoidprepare_packed_git_one(char*objdir,intlocal){-/* Ensure that this buffer is large enough so that we can-append"/pack/"withoutclobberingthestackevenif-strlen(objdir)werePATH_MAX.*/-charpath[PATH_MAX+1+4+1+1];-intlen;+structstrbufpath=STRBUF_INIT;+size_tdirnamelen;DIR*dir;structdirent*de;structstring_listgarbage=STRING_LIST_INIT_DUP;-sprintf(path,"%s/pack",objdir);-len=strlen(path);-dir=opendir(path);+strbuf_addstr(&path,objdir);+strbuf_addstr(&path,"/pack");+dir=opendir(path.buf);if(!dir){if(errno!=ENOENT)error("unable to open object pack directory: %s: %s",-path,strerror(errno));+path.buf,strerror(errno));return;}-path[len++]='/';+strbuf_addch(&path,'/');+dirnamelen=path.len;while((de=readdir(dir))!=NULL){-intnamelen=strlen(de->d_name);structpacked_git*p;-if(len+namelen+1>sizeof(path)){-if(report_garbage){-structstrbufsb=STRBUF_INIT;-strbuf_addf(&sb,"%.*s/%s",len-1,path,de->d_name);-report_garbage("path too long",sb.buf);-strbuf_release(&sb);-}-continue;-}-if(is_dot_or_dotdot(de->d_name))continue;-strcpy(path+len,de->d_name);+strbuf_setlen(&path,dirnamelen);+strbuf_addstr(&path,de->d_name);if(has_extension(de->d_name,".idx")){/* Don't reopen a pack we already have. */for(p=packed_git;p;p=p->next){-if(!memcmp(path,p->pack_name,len+namelen-4))+if(!memcmp(path.buf,p->pack_name,path.len-4))break;}if(p==NULL&&
@@ -1226,7 +1214,7 @@ static void prepare_packed_git_one(char *objdir, int local)*Seeifitreallyisavalid.idxfilewith*corresponding.packfilethatwecanmap.*/-(p=add_packed_git(path,len+namelen,local))!=NULL)+(p=add_packed_git(path.buf,path.len,local))!=NULL)install_packed_git(p);}
From: René Scharfe <hidden> Date: 2016-06-15 23:01:46
Avoid overrunning the existing pack name (p->pack_name, a C string) in
the case that the new path is longer by using strncmp instead of memcmp
for comparing. While at it, replace the magic constant 4 with a
strlen call to document its meaning.
Signed-off-by: Rene Scharfe <redacted>
---
sha1_file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: René Scharfe <hidden> Date: 2016-06-15 23:01:46
Instead of using strbuf to create a message string in case a path is
too long for our fixed-size buffer, replace that buffer with a strbuf
and thus get rid of the limitation.
Helped-by: Duy Nguyen [off-list ref]
Signed-off-by: Rene Scharfe <redacted>
---
@Duy: Thanks for catching the missing strbuf_release in the opendir
error case.
sha1_file.c | 42 ++++++++++++++++--------------------------
1 file changed, 16 insertions(+), 26 deletions(-)
@@ -1177,48 +1177,37 @@ static void report_pack_garbage(struct string_list *list)staticvoidprepare_packed_git_one(char*objdir,intlocal){-/* Ensure that this buffer is large enough so that we can-append"/pack/"withoutclobberingthestackevenif-strlen(objdir)werePATH_MAX.*/-charpath[PATH_MAX+1+4+1+1];-intlen;+structstrbufpath=STRBUF_INIT;+size_tdirnamelen;DIR*dir;structdirent*de;structstring_listgarbage=STRING_LIST_INIT_DUP;-sprintf(path,"%s/pack",objdir);-len=strlen(path);-dir=opendir(path);+strbuf_addstr(&path,objdir);+strbuf_addstr(&path,"/pack");+dir=opendir(path.buf);if(!dir){if(errno!=ENOENT)error("unable to open object pack directory: %s: %s",-path,strerror(errno));+path.buf,strerror(errno));+strbuf_release(&path);return;}-path[len++]='/';+strbuf_addch(&path,'/');+dirnamelen=path.len;while((de=readdir(dir))!=NULL){-intnamelen=strlen(de->d_name);structpacked_git*p;-if(len+namelen+1>sizeof(path)){-if(report_garbage){-structstrbufsb=STRBUF_INIT;-strbuf_addf(&sb,"%.*s/%s",len-1,path,de->d_name);-report_garbage("path too long",sb.buf);-strbuf_release(&sb);-}-continue;-}-if(is_dot_or_dotdot(de->d_name))continue;-strcpy(path+len,de->d_name);+strbuf_setlen(&path,dirnamelen);+strbuf_addstr(&path,de->d_name);if(has_extension(de->d_name,".idx")){/* Don't reopen a pack we already have. */for(p=packed_git;p;p=p->next){-if(!memcmp(path,p->pack_name,len+namelen-4))+if(!memcmp(path.buf,p->pack_name,path.len-4))break;}if(p==NULL&&
@@ -1226,7 +1215,7 @@ static void prepare_packed_git_one(char *objdir, int local)*Seeifitreallyisavalid.idxfilewith*corresponding.packfilethatwecanmap.*/-(p=add_packed_git(path,len+namelen,local))!=NULL)+(p=add_packed_git(path.buf,path.len,local))!=NULL)install_packed_git(p);}
From: René Scharfe <hidden> Date: 2016-06-15 23:01:46
Avoid overrunning the existing pack name (p->pack_name, a C string) in
the case that the new path is longer by using strncmp instead of memcmp
for comparing. While at it, replace the magic constant 4 with a
strlen call to document its meaning.
Signed-off-by: Rene Scharfe <redacted>
---
No changes from intial round.
sha1_file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
On Sun, Jun 29, 2014 at 07:43:17AM +0200, René Scharfe wrote:
Instead of using strbuf to create a message string in case a path is
too long for our fixed-size buffer, replace that buffer with a strbuf
and thus get rid of the limitation.
Yay. Safer, and the end result is much more readable.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
On Sun, Jun 29, 2014 at 07:56:25AM +0200, René Scharfe wrote:
Avoid overrunning the existing pack name (p->pack_name, a C string) in
the case that the new path is longer by using strncmp instead of memcmp
for comparing. While at it, replace the magic constant 4 with a
strlen call to document its meaning.
An unwritten assumption with this code is that we have "foo.idx" and we
want to make sure that we are matching "foo.pack" from the existing pack
list. Both before and after your patch, I think we would match
"foobar.pack". It's probably not a big deal, as we don't expect random
junk in the pack directory, but I wonder if it would be better to be
explicit, like:
/*
* like ends_with, but return the truncated size of str via
* the "len" parameter.
*/
int strip_suffix(const char *str, const char *suffix, size_t *len)
{
size_t suflen = strlen(suffix);
*len = strlen(str);
if (len < suflen)
return 0;
else if (strcmp(str + len - suflen, suffix))
return 0;
else {
*len -= suflen;
return 1;
}
}
int idx_matches_pack(const char *idx, const char *pack)[
{
size_t idx_len, pack_len;
if (!strip_suffix(idx, ".idx", &idx_len) ||
!strip_suffix(pack, ".pack", &pack_len))
return 0;
if (idx_len != pack_len)
return 0;
return !memcmp(idx, pack, idx_len);
}
You'd perhaps want to split idx_matches_pack across the loop below (our
idx is actually a strbuf, so you can reuse path->len to avoid a strlen,
and you do not have to verify idx_len each time through the loop).
I think strip_suffix would have other uses, too. It's a more featureful
version of ends_with, as skip_prefix is to starts_with. E.g.,
builtin/remote.c:config_read_branches could use it to avoid some magic
numbers.
@@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)if(has_extension(de->d_name,".idx")){/* Don't reopen a pack we already have. */
If we don't follow my suggestion above, we still have this
has_extension. This is a reimplementation of ends_with, isn't it? We can
probably drop it and just use ends_with.
-Peff
@@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)if(has_extension(de->d_name,".idx")){/* Don't reopen a pack we already have. */
If we don't follow my suggestion above, we still have this
has_extension. This is a reimplementation of ends_with, isn't it? We can
probably drop it and just use ends_with.
This calls for another patch if we just want to kill has_extension()
in favor of ends_with(). There are 12 call sites of it.
--
Duy
@@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)if(has_extension(de->d_name,".idx")){/* Don't reopen a pack we already have. */
If we don't follow my suggestion above, we still have this
has_extension. This is a reimplementation of ends_with, isn't it? We can
probably drop it and just use ends_with.
This calls for another patch if we just want to kill has_extension()
in favor of ends_with(). There are 12 call sites of it.
Yes. Some of those would want to become ends_with, and some would
actually want to become strip_suffix. I'm working up a series now.
-Peff
From: René Scharfe <hidden> Date: 2016-06-15 23:01:46
Am 30.06.2014 15:43, schrieb Jeff King:
On Sun, Jun 29, 2014 at 07:56:25AM +0200, René Scharfe wrote:
quoted
Avoid overrunning the existing pack name (p->pack_name, a C string) in
the case that the new path is longer by using strncmp instead of memcmp
for comparing. While at it, replace the magic constant 4 with a
strlen call to document its meaning.
An unwritten assumption with this code is that we have "foo.idx" and we
want to make sure that we are matching "foo.pack" from the existing pack
list. Both before and after your patch, I think we would match
"foobar.pack".
Yes, indeed.
It's probably not a big deal, as we don't expect random
junk in the pack directory, but I wonder if it would be better to be
explicit, like:
<snip>
Here's a simpler approach:
-- >8 --
Subject: [PATCH v2 3/2] sha1_file: more precise packname matching
Consider the full length of the already loaded pack names when checking
for duplicates.
Signed-off-by: Rene Scharfe <redacted>
---
sha1_file.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
@@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)if(has_extension(de->d_name,".idx")){/* Don't reopen a pack we already have. */
If we don't follow my suggestion above, we still have this
has_extension. This is a reimplementation of ends_with, isn't it? We can
probably drop it and just use ends_with.
This calls for another patch if we just want to kill has_extension()
in favor of ends_with(). There are 12 call sites of it.
Yes. Some of those would want to become ends_with, and some would
actually want to become strip_suffix. I'm working up a series now.
NB: has_extension is almost the same as ends_with, but it also checks if
the string is longer than just the extension:
ends_with("x", "x") => 1
has_extension("x", "x") => 0
René
@@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)if(has_extension(de->d_name,".idx")){/* Don't reopen a pack we already have. */
If we don't follow my suggestion above, we still have this
has_extension. This is a reimplementation of ends_with, isn't it? We can
probably drop it and just use ends_with.
This calls for another patch if we just want to kill has_extension()
in favor of ends_with(). There are 12 call sites of it.
Yes. Some of those would want to become ends_with, and some would
actually want to become strip_suffix. I'm working up a series now.
NB: has_extension is almost the same as ends_with, but it also checks if the
string is longer than just the extension:
ends_with("x", "x") => 1
has_extension("x", "x") => 0
Thanks, I didn't notice that. I don't think the distinction is
important in any callers, but I'll double check.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
On Mon, Jun 30, 2014 at 06:35:50PM +0200, René Scharfe wrote:
quoted
It's probably not a big deal, as we don't expect random
junk in the pack directory, but I wonder if it would be better to be
explicit, like:
<snip>
Here's a simpler approach:
I agree that solves the problem. However, I'm about to post an
alternative series that also replaces has_extension with strip_suffix,
which I think ends up a bit nicer.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
Here's a series to do for ends_with what the recent skip_prefix series
did for starts_with. Namely: drop some magic numbers and repeated string
literals, and hopefully make things more readable.
The first patch is René's patch 1/2, with the leak fix from Duy and typo
fixes in the commit message from me. The rest are new, and replace the
changes to prepare_packed_git_one done elsewhere in the thread.
[1/9]: sha1_file: replace PATH_MAX buffer with strbuf in prepare_packed_git_one()
[2/9]: add strip_suffix function
[3/9]: implement ends_with via strip_suffix
[4/9]: replace has_extension with ends_with
[5/9]: use strip_suffix instead of ends_with in simple cases
[6/9]: index-pack: use strip_suffix to avoid magic numbers
[7/9]: strbuf: implement strbuf_strip_suffix
[8/9]: verify-pack: use strbuf_strip_suffix
[9/9]: prepare_packed_git_one: refactor duplicate-pack check
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
From: René Scharfe <redacted>
Instead of using strbuf to create a message string in case a path is
too long for our fixed-size buffer, replace that buffer with a strbuf
and thus get rid of the limitation.
Helped-by: Duy Nguyen [off-list ref]
Signed-off-by: Rene Scharfe <redacted>
Signed-off-by: Jeff King <redacted>
---
sha1_file.c | 42 ++++++++++++++++--------------------------
1 file changed, 16 insertions(+), 26 deletions(-)
@@ -1177,48 +1177,37 @@ static void report_pack_garbage(struct string_list *list)staticvoidprepare_packed_git_one(char*objdir,intlocal){-/* Ensure that this buffer is large enough so that we can-append"/pack/"withoutclobberingthestackevenif-strlen(objdir)werePATH_MAX.*/-charpath[PATH_MAX+1+4+1+1];-intlen;+structstrbufpath=STRBUF_INIT;+size_tdirnamelen;DIR*dir;structdirent*de;structstring_listgarbage=STRING_LIST_INIT_DUP;-sprintf(path,"%s/pack",objdir);-len=strlen(path);-dir=opendir(path);+strbuf_addstr(&path,objdir);+strbuf_addstr(&path,"/pack");+dir=opendir(path.buf);if(!dir){if(errno!=ENOENT)error("unable to open object pack directory: %s: %s",-path,strerror(errno));+path.buf,strerror(errno));+strbuf_release(&path);return;}-path[len++]='/';+strbuf_addch(&path,'/');+dirnamelen=path.len;while((de=readdir(dir))!=NULL){-intnamelen=strlen(de->d_name);structpacked_git*p;-if(len+namelen+1>sizeof(path)){-if(report_garbage){-structstrbufsb=STRBUF_INIT;-strbuf_addf(&sb,"%.*s/%s",len-1,path,de->d_name);-report_garbage("path too long",sb.buf);-strbuf_release(&sb);-}-continue;-}-if(is_dot_or_dotdot(de->d_name))continue;-strcpy(path+len,de->d_name);+strbuf_setlen(&path,dirnamelen);+strbuf_addstr(&path,de->d_name);if(has_extension(de->d_name,".idx")){/* Don't reopen a pack we already have. */for(p=packed_git;p;p=p->next){-if(!memcmp(path,p->pack_name,len+namelen-4))+if(!memcmp(path.buf,p->pack_name,path.len-4))break;}if(p==NULL&&
@@ -1226,7 +1215,7 @@ static void prepare_packed_git_one(char *objdir, int local)*Seeifitreallyisavalid.idxfilewith*corresponding.packfilethatwecanmap.*/-(p=add_packed_git(path,len+namelen,local))!=NULL)+(p=add_packed_git(path.buf,path.len,local))!=NULL)install_packed_git(p);}
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
Many callers of ends_with want to not only find out whether
a string has a suffix, but want to also strip it off. Doing
that separately has two minor problems:
1. We often run over the string twice (once to find
the suffix, and then once more to find its length to
subtract the suffix length).
2. We have to specify the suffix length again, which means
either a magic number, or repeating ourselves with
strlen("suffix").
Just as we have skip_prefix to avoid these cases with
starts_with, we can add a strip_suffix to avoid them with
ends_with.
Note that we add two forms of strip_suffix here: one that
takes a string, with the resulting length as an
out-parameter; and one that takes a pointer/length pair, and
reuses the length as an out-parameter. The latter is more
efficient when the caller already has the length (e.g., when
using strbufs), but it can be easy to confuse the two, as
they take the same number and types of parameters.
For that reason, the "mem" form puts its length parameter
next to the buffer (since they are a pair), and the string
form puts it at the end (since it is an out-parameter). The
compiler can notice when you get the order wrong, which
should help prevent writing one when you meant the other.
Signed-off-by: Jeff King <redacted>
---
I hope the word "strip" is OK, as it does not actually NUL-terminate
(doing so would make it unusable for many cases). Between the comment
below and the "const" in the parameter, I think it should be pretty
clear that it does not touch the string. And I could not think of a
better word.
git-compat-util.h | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
The ends_with function is essentially a simplified version
of strip_suffix, in which we throw away the stripped length.
Implementing it as an inline on top of strip_suffix has two
advantages:
1. We save a bit of duplicated code.
2. The suffix is typically a string literal, and we call
strlen on it. By making the function inline, many
compilers can replace the strlen call with a constant.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 7 ++++++-
strbuf.c | 9 ---------
2 files changed, 6 insertions(+), 10 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
These two are almost the same function, with the exception
that has_extension only matches if there is content before
the suffix. So ends_with(".exe", ".exe") is true, but
has_extension would not be.
This distinction does not matter to any of the callers,
though, and we can just replace uses of has_extension with
ends_with. We prefer the "ends_with" name because it is more
generic, and there is nothing about the function that
requires it to be used for file extensions.
Signed-off-by: Jeff King <redacted>
---
builtin/index-pack.c | 4 ++--
builtin/verify-pack.c | 4 ++--
git-compat-util.h | 7 -------
help.c | 2 +-
refs.c | 4 ++--
sha1_file.c | 10 +++++-----
6 files changed, 12 insertions(+), 19 deletions(-)
@@ -1603,7 +1603,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)die(_("--fix-thin cannot be used without --stdin"));if(!index_name&&pack_name){intlen=strlen(pack_name);-if(!has_extension(pack_name,".pack"))+if(!ends_with(pack_name,".pack"))die(_("packfile name '%s' does not end with '.pack'"),pack_name);index_name_buf=xmalloc(len);
@@ -1613,7 +1613,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)}if(keep_msg&&!keep_name&&pack_name){intlen=strlen(pack_name);-if(!has_extension(pack_name,".pack"))+if(!ends_with(pack_name,".pack"))die(_("packfile name '%s' does not end with '.pack'"),pack_name);keep_name_buf=xmalloc(len);
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
When stripping a suffix like:
if (ends_with(str, "foo"))
buf = xmemdupz(str, strlen(str) - 3);
we can instead use strip_suffix to avoid the constant 3,
which must match the literal "foo" (we sometimes use
strlen("foo") instead, but that means we are repeating
ourselves). The example above becomes:
if (strip_suffix(str, "foo", &len))
buf = xmemdupz(str, len);
This also saves a strlen(), since we calculate the string
length when detecting the suffix.
Note that in some cases we also switch from xstrndup to
xmemdupz, which saves a further strlen call.
Signed-off-by: Jeff King <redacted>
---
builtin/remote.c | 13 +++++++------
builtin/repack.c | 5 ++---
connected.c | 6 +++---
help.c | 5 ++---
4 files changed, 14 insertions(+), 15 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
We also switch to using strbufs, which lets us avoid the
potentially dangerous combination of a manual malloc
followed by a strcpy.
Signed-off-by: Jeff King <redacted>
---
builtin/index-pack.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
@@ -1602,24 +1603,22 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)if(fix_thin_pack&&!from_stdin)die(_("--fix-thin cannot be used without --stdin"));if(!index_name&&pack_name){-intlen=strlen(pack_name);-if(!ends_with(pack_name,".pack"))+size_tlen;+if(!strip_suffix(pack_name,".pack",&len))die(_("packfile name '%s' does not end with '.pack'"),pack_name);-index_name_buf=xmalloc(len);-memcpy(index_name_buf,pack_name,len-5);-strcpy(index_name_buf+len-5,".idx");-index_name=index_name_buf;+strbuf_add(&index_name_buf,pack_name,len);+strbuf_addstr(&index_name_buf,".idx");+index_name=index_name_buf.buf;}if(keep_msg&&!keep_name&&pack_name){-intlen=strlen(pack_name);-if(!ends_with(pack_name,".pack"))+size_tlen;+if(!strip_suffix(pack_name,".pack",&len))die(_("packfile name '%s' does not end with '.pack'"),pack_name);-keep_name_buf=xmalloc(len);-memcpy(keep_name_buf,pack_name,len-5);-strcpy(keep_name_buf+len-5,".keep");-keep_name=keep_name_buf;+strbuf_add(&keep_name_buf,pack_name,len);+strbuf_addstr(&keep_name_buf,".idx");+keep_name=keep_name_buf.buf;}if(verify){if(!index_name)
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
You can almost get away with just calling "strip_suffix_mem"
on a strbuf's buf and len fields. But we also need to move
the NUL-terminator to satisfy strbuf's invariants. Let's
provide a convenience wrapper that handles this.
Signed-off-by: Jeff King <redacted>
---
I called strbuf_setlen here because it seemed sensible to use that as an
opaque building block, but we are violating the invariant here for a
moment by setting sb->len for a moment. I think that is fine, as this is
a strbuf function, and can violate the invariant for a moment if it
wants to.
But if we care, we can keep a separate "size_t len" variable, and
strbuf_setlen to that.
Or we can make it less opaque, and replace the setlen with
"sb->buf[sb->len] = 0".
strbuf.h | 9 +++++++++
1 file changed, 9 insertions(+)
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
In this code, we try to convert both "foo.idx" and "foo"
into "foo.pack". By stripping the suffix, we can avoid a
confusing use of strbuf_splice, and make it clear that both
cases are adding ".pack" to the end.
Signed-off-by: Jeff King <redacted>
---
builtin/verify-pack.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:01:46
When we are reloading the list of packs, we check whether a
particular pack has been loaded. This is slightly tricky,
because we load packs based on the presence of their ".idx"
files, but record the name of the matching ".pack" file.
Therefore we want to compare their bases.
The existing code stripped off ".idx" from a file we found,
then compared that whole base length to strings containing
the ".pack" version. This meant we could end up comparing
bytes past what the ".pack" string contained, if the ".idx"
file name was much longer.
In practice, it worked OK because memcmp would end up seeing
a difference in the two strings and would return early
before hitting the full length. However, memcmp may
sometimes read extra bytes past a difference (e.g., because
it is comparing 64-bit words), or is even free to compare in
reverse order.
Furthermore, our memcmp made no guarantees that we matched
the whole pack name, up to ".pack". So "foo.idx" would match
"foo-bar.pack", which is wrong (but does not typically
happen, because our pack names have a fixed size).
We can fix both issues, avoid magic numbers, and document
that we expect to compare against a string with ".pack" by
using strip_suffix.
Signed-off-by: Jeff King <redacted>
---
This is a teeny bit less efficient than it could be, because we are
verifying our assumption at run-time that each pack name ends in
".pack". I'd venture to say if we cared about efficiency here, the low
hanging fruit would be to avoid the O(n^2) loop to find duplicate pack
names in the first place.
sha1_file.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 23:01:47
On Mon, Jun 30, 2014 at 9:55 AM, Jeff King [off-list ref] wrote:
Here's a series to do for ends_with what the recent skip_prefix series
did for starts_with. Namely: drop some magic numbers and repeated string
literals, and hopefully make things more readable.
The first patch is René's patch 1/2, with the leak fix from Duy and typo
fixes in the commit message from me. The rest are new, and replace the
changes to prepare_packed_git_one done elsewhere in the thread.
[1/9]: sha1_file: replace PATH_MAX buffer with strbuf in prepare_packed_git_one()
[2/9]: add strip_suffix function
[3/9]: implement ends_with via strip_suffix
[4/9]: replace has_extension with ends_with
[5/9]: use strip_suffix instead of ends_with in simple cases
[6/9]: index-pack: use strip_suffix to avoid magic numbers
[7/9]: strbuf: implement strbuf_strip_suffix
[8/9]: verify-pack: use strbuf_strip_suffix
[9/9]: prepare_packed_git_one: refactor duplicate-pack check