Re: git-p4: t9819 failing
On 23 September 2015 at 13:28, Lars Schneider wrote:
>
>> Here's the last bit of the crash dump from git-p4 I get:
>>
>> File "/home/ldiamand/git/git/git-p4", line 2580, in streamP4FilesCbSelf
>>self.streamP4FilesCb(entry)
>> File "/home/ldiamand/git/git/git-p4", line 2497, in streamP4FilesCb
>>required_bytes = int((4 * int(self.stream_file["fileSize"])) -
>> calcDiskFree(self.cloneDestination))
>> File "/home/ldiamand/git/git/git-p4", line 116, in calcDiskFree
>>st = os.statvfs(dirname)
>> OSError: [Errno 2] No such file or directory: 'lc'
>>
>> Luke
> Confirmed. What do you think about this fix?
Works for me!
>
> Thank you,
> Lars
>
> ---
> git-p4.py | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index 1d1bb87..66c0a4e 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -3478,6 +3478,7 @@ class P4Clone(P4Sync):
>
> print "Importing from %s into %s" % (', '.join(depotPaths),
> self.cloneDestination)
>
> +self.cloneDestination = os.path.abspath(self.cloneDestination)
> if not os.path.exists(self.cloneDestination):
> os.makedirs(self.cloneDestination)
> chdir(self.cloneDestination)
> --
--
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
[PATCH 2/4] pack-objects: do not get distracted by stale refs
It is quite possible for, say, a remote HEAD to become stale, e.g. when the default branch was renamed. We should still be able to pack our objects when such a thing happens; simply ignore invalid refs (because they cannot matter for the packing process anyway). Signed-off-by: Johannes Schindelin --- builtin/pack-objects.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 1c63f8f..ef2f794 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -2499,6 +2499,7 @@ static void get_object_list(int ac, const char **av) int flags = 0; init_revisions(&revs, NULL); + revs.ignore_missing = 1; save_commit_buffer = 0; setup_revisions(ac, av, &revs, NULL); -- 2.5.2.windows.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
[PATCH 1/4] gc: demonstrate failure with stale remote HEAD
Signed-off-by: Johannes Schindelin --- t/t6500-gc.sh | 15 +++ 1 file changed, 15 insertions(+) diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index 63194d8..b736774 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -30,4 +30,19 @@ test_expect_success 'gc -h with invalid configuration' ' test_i18ngrep "[Uu]sage" broken/usage ' +test_expect_failure 'gc removes broken refs/remotes//HEAD' ' + git init remote && + ( + cd remote && + test_commit initial && + git clone . ../client && + git branch -m develop && + cd ../client && + git fetch --prune && + git gc && + git branch --list -r origin/HEAD >actual && + test_line_count = 0 actual + ) +' + test_done -- 2.5.2.windows.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
[PATCH 0/4] Fix gc failure when a remote HEAD goes stale
There has been a report in the Git for Windows project that gc fails sometimes: https://github.com/git-for-windows/git/issues/423 It turns out that there are cases when a remote HEAD can go stale and it is not the user's fault at all. It can happen, for example, if the active branch in the remote repository gets renamed. Git's garbage collector should handle this gracefully. The best this developer could come up with, is to simply ignore and delete the now-broken refs. Johannes Schindelin (4): gc: demonstrate failure with stale remote HEAD pack-objects: do not get distracted by stale refs mark_reachable_objects(): optionally collect broken refs gc: remove broken refs builtin/pack-objects.c | 1 + builtin/prune.c| 12 +++- builtin/reflog.c | 2 +- reachable.c| 26 -- reachable.h| 3 ++- t/t6500-gc.sh | 15 +++ 6 files changed, 50 insertions(+), 9 deletions(-) -- 2.5.2.windows.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
[PATCH 3/4] mark_reachable_objects(): optionally collect broken refs
The behavior of `mark_reachable_objects()` without this patch is that it
dies if it encounters a broken ref. This is sometimes undesirable, e.g.
when garbage collecting in a repository with a stale remote HEAD.
So let's introduce an optional parameter to collect such broken refs. The
behavior of the function is unchanged if that parameter is `NULL`.
Signed-off-by: Johannes Schindelin
---
builtin/prune.c | 2 +-
builtin/reflog.c | 2 +-
reachable.c | 26 --
reachable.h | 3 ++-
4 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/builtin/prune.c b/builtin/prune.c
index 10b03d3..d6f664f 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -136,7 +136,7 @@ int cmd_prune(int argc, const char **argv, const char
*prefix)
if (show_progress)
progress = start_progress_delay(_("Checking connectivity"), 0,
0, 2);
- mark_reachable_objects(&revs, 1, expire, progress);
+ mark_reachable_objects(&revs, 1, expire, progress, NULL);
stop_progress(&progress);
for_each_loose_file_in_objdir(get_object_directory(), prune_object,
prune_cruft, prune_subdir, NULL);
diff --git a/builtin/reflog.c b/builtin/reflog.c
index f96ca2a..cb8758a 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -583,7 +583,7 @@ static int cmd_reflog_expire(int argc, const char **argv,
const char *prefix)
init_revisions(&cb.cmd.revs, prefix);
if (flags & EXPIRE_REFLOGS_VERBOSE)
printf("Marking reachable objects...");
- mark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);
+ mark_reachable_objects(&cb.cmd.revs, 0, 0, NULL, NULL);
if (flags & EXPIRE_REFLOGS_VERBOSE)
putchar('\n');
}
diff --git a/reachable.c b/reachable.c
index 9cff25b..1fc7ada 100644
--- a/reachable.c
+++ b/reachable.c
@@ -15,6 +15,11 @@ struct connectivity_progress {
unsigned long count;
};
+struct add_one_data {
+ struct rev_info *revs;
+ struct string_list *broken_refs;
+};
+
static void update_progress(struct connectivity_progress *cp)
{
cp->count++;
@@ -25,10 +30,14 @@ static void update_progress(struct connectivity_progress
*cp)
static int add_one_ref(const char *path, const struct object_id *oid,
int flag, void *cb_data)
{
- struct object *object = parse_object_or_die(oid->hash, path);
- struct rev_info *revs = (struct rev_info *)cb_data;
+ struct add_one_data *data = (struct add_one_data *)cb_data;
+ struct object *object = data->broken_refs ? parse_object(oid->hash) :
+ parse_object_or_die(oid->hash, path);
- add_pending_object(revs, object, "");
+ if (!object)
+ string_list_append(data->broken_refs, path);
+ else
+ add_pending_object(data->revs, object, "");
return 0;
}
@@ -153,9 +162,11 @@ int add_unseen_recent_objects_to_traversal(struct rev_info
*revs,
void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
unsigned long mark_recent,
- struct progress *progress)
+ struct progress *progress,
+ struct string_list *broken_refs)
{
struct connectivity_progress cp;
+ struct add_one_data data;
/*
* Set up revision parsing, and mark us as being interested
@@ -168,11 +179,14 @@ void mark_reachable_objects(struct rev_info *revs, int
mark_reflog,
/* Add all refs from the index file */
add_index_objects_to_pending(revs, 0);
+ data.revs = revs;
+ data.broken_refs = broken_refs;
+
/* Add all external refs */
- for_each_ref(add_one_ref, revs);
+ for_each_ref(add_one_ref, &data);
/* detached HEAD is not included in the list above */
- head_ref(add_one_ref, revs);
+ head_ref(add_one_ref, &data);
/* Add all reflog info */
if (mark_reflog)
diff --git a/reachable.h b/reachable.h
index d23efc3..39de1c7 100644
--- a/reachable.h
+++ b/reachable.h
@@ -5,6 +5,7 @@ struct progress;
extern int add_unseen_recent_objects_to_traversal(struct rev_info *revs,
unsigned long timestamp);
extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
- unsigned long mark_recent, struct progress
*);
+ unsigned long mark_recent, struct progress *,
+ struct string_list *broken_refs);
#endif
--
2.5.2.windows.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
[PATCH 4/4] gc: remove broken refs
When encountering broken refs, such as a stale remote HEAD (which can
happen if the active branch was renamed in the remote), it is more
helpful to remove those refs than to exit with an error.
This fixes https://github.com/git-for-windows/git/issues/423
Signed-off-by: Johannes Schindelin
---
builtin/prune.c | 12 +++-
t/t6500-gc.sh | 2 +-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/builtin/prune.c b/builtin/prune.c
index d6f664f..1a30f65 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -6,6 +6,7 @@
#include "reachable.h"
#include "parse-options.h"
#include "progress.h"
+#include "refs.h"
static const char * const prune_usage[] = {
N_("git prune [-n] [-v] [--expire ] [--] [...]"),
@@ -100,6 +101,7 @@ static void remove_temporary_files(const char *path)
int cmd_prune(int argc, const char **argv, const char *prefix)
{
struct rev_info revs;
+ struct string_list broken_refs = STRING_LIST_INIT_DUP;
struct progress *progress = NULL;
const struct option options[] = {
OPT__DRY_RUN(&show_only, N_("do not remove, show only")),
@@ -110,6 +112,7 @@ int cmd_prune(int argc, const char **argv, const char
*prefix)
OPT_END()
};
char *s;
+ int i;
expire = ULONG_MAX;
save_commit_buffer = 0;
@@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char
*prefix)
if (show_progress)
progress = start_progress_delay(_("Checking connectivity"), 0,
0, 2);
- mark_reachable_objects(&revs, 1, expire, progress, NULL);
+ revs.ignore_missing = 1;
+ mark_reachable_objects(&revs, 1, expire, progress, &broken_refs);
+ for (i = 0; i < broken_refs.nr; i++) {
+ char *path = broken_refs.items[i].string;
+ printf("Removing stale ref %s\n", path);
+ if (!show_only && delete_ref(path, NULL, REF_NODEREF))
+ die("Could not remove stale ref %s", path);
+ }
stop_progress(&progress);
for_each_loose_file_in_objdir(get_object_directory(), prune_object,
prune_cruft, prune_subdir, NULL);
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index b736774..0ae4271 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '
test_i18ngrep "[Uu]sage" broken/usage
'
-test_expect_failure 'gc removes broken refs/remotes//HEAD' '
+test_expect_success 'gc removes broken refs/remotes//HEAD' '
git init remote &&
(
cd remote &&
--
2.5.2.windows.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
Re: What's cooking in git.git (Sep 2015, #06; Wed, 23)
Hi Junio, On 2015-09-24 00:56, Junio C Hamano wrote: > * jc/fsck-dropped-errors (2015-09-23) 1 commit > - fsck: exit with non-zero when problems are found > > There were some classes of errors that "git fsck" diagnosed to its > standard error that did not cause it to exit with non-zero status. Thanks for this! > * sb/submodule-parallel-fetch (2015-09-23) 9 commits > - submodules: allow parallel fetching, add tests and documentation > - fetch_populated_submodules: use new parallel job processing > - SQUASH??? > - run-command: add an asynchronous parallel child processor > - run-command: factor out return value computation > - strbuf: add strbuf_read_once to read without blocking > - xread_nonblock: add functionality to read from fds without blocking > - xread: poll on non blocking fds > - submodule.c: write "Fetching submodule " to stderr > (this branch uses sb/submodule-helper.) > > Add a framework to spawn a group of processes in parallel, and use > it to run "git fetch --recurse-submodules" in parallel. > > It seems that the fundamentals are almost there, modulo polishing > the overall structure to ensure that future enhancement can be made > cleanly. Awesome. As I stated before, I am a big fan of this enhancement, even if I am sad that I have too little time to participate effectively. Ciao, Dscho -- 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
[PATCH] branch: handle errors on setting tracking branches
The function `install_branch_config`, which is used to set up
tracking branches, does not verify return codes of
`git_config_set`. Due to this we may incorrectly print that a
tracking branch has been set up when in fact we did not due to an
error.
Fix this by checking the return value of `git_config_set` and
returning early in the case of an error. The
`install_branch_config` function has been changed to return an
error code that reflects whether it has been successful.
Signed-off-by: Patrick Steinhardt
---
I've run into this issue when a stale config.lock has been left:
> git branch --set-upstream-to=origin/next
error: could not lock config file /path/to/repo/git/config: File exists
error: could not lock config file /path/to/repo/git/config: File exists
Branch next set up to track remote branch next from origin.
While we spit out some errors, we still claim that we set up the
tracking branch correctly, which is not the case.
Actually, there are quite a few places where we don't check the
return values of git_config_set and related functions. I didn't
go through them yet, but might do so if you deem this to be of
value.
branch.c | 24
branch.h | 3 ++-
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/branch.c b/branch.c
index d013374..5ab3ad4 100644
--- a/branch.c
+++ b/branch.c
@@ -48,22 +48,24 @@ static int should_setup_rebase(const char *origin)
return 0;
}
-void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
+int install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
const char *shortname = NULL;
struct strbuf key = STRBUF_INIT;
- int rebasing = should_setup_rebase(origin);
+ int ret = 0, rebasing = should_setup_rebase(origin);
if (skip_prefix(remote, "refs/heads/", &shortname)
&& !strcmp(local, shortname)
&& !origin) {
warning(_("Not setting branch %s as its own upstream."),
local);
- return;
+ return ret;
}
strbuf_addf(&key, "branch.%s.remote", local);
- git_config_set(key.buf, origin ? origin : ".");
+ ret = git_config_set(key.buf, origin ? origin : ".");
+ if (ret)
+ goto out;
strbuf_reset(&key);
strbuf_addf(&key, "branch.%s.merge", local);
@@ -72,9 +74,10 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
if (rebasing) {
strbuf_reset(&key);
strbuf_addf(&key, "branch.%s.rebase", local);
- git_config_set(key.buf, "true");
+ ret = git_config_set(key.buf, "true");
+ if (ret)
+ goto out;
}
- strbuf_release(&key);
if (flag & BRANCH_CONFIG_VERBOSE) {
if (shortname) {
@@ -101,6 +104,10 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
local, remote);
}
}
+
+out:
+ strbuf_release(&key);
+ return ret;
}
/*
@@ -133,8 +140,9 @@ static int setup_tracking(const char *new_ref, const char
*orig_ref,
return error(_("Not tracking: ambiguous information for ref
%s"),
orig_ref);
- install_branch_config(config_flags, new_ref, tracking.remote,
- tracking.src ? tracking.src : orig_ref);
+ if (install_branch_config(config_flags, new_ref, tracking.remote,
+ tracking.src ? tracking.src : orig_ref))
+ return error(_("Could not setup tracking branch"));
free(tracking.src);
return 0;
diff --git a/branch.h b/branch.h
index d3446ed..d287884 100644
--- a/branch.h
+++ b/branch.h
@@ -43,9 +43,10 @@ void remove_branch_state(void);
/*
* Configure local branch "local" as downstream to branch "remote"
* from remote "origin". Used by git branch --set-upstream.
+ * Returns zero on success, non-zero otherwise.
*/
#define BRANCH_CONFIG_VERBOSE 01
-extern void install_branch_config(int flag, const char *local, const char
*origin, const char *remote);
+extern int install_branch_config(int flag, const char *local, const char
*origin, const char *remote);
/*
* Read branch description
--
2.5.3
--
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
[PATCH] connect: Fix typo in result string of prot_name()
Replace 'unkown' with 'unknown'. Signed-off-by: Tobias Klauser --- connect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/connect.c b/connect.c index c0144d8..777f31c 100644 --- a/connect.c +++ b/connect.c @@ -254,7 +254,7 @@ static const char *prot_name(enum protocol protocol) case PROTO_GIT: return "git"; default: - return "unkown protocol"; + return "unknown protocol"; } } -- 2.5.3 -- 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] branch: handle errors on setting tracking branches
On Thu, Sep 24, 2015 at 12:32:01PM +0200, Patrick Steinhardt wrote: > The function `install_branch_config`, which is used to set up > tracking branches, does not verify return codes of > `git_config_set`. Due to this we may incorrectly print that a > tracking branch has been set up when in fact we did not due to an > error. > > Fix this by checking the return value of `git_config_set` and > returning early in the case of an error. The > `install_branch_config` function has been changed to return an > error code that reflects whether it has been successful. I think it's nice to tell the user when something has gone wrong, so in that sense this is a good change. Though git_config_set does already print something, so the main improvement here is passing up the error code from install_branch_config. What happens to that error? I count 4 callers in the current code, and none of them currently looks at the return value. Your patch updates setup_tracking() to propagate the error, which is good. But that is called from exactly one place, create_branch(), which also ignores the outcome. :-/ I don't think we want to die() when the config fails. That might be the right thing for "branch --set-upstream-to", but probably not for "checkout -b". I think we need to look at each call site and see whether we should be propagating the error back. With the hope that we would ultimately affect the exit code of the command. In the case of branch creation, we are in a two-step procedure: create the branch, then set up its tracking config. We _could_ rollback the first step when the second step fails, but that is probably not worth the complication. > Actually, there are quite a few places where we don't check the > return values of git_config_set and related functions. I didn't > go through them yet, but might do so if you deem this to be of > value. I think more error-checking is better than less. But I do think it's worth plumbing through the error codes in each case. It's tempting to just die() when the operation fails, but as discussed above, that's not always the most appropriate thing. > branch.c | 24 > branch.h | 3 ++- > 2 files changed, 18 insertions(+), 9 deletions(-) The patch itself looks OK to me from a cursory read. -Peff -- 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/4] pack-objects: do not get distracted by stale refs
On Thu, Sep 24, 2015 at 11:13:44AM +0200, Johannes Schindelin wrote: > It is quite possible for, say, a remote HEAD to become stale, e.g. when > the default branch was renamed. > > We should still be able to pack our objects when such a thing happens; > simply ignore invalid refs (because they cannot matter for the packing > process anyway). > [...] > init_revisions(&revs, NULL); > + revs.ignore_missing = 1; I think this is dangerous, because a repack may drop unreferenced history. Imagine you have a corrupted repository where you are missing the commit at the tip of "master", but still have "master^" and the rest of the history, and "git gc --auto" triggers. Right now, pack-objects will barf and refuse to pack. Your repo is still corrupted, but you can use git-fsck to recover the history minus the single commit. With your patch, the repack will delete the entire history. Of course there are complications like other branches referencing the history, reflogs if it is a non-bare repo, and the 2-week expiration time. But there's definitely a potential to make things worse. I think commands that drop objects (like repack and prune) need to be more careful than other commands about refusing to run when there is something fishy in the repository. I think we would want to introduce a flag to ignore dangling symbolic refs when running for_each_ref, as those should be harmless. In fact, I think we could omit symrefs completely for reachability analysis; we would hit the ref they refer to anyway during the rest of the traversal. -Peff -- 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
[PATCH v2 1/1] Makefile: link libcurl before openssl and crypto
For static linking especially library order while linking is important. For example libssl contains symbol from libcrypto so the former should be linked before the latter. The global link order should be libcurl then libssl then libcrypto then libintl and finally zlib. Signed-off-by: Remi Pommarel --- Makefile | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index ce0cfe2..59a0747 100644 --- a/Makefile +++ b/Makefile @@ -1029,7 +1029,6 @@ ifdef HAVE_ALLOCA_H endif IMAP_SEND_BUILDDEPS = -IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO) ifdef NO_CURL BASIC_CFLAGS += -DNO_CURL @@ -1086,6 +1085,7 @@ else endif endif endif +IMAP_SEND_LDFLAGS += $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO) ifdef ZLIB_PATH BASIC_CFLAGS += -I$(ZLIB_PATH)/include @@ -1971,10 +1971,10 @@ git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS) git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ - $(LIBS) $(CURL_LIBCURL) + $(CURL_LIBCURL) $(LIBS) git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ - $(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) + $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS $(GITLIBS) $(VCSSVN_LIB) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) \ @@ -1988,7 +1988,7 @@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ - $(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) + $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) $(LIB_FILE): $(LIB_OBJS) $(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^ -- 2.0.1 -- 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 3/4] mark_reachable_objects(): optionally collect broken refs
On Thu, Sep 24, 2015 at 11:13:52AM +0200, Johannes Schindelin wrote: > The behavior of `mark_reachable_objects()` without this patch is that it > dies if it encounters a broken ref. This is sometimes undesirable, e.g. > when garbage collecting in a repository with a stale remote HEAD. > > So let's introduce an optional parameter to collect such broken refs. The > behavior of the function is unchanged if that parameter is `NULL`. Similar comment to the last one. :) I suspect the issues you are seeing are largely new due to the ref-paranoia work I did (merged in 05e816e37). We used to ignore broken refs at the for_each_ref() level, but now we feed them to the calling code (which generally chokes). So in that sense, a simpler fix than your series would be to simply revert 8d42299 and ff4056bbc. :) But I think those new checks are valuable, and we really just need to gracefully ignore the dangling-symref case (which is _a_ breakage, but not a dangerous one). -Peff -- 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 4/4] gc: remove broken refs
On Thu, Sep 24, 2015 at 11:14:02AM +0200, Johannes Schindelin wrote: > When encountering broken refs, such as a stale remote HEAD (which can > happen if the active branch was renamed in the remote), it is more > helpful to remove those refs than to exit with an error. For the same reasons as in my earlier responses, I think it's dangerous to remove broken refs (it makes a small corruption much worse). It seems reasonably sane to remove a dangling symref, though if we teach for_each_ref to gracefully skip them, then leaving them in place isn't a problem. -Peff -- 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
[PATCH v6b 5/8] branch: drop non-commit error reporting
Remove the error "branch '%s' does not point at a commit" in
apppend_ref() which reports branch refs which do not point to
commits. Also remove the error "some refs could not be read" in
print_ref_list() which is triggered as a consequence of the first
error.
This seems to be the wrong codepath whose purpose is not to diagnose
and report a repository corruption. If we care about such a repository
corruption, we should report it from fsck instead.
This also helps in a smooth port of branch.c to use ref-filter APIs
over the following patches. On the other hand, ref-filter ignores refs
which do not point at commits silently.
Based-on-patch-by: Jeff King
Helped-by: Junio C Hamano
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
builtin/branch.c | 18 --
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 1a664ed..ebc3742 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -313,7 +313,6 @@ static char *resolve_symref(const char *src, const char
*prefix)
struct append_ref_cb {
struct ref_list *ref_list;
const char **pattern;
- int ret;
};
static int match_patterns(const char **pattern, const char *refname)
@@ -370,10 +369,8 @@ static int append_ref(const char *refname, const struct
object_id *oid, int flag
commit = NULL;
if (ref_list->verbose || ref_list->with_commit || merge_filter !=
NO_FILTER) {
commit = lookup_commit_reference_gently(oid->hash, 1);
- if (!commit) {
- cb->ret = error(_("branch '%s' does not point at a
commit"), refname);
+ if (!commit)
return 0;
- }
/* Filter with with_commit if specified */
if (!is_descendant_of(commit, ref_list->with_commit))
@@ -617,7 +614,7 @@ static int calc_maxwidth(struct ref_list *refs, int
remote_bonus)
return max;
}
-static int print_ref_list(int kinds, int detached, int verbose, int abbrev,
struct commit_list *with_commit, const char **pattern)
+static void print_ref_list(int kinds, int detached, int verbose, int abbrev,
struct commit_list *with_commit, const char **pattern)
{
int i;
struct append_ref_cb cb;
@@ -642,7 +639,6 @@ static int print_ref_list(int kinds, int detached, int
verbose, int abbrev, stru
init_revisions(&ref_list.revs, NULL);
cb.ref_list = &ref_list;
cb.pattern = pattern;
- cb.ret = 0;
/*
* First we obtain all regular branch refs and if the HEAD is
* detached then we insert that ref to the end of the ref_fist
@@ -693,11 +689,6 @@ static int print_ref_list(int kinds, int detached, int
verbose, int abbrev, stru
abbrev, detached, remote_prefix);
free_ref_list(&ref_list);
-
- if (cb.ret)
- error(_("some refs could not be read"));
-
- return cb.ret;
}
static void rename_branch(const char *oldname, const char *newname, int force)
@@ -913,15 +904,14 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
die(_("branch name required"));
return delete_branches(argc, argv, delete > 1, kinds, quiet);
} else if (list) {
- int ret;
/* git branch --local also shows HEAD when it is detached */
if (kinds & REF_LOCAL_BRANCH)
kinds |= REF_DETACHED_HEAD;
- ret = print_ref_list(kinds, detached, verbose, abbrev,
+ print_ref_list(kinds, detached, verbose, abbrev,
with_commit, argv);
print_columns(&output, colopts, NULL);
string_list_clear(&output, 0);
- return ret;
+ return 0;
}
else if (edit_description) {
const char *branch_name;
--
2.5.1
--
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
[PATCH] t5561: get rid of racy appending to logfile
The definition of log_div() appended information to the web server's
logfile to make the test more readable. However, log_div() was called
right after a request is served (which is done by git-http-backend);
the web server waits for the git-http-backend process to exit before
it writes to the log file. When the duration between serving a request
and exiting was long, the log_div() output was written before the last
request's log, and the test failed. (This duration could become
especially long for PROFILE=GEN builds.)
To get rid of this behavior, we should not change the logfile at all.
This commit removes log_div() and its calls. The additional information
is kept in the test (for readability reasons) but filtered out before
comparing it to the actual logfile.
Signed-off-by: Stephan Beyer
---
Okay Peff, I added the information to the commit message (in my own
words). Past tense for the situation before the patch, present tense
for the situation after (hope that's right but should not be too
important).
I also used your proposed grep line because it is probably more robust.
t/t5560-http-backend-noserver.sh | 4
t/t5561-http-backend.sh | 8 +---
t/t556x_common | 12
3 files changed, 1 insertion(+), 23 deletions(-)
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index aa73eea..9fafcf1 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -44,10 +44,6 @@ POST() {
test_cmp exp act
}
-log_div() {
- return 0
-}
-
. "$TEST_DIRECTORY"/t556x_common
expect_aliased() {
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 19afe96..73dcb29 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -29,15 +29,9 @@ POST() {
test_cmp exp act
}
-log_div() {
- echo >>"$HTTPD_ROOT_PATH"/access.log
- echo "### $1" >>"$HTTPD_ROOT_PATH"/access.log
- echo "###" >>"$HTTPD_ROOT_PATH"/access.log
-}
-
. "$TEST_DIRECTORY"/t556x_common
-cat >exp
Re: [PATCH] t5561: get rid of racy appending to logfile
On Thu, Sep 24, 2015 at 08:12:22PM +0200, Stephan Beyer wrote: > The definition of log_div() appended information to the web server's > logfile to make the test more readable. However, log_div() was called > right after a request is served (which is done by git-http-backend); > the web server waits for the git-http-backend process to exit before > it writes to the log file. When the duration between serving a request > and exiting was long, the log_div() output was written before the last > request's log, and the test failed. (This duration could become > especially long for PROFILE=GEN builds.) > > To get rid of this behavior, we should not change the logfile at all. > This commit removes log_div() and its calls. The additional information > is kept in the test (for readability reasons) but filtered out before > comparing it to the actual logfile. > > Signed-off-by: Stephan Beyer > --- > Okay Peff, I added the information to the commit message (in my own > words). Past tense for the situation before the patch, present tense > for the situation after (hope that's right but should not be too > important). > > I also used your proposed grep line because it is probably more robust. This all looks good to me. Thanks so much for working on this. > -cat >exp < +grep '^[^#]' >exp
[PATCH 1/1] configure.ac: detect ssl need with libcurl
When libcurl has been statically compiled with openssl support they both
need to be linked in everytime libcurl is used.
During configuration this can be detected by looking for Curl_ssl_init
function symbol in libcurl, which will only be present if libcurl has been
compiled statically built with openssl.
configure.ac checks for Curl_ssl_init function in libcurl and if such function
exists; it sets NEEDS_SSL_WITH_CURL that is used by the Makefile to include
-lssl alongside with -lcurl.
Signed-off-by: Remi Pommarel
---
configure.ac | 10 ++
1 file changed, 10 insertions(+)
diff --git a/configure.ac b/configure.ac
index 14012fa..39837c8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -521,6 +521,16 @@ AC_CHECK_LIB([curl], [curl_global_init],
[NO_CURL=],
[NO_CURL=YesPlease])
+if test -z "${NO_CURL}" && test -z "${NO_OPENSSL}"; then
+
+AC_CHECK_LIB([curl], [Curl_ssl_init],
+[NEEDS_SSL_WITH_CURL=YesPlease],
+[NEEDS_SSL_WITH_CURL=])
+
+GIT_CONF_SUBST([NEEDS_SSL_WITH_CURL])
+
+fi
+
GIT_UNSTASH_FLAGS($CURLDIR)
GIT_CONF_SUBST([NO_CURL])
--
2.0.1
--
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
Business Deal
I have a business deal for you worth 24.5m usd, Reply: wl548...@gmail.com for more details if interested. Thank You --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus -- 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: git-p4: t9819 failing
OK, slight correction there - it now doesn't crash getting the disk
usage, but I think it still needs to be updated following the other
changes to case-handling.
Luke
On 24 September 2015 at 08:45, Luke Diamand wrote:
> On 23 September 2015 at 13:28, Lars Schneider
> wrote:
>>
>>> Here's the last bit of the crash dump from git-p4 I get:
>>>
>>> File "/home/ldiamand/git/git/git-p4", line 2580, in streamP4FilesCbSelf
>>>self.streamP4FilesCb(entry)
>>> File "/home/ldiamand/git/git/git-p4", line 2497, in streamP4FilesCb
>>>required_bytes = int((4 * int(self.stream_file["fileSize"])) -
>>> calcDiskFree(self.cloneDestination))
>>> File "/home/ldiamand/git/git/git-p4", line 116, in calcDiskFree
>>>st = os.statvfs(dirname)
>>> OSError: [Errno 2] No such file or directory: 'lc'
>>>
>>> Luke
>> Confirmed. What do you think about this fix?
>
> Works for me!
>
>
>
>>
>> Thank you,
>> Lars
>>
>> ---
>> git-p4.py | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 1d1bb87..66c0a4e 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -3478,6 +3478,7 @@ class P4Clone(P4Sync):
>>
>> print "Importing from %s into %s" % (', '.join(depotPaths),
>> self.cloneDestination)
>>
>> +self.cloneDestination = os.path.abspath(self.cloneDestination)
>> if not os.path.exists(self.cloneDestination):
>> os.makedirs(self.cloneDestination)
>> chdir(self.cloneDestination)
>> --
--
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 v7 0/7] git-p4: add support for large file systems
On 23 September 2015 at 12:42, Lars Schneider wrote: > > On 23 Sep 2015, at 13:25, Luke Diamand wrote: > >> Adding back git@vger.kernel.org, which I inadvertently dropped off the >> thread. >> >> On 23 September 2015 at 12:22, Luke Diamand wrote: >>> On 23 September 2015 at 11:09, Lars Schneider >>> wrote: On 23 Sep 2015, at 11:22, Luke Diamand wrote: > On 23 September 2015 at 09:50, Lars Schneider > wrote: >> >> On 23 Sep 2015, at 10:18, Lars Schneider >> wrote: > > > >> >> I think I found an easy fix. Can you try it? >> >> (in case my mail app messes something up: I changed line 2240 in >> git-p4.py to 'self.cloneDestination = os.getcwd()’) > > It fixes the problem, but in re-running the tests, I'm seeing t9808 > fail which doesn't happen on next. Confirmed. I fixed it. Do you think it makes sense to create a new roll v8 for Junio? >>> >>> How about we leave it a day or two in case anything else crawls out of >>> the woodwork? >>> >>> Thanks! >>> Luke > sounds good to me! OK, not sure if I'm just doing something daft herebut it seems to be ignoring the size limit! I've got the version from the branch: 8fee565 git-p4: add Git LFS backend for large file system Plus the couple of oneliner fixes for cloneDestination. I've created a repo with a file called foo, size 16MB, and another file called foo.mp4, which is just a symlink to foo. I then do: $ mkdir a $ cd a $ git init . $ git config git-p4.largeFileSystem GitLFS $ git config git-p4.largeFileExtensions mp4 $ git config git-p4.largeFileThreshold 100k $ git-p4.py sync -v //depot That reports that both foo and foo.mp4 have been pushed into LFS: //depot/foo --> foo (16 MB) Reading pipe: ['git', 'config', '--bool', 'git-p4.pushLargeFiles'] foo moved to large file system (/home/lgd/git/git/x/git/a/.git/lfs/objects/08/0a/080acf35a507ac9849cfcba47dc2ad83e01b75663a516279c8b9d243b719643e) //depot/foo.mp4 --> foo.mp4 (0 MB) foo.mp4 moved to large file system (/home/lgd/git/git/x/git/a/.git/lfs/objects/2c/26/2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae) But the file system says otherwise: $ ls -l -rw-r--r-- 1 lgd lgd 16777216 Sep 24 21:38 foo -rw-r--r-- 1 lgd lgd3 Sep 24 21:38 foo.mp4 As does git: git ls-files --debug .gitattributes ctime: 1443127106:535552029 mtime: 1443127106:535552029 dev: 65025ino: 13638459 uid: 1000 gid: 1000 size: 94 flags: 0 foo ctime: 1443127106:579552030 mtime: 1443127106:579552030 dev: 65025ino: 13638462 uid: 1000 gid: 1000 size: 16777216flags: 0< Quite large! foo.mp4 ctime: 1443127106:599552030 mtime: 1443127106:599552030 dev: 65025ino: 13638463 uid: 1000 gid: 1000 size: 3 flags: 0 What's going on? Thanks! Luke -- 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
[PATCH 1/1] configure: make curl-config path configurable
There are situations, ie during cross compilation, where curl-config program is not present in the PATH. Make configure check that a custom curl-config program is passed by the user through ac_cv_prog_CURL_CONFIG then set CURL_CONFIG variable accordingly in config.mak.autogen. Makefile uses this variable to get the target's curl configuration program. Signed-off-by: Remi Pommarel --- Makefile | 5 +++-- configure.ac | 10 ++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index ce0cfe2..81ac5bb 100644 --- a/Makefile +++ b/Makefile @@ -374,6 +374,7 @@ LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) STRIP ?= strip +CURL_CONFIG=curl-config # Among the variables below, these: # gitexecdir @@ -1059,13 +1060,13 @@ else REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES) PROGRAM_OBJS += http-fetch.o PROGRAMS += $(REMOTE_CURL_NAMES) - curl_check := $(shell (echo 070908; curl-config --vernum | sed -e '/^70[BC]/s/^/0/') 2>/dev/null | sort -r | sed -ne 2p) + curl_check := $(shell (echo 070908; $(CURL_CONFIG) --vernum | sed -e '/^70[BC]/s/^/0/') 2>/dev/null | sort -r | sed -ne 2p) ifeq "$(curl_check)" "070908" ifndef NO_EXPAT PROGRAM_OBJS += http-push.o endif endif - curl_check := $(shell (echo 072200; curl-config --vernum | sed -e '/^70[BC]/s/^/0/') 2>/dev/null | sort -r | sed -ne 2p) + curl_check := $(shell (echo 072200; $(CURL_CONFIG) --vernum | sed -e '/^70[BC]/s/^/0/') 2>/dev/null | sort -r | sed -ne 2p) ifeq "$(curl_check)" "072200" USE_CURL_FOR_IMAP_SEND = YesPlease endif diff --git a/configure.ac b/configure.ac index 14012fa..acc23fb 100644 --- a/configure.ac +++ b/configure.ac @@ -525,6 +525,16 @@ GIT_UNSTASH_FLAGS($CURLDIR) GIT_CONF_SUBST([NO_CURL]) +if test -z "$NO_CURL"; then + +AC_CHECK_PROG([CURL_CONFIG], [curl-config], [curl-config], [no]) +if test $CURL_CONFIG != no; then +GIT_CONF_SUBST([CURL_CONFIG]) +fi + +fi + + # # Define NO_EXPAT if you do not have expat installed. git-http-push is # not built, and you cannot push using http:// and https:// transports. -- 2.0.1 -- 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
[PATCH v2 0/68] war on sprintf
This is a revised version of the series I sent earlier[1]. For those just joining us, the goal is to remove sprintf, strcpy, etc, to make it easier to audit the code base for buffer overflows. I've been addressing review comments for individual patches as the discussion progressed, so there's nothing here that hasn't been on the list (and I think I've addressed all of the comments from the first round of review). Here's a list of the commits, with an overview[2] of the changes from v1 (patches without comment are the same as in v1): [01/68]: show-branch: avoid segfault with --reflog of unborn branch [02/68]: mailsplit: fix FILE* leak in split_maildir [03/68]: archive-tar: fix minor indentation violation [04/68]: fsck: don't fsck alternates for connectivity-only check [05/68]: add xsnprintf helper function [06/68]: add git_path_buf helper function [07/68]: strbuf: make strbuf_complete_line more generic Docstring now makes clear the behavior when the strbuf is empty. [08/68]: add reentrant variants of sha1_to_hex and find_unique_abbrev These are now called sha1_to_hex_r, etc. The re-entrancy conditions are documented. [09/68]: fsck: use strbuf to generate alternate directories [10/68]: mailsplit: make PATH_MAX buffers dynamic Use xstrfmt consistently instead of sometimes using a strbuf. [11/68]: trace: use strbuf for quote_crnl output Further cleanup of "p2" variable in quote_crnl. [12/68]: progress: store throughput display in a strbuf [13/68]: test-dump-cache-tree: avoid overflow of cache-tree name [14/68]: compat/inet_ntop: fix off-by-one in inet_ntop4 [15/68]: convert trivial sprintf / strcpy calls to xsnprintf Prefer "fixed-size" to "static" for clarity in commit message. [16/68]: archive-tar: use xsnprintf for trivial formatting [17/68]: use xsnprintf for generating git object headers [18/68]: find_short_object_filename: convert sprintf to xsnprintf [19/68]: stop_progress_msg: convert sprintf to xsnprintf Ditto on "fixed-size" versus "static". [20/68]: compat/hstrerror: convert sprintf to snprintf [21/68]: grep: use xsnprintf to format failure message [22/68]: entry.c: convert strcpy to xsnprintf [23/68]: add_packed_git: convert strcpy into xsnprintf Drop useless comment. Add comment on magic strlen(). [24/68]: http-push: replace strcat with xsnprintf [25/68]: receive-pack: convert strncpy to xsnprintf [26/68]: replace trivial malloc + sprintf / strcpy calls with xstrfmt Include NUL in base64 of imap-send cram response. My guess is this is a bug in what we send that is overlooked by most servers, but I'd prefer to be conservative here and keep the behavior the same. [27/68]: config: use xstrfmt in normalize_value [28/68]: fetch: replace static buffer with xstrfmt [29/68]: use strip_suffix and xstrfmt to replace suffix [30/68]: ref-filter: drop sprintf and strcpy calls [31/68]: help: drop prepend function in favor of xstrfmt [32/68]: mailmap: replace strcpy with xstrdup [33/68]: read_branches_file: simplify string handling This goes much further than the original in cleaning up the use of a static buffer. From what I sent earlier during review, I dropped strbuf_read_file in favor of strbuf_getline, to keep the behavior identical to the original. [34/68]: read_remotes_file: simplify string handling New in this iteration; cleanups to match those in 33/68. [35/68]: resolve_ref: use strbufs for internal buffers [36/68]: upload-archive: convert sprintf to strbuf [37/68]: remote-ext: simplify git pkt-line generation The v1 of this patch was totally buggy. This is a rewrite to use packet_write(), which has several advantages, and includes tests. [38/68]: http-push: use strbuf instead of fwrite_buffer [39/68]: http-walker: store url in a strbuf [40/68]: sha1_get_pack_name: use a strbuf [41/68]: init: use strbufs to store paths [42/68]: apply: convert root string to strbuf [43/68]: transport: use strbufs for status table "quickref" strings [44/68]: merge-recursive: convert malloc / strcpy to strbuf [45/68]: enter_repo: convert fixed-size buffers to strbufs [46/68]: remove_leading_path: use a strbuf for internal storage [47/68]: write_loose_object: convert to strbuf Clarify comment on subtle mkstemp behavior. [48/68]: diagnose_invalid_index_path: use strbuf to avoid strcpy/strcat [49/68]: fetch-pack: use argv_array for index-pack / unpack-objects [50/68]: http-push: use an argv_array for setup_revisions [51/68]: stat_tracking_info: convert to argv_array [52/68]: daemon: use cld->env_array when re-spawning [53/68]: use sha1_to_hex_r() instead of strcpy Use "_r" versions to match change in patch 8. [54/68]: drop strcpy in favor of raw sha1_to_hex More history in the commit message, courtesy of Eric. [55/68]: color: add overflow checks for parsing colors Be more careful with OUT() macro.
[PATCH 02/68] mailsplit: fix FILE* leak in split_maildir
If we encounter an error while splitting a maildir, we exit
the function early, leaking the open filehandle. This isn't
a big deal, since we exit the program soon after, but it's
easy enough to be careful.
Signed-off-by: Jeff King
---
builtin/mailsplit.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 8e02ea1..9de06e3 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -150,6 +150,7 @@ static int split_maildir(const char *maildir, const char
*dir,
{
char file[PATH_MAX];
char name[PATH_MAX];
+ FILE *f = NULL;
int ret = -1;
int i;
struct string_list list = STRING_LIST_INIT_DUP;
@@ -160,7 +161,6 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
for (i = 0; i < list.nr; i++) {
- FILE *f;
snprintf(file, sizeof(file), "%s/%s", maildir,
list.items[i].string);
f = fopen(file, "r");
if (!f) {
@@ -177,10 +177,13 @@ static int split_maildir(const char *maildir, const char
*dir,
split_one(f, name, 1);
fclose(f);
+ f = NULL;
}
ret = skip;
out:
+ if (f)
+ fclose(f);
string_list_clear(&list, 1);
return ret;
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 01/68] show-branch: avoid segfault with --reflog of unborn branch
When no branch is given to the "--reflog" option, we resolve
HEAD to get the default branch. However, if HEAD points to
an unborn branch, resolve_ref returns NULL, and we later
segfault trying to access it.
Signed-off-by: Jeff King
---
builtin/show-branch.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 408ce70..092b59b 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -743,6 +743,8 @@ int cmd_show_branch(int ac, const char **av, const char
*prefix)
fake_av[1] = NULL;
av = fake_av;
ac = 1;
+ if (!*av)
+ die("no branches given, and HEAD is not valid");
}
if (ac != 1)
die("--reflog option needs one branch name");
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 03/68] archive-tar: fix minor indentation violation
This looks like a simple omission from 8539070 (archive-tar:
unindent write_tar_entry by one level, 2012-05-03).
Signed-off-by: Jeff King
---
archive-tar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/archive-tar.c b/archive-tar.c
index 0d1e6bd..b6b30bb 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -233,7 +233,7 @@ static int write_tar_entry(struct archiver_args *args,
size_t rest = pathlen - plen - 1;
if (plen > 0 && rest <= sizeof(header.name)) {
memcpy(header.prefix, path, plen);
- memcpy(header.name, path + plen + 1, rest);
+ memcpy(header.name, path + plen + 1, rest);
} else {
sprintf(header.name, "%s.data",
sha1_to_hex(sha1));
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 04/68] fsck: don't fsck alternates for connectivity-only check
Commit 02976bf (fsck: introduce `git fsck --connectivity-only`,
2015-06-22) recently gave fsck an option to perform only a
subset of the checks, by skipping the fsck_object_dir()
call. However, it does so only for the local object
directory, and we still do expensive checks on any alternate
repos. We should skip them in this case, too.
Signed-off-by: Jeff King
---
builtin/fsck.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 0794703..46c7235 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -678,16 +678,17 @@ int cmd_fsck(int argc, const char **argv, const char
*prefix)
git_config(fsck_config, NULL);
fsck_head_link();
- if (!connectivity_only)
+ if (!connectivity_only) {
fsck_object_dir(get_object_directory());
- prepare_alt_odb();
- for (alt = alt_odb_list; alt; alt = alt->next) {
- char namebuf[PATH_MAX];
- int namelen = alt->name - alt->base;
- memcpy(namebuf, alt->base, namelen);
- namebuf[namelen - 1] = 0;
- fsck_object_dir(namebuf);
+ prepare_alt_odb();
+ for (alt = alt_odb_list; alt; alt = alt->next) {
+ char namebuf[PATH_MAX];
+ int namelen = alt->name - alt->base;
+ memcpy(namebuf, alt->base, namelen);
+ namebuf[namelen - 1] = 0;
+ fsck_object_dir(namebuf);
+ }
}
if (check_full) {
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 17/68] use xsnprintf for generating git object headers
We generally use 32-byte buffers to format git's "type size"
header fields. These should not generally overflow unless
you can produce some truly gigantic objects (and our types
come from our internal array of constant strings). But it is
a good idea to use xsnprintf to make sure this is the case.
Note that we slightly modify the interface to
write_sha1_file_prepare, which nows uses "hdrlen" as an "in"
parameter as well as an "out" (on the way in it stores the
allocated size of the header, and on the way out it returns
the ultimate size of the header).
Signed-off-by: Jeff King
---
builtin/index-pack.c | 2 +-
bulk-checkin.c | 4 ++--
fast-import.c| 4 ++--
http-push.c | 2 +-
sha1_file.c | 13 +++--
5 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3431de2..1ad1bde 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -441,7 +441,7 @@ static void *unpack_entry_data(unsigned long offset,
unsigned long size,
int hdrlen;
if (!is_delta_type(type)) {
- hdrlen = sprintf(hdr, "%s %lu", typename(type), size) + 1;
+ hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(type),
size) + 1;
git_SHA1_Init(&c);
git_SHA1_Update(&c, hdr, hdrlen);
} else
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 7cffc3a..4347f5c 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -200,8 +200,8 @@ static int deflate_to_pack(struct bulk_checkin_state *state,
if (seekback == (off_t) -1)
return error("cannot find the current offset");
- header_len = sprintf((char *)obuf, "%s %" PRIuMAX,
-typename(type), (uintmax_t)size) + 1;
+ header_len = xsnprintf((char *)obuf, sizeof(obuf), "%s %" PRIuMAX,
+ typename(type), (uintmax_t)size) + 1;
git_SHA1_Init(&ctx);
git_SHA1_Update(&ctx, obuf, header_len);
diff --git a/fast-import.c b/fast-import.c
index 6c7c3c9..d0c2502 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1035,8 +1035,8 @@ static int store_object(
git_SHA_CTX c;
git_zstream s;
- hdrlen = sprintf((char *)hdr,"%s %lu", typename(type),
- (unsigned long)dat->len) + 1;
+ hdrlen = xsnprintf((char *)hdr, sizeof(hdr), "%s %lu",
+ typename(type), (unsigned long)dat->len) + 1;
git_SHA1_Init(&c);
git_SHA1_Update(&c, hdr, hdrlen);
git_SHA1_Update(&c, dat->buf, dat->len);
diff --git a/http-push.c b/http-push.c
index 154e67b..1f3788f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -361,7 +361,7 @@ static void start_put(struct transfer_request *request)
git_zstream stream;
unpacked = read_sha1_file(request->obj->sha1, &type, &len);
- hdrlen = sprintf(hdr, "%s %lu", typename(type), len) + 1;
+ hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(type), len) + 1;
/* Set it up */
git_deflate_init(&stream, zlib_compression_level);
diff --git a/sha1_file.c b/sha1_file.c
index d295a32..f106091 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1464,7 +1464,7 @@ int check_sha1_signature(const unsigned char *sha1, void
*map,
return -1;
/* Generate the header */
- hdrlen = sprintf(hdr, "%s %lu", typename(obj_type), size) + 1;
+ hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(obj_type),
size) + 1;
/* Sha1.. */
git_SHA1_Init(&c);
@@ -2930,7 +2930,7 @@ static void write_sha1_file_prepare(const void *buf,
unsigned long len,
git_SHA_CTX c;
/* Generate the header */
- *hdrlen = sprintf(hdr, "%s %lu", type, len)+1;
+ *hdrlen = xsnprintf(hdr, *hdrlen, "%s %lu", type, len)+1;
/* Sha1.. */
git_SHA1_Init(&c);
@@ -2993,7 +2993,7 @@ int hash_sha1_file(const void *buf, unsigned long len,
const char *type,
unsigned char *sha1)
{
char hdr[32];
- int hdrlen;
+ int hdrlen = sizeof(hdr);
write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);
return 0;
}
@@ -3139,7 +3139,7 @@ static int freshen_packed_object(const unsigned char
*sha1)
int write_sha1_file(const void *buf, unsigned long len, const char *type,
unsigned char *sha1)
{
char hdr[32];
- int hdrlen;
+ int hdrlen = sizeof(hdr);
/* Normally if we have it in the pack then we do not bother writing
* it out into .git/objects/??/?{38} file.
@@ -3157,7 +3157,8 @@ int hash_sha1_file_literally(const void *buf, unsigned
long len, const char *typ
int hdrlen, status = 0;
/* type string, SP, %lu of the length plus NUL must fit this */
- header = xmalloc(strlen(type) + 32);
+ hdrlen = strlen(type) + 32;
+ header = xmalloc(hdrlen);
write_sha1_file_prepare(buf, len, type, sha1, header, &hdrlen);
if (!(flags
[PATCH 16/68] archive-tar: use xsnprintf for trivial formatting
When we generate tar headers, we sprintf() values directly
into a struct with the fixed-size header values. For the
most part this is fine, as we are formatting small values
(e.g., the octal format of "mode & 0x" is of fixed
length). But it's still a good idea to use xsnprintf here.
It communicates to readers what our expectation is, and it
provides a run-time check that we are not overflowing the
buffers.
The one exception here is the mtime, which comes from the
epoch time of the commit we are archiving. For sane values,
this fits into the 12-byte value allocated in the header.
But since git can handle 64-bit times, if I claim to be a
visitor from the year 10,000 AD, I can overflow the buffer.
This turns out to be harmless, as we simply overflow into
the chksum field, which is then overwritten.
This case is also best as an xsnprintf. It should never come
up, short of extremely malformed dates, and in that case we
are probably better off dying than silently truncating the
date value (and we cannot expand the size of the buffer,
since it is dictated by the ustar format). Our friends in
the year 5138 (when we legitimately flip to a 12-digit
epoch) can deal with that problem then.
Signed-off-by: Jeff King
---
archive-tar.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index d543f93..501ca97 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -167,21 +167,21 @@ static void prepare_header(struct archiver_args *args,
struct ustar_header *header,
unsigned int mode, unsigned long size)
{
- sprintf(header->mode, "%07o", mode & 0);
- sprintf(header->size, "%011lo", S_ISREG(mode) ? size : 0);
- sprintf(header->mtime, "%011lo", (unsigned long) args->time);
+ xsnprintf(header->mode, sizeof(header->mode), "%07o", mode & 0);
+ xsnprintf(header->size, sizeof(header->size), "%011lo", S_ISREG(mode) ?
size : 0);
+ xsnprintf(header->mtime, sizeof(header->mtime), "%011lo", (unsigned
long) args->time);
- sprintf(header->uid, "%07o", 0);
- sprintf(header->gid, "%07o", 0);
+ xsnprintf(header->uid, sizeof(header->uid), "%07o", 0);
+ xsnprintf(header->gid, sizeof(header->gid), "%07o", 0);
strlcpy(header->uname, "root", sizeof(header->uname));
strlcpy(header->gname, "root", sizeof(header->gname));
- sprintf(header->devmajor, "%07o", 0);
- sprintf(header->devminor, "%07o", 0);
+ xsnprintf(header->devmajor, sizeof(header->devmajor), "%07o", 0);
+ xsnprintf(header->devminor, sizeof(header->devminor), "%07o", 0);
memcpy(header->magic, "ustar", 6);
memcpy(header->version, "00", 2);
- sprintf(header->chksum, "%07o", ustar_header_chksum(header));
+ snprintf(header->chksum, sizeof(header->chksum), "%07o",
ustar_header_chksum(header));
}
static int write_extended_header(struct archiver_args *args,
@@ -193,7 +193,7 @@ static int write_extended_header(struct archiver_args *args,
memset(&header, 0, sizeof(header));
*header.typeflag = TYPEFLAG_EXT_HEADER;
mode = 0100666;
- sprintf(header.name, "%s.paxheader", sha1_to_hex(sha1));
+ xsnprintf(header.name, sizeof(header.name), "%s.paxheader",
sha1_to_hex(sha1));
prepare_header(args, &header, mode, size);
write_blocked(&header, sizeof(header));
write_blocked(buffer, size);
@@ -235,8 +235,8 @@ static int write_tar_entry(struct archiver_args *args,
memcpy(header.prefix, path, plen);
memcpy(header.name, path + plen + 1, rest);
} else {
- sprintf(header.name, "%s.data",
- sha1_to_hex(sha1));
+ xsnprintf(header.name, sizeof(header.name), "%s.data",
+ sha1_to_hex(sha1));
strbuf_append_ext_header(&ext_header, "path",
path, pathlen);
}
@@ -259,8 +259,8 @@ static int write_tar_entry(struct archiver_args *args,
if (S_ISLNK(mode)) {
if (size > sizeof(header.linkname)) {
- sprintf(header.linkname, "see %s.paxheader",
- sha1_to_hex(sha1));
+ xsnprintf(header.linkname, sizeof(header.linkname),
+ "see %s.paxheader", sha1_to_hex(sha1));
strbuf_append_ext_header(&ext_header, "linkpath",
buffer, size);
} else
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 06/68] add git_path_buf helper function
If you have a function that uses git_path a lot, but would
prefer to avoid the static buffers, it's useful to keep a
single scratch buffer locally and reuse it for each call.
You used to be able to do this with git_snpath:
char buf[PATH_MAX];
foo(git_snpath(buf, sizeof(buf), "foo"));
bar(git_snpath(buf, sizeof(buf), "bar"));
but since 1a83c24, git_snpath has been replaced with
strbuf_git_path. This is good, because it removes the
arbitrary PATH_MAX limit. But using strbuf_git_path is more
awkward for two reasons:
1. It adds to the buffer, rather than replacing it. This
is consistent with other strbuf functions, but makes
reuse of a single buffer more tedious.
2. It doesn't return the buffer, so you can't format
as part of a function's arguments.
The new git_path_buf solves both of these, so you can use it
like:
struct strbuf buf = STRBUF_INIT;
foo(git_path_buf(&buf, "foo"));
bar(git_path_buf(&buf, "bar"));
strbuf_release(&buf);
Signed-off-by: Jeff King
---
cache.h | 2 ++
path.c | 10 ++
2 files changed, 12 insertions(+)
diff --git a/cache.h b/cache.h
index 79066e5..e231e47 100644
--- a/cache.h
+++ b/cache.h
@@ -723,6 +723,8 @@ extern char *mksnpath(char *buf, size_t n, const char *fmt,
...)
__attribute__((format (printf, 3, 4)));
extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
__attribute__((format (printf, 2, 3)));
+extern char *git_path_buf(struct strbuf *buf, const char *fmt, ...)
+ __attribute__((format (printf, 2, 3)));
extern void strbuf_git_path_submodule(struct strbuf *sb, const char *path,
const char *fmt, ...)
__attribute__((format (printf, 3, 4)));
diff --git a/path.c b/path.c
index 95acbaf..46a4d27 100644
--- a/path.c
+++ b/path.c
@@ -175,6 +175,16 @@ static void do_git_path(struct strbuf *buf, const char
*fmt, va_list args)
strbuf_cleanup_path(buf);
}
+char *git_path_buf(struct strbuf *buf, const char *fmt, ...)
+{
+ va_list args;
+ strbuf_reset(buf);
+ va_start(args, fmt);
+ do_git_path(buf, fmt, args);
+ va_end(args);
+ return buf->buf;
+}
+
void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
{
va_list args;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 07/68] strbuf: make strbuf_complete_line more generic
The strbuf_complete_line function makes sure that a buffer
ends in a newline. But we may want to do this for any
character (e.g., "/" on the end of a path). Let's factor out
a generic version, and keep strbuf_complete_line as a thin
wrapper.
Signed-off-by: Jeff King
---
strbuf.h | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/strbuf.h b/strbuf.h
index aef2794..43f27c3 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -491,10 +491,21 @@ extern void strbuf_add_lines(struct strbuf *sb, const
char *prefix, const char *
*/
extern void strbuf_addstr_xml_quoted(struct strbuf *sb, const char *s);
+/**
+ * "Complete" the contents of `sb` by ensuring that either it ends with the
+ * character `term`, or it is empty. This can be used, for example,
+ * to ensure that text ends with a newline, but without creating an empty
+ * blank line if there is no content in the first place.
+ */
+static inline void strbuf_complete(struct strbuf *sb, char term)
+{
+ if (sb->len && sb->buf[sb->len - 1] != term)
+ strbuf_addch(sb, term);
+}
+
static inline void strbuf_complete_line(struct strbuf *sb)
{
- if (sb->len && sb->buf[sb->len - 1] != '\n')
- strbuf_addch(sb, '\n');
+ strbuf_complete(sb, '\n');
}
extern int strbuf_branchname(struct strbuf *sb, const char *name);
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 26/68] replace trivial malloc + sprintf / strcpy calls with xstrfmt
It's a common pattern to do:
foo = xmalloc(strlen(one) + strlen(two) + 1 + 1);
sprintf(foo, "%s %s", one, two);
(or possibly some variant with strcpy()s or a more
complicated length computation). We can switch these to use
xstrfmt, which is shorter, involves less error-prone manual
computation, and removes many sprintf and strcpy calls which
make it harder to audit the code for real buffer overflows.
Signed-off-by: Jeff King
---
builtin/apply.c | 5 +
builtin/ls-remote.c | 8 ++--
builtin/name-rev.c | 13 +
environment.c | 7 ++-
imap-send.c | 5 ++---
reflog-walk.c | 7 +++
remote.c| 7 +--
setup.c | 12 +++-
unpack-trees.c | 4 +---
9 files changed, 20 insertions(+), 48 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 4aa53f7..094a20f 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -698,10 +698,7 @@ static char *find_name_common(const char *line, const char
*def,
}
if (root) {
- char *ret = xmalloc(root_len + len + 1);
- strcpy(ret, root);
- memcpy(ret + root_len, start, len);
- ret[root_len + len] = '\0';
+ char *ret = xstrfmt("%s%.*s", root, len, start);
return squash_slash(ret);
}
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 4554dbc..5b6d679 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -93,12 +93,8 @@ int cmd_ls_remote(int argc, const char **argv, const char
*prefix)
if (argv[i]) {
int j;
pattern = xcalloc(argc - i + 1, sizeof(const char *));
- for (j = i; j < argc; j++) {
- int len = strlen(argv[j]);
- char *p = xmalloc(len + 3);
- sprintf(p, "*/%s", argv[j]);
- pattern[j - i] = p;
- }
+ for (j = i; j < argc; j++)
+ pattern[j - i] = xstrfmt("*/%s", argv[j]);
}
remote = remote_get(dest);
if (!remote) {
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 248a3eb..8a3a0cd 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -56,19 +56,16 @@ copy_data:
parents = parents->next, parent_number++) {
if (parent_number > 1) {
int len = strlen(tip_name);
- char *new_name = xmalloc(len +
- 1 + decimal_length(generation) + /* ~ */
- 1 + 2 + /* ^NN */
- 1);
+ char *new_name;
if (len > 2 && !strcmp(tip_name + len - 2, "^0"))
len -= 2;
if (generation > 0)
- sprintf(new_name, "%.*s~%d^%d", len, tip_name,
- generation, parent_number);
+ new_name = xstrfmt("%.*s~%d^%d", len, tip_name,
+ generation, parent_number);
else
- sprintf(new_name, "%.*s^%d", len, tip_name,
- parent_number);
+ new_name = xstrfmt("%.*s^%d", len, tip_name,
+ parent_number);
name_rev(parents->item, new_name, 0,
distance + MERGE_TRAVERSAL_WEIGHT, 0);
diff --git a/environment.c b/environment.c
index a533aed..c5b65f5 100644
--- a/environment.c
+++ b/environment.c
@@ -143,11 +143,8 @@ static char *git_path_from_env(const char *envvar, const
char *git_dir,
const char *path, int *fromenv)
{
const char *value = getenv(envvar);
- if (!value) {
- char *buf = xmalloc(strlen(git_dir) + strlen(path) + 2);
- sprintf(buf, "%s/%s", git_dir, path);
- return buf;
- }
+ if (!value)
+ return xstrfmt("%s/%s", git_dir, path);
if (fromenv)
*fromenv = 1;
return xstrdup(value);
diff --git a/imap-send.c b/imap-send.c
index 37ac4aa..e9faaea 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -889,9 +889,8 @@ static char *cram(const char *challenge_64, const char
*user, const char *pass)
}
/* response: " " */
- resp_len = strlen(user) + 1 + strlen(hex) + 1;
- response = xmalloc(resp_len);
- sprintf(response, "%s %s", user, hex);
+ response = xstrfmt("%s %s", user, hex);
+ resp_len = strlen(response) + 1;
response_64 = xmalloc(ENCODED_SIZE(resp_len) + 1);
encoded_len = EVP_EncodeBlock((unsigned char *)response_64,
diff --git a/reflog-walk.c b/reflog-walk.c
index f8e743a..85b8a54 100
[PATCH 18/68] find_short_object_filename: convert sprintf to xsnprintf
We use sprintf() to format some hex data into a buffer. The
buffer is clearly long enough, and using snprintf here is
not necessary. And in fact, it does not really make anything
easier to audit, as the size we feed to snprintf accounts
for the magic extra 42 bytes found in each alt->name field
of struct alternate_object_database (which is there exactly
to do this formatting).
Still, it is nice to remove an sprintf call and replace it
with an xsnprintf and explanatory comment, which makes it
easier to audit the code base for overflows.
Signed-off-by: Jeff King
---
sha1_name.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index c58b477..80753b6 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -96,11 +96,15 @@ static void find_short_object_filename(int len, const char
*hex_pfx, struct disa
}
fakeent->next = alt_odb_list;
- sprintf(hex, "%.2s", hex_pfx);
+ xsnprintf(hex, sizeof(hex), "%.2s", hex_pfx);
for (alt = fakeent; alt && !ds->ambiguous; alt = alt->next) {
struct dirent *de;
DIR *dir;
- sprintf(alt->name, "%.2s/", hex_pfx);
+ /*
+* every alt_odb struct has 42 extra bytes after the base
+* for exactly this purpose
+*/
+ xsnprintf(alt->name, 42, "%.2s/", hex_pfx);
dir = opendir(alt->base);
if (!dir)
continue;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 23/68] add_packed_git: convert strcpy into xsnprintf
We have the path "foo.idx", and we create a buffer big
enough to hold "foo.pack" and "foo.keep", and then strcpy
straight into it. This isn't a bug (we have enough space),
but it's very hard to tell from the strcpy that this is so.
Let's instead use strip_suffix to take off the ".idx",
record the size of our allocation, and use xsnprintf to make
sure we don't violate our assumptions.
Signed-off-by: Jeff King
---
cache.h | 2 +-
sha1_file.c | 21 +
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index 030b880..d206d64 100644
--- a/cache.h
+++ b/cache.h
@@ -1309,7 +1309,7 @@ extern void close_pack_windows(struct packed_git *);
extern void unuse_pack(struct pack_window **);
extern void free_pack_by_name(const char *);
extern void clear_delta_base_cache(void);
-extern struct packed_git *add_packed_git(const char *, int, int);
+extern struct packed_git *add_packed_git(const char *path, size_t path_len,
int local);
/*
* Return the SHA-1 of the nth object within the specified packfile.
diff --git a/sha1_file.c b/sha1_file.c
index f106091..592226e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1146,11 +1146,12 @@ static void try_to_free_pack_memory(size_t size)
release_pack_memory(size);
}
-struct packed_git *add_packed_git(const char *path, int path_len, int local)
+struct packed_git *add_packed_git(const char *path, size_t path_len, int local)
{
static int have_set_try_to_free_routine;
struct stat st;
- struct packed_git *p = alloc_packed_git(path_len + 2);
+ size_t alloc;
+ struct packed_git *p;
if (!have_set_try_to_free_routine) {
have_set_try_to_free_routine = 1;
@@ -1161,18 +1162,22 @@ struct packed_git *add_packed_git(const char *path, int
path_len, int local)
* Make sure a corresponding .pack file exists and that
* the index looks sane.
*/
- path_len -= strlen(".idx");
- if (path_len < 1) {
- free(p);
+ if (!strip_suffix_mem(path, &path_len, ".idx"))
return NULL;
- }
+
+ /*
+* ".pack" is long enough to hold any suffix we're adding (and
+* the use xsnprintf double-checks that)
+*/
+ alloc = path_len + strlen(".pack") + 1;
+ p = alloc_packed_git(alloc);
memcpy(p->pack_name, path, path_len);
- strcpy(p->pack_name + path_len, ".keep");
+ xsnprintf(p->pack_name + path_len, alloc - path_len, ".keep");
if (!access(p->pack_name, F_OK))
p->pack_keep = 1;
- strcpy(p->pack_name + path_len, ".pack");
+ xsnprintf(p->pack_name + path_len, alloc - path_len, ".pack");
if (stat(p->pack_name, &st) || !S_ISREG(st.st_mode)) {
free(p);
return NULL;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 10/68] mailsplit: make PATH_MAX buffers dynamic
There are several PATH_MAX-sized buffers in mailsplit, along
with some questionable uses of sprintf. These are not
really of security interest, as local mailsplit pathnames
are not typically under control of an attacker, and you
could generally only overflow a few numbers at the end of a
path that approaches PATH_MAX (a longer path would choke
mailsplit long before). But it does not hurt to be careful,
and as a bonus we lift some limits for systems with
too-small PATH_MAX varibles.
Signed-off-by: Jeff King
---
builtin/mailsplit.c | 34 +++---
1 file changed, 23 insertions(+), 11 deletions(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 9de06e3..104277a 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -98,30 +98,37 @@ static int populate_maildir_list(struct string_list *list,
const char *path)
{
DIR *dir;
struct dirent *dent;
- char name[PATH_MAX];
+ char *name = NULL;
char *subs[] = { "cur", "new", NULL };
char **sub;
+ int ret = -1;
for (sub = subs; *sub; ++sub) {
- snprintf(name, sizeof(name), "%s/%s", path, *sub);
+ free(name);
+ name = xstrfmt("%s/%s", path, *sub);
if ((dir = opendir(name)) == NULL) {
if (errno == ENOENT)
continue;
error("cannot opendir %s (%s)", name, strerror(errno));
- return -1;
+ goto out;
}
while ((dent = readdir(dir)) != NULL) {
if (dent->d_name[0] == '.')
continue;
- snprintf(name, sizeof(name), "%s/%s", *sub,
dent->d_name);
+ free(name);
+ name = xstrfmt("%s/%s", *sub, dent->d_name);
string_list_insert(list, name);
}
closedir(dir);
}
- return 0;
+ ret = 0;
+
+out:
+ free(name);
+ return ret;
}
static int maildir_filename_cmp(const char *a, const char *b)
@@ -148,8 +155,7 @@ static int maildir_filename_cmp(const char *a, const char
*b)
static int split_maildir(const char *maildir, const char *dir,
int nr_prec, int skip)
{
- char file[PATH_MAX];
- char name[PATH_MAX];
+ char *file = NULL;
FILE *f = NULL;
int ret = -1;
int i;
@@ -161,7 +167,11 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
for (i = 0; i < list.nr; i++) {
- snprintf(file, sizeof(file), "%s/%s", maildir,
list.items[i].string);
+ char *name;
+
+ free(file);
+ file = xstrfmt("%s/%s", maildir, list.items[i].string);
+
f = fopen(file, "r");
if (!f) {
error("cannot open mail %s (%s)", file,
strerror(errno));
@@ -173,8 +183,9 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
}
- sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
+ name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
split_one(f, name, 1);
+ free(name);
fclose(f);
f = NULL;
@@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char
*dir,
out:
if (f)
fclose(f);
+ free(file);
string_list_clear(&list, 1);
return ret;
}
@@ -191,7 +203,6 @@ out:
static int split_mbox(const char *file, const char *dir, int allow_bare,
int nr_prec, int skip)
{
- char name[PATH_MAX];
int ret = -1;
int peek;
@@ -218,8 +229,9 @@ static int split_mbox(const char *file, const char *dir,
int allow_bare,
}
while (!file_done) {
- sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
+ char *name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
file_done = split_one(f, name, allow_bare);
+ free(name);
}
if (f != stdin)
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 20/68] compat/hstrerror: convert sprintf to snprintf
This is a trivially correct use of sprintf, as our error number should not be excessively long. But it's still nice to drop an sprintf call. Note that we cannot use xsnprintf here, because this is compat code which does not load git-compat-util.h. Signed-off-by: Jeff King --- compat/hstrerror.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/hstrerror.c b/compat/hstrerror.c index 069c555..b85a2fa 100644 --- a/compat/hstrerror.c +++ b/compat/hstrerror.c @@ -16,6 +16,6 @@ const char *githstrerror(int err) case TRY_AGAIN: return "Non-authoritative \"host not found\", or SERVERFAIL"; } - sprintf(buffer, "Name resolution error %d", err); + snprintf(buffer, sizeof(buffer), "Name resolution error %d", err); return buffer; } -- 2.6.0.rc3.454.g204ad51 -- 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
[PATCH 13/68] test-dump-cache-tree: avoid overflow of cache-tree name
When dumping a cache-tree, we sprintf sub-tree names directly into a fixed-size buffer, which can overflow. We can trivially fix this by converting to xsnprintf to at least notice and die. This probably should handle arbitrary-sized names, but there's not much point. It's used only by the test scripts, so the trivial fix is enough. Signed-off-by: Jeff King --- test-dump-cache-tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c index 54c0872..bb53c0a 100644 --- a/test-dump-cache-tree.c +++ b/test-dump-cache-tree.c @@ -47,7 +47,7 @@ static int dump_cache_tree(struct cache_tree *it, struct cache_tree_sub *rdwn; rdwn = cache_tree_sub(ref, down->name); - sprintf(path, "%s%.*s/", pfx, down->namelen, down->name); + xsnprintf(path, sizeof(path), "%s%.*s/", pfx, down->namelen, down->name); if (dump_cache_tree(down->cache_tree, rdwn->cache_tree, path)) errs = 1; } -- 2.6.0.rc3.454.g204ad51 -- 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
[PATCH 14/68] compat/inet_ntop: fix off-by-one in inet_ntop4
Our compat inet_ntop4 function writes to a temporary buffer
with snprintf, and then uses strcpy to put the result into
the final "dst" buffer. We check the return value of
snprintf against the size of "dst", but fail to account for
the NUL terminator. As a result, we may overflow "dst" with
a single NUL. In practice, this doesn't happen because the
output of inet_ntop is limited, and we provide buffers that
are way oversized.
We can fix the off-by-one check easily, but while we are
here let's also use strlcpy for increased safety, just in
case there are other bugs lurking.
As a side note, this compat code seems to be BSD-derived.
Searching for "vixie inet_ntop" turns up NetBSD's latest
version of the same code, which has an identical fix (and
switches to strlcpy, too!).
Signed-off-by: Jeff King
---
compat/inet_ntop.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/compat/inet_ntop.c b/compat/inet_ntop.c
index 90b7cc4..6830726 100644
--- a/compat/inet_ntop.c
+++ b/compat/inet_ntop.c
@@ -53,11 +53,11 @@ inet_ntop4(const u_char *src, char *dst, size_t size)
nprinted = snprintf(tmp, sizeof(tmp), fmt, src[0], src[1], src[2],
src[3]);
if (nprinted < 0)
return (NULL); /* we assume "errno" was set by "snprintf()" */
- if ((size_t)nprinted > size) {
+ if ((size_t)nprinted >= size) {
errno = ENOSPC;
return (NULL);
}
- strcpy(dst, tmp);
+ strlcpy(dst, tmp, size);
return (dst);
}
@@ -154,7 +154,7 @@ inet_ntop6(const u_char *src, char *dst, size_t size)
errno = ENOSPC;
return (NULL);
}
- strcpy(dst, tmp);
+ strlcpy(dst, tmp, size);
return (dst);
}
#endif
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 05/68] add xsnprintf helper function
There are a number of places in the code where we call
sprintf(), with the assumption that the output will fit into
the buffer. In many cases this is true (e.g., formatting a
number into a large buffer), but it is hard to tell
immediately from looking at the code. It would be nice if we
had some run-time check to make sure that our assumption is
correct (and to communicate to readers of the code that we
are not blindly calling sprintf, but have actually thought
about this case).
This patch introduces xsnprintf, which behaves just like
snprintf, except that it dies whenever the output is
truncated. This acts as a sort of assert() for these cases,
which can help find places where the assumption is violated
(as opposed to truncating and proceeding, which may just
silently give a wrong answer).
Signed-off-by: Jeff King
---
git-compat-util.h | 3 +++
wrapper.c | 16
2 files changed, 19 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index f649e81..348b9dc 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -744,6 +744,9 @@ static inline size_t xsize_t(off_t len)
return (size_t)len;
}
+__attribute__((format (printf, 3, 4)))
+extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);
+
/* in ctype.c, for kwset users */
extern const unsigned char tolower_trans_tbl[256];
diff --git a/wrapper.c b/wrapper.c
index 0e22d43..6fcaa4d 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -621,6 +621,22 @@ char *xgetcwd(void)
return strbuf_detach(&sb, NULL);
}
+int xsnprintf(char *dst, size_t max, const char *fmt, ...)
+{
+ va_list ap;
+ int len;
+
+ va_start(ap, fmt);
+ len = vsnprintf(dst, max, fmt, ap);
+ va_end(ap);
+
+ if (len < 0)
+ die("BUG: your snprintf is broken");
+ if (len >= max)
+ die("BUG: attempt to snprintf into too-small buffer");
+ return len;
+}
+
static int write_file_v(const char *path, int fatal,
const char *fmt, va_list params)
{
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 21/68] grep: use xsnprintf to format failure message
This looks at first glance like the sprintf can overflow our buffer, but it's actually fine; the p->origin string is something constant and small, like "command line" or "-e option". Signed-off-by: Jeff King --- grep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/grep.c b/grep.c index b58c7c6..6c68d5b 100644 --- a/grep.c +++ b/grep.c @@ -306,9 +306,9 @@ static NORETURN void compile_regexp_failed(const struct grep_pat *p, char where[1024]; if (p->no) - sprintf(where, "In '%s' at %d, ", p->origin, p->no); + xsnprintf(where, sizeof(where), "In '%s' at %d, ", p->origin, p->no); else if (p->origin) - sprintf(where, "%s, ", p->origin); + xsnprintf(where, sizeof(where), "%s, ", p->origin); else where[0] = 0; -- 2.6.0.rc3.454.g204ad51 -- 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
[PATCH 19/68] stop_progress_msg: convert sprintf to xsnprintf
The usual arguments for using xsnprintf over sprintf apply, but this case is a little tricky. We print to a fixed-size buffer if we have room, and otherwise to an allocated buffer. So there should be no overflow here, but it is still good to communicate our intention, as well as to check our earlier math for how much space the string will need. Signed-off-by: Jeff King --- progress.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/progress.c b/progress.c index a3efcfd..353bd37 100644 --- a/progress.c +++ b/progress.c @@ -254,7 +254,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg) throughput_string(&tp->display, tp->curr_total, rate); } progress_update = 1; - sprintf(bufp, ", %s.\n", msg); + xsnprintf(bufp, len + 1, ", %s.\n", msg); display(progress, progress->last_value, bufp); if (buf != bufp) free(bufp); -- 2.6.0.rc3.454.g204ad51 -- 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
[PATCH 15/68] convert trivial sprintf / strcpy calls to xsnprintf
We sometimes sprintf into fixed-size buffers when we know
that the buffer is large enough to fit the input (either
because it's a constant, or because it's numeric input that
is bounded in size). Likewise with strcpy of constant
strings.
However, these sites make it hard to audit sprintf and
strcpy calls for buffer overflows, as a reader has to
cross-reference the size of the array with the input. Let's
use xsnprintf instead, which communicates to a reader that
we don't expect this to overflow (and catches the mistake in
case we do).
Signed-off-by: Jeff King
---
archive-tar.c | 2 +-
builtin/gc.c | 2 +-
builtin/init-db.c | 11 ++-
builtin/ls-tree.c | 9 +
builtin/merge-index.c | 2 +-
builtin/merge-recursive.c | 2 +-
builtin/read-tree.c | 2 +-
builtin/unpack-file.c | 2 +-
compat/mingw.c| 8 +---
compat/winansi.c | 2 +-
connect.c | 2 +-
convert.c | 3 ++-
daemon.c | 4 ++--
diff.c| 12 ++--
http-push.c | 2 +-
http.c| 6 +++---
ll-merge.c| 12 ++--
refs.c| 8
sideband.c| 4 ++--
strbuf.c | 4 ++--
20 files changed, 52 insertions(+), 47 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index b6b30bb..d543f93 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -301,7 +301,7 @@ static int write_global_extended_header(struct
archiver_args *args)
memset(&header, 0, sizeof(header));
*header.typeflag = TYPEFLAG_GLOBAL_HEADER;
mode = 0100666;
- strcpy(header.name, "pax_global_header");
+ xsnprintf(header.name, sizeof(header.name), "pax_global_header");
prepare_header(args, &header, mode, ext_header.len);
write_blocked(&header, sizeof(header));
write_blocked(ext_header.buf, ext_header.len);
diff --git a/builtin/gc.c b/builtin/gc.c
index 0ad8d30..57584bc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -194,7 +194,7 @@ static const char *lock_repo_for_gc(int force, pid_t*
ret_pid)
return NULL;
if (gethostname(my_host, sizeof(my_host)))
- strcpy(my_host, "unknown");
+ xsnprintf(my_host, sizeof(my_host), "unknown");
pidfile_path = git_pathdup("gc.pid");
fd = hold_lock_file_for_update(&lock, pidfile_path,
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 69323e1..e7d0e31 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -262,7 +262,8 @@ static int create_default_files(const char *template_path)
}
/* This forces creation of new config file */
- sprintf(repo_version_string, "%d", GIT_REPO_VERSION);
+ xsnprintf(repo_version_string, sizeof(repo_version_string),
+ "%d", GIT_REPO_VERSION);
git_config_set("core.repositoryformatversion", repo_version_string);
path[len] = 0;
@@ -414,13 +415,13 @@ int init_db(const char *template_dir, unsigned int flags)
*/
if (shared_repository < 0)
/* force to the mode value */
- sprintf(buf, "0%o", -shared_repository);
+ xsnprintf(buf, sizeof(buf), "0%o", -shared_repository);
else if (shared_repository == PERM_GROUP)
- sprintf(buf, "%d", OLD_PERM_GROUP);
+ xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_GROUP);
else if (shared_repository == PERM_EVERYBODY)
- sprintf(buf, "%d", OLD_PERM_EVERYBODY);
+ xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_EVERYBODY);
else
- die("oops");
+ die("BUG: invalid value for shared_repository");
git_config_set("core.sharedrepository", buf);
git_config_set("receive.denyNonFastforwards", "true");
}
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 3b04a0f..0e30d86 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -96,12 +96,13 @@ static int show_tree(const unsigned char *sha1, struct
strbuf *base,
if (!strcmp(type, blob_type)) {
unsigned long size;
if (sha1_object_info(sha1, &size) == OBJ_BAD)
- strcpy(size_text, "BAD");
+ xsnprintf(size_text, sizeof(size_text),
+ "BAD");
else
- snprintf(size_text, sizeof(size_text),
-"%lu", size);
+ xsnprintf(size_text, sizeof(size_text),
+ "%lu", size);
[PATCH 12/68] progress: store throughput display in a strbuf
Coverity noticed that we strncpy() into a fixed-size buffer
without making sure that it actually ended up
NUL-terminated. This is unlikely to be a bug in practice,
since throughput strings rarely hit 32 characters, but it
would be nice to clean it up.
The most obvious way to do so is to add a NUL-terminator.
But instead, this patch switches the fixed-size buffer out
for a strbuf. At first glance this seems much less
efficient, until we realize that filling in the fixed-size
buffer is done by writing into a strbuf and copying the
result!
By writing straight to the buffer, we actually end up more
efficient:
1. We avoid an extra copy of the bytes.
2. Rather than malloc/free each time progress is shown, we
can strbuf_reset and use the same buffer each time.
Signed-off-by: Jeff King
---
progress.c | 18 --
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/progress.c b/progress.c
index 2e31bec..a3efcfd 100644
--- a/progress.c
+++ b/progress.c
@@ -25,7 +25,7 @@ struct throughput {
unsigned int last_bytes[TP_IDX_MAX];
unsigned int last_misecs[TP_IDX_MAX];
unsigned int idx;
- char display[32];
+ struct strbuf display;
};
struct progress {
@@ -98,7 +98,7 @@ static int display(struct progress *progress, unsigned n,
const char *done)
}
progress->last_value = n;
- tp = (progress->throughput) ? progress->throughput->display : "";
+ tp = (progress->throughput) ? progress->throughput->display.buf : "";
eol = done ? done : " \r";
if (progress->total) {
unsigned percent = n * 100 / progress->total;
@@ -129,6 +129,7 @@ static int display(struct progress *progress, unsigned n,
const char *done)
static void throughput_string(struct strbuf *buf, off_t total,
unsigned int rate)
{
+ strbuf_reset(buf);
strbuf_addstr(buf, ", ");
strbuf_humanise_bytes(buf, total);
strbuf_addstr(buf, " | ");
@@ -141,7 +142,6 @@ void display_throughput(struct progress *progress, off_t
total)
struct throughput *tp;
uint64_t now_ns;
unsigned int misecs, count, rate;
- struct strbuf buf = STRBUF_INIT;
if (!progress)
return;
@@ -154,6 +154,7 @@ void display_throughput(struct progress *progress, off_t
total)
if (tp) {
tp->prev_total = tp->curr_total = total;
tp->prev_ns = now_ns;
+ strbuf_init(&tp->display, 0);
}
return;
}
@@ -193,9 +194,7 @@ void display_throughput(struct progress *progress, off_t
total)
tp->last_misecs[tp->idx] = misecs;
tp->idx = (tp->idx + 1) % TP_IDX_MAX;
- throughput_string(&buf, total, rate);
- strncpy(tp->display, buf.buf, sizeof(tp->display));
- strbuf_release(&buf);
+ throughput_string(&tp->display, total, rate);
if (progress->last_value != -1 && progress_update)
display(progress, progress->last_value, NULL);
}
@@ -250,12 +249,9 @@ void stop_progress_msg(struct progress **p_progress, const
char *msg)
bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);
if (tp) {
- struct strbuf strbuf = STRBUF_INIT;
unsigned int rate = !tp->avg_misecs ? 0 :
tp->avg_bytes / tp->avg_misecs;
- throughput_string(&strbuf, tp->curr_total, rate);
- strncpy(tp->display, strbuf.buf, sizeof(tp->display));
- strbuf_release(&strbuf);
+ throughput_string(&tp->display, tp->curr_total, rate);
}
progress_update = 1;
sprintf(bufp, ", %s.\n", msg);
@@ -264,6 +260,8 @@ void stop_progress_msg(struct progress **p_progress, const
char *msg)
free(bufp);
}
clear_progress_signal();
+ if (progress->throughput)
+ strbuf_release(&progress->throughput->display);
free(progress->throughput);
free(progress);
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 08/68] add reentrant variants of sha1_to_hex and find_unique_abbrev
The sha1_to_hex and find_unique_abbrev functions always
write into reusable static buffers. There are a few problems
with this:
- future calls overwrite our result. This is especially
annoying with find_unique_abbrev, which does not have a
ring of buffers, so you cannot even printf() a result
that has two abbreviated sha1s.
- if you want to put the result into another buffer, we
often strcpy, which looks suspicious when auditing for
overflows.
This patch introduces sha1_to_hex_r and find_unique_abbrev_r,
which write into a user-provided buffer. Of course this is
just punting on the overflow-auditing, as the buffer
obviously needs to be GIT_SHA1_HEXSZ + 1 bytes. But it is
much easier to audit, since that is a well-known size.
We retain the non-reentrant forms, which just become thin
wrappers around the reentrant ones. This patch also adds a
strbuf variant of find_unique_abbrev, which will be handy in
later patches.
Signed-off-by: Jeff King
---
cache.h | 31 ++-
hex.c | 13 +
sha1_name.c | 16 +++-
strbuf.c| 9 +
strbuf.h| 8
5 files changed, 67 insertions(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index e231e47..030b880 100644
--- a/cache.h
+++ b/cache.h
@@ -785,7 +785,24 @@ extern char *sha1_pack_name(const unsigned char *sha1);
*/
extern char *sha1_pack_index_name(const unsigned char *sha1);
-extern const char *find_unique_abbrev(const unsigned char *sha1, int);
+/*
+ * Return an abbreviated sha1 unique within this repository's object database.
+ * The result will be at least `len` characters long, and will be NUL
+ * terminated.
+ *
+ * The non-`_r` version returns a static buffer which will be overwritten by
+ * subsequent calls.
+ *
+ * The `_r` variant writes to a buffer supplied by the caller, which must be at
+ * least `GIT_SHA1_HEXSZ + 1` bytes. The return value is the number of bytes
+ * written (excluding the NUL terminator).
+ *
+ * Note that while this version avoids the static buffer, it is not fully
+ * reentrant, as it calls into other non-reentrant git code.
+ */
+extern const char *find_unique_abbrev(const unsigned char *sha1, int len);
+extern int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len);
+
extern const unsigned char null_sha1[GIT_SHA1_RAWSZ];
static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
@@ -1067,6 +1084,18 @@ extern int for_each_abbrev(const char *prefix,
each_abbrev_fn, void *);
extern int get_sha1_hex(const char *hex, unsigned char *sha1);
extern int get_oid_hex(const char *hex, struct object_id *sha1);
+/*
+ * Convert a binary sha1 to its hex equivalent. The `_r` variant is reentrant,
+ * and writes the NUL-terminated output to the buffer `out`, which must be at
+ * least `GIT_SHA1_HEXSZ + 1` bytes, and returns a pointer to out for
+ * convenience.
+ *
+ * The non-`_r` variant returns a static buffer, but uses a ring of 4
+ * buffers, making it safe to make multiple calls for a single statement, like:
+ *
+ * printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
+ */
+extern char *sha1_to_hex_r(char *out, const unsigned char *sha1);
extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer
result! */
extern char *oid_to_hex(const struct object_id *oid); /* same static buffer
as sha1_to_hex */
diff --git a/hex.c b/hex.c
index 899b74a..0519f85 100644
--- a/hex.c
+++ b/hex.c
@@ -61,12 +61,10 @@ int get_oid_hex(const char *hex, struct object_id *oid)
return get_sha1_hex(hex, oid->hash);
}
-char *sha1_to_hex(const unsigned char *sha1)
+char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
{
- static int bufno;
- static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
static const char hex[] = "0123456789abcdef";
- char *buffer = hexbuffer[3 & ++bufno], *buf = buffer;
+ char *buf = buffer;
int i;
for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
@@ -79,6 +77,13 @@ char *sha1_to_hex(const unsigned char *sha1)
return buffer;
}
+char *sha1_to_hex(const unsigned char *sha1)
+{
+ static int bufno;
+ static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
+ return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
+}
+
char *oid_to_hex(const struct object_id *oid)
{
return sha1_to_hex(oid->hash);
diff --git a/sha1_name.c b/sha1_name.c
index da6874c..c58b477 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -368,14 +368,13 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn
fn, void *cb_data)
return ds.ambiguous;
}
-const char *find_unique_abbrev(const unsigned char *sha1, int len)
+int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
{
int status, exists;
- static char hex[41];
- memcpy(hex, sha1_to_hex(sha1), 40);
+ sha1_to_hex_r(hex, sha1);
if (len == 40 || !len)
- return hex;
+ return 40;
exist
[PATCH 11/68] trace: use strbuf for quote_crnl output
When we output GIT_TRACE_SETUP paths, we quote any
meta-characters. But our buffer to hold the result is only
PATH_MAX bytes, and we could double the size of the input
path (if every character needs quoting). We could use a
2*PATH_MAX buffer, if we assume the input will never be more
than PATH_MAX. But it's easier still to just switch to a
strbuf and not worry about whether the input can exceed
PATH_MAX or not.
The original copied the "p2" pointer to "p1", advancing
both. Since this gets rid of "p1", let's also drop "p2",
whose name is now confusing. We can just advance the
original "path" pointer.
Signed-off-by: Jeff King
---
trace.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/trace.c b/trace.c
index 7393926..4aeea60 100644
--- a/trace.c
+++ b/trace.c
@@ -277,25 +277,24 @@ void trace_performance_fl(const char *file, int line,
uint64_t nanos,
static const char *quote_crnl(const char *path)
{
- static char new_path[PATH_MAX];
- const char *p2 = path;
- char *p1 = new_path;
+ static struct strbuf new_path = STRBUF_INIT;
if (!path)
return NULL;
- while (*p2) {
- switch (*p2) {
- case '\\': *p1++ = '\\'; *p1++ = '\\'; break;
- case '\n': *p1++ = '\\'; *p1++ = 'n'; break;
- case '\r': *p1++ = '\\'; *p1++ = 'r'; break;
+ strbuf_reset(&new_path);
+
+ while (*path) {
+ switch (*path) {
+ case '\\': strbuf_addstr(&new_path, ""); break;
+ case '\n': strbuf_addstr(&new_path, "\\n"); break;
+ case '\r': strbuf_addstr(&new_path, "\\r"); break;
default:
- *p1++ = *p2;
+ strbuf_addch(&new_path, *path);
}
- p2++;
+ path++;
}
- *p1 = '\0';
- return new_path;
+ return new_path.buf;
}
/* FIXME: move prefix to startup_info struct and get rid of this arg */
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 25/68] receive-pack: convert strncpy to xsnprintf
This strncpy is pointless; we pass the strlen() of the src
string, meaning that it works just like a memcpy. Worse,
though, is that the size has no relation to the destination
buffer, meaning it is a potential overflow. In practice,
it's not. We pass only short constant strings like
"warning: " and "error: ", which are much smaller than the
destination buffer.
We can make this much simpler by just using xsnprintf, which
will check for overflow and return the size for our next
vsnprintf, without us having to run a separate strlen().
Signed-off-by: Jeff King
---
builtin/receive-pack.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e6b93d0..04d2bdf 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -280,10 +280,10 @@ static void rp_warning(const char *err, ...)
__attribute__((format (printf, 1, 2
static void report_message(const char *prefix, const char *err, va_list params)
{
- int sz = strlen(prefix);
+ int sz;
char msg[4096];
- strncpy(msg, prefix, sz);
+ sz = xsnprintf(msg, sizeof(msg), "%s", prefix);
sz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params);
if (sz > (sizeof(msg) - 1))
sz = sizeof(msg) - 1;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 22/68] entry.c: convert strcpy to xsnprintf
This particular conversion is non-obvious, because nobody
has passed our function the length of the destination
buffer. However, the interface to checkout_entry specifies
that the buffer must be at least TEMPORARY_FILENAME_LENGTH
bytes long, so we can check that (meaning the existing code
was not buggy, but merely worrisome to somebody reading it).
Signed-off-by: Jeff King
---
entry.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/entry.c b/entry.c
index 1eda8e9..582c400 100644
--- a/entry.c
+++ b/entry.c
@@ -96,8 +96,8 @@ static int open_output_fd(char *path, const struct
cache_entry *ce, int to_tempf
{
int symlink = (ce->ce_mode & S_IFMT) != S_IFREG;
if (to_tempfile) {
- strcpy(path, symlink
- ? ".merge_link_XX" : ".merge_file_XX");
+ xsnprintf(path, TEMPORARY_FILENAME_LENGTH, "%s",
+ symlink ? ".merge_link_XX" :
".merge_file_XX");
return mkstemp(path);
} else {
return create_file(path, !symlink ? ce->ce_mode : 0666);
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 09/68] fsck: use strbuf to generate alternate directories
When fsck-ing alternates, we make a copy of the alternate
directory in a fixed PATH_MAX buffer. We memcpy directly,
without any check whether we are overflowing the buffer.
This is OK if PATH_MAX is a true representation of the
maximum path on the system, because any path here will have
already been vetted by the alternates subsystem. But that is
not true on every system, so we should be more careful.
Signed-off-by: Jeff King
---
builtin/fsck.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 46c7235..a019f4a 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -683,11 +683,12 @@ int cmd_fsck(int argc, const char **argv, const char
*prefix)
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
- char namebuf[PATH_MAX];
- int namelen = alt->name - alt->base;
- memcpy(namebuf, alt->base, namelen);
- namebuf[namelen - 1] = 0;
- fsck_object_dir(namebuf);
+ /* directory name, minus trailing slash */
+ size_t namelen = alt->name - alt->base - 1;
+ struct strbuf name = STRBUF_INIT;
+ strbuf_add(&name, alt->base, namelen);
+ fsck_object_dir(name.buf);
+ strbuf_release(&name);
}
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 42/68] apply: convert root string to strbuf
We use manual computation and strcpy to allocate the "root"
variable. This would be much simpler using xstrfmt. But
since we store the length, too, we can just use a strbuf,
which handles that for us.
Note that we stop distinguishing between "no root" and
"empty root" in some cases, but that's OK; the results are
the same (e.g., inserting an empty string is a noop).
Signed-off-by: Jeff King
---
builtin/apply.c | 26 ++
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 094a20f..1d4d439 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -77,8 +77,7 @@ static enum ws_ignore {
static const char *patch_input_file;
-static const char *root;
-static int root_len;
+struct strbuf root = STRBUF_INIT;
static int read_stdin = 1;
static int options;
@@ -494,8 +493,8 @@ static char *find_name_gnu(const char *line, const char
*def, int p_value)
}
strbuf_remove(&name, 0, cp - name.buf);
- if (root)
- strbuf_insert(&name, 0, root, root_len);
+ if (root.len)
+ strbuf_insert(&name, 0, root.buf, root.len);
return squash_slash(strbuf_detach(&name, NULL));
}
@@ -697,8 +696,8 @@ static char *find_name_common(const char *line, const char
*def,
return squash_slash(xstrdup(def));
}
- if (root) {
- char *ret = xstrfmt("%s%.*s", root, len, start);
+ if (root.len) {
+ char *ret = xstrfmt("%s%.*s", root.buf, len, start);
return squash_slash(ret);
}
@@ -1274,8 +1273,8 @@ static int parse_git_header(const char *line, int len,
unsigned int size, struct
* the default name from the header.
*/
patch->def_name = git_header_name(line, len);
- if (patch->def_name && root) {
- char *s = xstrfmt("%s%s", root, patch->def_name);
+ if (patch->def_name && root.len) {
+ char *s = xstrfmt("%s%s", root.buf, patch->def_name);
free(patch->def_name);
patch->def_name = s;
}
@@ -4498,14 +4497,9 @@ static int option_parse_whitespace(const struct option
*opt,
static int option_parse_directory(const struct option *opt,
const char *arg, int unset)
{
- root_len = strlen(arg);
- if (root_len && arg[root_len - 1] != '/') {
- char *new_root;
- root = new_root = xmalloc(root_len + 2);
- strcpy(new_root, arg);
- strcpy(new_root + root_len++, "/");
- } else
- root = arg;
+ strbuf_reset(&root);
+ strbuf_addstr(&root, arg);
+ strbuf_complete(&root, '/');
return 0;
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 63/68] convert strncpy to memcpy
strncpy is known to be a confusing function because of its
termination semantics. These calls are all correct, but it
takes some examination to see why. In particular, every one
of them expects to copy up to the length limit, and then
makes some arrangement for terminating the result.
We can just use memcpy, along with noting explicitly how the
result is terminated (if it is not already obvious). That
should make it more clear to a reader that we are doing the
right thing.
Signed-off-by: Jeff King
---
builtin/help.c | 4 ++--
fast-import.c | 2 +-
tag.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/help.c b/builtin/help.c
index e1650ab..1cd0c1e 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -176,7 +176,7 @@ static void add_man_viewer(const char *name)
while (*p)
p = &((*p)->next);
*p = xcalloc(1, (sizeof(**p) + len + 1));
- strncpy((*p)->name, name, len);
+ memcpy((*p)->name, name, len); /* NUL-terminated by xcalloc */
}
static int supported_man_viewer(const char *name, size_t len)
@@ -192,7 +192,7 @@ static void do_add_man_viewer_info(const char *name,
{
struct man_viewer_info_list *new = xcalloc(1, sizeof(*new) + len + 1);
- strncpy(new->name, name, len);
+ memcpy(new->name, name, len); /* NUL-terminated by xcalloc */
new->info = xstrdup(value);
new->next = man_viewer_info_list;
man_viewer_info_list = new;
diff --git a/fast-import.c b/fast-import.c
index cf6d8bc..4d01efc 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -703,7 +703,7 @@ static struct atom_str *to_atom(const char *s, unsigned
short len)
c = pool_alloc(sizeof(struct atom_str) + len + 1);
c->str_len = len;
- strncpy(c->str_dat, s, len);
+ memcpy(c->str_dat, s, len);
c->str_dat[len] = 0;
c->next_atom = atom_table[hc];
atom_table[hc] = c;
diff --git a/tag.c b/tag.c
index 5b0ac62..5b2a06d 100644
--- a/tag.c
+++ b/tag.c
@@ -82,7 +82,7 @@ int parse_tag_buffer(struct tag *item, const void *data,
unsigned long size)
nl = memchr(bufptr, '\n', tail - bufptr);
if (!nl || sizeof(type) <= (nl - bufptr))
return -1;
- strncpy(type, bufptr, nl - bufptr);
+ memcpy(type, bufptr, nl - bufptr);
type[nl - bufptr] = '\0';
bufptr = nl + 1;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 56/68] use alloc_ref rather than hand-allocating "struct ref"
This saves us some manual computation, and eliminates a call
to strcpy.
Signed-off-by: Jeff King
---
builtin/fetch.c | 3 +--
remote-curl.c | 5 +
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 841880e..ed84963 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -639,8 +639,7 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
continue;
if (rm->peer_ref) {
- ref = xcalloc(1, sizeof(*ref) +
strlen(rm->peer_ref->name) + 1);
- strcpy(ref->name, rm->peer_ref->name);
+ ref = alloc_ref(rm->peer_ref->name);
hashcpy(ref->old_sha1, rm->peer_ref->old_sha1);
hashcpy(ref->new_sha1, rm->old_sha1);
ref->force = rm->peer_ref->force;
diff --git a/remote-curl.c b/remote-curl.c
index 71fbbb6..cc7a8a6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -168,10 +168,7 @@ static struct ref *parse_info_refs(struct discovery *heads)
url.buf);
data[i] = 0;
ref_name = mid + 1;
- ref = xmalloc(sizeof(struct ref) +
- strlen(ref_name) + 1);
- memset(ref, 0, sizeof(struct ref));
- strcpy(ref->name, ref_name);
+ ref = alloc_ref(ref_name);
get_sha1_hex(start, ref->old_sha1);
if (!refs)
refs = ref;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 33/68] read_branches_file: simplify string handling
This function does a lot of manual string handling, and has
some unnecessary limits. This patch cleans up a number of
things:
1. Drop the arbitrary 1000-byte limit on the size of the
remote name (we do not have such a limit in any of the
other remote-reading mechanisms).
2. Replace fgets into a fixed-size buffer with a strbuf,
eliminating any limits on the length of the URL.
3. Replace manual whitespace handling with strbuf_trim
(since we now have a strbuf). This also gets rid
of a call to strcpy, and the confusing reuse of the "p"
pointer for multiple purposes.
4. We currently build up the refspecs over multiple strbuf
calls. We do this to handle the fact that the URL "frag"
may not be present. But rather than have multiple
conditionals, let's just default "frag" to "master".
This lets us format the refspecs with a single xstrfmt.
It's shorter, and easier to see what the final string
looks like.
We also update the misleading comment in this area (the
local branch is named after the remote name, not after
the branch name on the remote side).
Signed-off-by: Jeff King
---
remote.c | 54 --
1 file changed, 20 insertions(+), 34 deletions(-)
diff --git a/remote.c b/remote.c
index 5ab0f7f..22a60fc 100644
--- a/remote.c
+++ b/remote.c
@@ -293,56 +293,42 @@ static void read_remotes_file(struct remote *remote)
static void read_branches_file(struct remote *remote)
{
char *frag;
- struct strbuf branch = STRBUF_INIT;
- int n = 1000;
- FILE *f = fopen(git_path("branches/%.*s", n, remote->name), "r");
- char *s, *p;
- int len;
+ struct strbuf buf = STRBUF_INIT;
+ FILE *f = fopen(git_path("branches/%s", remote->name), "r");
if (!f)
return;
- s = fgets(buffer, BUF_SIZE, f);
- fclose(f);
- if (!s)
- return;
- while (isspace(*s))
- s++;
- if (!*s)
+
+ strbuf_getline(&buf, f, '\n');
+ strbuf_trim(&buf);
+ if (!buf.len) {
+ strbuf_release(&buf);
return;
+ }
+
remote->origin = REMOTE_BRANCHES;
- p = s + strlen(s);
- while (isspace(p[-1]))
- *--p = 0;
- len = p - s;
- p = xmalloc(len + 1);
- strcpy(p, s);
/*
* The branches file would have URL and optionally
* #branch specified. The "master" (or specified) branch is
-* fetched and stored in the local branch of the same name.
+* fetched and stored in the local branch matching the
+* remote name.
*/
- frag = strchr(p, '#');
- if (frag) {
+ frag = strchr(buf.buf, '#');
+ if (frag)
*(frag++) = '\0';
- strbuf_addf(&branch, "refs/heads/%s", frag);
- } else
- strbuf_addstr(&branch, "refs/heads/master");
+ else
+ frag = "master";
+
+ add_url_alias(remote, strbuf_detach(&buf, NULL));
+ add_fetch_refspec(remote, xstrfmt("refs/heads/%s:refs/heads/%s",
+ frag, remote->name));
- strbuf_addf(&branch, ":refs/heads/%s", remote->name);
- add_url_alias(remote, p);
- add_fetch_refspec(remote, strbuf_detach(&branch, NULL));
/*
* Cogito compatible push: push current HEAD to remote #branch
* (master if missing)
*/
- strbuf_init(&branch, 0);
- strbuf_addstr(&branch, "HEAD");
- if (frag)
- strbuf_addf(&branch, ":refs/heads/%s", frag);
- else
- strbuf_addstr(&branch, ":refs/heads/master");
- add_push_refspec(remote, strbuf_detach(&branch, NULL));
+ add_push_refspec(remote, xstrfmt("HEAD:refs/heads/%s", frag));
remote->fetch_tags = 1; /* always auto-follow */
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 29/68] use strip_suffix and xstrfmt to replace suffix
When we want to convert "foo.pack" to "foo.idx", we do it by
duplicating the original string and then munging the bytes
in place. Let's use strip_suffix and xstrfmt instead, which
has several advantages:
1. It's more clear what the intent is.
2. It does not implicitly rely on the fact that
strlen(".idx") <= strlen(".pack") to avoid an overflow.
3. We communicate the assumption that the input file ends
with ".pack" (and get a run-time check that this is so).
4. We drop calls to strcpy, which makes auditing the code
base easier.
Likewise, we can do this to convert ".pack" to ".bitmap",
avoiding some manual memory computation.
Signed-off-by: Jeff King
---
http.c| 7 ---
pack-bitmap.c | 13 -
sha1_file.c | 6 --
3 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/http.c b/http.c
index 7b02259..e0ff876 100644
--- a/http.c
+++ b/http.c
@@ -1511,6 +1511,7 @@ int finish_http_pack_request(struct http_pack_request
*preq)
struct packed_git **lst;
struct packed_git *p = preq->target;
char *tmp_idx;
+ size_t len;
struct child_process ip = CHILD_PROCESS_INIT;
const char *ip_argv[8];
@@ -1524,9 +1525,9 @@ int finish_http_pack_request(struct http_pack_request
*preq)
lst = &((*lst)->next);
*lst = (*lst)->next;
- tmp_idx = xstrdup(preq->tmpfile);
- strcpy(tmp_idx + strlen(tmp_idx) - strlen(".pack.temp"),
- ".idx.temp");
+ if (!strip_suffix(preq->tmpfile, ".pack.temp", &len))
+ die("BUG: pack tmpfile does not end in .pack.temp?");
+ tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile);
ip_argv[0] = "index-pack";
ip_argv[1] = "-o";
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 637770a..7dfcb34 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -252,16 +252,11 @@ static int load_bitmap_entries_v1(struct bitmap_index
*index)
static char *pack_bitmap_filename(struct packed_git *p)
{
- char *idx_name;
- int len;
-
- len = strlen(p->pack_name) - strlen(".pack");
- idx_name = xmalloc(len + strlen(".bitmap") + 1);
-
- memcpy(idx_name, p->pack_name, len);
- memcpy(idx_name + len, ".bitmap", strlen(".bitmap") + 1);
+ size_t len;
- return idx_name;
+ if (!strip_suffix(p->pack_name, ".pack", &len))
+ die("BUG: pack_name does not end in .pack");
+ return xstrfmt("%.*s.bitmap", (int)len, p->pack_name);
}
static int open_pack_bitmap_1(struct packed_git *packfile)
diff --git a/sha1_file.c b/sha1_file.c
index 592226e..2be1afd 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -671,13 +671,15 @@ static int check_packed_git_idx(const char *path, struct
packed_git *p)
int open_pack_index(struct packed_git *p)
{
char *idx_name;
+ size_t len;
int ret;
if (p->index_data)
return 0;
- idx_name = xstrdup(p->pack_name);
- strcpy(idx_name + strlen(idx_name) - strlen(".pack"), ".idx");
+ if (!strip_suffix(p->pack_name, ".pack", &len))
+ die("BUG: pack_name does not end in .pack");
+ idx_name = xstrfmt("%.*s.idx", (int)len, p->pack_name);
ret = check_packed_git_idx(idx_name, p);
free(idx_name);
return ret;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 58/68] receive-pack: simplify keep_arg computation
To generate "--keep=receive-pack $pid on $host", we write
progressively into a single buffer, which requires keeping
track of how much we've written so far. But since the result
is destined to go into our argv array, we can simply use
argv_array_pushf.
Unfortunately we still have to have a fixed-size buffer for
the gethostname() call, but at least it now doesn't involve
any extra size computation. And as a bonus, we drop an
sprintf and a strcpy call.
Signed-off-by: Jeff King
---
builtin/receive-pack.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 39eb064..bcb624b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1524,15 +1524,18 @@ static const char *unpack(int err_fd, struct
shallow_info *si)
if (status)
return "unpack-objects abnormal exit";
} else {
- int s;
- char keep_arg[256];
-
- s = sprintf(keep_arg, "--keep=receive-pack %"PRIuMAX" on ",
(uintmax_t) getpid());
- if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
- strcpy(keep_arg + s, "localhost");
+ char hostname[256];
argv_array_pushl(&child.args, "index-pack",
-"--stdin", hdr_arg, keep_arg, NULL);
+"--stdin", hdr_arg, NULL);
+
+ if (gethostname(hostname, sizeof(hostname)))
+ xsnprintf(hostname, sizeof(hostname), "localhost");
+ argv_array_pushf(&child.args,
+"--keep=receive-pack %"PRIuMAX" on %s",
+(uintmax_t)getpid(),
+hostname);
+
if (fsck_objects)
argv_array_pushf(&child.args, "--strict%s",
fsck_msg_types.buf);
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 57/68] avoid sprintf and strcpy with flex arrays
When we are allocating a struct with a FLEX_ARRAY member, we
generally compute the size of the array and then sprintf or
strcpy into it. Normally we could improve a dynamic allocation
like this by using xstrfmt, but it doesn't work here; we
have to account for the size of the rest of the struct.
But we can improve things a bit by storing the length that
we use for the allocation, and then feeding it to xsnprintf
or memcpy, which makes it more obvious that we are not
writing more than the allocated number of bytes.
It would be nice if we had some kind of helper for
allocating generic flex arrays, but it doesn't work that
well:
- the call signature is a little bit unwieldy:
d = flex_struct(sizeof(*d), offsetof(d, path), fmt, ...);
You need offsetof here instead of just writing to the
end of the base size, because we don't know how the
struct is packed (partially this is because FLEX_ARRAY
might not be zero, though we can account for that; but
the size of the struct may actually be rounded up for
alignment, and we can't know that).
- some sites do clever things, like over-allocating because
they know they will write larger things into the buffer
later (e.g., struct packed_git here).
So we're better off to just write out each allocation (or
add type-specific helpers, though many of these are one-off
allocations anyway).
Signed-off-by: Jeff King
---
archive.c | 5 +++--
builtin/blame.c | 5 +++--
fast-import.c | 6 --
refs.c | 8
sha1_file.c | 5 +++--
submodule.c | 6 --
6 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/archive.c b/archive.c
index 01b0899..4ac86c8 100644
--- a/archive.c
+++ b/archive.c
@@ -171,13 +171,14 @@ static void queue_directory(const unsigned char *sha1,
unsigned mode, int stage, struct archiver_context *c)
{
struct directory *d;
- d = xmallocz(sizeof(*d) + base->len + 1 + strlen(filename));
+ size_t len = base->len + 1 + strlen(filename) + 1;
+ d = xmalloc(sizeof(*d) + len);
d->up = c->bottom;
d->baselen = base->len;
d->mode= mode;
d->stage = stage;
c->bottom = d;
- d->len = sprintf(d->path, "%.*s%s/", (int)base->len, base->buf,
filename);
+ d->len = xsnprintf(d->path, len, "%.*s%s/", (int)base->len, base->buf,
filename);
hashcpy(d->oid.hash, sha1);
}
diff --git a/builtin/blame.c b/builtin/blame.c
index e253ac0..e70fb6d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -459,12 +459,13 @@ static void queue_blames(struct scoreboard *sb, struct
origin *porigin,
static struct origin *make_origin(struct commit *commit, const char *path)
{
struct origin *o;
- o = xcalloc(1, sizeof(*o) + strlen(path) + 1);
+ size_t pathlen = strlen(path) + 1;
+ o = xcalloc(1, sizeof(*o) + pathlen);
o->commit = commit;
o->refcnt = 1;
o->next = commit->util;
commit->util = o;
- strcpy(o->path, path);
+ memcpy(o->path, path, pathlen); /* includes NUL */
return o;
}
diff --git a/fast-import.c b/fast-import.c
index d0c2502..895c6b4 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -863,13 +863,15 @@ static void start_packfile(void)
{
static char tmp_file[PATH_MAX];
struct packed_git *p;
+ int namelen;
struct pack_header hdr;
int pack_fd;
pack_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),
"pack/tmp_pack_XX");
- p = xcalloc(1, sizeof(*p) + strlen(tmp_file) + 2);
- strcpy(p->pack_name, tmp_file);
+ namelen = strlen(tmp_file) + 2;
+ p = xcalloc(1, sizeof(*p) + namelen);
+ xsnprintf(p->pack_name, namelen, "%s", tmp_file);
p->pack_fd = pack_fd;
p->do_not_close = 1;
pack_file = sha1fd(pack_fd, p->pack_name);
diff --git a/refs.c b/refs.c
index c2709de..9937a40 100644
--- a/refs.c
+++ b/refs.c
@@ -2695,7 +2695,7 @@ static int pack_if_possible_fn(struct ref_entry *entry,
void *cb_data)
int namelen = strlen(entry->name) + 1;
struct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);
hashcpy(n->sha1, entry->u.value.oid.hash);
- strcpy(n->name, entry->name);
+ memcpy(n->name, entry->name, namelen); /* includes NUL */
n->next = cb->ref_to_prune;
cb->ref_to_prune = n;
}
@@ -3984,10 +3984,10 @@ void ref_transaction_free(struct ref_transaction
*transaction)
static struct ref_update *add_update(struct ref_transaction *transaction,
const char *refname)
{
- size_t len = strlen(refname);
- struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
+ size_t len = strlen(refname) + 1;
+ struct ref_update *update = xcalloc(1, sizeof(*update) + len);
- strcpy((char *)update->refname, refname);
+ memcpy((c
[PATCH 59/68] help: clean up kfmclient munging
When we are going to launch "/path/to/konqueror", we instead
rewrite this into "/path/to/kfmclient" by duplicating the
original string and writing over the ending bits. This can
be done more obviously with strip_suffix and xstrfmt.
Note that we also fix a subtle bug with the "filename"
parameter, which is passed as argv[0] to the child. If the
user has configured a program name with no directory
component, we always pass the string "kfmclient", even if
your program is called something else. But if you give a
full path, we give the basename of that path. But more
bizarrely, if we rewrite "konqueror" to "kfmclient", we
still pass "konqueror".
The history of this function doesn't reveal anything
interesting, so it looks like just an oversight from
combining the suffix-munging with the basename-finding.
Let's just call basename on the munged path, which produces
consistent results (if you gave a program, whether a full
path or not, we pass its basename).
Probably this doesn't matter at all in practice, but it
makes the code slightly less confusing to read.
Signed-off-by: Jeff King
---
builtin/help.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/builtin/help.c b/builtin/help.c
index fba8c01..e1650ab 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -140,17 +140,10 @@ static void exec_man_konqueror(const char *path, const
char *page)
/* It's simpler to launch konqueror using kfmclient. */
if (path) {
- const char *file = strrchr(path, '/');
- if (file && !strcmp(file + 1, "konqueror")) {
- char *new = xstrdup(path);
- char *dest = strrchr(new, '/');
-
- /* strlen("konqueror") == strlen("kfmclient") */
- strcpy(dest + 1, "kfmclient");
- path = new;
- }
- if (file)
- filename = file;
+ size_t len;
+ if (strip_suffix(path, "/konqueror", &len))
+ path = xstrfmt("%.*s/kfmclient", (int)len,
path);
+ filename = basename((char *)path);
} else
path = "kfmclient";
strbuf_addf(&man_page, "man:%s(1)", page);
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 27/68] config: use xstrfmt in normalize_value
We xmalloc a fixed-size buffer and sprintf into it; this is
OK because the size of our formatting types is finite, but
that's not immediately clear to a reader auditing sprintf
calls. Let's switch to xstrfmt, which is shorter and
obviously correct.
Note that just dropping the common xmalloc here causes gcc
to complain with -Wmaybe-uninitialized. That's because if
"types" does not match any of our known types, we never
write anything into the "normalized" pointer. With the
current code, gcc doesn't notice because we always return a
valid pointer (just one which might point to uninitialized
data, but the compiler doesn't know that). In other words,
the current code is potentially buggy if new types are added
without updating this spot.
So let's take this opportunity to clean up the function a
bit more. We can drop the "normalized" pointer entirely, and
just return directly from each code path. And then add an
assertion at the end in case we haven't covered any cases.
Signed-off-by: Jeff King
---
builtin/config.c | 34 +-
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 71acc44..adc7727 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -246,8 +246,6 @@ free_strings:
static char *normalize_value(const char *key, const char *value)
{
- char *normalized;
-
if (!value)
return NULL;
@@ -258,27 +256,21 @@ static char *normalize_value(const char *key, const char
*value)
* "~/foobar/" in the config file, and to expand the ~
* when retrieving the value.
*/
- normalized = xstrdup(value);
- else {
- normalized = xmalloc(64);
- if (types == TYPE_INT) {
- int64_t v = git_config_int64(key, value);
- sprintf(normalized, "%"PRId64, v);
- }
- else if (types == TYPE_BOOL)
- sprintf(normalized, "%s",
- git_config_bool(key, value) ? "true" : "false");
- else if (types == TYPE_BOOL_OR_INT) {
- int is_bool, v;
- v = git_config_bool_or_int(key, value, &is_bool);
- if (!is_bool)
- sprintf(normalized, "%d", v);
- else
- sprintf(normalized, "%s", v ? "true" : "false");
- }
+ return xstrdup(value);
+ if (types == TYPE_INT)
+ return xstrfmt("%"PRId64, git_config_int64(key, value));
+ if (types == TYPE_BOOL)
+ return xstrdup(git_config_bool(key, value) ? "true" : "false");
+ if (types == TYPE_BOOL_OR_INT) {
+ int is_bool, v;
+ v = git_config_bool_or_int(key, value, &is_bool);
+ if (!is_bool)
+ return xstrfmt("%d", v);
+ else
+ return xstrdup(v ? "true" : "false");
}
- return normalized;
+ die("BUG: cannot normalize type %d", types);
}
static int get_color_found;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 28/68] fetch: replace static buffer with xstrfmt
We parse the INFINITE_DEPTH constant into a static,
fixed-size buffer using sprintf. This buffer is sufficiently
large for the current constant, but it's a suspicious
pattern, as the constant is defined far away, and it's not
immediately obvious that 12 bytes are large enough to hold
it.
We can just use xstrfmt here, which gets rid of any question
of the buffer size. It also removes any concerns with object
lifetime, which means we do not have to wonder why this
buffer deep within a conditional is marked "static" (we
never free our newly allocated result, of course, but that's
OK; it's global that lasts the lifetime of the whole program
anyway).
Signed-off-by: Jeff King
---
builtin/fetch.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9a3869f..4703725 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1156,11 +1156,8 @@ int cmd_fetch(int argc, const char **argv, const char
*prefix)
die(_("--depth and --unshallow cannot be used
together"));
else if (!is_repository_shallow())
die(_("--unshallow on a complete repository does not
make sense"));
- else {
- static char inf_depth[12];
- sprintf(inf_depth, "%d", INFINITE_DEPTH);
- depth = inf_depth;
- }
+ else
+ depth = xstrfmt("%d", INFINITE_DEPTH);
}
/* no need to be strict, transport_set_option() will validate it again
*/
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 35/68] resolve_ref: use strbufs for internal buffers
resolve_ref already uses a strbuf internally when generating
pathnames, but it uses fixed-size buffers for storing the
refname and symbolic refs. This means that you cannot
actually point HEAD to a ref that is larger than 256 bytes.
We can lift this limit by using strbufs here, too. Like
sb_path, we pass the the buffers into our helper function,
so that we can easily clean up all output paths. We can also
drop the "unsafe" name from our helper function, as it no
longer uses a single static buffer (but of course
resolve_ref_unsafe is still unsafe, because the static
buffers moved there).
As a bonus, we also get to drop some strcpy calls between
the two fixed buffers (that cannot currently overflow
because the two buffers are sized identically).
Signed-off-by: Jeff King
---
refs.c | 57 ++---
t/t1401-symbolic-ref.sh | 29 +
2 files changed, 59 insertions(+), 27 deletions(-)
diff --git a/refs.c b/refs.c
index d5c8b2f..c2709de 100644
--- a/refs.c
+++ b/refs.c
@@ -1579,16 +1579,15 @@ static int resolve_missing_loose_ref(const char
*refname,
}
/* This function needs to return a meaningful errno on failure */
-static const char *resolve_ref_unsafe_1(const char *refname,
- int resolve_flags,
- unsigned char *sha1,
- int *flags,
- struct strbuf *sb_path)
+static const char *resolve_ref_1(const char *refname,
+int resolve_flags,
+unsigned char *sha1,
+int *flags,
+struct strbuf *sb_refname,
+struct strbuf *sb_path,
+struct strbuf *sb_contents)
{
int depth = MAXDEPTH;
- ssize_t len;
- char buffer[256];
- static char refname_buffer[256];
int bad_name = 0;
if (flags)
@@ -1654,19 +1653,18 @@ static const char *resolve_ref_unsafe_1(const char
*refname,
/* Follow "normalized" - ie "refs/.." symlinks by hand */
if (S_ISLNK(st.st_mode)) {
- len = readlink(path, buffer, sizeof(buffer)-1);
- if (len < 0) {
+ strbuf_reset(sb_contents);
+ if (strbuf_readlink(sb_contents, path, 0) < 0) {
if (errno == ENOENT || errno == EINVAL)
/* inconsistent with lstat; retry */
goto stat_ref;
else
return NULL;
}
- buffer[len] = 0;
- if (starts_with(buffer, "refs/") &&
- !check_refname_format(buffer, 0)) {
- strcpy(refname_buffer, buffer);
- refname = refname_buffer;
+ if (starts_with(sb_contents->buf, "refs/") &&
+ !check_refname_format(sb_contents->buf, 0)) {
+ strbuf_swap(sb_refname, sb_contents);
+ refname = sb_refname->buf;
if (flags)
*flags |= REF_ISSYMREF;
if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
@@ -1695,28 +1693,26 @@ static const char *resolve_ref_unsafe_1(const char
*refname,
else
return NULL;
}
- len = read_in_full(fd, buffer, sizeof(buffer)-1);
- if (len < 0) {
+ strbuf_reset(sb_contents);
+ if (strbuf_read(sb_contents, fd, 256) < 0) {
int save_errno = errno;
close(fd);
errno = save_errno;
return NULL;
}
close(fd);
- while (len && isspace(buffer[len-1]))
- len--;
- buffer[len] = '\0';
+ strbuf_rtrim(sb_contents);
/*
* Is it a symbolic ref?
*/
- if (!starts_with(buffer, "ref:")) {
+ if (!starts_with(sb_contents->buf, "ref:")) {
/*
* Please note that FETCH_HEAD has a second
* line containing other data.
*/
- if (get_sha1_hex(buffer, sha1) ||
- (buffer[40] != '\0' && !isspace(buffer[40]))) {
+ if (get_sha1_hex(sb_contents->buf, sha1) ||
+ (sb_contents->buf[40] != '\0' &&
!isspace(sb_contents->buf[
[PATCH 43/68] transport: use strbufs for status table "quickref" strings
We generate range strings like "1234abcd...5678efab" for use
in the the fetch and push status tables. We use fixed-size
buffers along with strcat to do so. These aren't buggy, as
our manual size computation is correct, but there's nothing
checking that this is so. Let's switch them to strbufs
instead, which are obviously correct, and make it easier to
audit the code base for problematic calls to strcat().
Signed-off-by: Jeff King
---
builtin/fetch.c | 22 --
transport.c | 13 +++--
2 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4703725..841880e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -528,36 +528,38 @@ static int update_local_ref(struct ref *ref,
}
if (in_merge_bases(current, updated)) {
- char quickref[83];
+ struct strbuf quickref = STRBUF_INIT;
int r;
- strcpy(quickref, find_unique_abbrev(current->object.sha1,
DEFAULT_ABBREV));
- strcat(quickref, "..");
- strcat(quickref, find_unique_abbrev(ref->new_sha1,
DEFAULT_ABBREV));
+ strbuf_add_unique_abbrev(&quickref, current->object.sha1,
DEFAULT_ABBREV);
+ strbuf_addstr(&quickref, "..");
+ strbuf_add_unique_abbrev(&quickref, ref->new_sha1,
DEFAULT_ABBREV);
if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
(recurse_submodules != RECURSE_SUBMODULES_ON))
check_for_new_submodule_commits(ref->new_sha1);
r = s_update_ref("fast-forward", ref, 1);
strbuf_addf(display, "%c %-*s %-*s -> %s%s",
r ? '!' : ' ',
- TRANSPORT_SUMMARY_WIDTH, quickref,
+ TRANSPORT_SUMMARY_WIDTH, quickref.buf,
REFCOL_WIDTH, remote, pretty_ref,
r ? _(" (unable to update local ref)") : "");
+ strbuf_release(&quickref);
return r;
} else if (force || ref->force) {
- char quickref[84];
+ struct strbuf quickref = STRBUF_INIT;
int r;
- strcpy(quickref, find_unique_abbrev(current->object.sha1,
DEFAULT_ABBREV));
- strcat(quickref, "...");
- strcat(quickref, find_unique_abbrev(ref->new_sha1,
DEFAULT_ABBREV));
+ strbuf_add_unique_abbrev(&quickref, current->object.sha1,
DEFAULT_ABBREV);
+ strbuf_addstr(&quickref, "...");
+ strbuf_add_unique_abbrev(&quickref, ref->new_sha1,
DEFAULT_ABBREV);
if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
(recurse_submodules != RECURSE_SUBMODULES_ON))
check_for_new_submodule_commits(ref->new_sha1);
r = s_update_ref("forced-update", ref, 1);
strbuf_addf(display, "%c %-*s %-*s -> %s (%s)",
r ? '!' : '+',
- TRANSPORT_SUMMARY_WIDTH, quickref,
+ TRANSPORT_SUMMARY_WIDTH, quickref.buf,
REFCOL_WIDTH, remote, pretty_ref,
r ? _("unable to update local ref") : _("forced
update"));
+ strbuf_release(&quickref);
return r;
} else {
strbuf_addf(display, "! %-*s %-*s -> %s %s",
diff --git a/transport.c b/transport.c
index 2d51348..3b47d49 100644
--- a/transport.c
+++ b/transport.c
@@ -654,23 +654,24 @@ static void print_ok_ref_status(struct ref *ref, int
porcelain)
"[new branch]"),
ref, ref->peer_ref, NULL, porcelain);
else {
- char quickref[84];
+ struct strbuf quickref = STRBUF_INIT;
char type;
const char *msg;
- strcpy(quickref, status_abbrev(ref->old_sha1));
+ strbuf_addstr(&quickref, status_abbrev(ref->old_sha1));
if (ref->forced_update) {
- strcat(quickref, "...");
+ strbuf_addstr(&quickref, "...");
type = '+';
msg = "forced update";
} else {
- strcat(quickref, "..");
+ strbuf_addstr(&quickref, "..");
type = ' ';
msg = NULL;
}
- strcat(quickref, status_abbrev(ref->new_sha1));
+ strbuf_addstr(&quickref, status_abbrev(ref->new_sha1));
- print_ref_status(type, quickref, ref, ref->peer_ref, msg,
porcelain);
+ print_ref_status(type, quickref.buf, ref, ref->peer_ref, msg,
porcelain);
+ strbuf_release(&quickref);
}
}
--
2.6.0.rc3.454.g204ad51
--
To unsubscribe from this list: send the line "unsubscribe
[PATCH 51/68] stat_tracking_info: convert to argv_array
In addition to dropping the magic number for the fixed-size
argv, we can also drop a fixed-length buffer and some
strcpy's into it.
Signed-off-by: Jeff King
---
remote.c | 26 --
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/remote.c b/remote.c
index a01f13a..1101f82 100644
--- a/remote.c
+++ b/remote.c
@@ -8,6 +8,7 @@
#include "tag.h"
#include "string-list.h"
#include "mergesort.h"
+#include "argv-array.h"
enum map_direction { FROM_SRC, FROM_DST };
@@ -1997,10 +1998,9 @@ int stat_tracking_info(struct branch *branch, int
*num_ours, int *num_theirs,
{
unsigned char sha1[20];
struct commit *ours, *theirs;
- char symmetric[84];
struct rev_info revs;
- const char *rev_argv[10], *base;
- int rev_argc;
+ const char *base;
+ struct argv_array argv = ARGV_ARRAY_INIT;
/* Cannot stat unless we are marked to build on top of somebody else. */
base = branch_get_upstream(branch, NULL);
@@ -2029,19 +2029,15 @@ int stat_tracking_info(struct branch *branch, int
*num_ours, int *num_theirs,
}
/* Run "rev-list --left-right ours...theirs" internally... */
- rev_argc = 0;
- rev_argv[rev_argc++] = NULL;
- rev_argv[rev_argc++] = "--left-right";
- rev_argv[rev_argc++] = symmetric;
- rev_argv[rev_argc++] = "--";
- rev_argv[rev_argc] = NULL;
-
- strcpy(symmetric, sha1_to_hex(ours->object.sha1));
- strcpy(symmetric + 40, "...");
- strcpy(symmetric + 43, sha1_to_hex(theirs->object.sha1));
+ argv_array_push(&argv, ""); /* ignored */
+ argv_array_push(&argv, "--left-right");
+ argv_array_pushf(&argv, "%s...%s",
+sha1_to_hex(ours->object.sha1),
+sha1_to_hex(theirs->object.sha1));
+ argv_array_push(&argv, "--");
init_revisions(&revs, NULL);
- setup_revisions(rev_argc, rev_argv, &revs, NULL);
+ setup_revisions(argv.argc, argv.argv, &revs, NULL);
if (prepare_revision_walk(&revs))
die("revision walk setup failed");
@@ -2061,6 +2057,8 @@ int stat_tracking_info(struct branch *branch, int
*num_ours, int *num_theirs,
/* clear object flags smudged by the above traversal */
clear_commit_marks(ours, ALL_REV_FLAGS);
clear_commit_marks(theirs, ALL_REV_FLAGS);
+
+ argv_array_clear(&argv);
return 0;
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 50/68] http-push: use an argv_array for setup_revisions
This drops the magic number for the fixed-size argv arrays,
so we do not have to wonder if we are overflowing it. We can
also drop some confusing sha1_to_hex memory allocation
(which seems to predate the ring of buffers allowing
multiple calls), and get rid of an unchecked sprintf call.
Signed-off-by: Jeff King
---
http-push.c | 32 ++--
1 file changed, 10 insertions(+), 22 deletions(-)
diff --git a/http-push.c b/http-push.c
index e501c28..43a9036 100644
--- a/http-push.c
+++ b/http-push.c
@@ -10,6 +10,7 @@
#include "remote.h"
#include "list-objects.h"
#include "sigchain.h"
+#include "argv-array.h"
#ifdef EXPAT_NEEDS_XMLPARSE_H
#include
@@ -1856,9 +1857,7 @@ int main(int argc, char **argv)
new_refs = 0;
for (ref = remote_refs; ref; ref = ref->next) {
char old_hex[60], *new_hex;
- const char *commit_argv[5];
- int commit_argc;
- char *new_sha1_hex, *old_sha1_hex;
+ struct argv_array commit_argv = ARGV_ARRAY_INIT;
if (!ref->peer_ref)
continue;
@@ -1937,27 +1936,15 @@ int main(int argc, char **argv)
}
/* Set up revision info for this refspec */
- commit_argc = 3;
- new_sha1_hex = xstrdup(sha1_to_hex(ref->new_sha1));
- old_sha1_hex = NULL;
- commit_argv[1] = "--objects";
- commit_argv[2] = new_sha1_hex;
- if (!push_all && !is_null_sha1(ref->old_sha1)) {
- old_sha1_hex = xmalloc(42);
- sprintf(old_sha1_hex, "^%s",
- sha1_to_hex(ref->old_sha1));
- commit_argv[3] = old_sha1_hex;
- commit_argc++;
- }
- commit_argv[commit_argc] = NULL;
+ argv_array_push(&commit_argv, ""); /* ignored */
+ argv_array_push(&commit_argv, "--objects");
+ argv_array_push(&commit_argv, sha1_to_hex(ref->new_sha1));
+ if (!push_all && !is_null_sha1(ref->old_sha1))
+ argv_array_pushf(&commit_argv, "^%s",
+sha1_to_hex(ref->old_sha1));
init_revisions(&revs, setup_git_directory());
- setup_revisions(commit_argc, commit_argv, &revs, NULL);
+ setup_revisions(commit_argv.argc, commit_argv.argv, &revs,
NULL);
revs.edge_hint = 0; /* just in case */
- free(new_sha1_hex);
- if (old_sha1_hex) {
- free(old_sha1_hex);
- commit_argv[1] = NULL;
- }
/* Generate a list of objects that need to be pushed */
pushing = 0;
@@ -1986,6 +1973,7 @@ int main(int argc, char **argv)
printf("%s %s\n", !rc ? "ok" : "error", ref->name);
unlock_remote(ref_lock);
check_locks();
+ argv_array_clear(&commit_argv);
}
/* Update remote server info if appropriate */
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 54/68] drop strcpy in favor of raw sha1_to_hex
In some cases where we strcpy() the result of sha1_to_hex(),
there's no need; the result goes directly into a printf
statement, and we can simply pass the return value from
sha1_to_hex() directly.
When this code was originally written, sha1_to_hex used a
single buffer, and it was not safe to use it twice within a
single expression. That changed as of dcb3450 (sha1_to_hex()
usage cleanup, 2006-05-03), but this code ewas never
updated.
History-dug-by: Eric Sunshine
Signed-off-by: Jeff King
---
http-push.c | 6 ++
walker.c| 5 ++---
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/http-push.c b/http-push.c
index 43a9036..48f39b7 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1856,7 +1856,6 @@ int main(int argc, char **argv)
new_refs = 0;
for (ref = remote_refs; ref; ref = ref->next) {
- char old_hex[60], *new_hex;
struct argv_array commit_argv = ARGV_ARRAY_INIT;
if (!ref->peer_ref)
@@ -1911,13 +1910,12 @@ int main(int argc, char **argv)
}
hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
new_refs++;
- strcpy(old_hex, sha1_to_hex(ref->old_sha1));
- new_hex = sha1_to_hex(ref->new_sha1);
fprintf(stderr, "updating '%s'", ref->name);
if (strcmp(ref->name, ref->peer_ref->name))
fprintf(stderr, " using '%s'", ref->peer_ref->name);
- fprintf(stderr, "\n from %s\n to %s\n", old_hex, new_hex);
+ fprintf(stderr, "\n from %s\n to %s\n",
+ sha1_to_hex(ref->old_sha1), sha1_to_hex(ref->new_sha1));
if (dry_run) {
if (helper_status)
printf("ok %s\n", ref->name);
diff --git a/walker.c b/walker.c
index 44a936c..cdeb63f 100644
--- a/walker.c
+++ b/walker.c
@@ -17,10 +17,9 @@ void walker_say(struct walker *walker, const char *fmt,
const char *hex)
static void report_missing(const struct object *obj)
{
- char missing_hex[41];
- strcpy(missing_hex, sha1_to_hex(obj->sha1));
fprintf(stderr, "Cannot obtain needed %s %s\n",
- obj->type ? typename(obj->type): "object", missing_hex);
+ obj->type ? typename(obj->type): "object",
+ sha1_to_hex(obj->sha1));
if (!is_null_sha1(current_commit_sha1))
fprintf(stderr, "while processing commit %s.\n",
sha1_to_hex(current_commit_sha1));
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 40/68] sha1_get_pack_name: use a strbuf
We do some manual memory computation here, and there's no
check that our 60 is not overflowed by the raw sprintf (it
isn't, because the "which" parameter is never longer than
"pack"). We can simplify this greatly with a strbuf.
Technically the end result is not identical, as the original
took care not to rewrite the object directory on each call
for performance reasons. We could do that here, too (by
saving the baselen and resetting to it), but it's not worth
the complexity; this function is not called a lot (generally
once per packfile that we open).
Signed-off-by: Jeff King
---
sha1_file.c | 39 ++-
1 file changed, 10 insertions(+), 29 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 2be1afd..c26fdcb 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -208,44 +208,25 @@ const char *sha1_file_name(const unsigned char *sha1)
* provided by the caller. which should be "pack" or "idx".
*/
static char *sha1_get_pack_name(const unsigned char *sha1,
- char **name, char **base, const char *which)
+ struct strbuf *buf,
+ const char *which)
{
- static const char hex[] = "0123456789abcdef";
- char *buf;
- int i;
-
- if (!*base) {
- const char *sha1_file_directory = get_object_directory();
- int len = strlen(sha1_file_directory);
- *base = xmalloc(len + 60);
- sprintf(*base,
"%s/pack/pack-1234567890123456789012345678901234567890.%s",
- sha1_file_directory, which);
- *name = *base + len + 11;
- }
-
- buf = *name;
-
- for (i = 0; i < 20; i++) {
- unsigned int val = *sha1++;
- *buf++ = hex[val >> 4];
- *buf++ = hex[val & 0xf];
- }
-
- return *base;
+ strbuf_reset(buf);
+ strbuf_addf(buf, "%s/pack/pack-%s.%s", get_object_directory(),
+ sha1_to_hex(sha1), which);
+ return buf->buf;
}
char *sha1_pack_name(const unsigned char *sha1)
{
- static char *name, *base;
-
- return sha1_get_pack_name(sha1, &name, &base, "pack");
+ static struct strbuf buf = STRBUF_INIT;
+ return sha1_get_pack_name(sha1, &buf, "pack");
}
char *sha1_pack_index_name(const unsigned char *sha1)
{
- static char *name, *base;
-
- return sha1_get_pack_name(sha1, &name, &base, "idx");
+ static struct strbuf buf = STRBUF_INIT;
+ return sha1_get_pack_name(sha1, &buf, "idx");
}
struct alternate_object_database *alt_odb_list;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 53/68] use sha1_to_hex_r() instead of strcpy
Before sha1_to_hex_r() existed, a simple way to get hex
sha1 into a buffer was with:
strcpy(buf, sha1_to_hex(sha1));
This isn't wrong (assuming the buf is 41 characters), but it
makes auditing the code base for bad strcpy() calls harder,
as these become false positives.
Let's convert them to sha1_to_hex_r(), and likewise for
some calls to find_unique_abbrev(). While we're here, we'll
double-check that all of the buffers are correctly sized,
and use the more obvious GIT_SHA1_HEXSZ constant.
Signed-off-by: Jeff King
---
builtin/blame.c| 8
builtin/merge-index.c | 4 ++--
builtin/merge.c| 20 ++--
builtin/receive-pack.c | 15 +--
builtin/rev-list.c | 4 ++--
diff.c | 9 -
6 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..e253ac0 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1867,9 +1867,9 @@ static void emit_porcelain(struct scoreboard *sb, struct
blame_entry *ent,
int cnt;
const char *cp;
struct origin *suspect = ent->suspect;
- char hex[41];
+ char hex[GIT_SHA1_HEXSZ + 1];
- strcpy(hex, sha1_to_hex(suspect->commit->object.sha1));
+ sha1_to_hex_r(hex, suspect->commit->object.sha1);
printf("%s %d %d %d\n",
hex,
ent->s_lno + 1,
@@ -1905,11 +1905,11 @@ static void emit_other(struct scoreboard *sb, struct
blame_entry *ent, int opt)
const char *cp;
struct origin *suspect = ent->suspect;
struct commit_info ci;
- char hex[41];
+ char hex[GIT_SHA1_HEXSZ + 1];
int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
get_commit_info(suspect->commit, &ci, 1);
- strcpy(hex, sha1_to_hex(suspect->commit->object.sha1));
+ sha1_to_hex_r(hex, suspect->commit->object.sha1);
cp = nth_line(sb, ent->lno);
for (cnt = 0; cnt < ent->num_lines; cnt++) {
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index 1d66111..1c3427c 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -9,7 +9,7 @@ static int merge_entry(int pos, const char *path)
{
int found;
const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
- char hexbuf[4][60];
+ char hexbuf[4][GIT_SHA1_HEXSZ + 1];
char ownbuf[4][60];
if (pos >= active_nr)
@@ -22,7 +22,7 @@ static int merge_entry(int pos, const char *path)
if (strcmp(ce->name, path))
break;
found++;
- strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
+ sha1_to_hex_r(hexbuf[stage], ce->sha1);
xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o",
ce->ce_mode);
arguments[stage] = hexbuf[stage];
arguments[stage + 4] = ownbuf[stage];
diff --git a/builtin/merge.c b/builtin/merge.c
index a0edaca..a0a9328 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1319,13 +1319,13 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
if (verify_signatures) {
for (p = remoteheads; p; p = p->next) {
struct commit *commit = p->item;
- char hex[41];
+ char hex[GIT_SHA1_HEXSZ + 1];
struct signature_check signature_check;
memset(&signature_check, 0, sizeof(signature_check));
check_commit_signature(commit, &signature_check);
- strcpy(hex, find_unique_abbrev(commit->object.sha1,
DEFAULT_ABBREV));
+ find_unique_abbrev_r(hex, commit->object.sha1,
DEFAULT_ABBREV);
switch (signature_check.result) {
case 'G':
break;
@@ -1415,15 +1415,15 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
/* Again the most common case of merging one remote. */
struct strbuf msg = STRBUF_INIT;
struct commit *commit;
- char hex[41];
- strcpy(hex, find_unique_abbrev(head_commit->object.sha1,
DEFAULT_ABBREV));
-
- if (verbosity >= 0)
- printf(_("Updating %s..%s\n"),
- hex,
-
find_unique_abbrev(remoteheads->item->object.sha1,
- DEFAULT_ABBREV));
+ if (verbosity >= 0) {
+ char from[GIT_SHA1_HEXSZ + 1], to[GIT_SHA1_HEXSZ + 1];
+ find_unique_abbrev_r(from, head_commit->object.sha1,
+ DEFAULT_ABBREV);
+ find_unique_abbrev_r(to, remoteheads->item->object.sha1,
+ DEFAULT_ABBREV);
+ printf(_("Updating %s..%s\n"), from, to);
+
[PATCH 38/68] http-push: use strbuf instead of fwrite_buffer
The http-push code defines an fwrite_buffer function for use
as a curl callback; it just writes to a strbuf. There's no
reason we need to use it ourselves, as we know we have a
strbuf. This lets us format directly into it, rather than
dealing with an extra temporary buffer (which required
manual length computation).
While we're here, let's also remove the literal tabs from
the source in favor of "\t", which is more visually obvious.
Signed-off-by: Jeff King
---
http-push.c | 21 +
1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/http-push.c b/http-push.c
index 37baff8..e501c28 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1459,8 +1459,6 @@ static void add_remote_info_ref(struct remote_ls_ctx *ls)
{
struct strbuf *buf = (struct strbuf *)ls->userData;
struct object *o;
- int len;
- char *ref_info;
struct ref *ref;
ref = alloc_ref(ls->dentry_name);
@@ -1484,23 +1482,14 @@ static void add_remote_info_ref(struct remote_ls_ctx
*ls)
return;
}
- len = strlen(ls->dentry_name) + 42;
- ref_info = xcalloc(len + 1, 1);
- sprintf(ref_info, "%s %s\n",
- sha1_to_hex(ref->old_sha1), ls->dentry_name);
- fwrite_buffer(ref_info, 1, len, buf);
- free(ref_info);
+ strbuf_addf(buf, "%s\t%s\n",
+ sha1_to_hex(ref->old_sha1), ls->dentry_name);
if (o->type == OBJ_TAG) {
o = deref_tag(o, ls->dentry_name, 0);
- if (o) {
- len = strlen(ls->dentry_name) + 45;
- ref_info = xcalloc(len + 1, 1);
- sprintf(ref_info, "%s %s^{}\n",
- sha1_to_hex(o->sha1), ls->dentry_name);
- fwrite_buffer(ref_info, 1, len, buf);
- free(ref_info);
- }
+ if (o)
+ strbuf_addf(buf, "%s\t%s^{}\n",
+ sha1_to_hex(o->sha1), ls->dentry_name);
}
free(ref);
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 62/68] notes: document length of fanout path with a constant
We know that a fanned-out sha1 in a notes tree cannot be
more than "aa/bb/cc/...", and we have an assert() to confirm
that. But let's factor out that length into a constant so we
can be sure it is used consistently. And even though we
assert() earlier, let's replace a strcpy with xsnprintf, so
it is clear to a reader that all cases are covered.
Signed-off-by: Jeff King
---
notes.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/notes.c b/notes.c
index eacd2a6..db77922 100644
--- a/notes.c
+++ b/notes.c
@@ -539,6 +539,9 @@ static unsigned char determine_fanout(struct int_node
*tree, unsigned char n,
return fanout + 1;
}
+/* hex SHA1 + 19 * '/' + NUL */
+#define FANOUT_PATH_MAX 40 + 19 + 1
+
static void construct_path_with_fanout(const unsigned char *sha1,
unsigned char fanout, char *path)
{
@@ -551,7 +554,7 @@ static void construct_path_with_fanout(const unsigned char
*sha1,
path[i++] = '/';
fanout--;
}
- strcpy(path + i, hex_sha1 + j);
+ xsnprintf(path + i, FANOUT_PATH_MAX - i, "%s", hex_sha1 + j);
}
static int for_each_note_helper(struct notes_tree *t, struct int_node *tree,
@@ -562,7 +565,7 @@ static int for_each_note_helper(struct notes_tree *t,
struct int_node *tree,
void *p;
int ret = 0;
struct leaf_node *l;
- static char path[40 + 19 + 1]; /* hex SHA1 + 19 * '/' + NUL */
+ static char path[FANOUT_PATH_MAX];
fanout = determine_fanout(tree, n, fanout);
for (i = 0; i < 16; i++) {
@@ -595,7 +598,7 @@ redo:
/* invoke callback with subtree */
unsigned int path_len =
l->key_sha1[19] * 2 + fanout;
- assert(path_len < 40 + 19);
+ assert(path_len < FANOUT_PATH_MAX - 1);
construct_path_with_fanout(l->key_sha1, fanout,
path);
/* Create trailing slash, if needed */
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 67/68] use strbuf_complete to conditionally append slash
When working with paths in strbufs, we frequently want to
ensure that a directory contains a trailing slash before
appending to it. We can shorten this code (and make the
intent more obvious) by calling strbuf_complete.
Most of these cases are trivially identical conversions, but
there are two things to note:
- in a few cases we did not check that the strbuf is
non-empty (which would lead to an out-of-bounds memory
access). These were generally not triggerable in
practice, either from earlier assertions, or typically
because we would have just fed the strbuf to opendir(),
which would choke on an empty path.
- in a few cases we indexed the buffer with "original_len"
or similar, rather than the current sb->len, and it is
not immediately obvious from the diff that they are the
same. In all of these cases, I manually verified that
the strbuf does not change between the assignment and
the strbuf_complete call.
This does not convert cases which look like:
if (sb->len && !is_dir_sep(sb->buf[sb->len - 1]))
strbuf_addch(sb, '/');
as those are obviously semantically different. Some of these
cases arguably should be doing that, but that is out of
scope for this change, which aims purely for cleanup with no
behavior change (and at least it will make such sites easier
to find and examine in the future, as we can grep for
strbuf_complete).
Signed-off-by: Jeff King
---
builtin/clean.c | 6 ++
builtin/log.c | 3 +--
diff-no-index.c | 6 ++
dir.c | 6 ++
path.c | 3 +--
refs.c | 3 +--
url.c | 3 +--
7 files changed, 10 insertions(+), 20 deletions(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index df53def..d7acb94 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -159,8 +159,7 @@ static int is_git_repository(struct strbuf *path)
int gitfile_error;
size_t orig_path_len = path->len;
assert(orig_path_len != 0);
- if (path->buf[orig_path_len - 1] != '/')
- strbuf_addch(path, '/');
+ strbuf_complete(path, '/');
strbuf_addstr(path, ".git");
if (read_gitfile_gently(path->buf, &gitfile_error) ||
is_git_directory(path->buf))
ret = 1;
@@ -206,8 +205,7 @@ static int remove_dirs(struct strbuf *path, const char
*prefix, int force_flag,
return res;
}
- if (path->buf[original_len - 1] != '/')
- strbuf_addch(path, '/');
+ strbuf_complete(path, '/');
len = path->len;
while ((e = readdir(dir)) != NULL) {
diff --git a/builtin/log.c b/builtin/log.c
index a491d3d..dda671d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -796,8 +796,7 @@ static int reopen_stdout(struct commit *commit, const char
*subject,
if (filename.len >=
PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len)
return error(_("name of output directory is too long"));
- if (filename.buf[filename.len - 1] != '/')
- strbuf_addch(&filename, '/');
+ strbuf_complete(&filename, '/');
}
if (rev->numbered_files)
diff --git a/diff-no-index.c b/diff-no-index.c
index 0320605..8e0fd27 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -136,15 +136,13 @@ static int queue_diff(struct diff_options *o,
if (name1) {
strbuf_addstr(&buffer1, name1);
- if (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')
- strbuf_addch(&buffer1, '/');
+ strbuf_complete(&buffer1, '/');
len1 = buffer1.len;
}
if (name2) {
strbuf_addstr(&buffer2, name2);
- if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
- strbuf_addch(&buffer2, '/');
+ strbuf_complete(&buffer2, '/');
len2 = buffer2.len;
}
diff --git a/dir.c b/dir.c
index 7b25634..79fdad8 100644
--- a/dir.c
+++ b/dir.c
@@ -1519,8 +1519,7 @@ static enum path_treatment treat_path_fast(struct
dir_struct *dir,
}
strbuf_addstr(path, cdir->ucd->name);
/* treat_one_path() does this before it calls treat_directory() */
- if (path->buf[path->len - 1] != '/')
- strbuf_addch(path, '/');
+ strbuf_complete(path, '/');
if (cdir->ucd->check_only)
/*
* check_only is set as a result of treat_directory() getting
@@ -2126,8 +2125,7 @@ static int remove_dir_recurse(struct strbuf *path, int
flag, int *kept_up)
else
return -1;
}
- if (path->buf[original_len - 1] != '/')
- strbuf_addch(path, '/');
+ strbuf_complete(path, '/');
len = path->len;
while ((e = readdir(dir)) != NULL) {
diff --git a/
[PATCH 49/68] fetch-pack: use argv_array for index-pack / unpack-objects
This cleans up a magic number that must be kept in sync with
the rest of the code (the number of argv slots). It also
lets us drop some fixed buffers and an sprintf (since we
can now use argv_array_pushf).
We do still have to keep one fixed buffer for calling
gethostname, but at least now the size computations for it
are much simpler.
Signed-off-by: Jeff King
---
fetch-pack.c | 56 +++-
1 file changed, 27 insertions(+), 29 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 820251a..2dabee9 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -681,11 +681,10 @@ static int get_pack(struct fetch_pack_args *args,
int xd[2], char **pack_lockfile)
{
struct async demux;
- const char *argv[22];
- char keep_arg[256];
- char hdr_arg[256];
- const char **av, *cmd_name;
int do_keep = args->keep_pack;
+ const char *cmd_name;
+ struct pack_header header;
+ int pass_header = 0;
struct child_process cmd = CHILD_PROCESS_INIT;
int ret;
@@ -705,17 +704,11 @@ static int get_pack(struct fetch_pack_args *args,
else
demux.out = xd[0];
- cmd.argv = argv;
- av = argv;
- *hdr_arg = 0;
if (!args->keep_pack && unpack_limit) {
- struct pack_header header;
if (read_pack_header(demux.out, &header))
die("protocol error: bad pack header");
- snprintf(hdr_arg, sizeof(hdr_arg),
-"--pack_header=%"PRIu32",%"PRIu32,
-ntohl(header.hdr_version), ntohl(header.hdr_entries));
+ pass_header = 1;
if (ntohl(header.hdr_entries) < unpack_limit)
do_keep = 0;
else
@@ -723,44 +716,49 @@ static int get_pack(struct fetch_pack_args *args,
}
if (alternate_shallow_file) {
- *av++ = "--shallow-file";
- *av++ = alternate_shallow_file;
+ argv_array_push(&cmd.args, "--shallow-file");
+ argv_array_push(&cmd.args, alternate_shallow_file);
}
if (do_keep) {
if (pack_lockfile)
cmd.out = -1;
- *av++ = cmd_name = "index-pack";
- *av++ = "--stdin";
+ cmd_name = "index-pack";
+ argv_array_push(&cmd.args, cmd_name);
+ argv_array_push(&cmd.args, "--stdin");
if (!args->quiet && !args->no_progress)
- *av++ = "-v";
+ argv_array_push(&cmd.args, "-v");
if (args->use_thin_pack)
- *av++ = "--fix-thin";
+ argv_array_push(&cmd.args, "--fix-thin");
if (args->lock_pack || unpack_limit) {
- int s = sprintf(keep_arg,
- "--keep=fetch-pack %"PRIuMAX " on ",
(uintmax_t) getpid());
- if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
- strcpy(keep_arg + s, "localhost");
- *av++ = keep_arg;
+ char hostname[256];
+ if (gethostname(hostname, sizeof(hostname)))
+ xsnprintf(hostname, sizeof(hostname),
"localhost");
+ argv_array_pushf(&cmd.args,
+ "--keep=fetch-pack %"PRIuMAX " on %s",
+ (uintmax_t)getpid(), hostname);
}
if (args->check_self_contained_and_connected)
- *av++ = "--check-self-contained-and-connected";
+ argv_array_push(&cmd.args,
"--check-self-contained-and-connected");
}
else {
- *av++ = cmd_name = "unpack-objects";
+ cmd_name = "unpack-objects";
+ argv_array_push(&cmd.args, cmd_name);
if (args->quiet || args->no_progress)
- *av++ = "-q";
+ argv_array_push(&cmd.args, "-q");
args->check_self_contained_and_connected = 0;
}
- if (*hdr_arg)
- *av++ = hdr_arg;
+
+ if (pass_header)
+ argv_array_pushf(&cmd.args, "--pack_header=%"PRIu32",%"PRIu32,
+ntohl(header.hdr_version),
+ntohl(header.hdr_entries));
if (fetch_fsck_objects >= 0
? fetch_fsck_objects
: transfer_fsck_objects >= 0
? transfer_fsck_objects
: 0)
- *av++ = "--strict";
- *av++ = NULL;
+ argv_array_push(&cmd.args, "--strict");
cmd.in = demux.out;
cmd.git_cmd = 1;
--
2.6.0.rc3.454.g204ad51
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
[PATCH 45/68] enter_repo: convert fixed-size buffers to strbufs
We use two PATH_MAX-sized buffers to represent the repo
path, and must make sure not to overflow them. We do take
care to check the lengths, but the logic is rather hard to
follow, as we use several magic numbers (e.g., "PATH_MAX -
10"). And in fact you _can_ overflow the buffer if you have
a ".git" file with an extremely long path in it.
By switching to strbufs, these problems all go away. We do,
however, retain the check that the initial input we get is
no larger than PATH_MAX. This function is an entry point for
untrusted repo names from the network, and it's a good idea
to keep a sanity check (both to avoid allocating arbitrary
amounts of memory, and also as a layer of defense against
any downstream users of the names).
Signed-off-by: Jeff King
---
path.c | 57 +
1 file changed, 29 insertions(+), 28 deletions(-)
diff --git a/path.c b/path.c
index 46a4d27..60e0390 100644
--- a/path.c
+++ b/path.c
@@ -391,8 +391,8 @@ return_null:
*/
const char *enter_repo(const char *path, int strict)
{
- static char used_path[PATH_MAX];
- static char validated_path[PATH_MAX];
+ static struct strbuf validated_path = STRBUF_INIT;
+ static struct strbuf used_path = STRBUF_INIT;
if (!path)
return NULL;
@@ -407,46 +407,47 @@ const char *enter_repo(const char *path, int strict)
while ((1 < len) && (path[len-1] == '/'))
len--;
+ /*
+* We can handle arbitrary-sized buffers, but this remains as a
+* sanity check on untrusted input.
+*/
if (PATH_MAX <= len)
return NULL;
- strncpy(used_path, path, len); used_path[len] = 0 ;
- strcpy(validated_path, used_path);
- if (used_path[0] == '~') {
- char *newpath = expand_user_path(used_path);
- if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
- free(newpath);
+ strbuf_reset(&used_path);
+ strbuf_reset(&validated_path);
+ strbuf_add(&used_path, path, len);
+ strbuf_add(&validated_path, path, len);
+
+ if (used_path.buf[0] == '~') {
+ char *newpath = expand_user_path(used_path.buf);
+ if (!newpath)
return NULL;
- }
- /*
-* Copy back into the static buffer. A pity
-* since newpath was not bounded, but other
-* branches of the if are limited by PATH_MAX
-* anyway.
-*/
- strcpy(used_path, newpath); free(newpath);
+ strbuf_attach(&used_path, newpath, strlen(newpath),
+ strlen(newpath));
}
- else if (PATH_MAX - 10 < len)
- return NULL;
- len = strlen(used_path);
for (i = 0; suffix[i]; i++) {
struct stat st;
- strcpy(used_path + len, suffix[i]);
- if (!stat(used_path, &st) &&
+ size_t baselen = used_path.len;
+ strbuf_addstr(&used_path, suffix[i]);
+ if (!stat(used_path.buf, &st) &&
(S_ISREG(st.st_mode) ||
- (S_ISDIR(st.st_mode) &&
is_git_directory(used_path {
- strcat(validated_path, suffix[i]);
+ (S_ISDIR(st.st_mode) &&
is_git_directory(used_path.buf {
+ strbuf_addstr(&validated_path, suffix[i]);
break;
}
+ strbuf_setlen(&used_path, baselen);
}
if (!suffix[i])
return NULL;
- gitfile = read_gitfile(used_path) ;
- if (gitfile)
- strcpy(used_path, gitfile);
- if (chdir(used_path))
+ gitfile = read_gitfile(used_path.buf) ;
+ if (gitfile) {
+ strbuf_reset(&used_path);
+ strbuf_addstr(&used_path, gitfile);
+ }
+ if (chdir(used_path.buf))
return NULL;
- path = validated_path;
+ path = validated_path.buf;
}
else if (chdir(path))
return NULL;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 65/68] Makefile: drop D_INO_IN_DIRENT build knob
Now that fsck has dropped its inode-sorting, there are no longer any users of this knob, and it can go away. Signed-off-by: Jeff King --- Makefile | 5 - config.mak.uname | 3 --- configure.ac | 7 --- 3 files changed, 15 deletions(-) diff --git a/Makefile b/Makefile index 8d5df7e..2f350ca 100644 --- a/Makefile +++ b/Makefile @@ -74,8 +74,6 @@ all:: # Define HAVE_PATHS_H if you have paths.h and want to use the default PATH # it specifies. # -# Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent. -# # Define NO_D_TYPE_IN_DIRENT if your platform defines DT_UNKNOWN but lacks # d_type in struct dirent (Cygwin 1.5, fixed in Cygwin 1.7). # @@ -1160,9 +1158,6 @@ endif ifdef NO_D_TYPE_IN_DIRENT BASIC_CFLAGS += -DNO_D_TYPE_IN_DIRENT endif -ifdef NO_D_INO_IN_DIRENT - BASIC_CFLAGS += -DNO_D_INO_IN_DIRENT -endif ifdef NO_GECOS_IN_PWENT BASIC_CFLAGS += -DNO_GECOS_IN_PWENT endif diff --git a/config.mak.uname b/config.mak.uname index 943c439..f34dcaa 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -166,7 +166,6 @@ endif ifeq ($(uname_O),Cygwin) ifeq ($(shell expr "$(uname_R)" : '1\.[1-6]\.'),4) NO_D_TYPE_IN_DIRENT = YesPlease - NO_D_INO_IN_DIRENT = YesPlease NO_STRCASESTR = YesPlease NO_MEMMEM = YesPlease NO_MKSTEMPS = YesPlease @@ -370,7 +369,6 @@ ifeq ($(uname_S),Windows) NO_POSIX_GOODIES = UnfortunatelyYes NATIVE_CRLF = YesPlease DEFAULT_HELP_FORMAT = html - NO_D_INO_IN_DIRENT = YesPlease CC = compat/vcbuild/scripts/clink.pl AR = compat/vcbuild/scripts/lib.pl @@ -520,7 +518,6 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_INET_NTOP = YesPlease NO_POSIX_GOODIES = UnfortunatelyYes DEFAULT_HELP_FORMAT = html - NO_D_INO_IN_DIRENT = YesPlease COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -DNOGDI -Icompat -Icompat/win32 COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" COMPAT_OBJS += compat/mingw.o compat/winansi.o \ diff --git a/configure.ac b/configure.ac index 14012fa..3fcca61 100644 --- a/configure.ac +++ b/configure.ac @@ -767,13 +767,6 @@ elif test x$ac_cv_member_struct_stat_st_mtim_tv_nsec != xyes; then GIT_CONF_SUBST([NO_NSEC]) fi # -# Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent. -AC_CHECK_MEMBER(struct dirent.d_ino, -[NO_D_INO_IN_DIRENT=], -[NO_D_INO_IN_DIRENT=YesPlease], -[#include ]) -GIT_CONF_SUBST([NO_D_INO_IN_DIRENT]) -# # Define NO_D_TYPE_IN_DIRENT if your platform defines DT_UNKNOWN but lacks # d_type in struct dirent (latest Cygwin -- will be fixed soonish). AC_CHECK_MEMBER(struct dirent.d_type, -- 2.6.0.rc3.454.g204ad51 -- 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
[PATCH 32/68] mailmap: replace strcpy with xstrdup
We want to make a copy of a string without any leading whitespace. To do so, we allocate a buffer large enough to hold the original, skip past the whitespace, then copy that. It's much simpler to just allocate after we've skipped, in which case we can just copy the remainder of the string, leaving no question of whether "len" is large enough. Signed-off-by: Jeff King --- mailmap.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/mailmap.c b/mailmap.c index 9e95897..f4a0f1c 100644 --- a/mailmap.c +++ b/mailmap.c @@ -162,11 +162,10 @@ static void read_mailmap_line(struct string_list *map, char *buffer, char *cp; free(*repo_abbrev); - *repo_abbrev = xmalloc(len); for (cp = buffer + abblen; isspace(*cp); cp++) ; /* nothing */ - strcpy(*repo_abbrev, cp); + *repo_abbrev = xstrdup(cp); } return; } -- 2.6.0.rc3.454.g204ad51 -- 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
[PATCH 39/68] http-walker: store url in a strbuf
We do an unchecked sprintf directly into our url buffer.
This doesn't overflow because we know that it was sized for
"$base/objects/info/http-alternates", and we are writing
"$base/objects/info/alternates", which must be smaller. But
that is not immediately obvious to a reader who is looking
for buffer overflows. Let's switch to a strbuf, so that we
do not have to think about this issue at all.
Signed-off-by: Jeff King
---
http-walker.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index 88da546..2c721f0 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -29,7 +29,7 @@ struct object_request {
struct alternates_request {
struct walker *walker;
const char *base;
- char *url;
+ struct strbuf *url;
struct strbuf *buffer;
struct active_request_slot *slot;
int http_specific;
@@ -195,10 +195,11 @@ static void process_alternates_response(void
*callback_data)
/* Try reusing the slot to get non-http alternates */
alt_req->http_specific = 0;
- sprintf(alt_req->url, "%s/objects/info/alternates",
- base);
+ strbuf_reset(alt_req->url);
+ strbuf_addf(alt_req->url, "%s/objects/info/alternates",
+ base);
curl_easy_setopt(slot->curl, CURLOPT_URL,
-alt_req->url);
+alt_req->url->buf);
active_requests++;
slot->in_use = 1;
if (slot->finished != NULL)
@@ -312,7 +313,7 @@ static void process_alternates_response(void *callback_data)
static void fetch_alternates(struct walker *walker, const char *base)
{
struct strbuf buffer = STRBUF_INIT;
- char *url;
+ struct strbuf url = STRBUF_INIT;
struct active_request_slot *slot;
struct alternates_request alt_req;
struct walker_data *cdata = walker->data;
@@ -338,7 +339,7 @@ static void fetch_alternates(struct walker *walker, const
char *base)
if (walker->get_verbosely)
fprintf(stderr, "Getting alternates list for %s\n", base);
- url = xstrfmt("%s/objects/info/http-alternates", base);
+ strbuf_addf(&url, "%s/objects/info/http-alternates", base);
/*
* Use a callback to process the result, since another request
@@ -351,10 +352,10 @@ static void fetch_alternates(struct walker *walker, const
char *base)
curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
- curl_easy_setopt(slot->curl, CURLOPT_URL, url);
+ curl_easy_setopt(slot->curl, CURLOPT_URL, url.buf);
alt_req.base = base;
- alt_req.url = url;
+ alt_req.url = &url;
alt_req.buffer = &buffer;
alt_req.http_specific = 1;
alt_req.slot = slot;
@@ -365,7 +366,7 @@ static void fetch_alternates(struct walker *walker, const
char *base)
cdata->got_alternates = -1;
strbuf_release(&buffer);
- free(url);
+ strbuf_release(&url);
}
static int fetch_indices(struct walker *walker, struct alt_base *repo)
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 47/68] write_loose_object: convert to strbuf
When creating a loose object tempfile, we use a fixed
PATH_MAX-sized buffer, and strcpy directly into it. This
isn't buggy, because we do a rough check of the size, but
there's no verification that our guesstimate of the required
space is enough (in fact, it's several bytes too big for the
current naming scheme).
Let's switch to a strbuf, which makes this much easier to
verify. The allocation overhead should be negligible, since
we are replacing a static buffer with a static strbuf, and
we'll only need to allocate on the first call.
While we're here, we can also document a subtle interaction
with mkstemp that would be easy to overlook.
Signed-off-by: Jeff King
---
sha1_file.c | 42 ++
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index c26fdcb..4211af1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3011,29 +3011,31 @@ static inline int directory_size(const char *filename)
* We want to avoid cross-directory filename renames, because those
* can have problems on various filesystems (FAT, NFS, Coda).
*/
-static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
+static int create_tmpfile(struct strbuf *tmp, const char *filename)
{
int fd, dirlen = directory_size(filename);
- if (dirlen + 20 > bufsiz) {
- errno = ENAMETOOLONG;
- return -1;
- }
- memcpy(buffer, filename, dirlen);
- strcpy(buffer + dirlen, "tmp_obj_XX");
- fd = git_mkstemp_mode(buffer, 0444);
+ strbuf_reset(tmp);
+ strbuf_add(tmp, filename, dirlen);
+ strbuf_addstr(tmp, "tmp_obj_XX");
+ fd = git_mkstemp_mode(tmp->buf, 0444);
if (fd < 0 && dirlen && errno == ENOENT) {
- /* Make sure the directory exists */
- memcpy(buffer, filename, dirlen);
- buffer[dirlen-1] = 0;
- if (mkdir(buffer, 0777) && errno != EEXIST)
+ /*
+* Make sure the directory exists; note that the contents
+* of the buffer are undefined after mkstemp returns an
+* error, so we have to rewrite the whole buffer from
+* scratch.
+*/
+ strbuf_reset(tmp);
+ strbuf_add(tmp, filename, dirlen - 1);
+ if (mkdir(tmp->buf, 0777) && errno != EEXIST)
return -1;
- if (adjust_shared_perm(buffer))
+ if (adjust_shared_perm(tmp->buf))
return -1;
/* Try again */
- strcpy(buffer + dirlen - 1, "/tmp_obj_XX");
- fd = git_mkstemp_mode(buffer, 0444);
+ strbuf_addstr(tmp, "/tmp_obj_XX");
+ fd = git_mkstemp_mode(tmp->buf, 0444);
}
return fd;
}
@@ -3046,10 +3048,10 @@ static int write_loose_object(const unsigned char
*sha1, char *hdr, int hdrlen,
git_zstream stream;
git_SHA_CTX c;
unsigned char parano_sha1[20];
- static char tmp_file[PATH_MAX];
+ static struct strbuf tmp_file = STRBUF_INIT;
const char *filename = sha1_file_name(sha1);
- fd = create_tmpfile(tmp_file, sizeof(tmp_file), filename);
+ fd = create_tmpfile(&tmp_file, filename);
if (fd < 0) {
if (errno == EACCES)
return error("insufficient permission for adding an
object to repository database %s", get_object_directory());
@@ -3098,12 +3100,12 @@ static int write_loose_object(const unsigned char
*sha1, char *hdr, int hdrlen,
struct utimbuf utb;
utb.actime = mtime;
utb.modtime = mtime;
- if (utime(tmp_file, &utb) < 0)
+ if (utime(tmp_file.buf, &utb) < 0)
warning("failed utime() on %s: %s",
- tmp_file, strerror(errno));
+ tmp_file.buf, strerror(errno));
}
- return finalize_object_file(tmp_file, filename);
+ return finalize_object_file(tmp_file.buf, filename);
}
static int freshen_loose_object(const unsigned char *sha1)
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 46/68] remove_leading_path: use a strbuf for internal storage
This function strcpy's directly into a PATH_MAX-sized
buffer. There's only one caller, which feeds the git_dir into
it, so it's not easy to trigger in practice (even if you fed
a large $GIT_DIR through the environment or .git file, it
would have to actually exist and be accessible on the
filesystem to get to this point). We can fix it by moving to
a strbuf.
Signed-off-by: Jeff King
---
path.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/path.c b/path.c
index 60e0390..c597473 100644
--- a/path.c
+++ b/path.c
@@ -632,7 +632,7 @@ const char *relative_path(const char *in, const char
*prefix,
*/
const char *remove_leading_path(const char *in, const char *prefix)
{
- static char buf[PATH_MAX + 1];
+ static struct strbuf buf = STRBUF_INIT;
int i = 0, j = 0;
if (!prefix || !prefix[0])
@@ -661,11 +661,13 @@ const char *remove_leading_path(const char *in, const
char *prefix)
return in;
while (is_dir_sep(in[j]))
j++;
+
+ strbuf_reset(&buf);
if (!in[j])
- strcpy(buf, ".");
+ strbuf_addstr(&buf, ".");
else
- strcpy(buf, in + j);
- return buf;
+ strbuf_addstr(&buf, in + j);
+ return buf.buf;
}
/*
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 48/68] diagnose_invalid_index_path: use strbuf to avoid strcpy/strcat
We dynamically allocate a buffer and then strcpy and strcat
into it. This isn't buggy, but we'd prefer to avoid these
suspicious functions.
This would be a good candidate for converstion to xstrfmt,
but we need to record the length for dealing with index
entries. A strbuf handles that for us.
Signed-off-by: Jeff King
---
sha1_name.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 80753b6..3242c5e 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1293,8 +1293,7 @@ static void diagnose_invalid_index_path(int stage,
const struct cache_entry *ce;
int pos;
unsigned namelen = strlen(filename);
- unsigned fullnamelen;
- char *fullname;
+ struct strbuf fullname = STRBUF_INIT;
if (!prefix)
prefix = "";
@@ -1314,21 +1313,19 @@ static void diagnose_invalid_index_path(int stage,
}
/* Confusion between relative and absolute filenames? */
- fullnamelen = namelen + strlen(prefix);
- fullname = xmalloc(fullnamelen + 1);
- strcpy(fullname, prefix);
- strcat(fullname, filename);
- pos = cache_name_pos(fullname, fullnamelen);
+ strbuf_addstr(&fullname, prefix);
+ strbuf_addstr(&fullname, filename);
+ pos = cache_name_pos(fullname.buf, fullname.len);
if (pos < 0)
pos = -pos - 1;
if (pos < active_nr) {
ce = active_cache[pos];
- if (ce_namelen(ce) == fullnamelen &&
- !memcmp(ce->name, fullname, fullnamelen))
+ if (ce_namelen(ce) == fullname.len &&
+ !memcmp(ce->name, fullname.buf, fullname.len))
die("Path '%s' is in the index, but not '%s'.\n"
"Did you mean ':%d:%s' aka ':%d:./%s'?",
- fullname, filename,
- ce_stage(ce), fullname,
+ fullname.buf, filename,
+ ce_stage(ce), fullname.buf,
ce_stage(ce), filename);
}
@@ -1338,7 +1335,7 @@ static void diagnose_invalid_index_path(int stage,
die("Path '%s' does not exist (neither on disk nor in the
index).",
filename);
- free(fullname);
+ strbuf_release(&fullname);
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 55/68] color: add overflow checks for parsing colors
Our color parsing is designed to never exceed COLOR_MAXLEN
bytes. But the relationship between that hand-computed
number and the parsing code is not at all obvious, and we
merely hope that it has been computed correctly for all
cases.
Let's mark the expected "end" pointer for the destination
buffer and make sure that we do not exceed it.
Signed-off-by: Jeff King
---
color.c | 41 ++---
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/color.c b/color.c
index 9027352..22782f8 100644
--- a/color.c
+++ b/color.c
@@ -150,22 +150,24 @@ int color_parse(const char *value, char *dst)
* already have the ANSI escape code in it. "out" should have enough
* space in it to fit any color.
*/
-static char *color_output(char *out, const struct color *c, char type)
+static char *color_output(char *out, int len, const struct color *c, char type)
{
switch (c->type) {
case COLOR_UNSPECIFIED:
case COLOR_NORMAL:
break;
case COLOR_ANSI:
+ if (len < 2)
+ die("BUG: color parsing ran out of space");
*out++ = type;
*out++ = '0' + c->value;
break;
case COLOR_256:
- out += sprintf(out, "%c8;5;%d", type, c->value);
+ out += xsnprintf(out, len, "%c8;5;%d", type, c->value);
break;
case COLOR_RGB:
- out += sprintf(out, "%c8;2;%d;%d;%d", type,
- c->red, c->green, c->blue);
+ out += xsnprintf(out, len, "%c8;2;%d;%d;%d", type,
+c->red, c->green, c->blue);
break;
}
return out;
@@ -180,12 +182,13 @@ int color_parse_mem(const char *value, int value_len,
char *dst)
{
const char *ptr = value;
int len = value_len;
+ char *end = dst + COLOR_MAXLEN;
unsigned int attr = 0;
struct color fg = { COLOR_UNSPECIFIED };
struct color bg = { COLOR_UNSPECIFIED };
if (!strncasecmp(value, "reset", len)) {
- strcpy(dst, GIT_COLOR_RESET);
+ xsnprintf(dst, end - dst, GIT_COLOR_RESET);
return 0;
}
@@ -224,12 +227,19 @@ int color_parse_mem(const char *value, int value_len,
char *dst)
goto bad;
}
+#undef OUT
+#define OUT(x) do { \
+ if (dst == end) \
+ die("BUG: color parsing ran out of space"); \
+ *dst++ = (x); \
+} while(0)
+
if (attr || !color_empty(&fg) || !color_empty(&bg)) {
int sep = 0;
int i;
- *dst++ = '\033';
- *dst++ = '[';
+ OUT('\033');
+ OUT('[');
for (i = 0; attr; i++) {
unsigned bit = (1 << i);
@@ -237,27 +247,28 @@ int color_parse_mem(const char *value, int value_len,
char *dst)
continue;
attr &= ~bit;
if (sep++)
- *dst++ = ';';
- dst += sprintf(dst, "%d", i);
+ OUT(';');
+ dst += xsnprintf(dst, end - dst, "%d", i);
}
if (!color_empty(&fg)) {
if (sep++)
- *dst++ = ';';
+ OUT(';');
/* foreground colors are all in the 3x range */
- dst = color_output(dst, &fg, '3');
+ dst = color_output(dst, end - dst, &fg, '3');
}
if (!color_empty(&bg)) {
if (sep++)
- *dst++ = ';';
+ OUT(';');
/* background colors are all in the 4x range */
- dst = color_output(dst, &bg, '4');
+ dst = color_output(dst, end - dst, &bg, '4');
}
- *dst++ = 'm';
+ OUT('m');
}
- *dst = 0;
+ OUT(0);
return 0;
bad:
return error(_("invalid color value: %.*s"), value_len, value);
+#undef OUT
}
int git_config_colorbool(const char *var, const char *value)
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 61/68] color: add color_set helper for copying raw colors
To set up default colors, we sometimes strcpy() from the
default string literals into our color buffers. This isn't a
bug (assuming the destination is COLOR_MAXLEN bytes), but
makes it harder to audit the code for problematic strcpy
calls.
Let's introduce a color_set which copies under the
assumption that there are COLOR_MAXLEN bytes in the
destination (of course you can call it on a smaller buffer,
so this isn't providing a huge amount of safety, but it's
more convenient than calling xsnprintf yourself).
Signed-off-by: Jeff King
---
color.c | 5 +
color.h | 7 +++
grep.c | 32
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/color.c b/color.c
index 22782f8..8f85153 100644
--- a/color.c
+++ b/color.c
@@ -145,6 +145,11 @@ int color_parse(const char *value, char *dst)
return color_parse_mem(value, strlen(value), dst);
}
+void color_set(char *dst, const char *color_bytes)
+{
+ xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
+}
+
/*
* Write the ANSI color codes for "c" to "out"; the string should
* already have the ANSI escape code in it. "out" should have enough
diff --git a/color.h b/color.h
index 7fe77fb..e155d13 100644
--- a/color.h
+++ b/color.h
@@ -75,6 +75,13 @@ extern int color_stdout_is_tty;
int git_color_config(const char *var, const char *value, void *cb);
int git_color_default_config(const char *var, const char *value, void *cb);
+/*
+ * Set the color buffer (which must be COLOR_MAXLEN bytes)
+ * to the raw color bytes; this is useful for initializing
+ * default color variables.
+ */
+void color_set(char *dst, const char *color_bytes);
+
int git_config_colorbool(const char *var, const char *value);
int want_color(int var);
int color_parse(const char *value, char *dst);
diff --git a/grep.c b/grep.c
index 6c68d5b..7b2b96a 100644
--- a/grep.c
+++ b/grep.c
@@ -31,14 +31,14 @@ void init_grep_defaults(void)
opt->max_depth = -1;
opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
opt->extended_regexp_option = 0;
- strcpy(opt->color_context, "");
- strcpy(opt->color_filename, "");
- strcpy(opt->color_function, "");
- strcpy(opt->color_lineno, "");
- strcpy(opt->color_match_context, GIT_COLOR_BOLD_RED);
- strcpy(opt->color_match_selected, GIT_COLOR_BOLD_RED);
- strcpy(opt->color_selected, "");
- strcpy(opt->color_sep, GIT_COLOR_CYAN);
+ color_set(opt->color_context, "");
+ color_set(opt->color_filename, "");
+ color_set(opt->color_function, "");
+ color_set(opt->color_lineno, "");
+ color_set(opt->color_match_context, GIT_COLOR_BOLD_RED);
+ color_set(opt->color_match_selected, GIT_COLOR_BOLD_RED);
+ color_set(opt->color_selected, "");
+ color_set(opt->color_sep, GIT_COLOR_CYAN);
opt->color = -1;
}
@@ -151,14 +151,14 @@ void grep_init(struct grep_opt *opt, const char *prefix)
opt->regflags = def->regflags;
opt->relative = def->relative;
- strcpy(opt->color_context, def->color_context);
- strcpy(opt->color_filename, def->color_filename);
- strcpy(opt->color_function, def->color_function);
- strcpy(opt->color_lineno, def->color_lineno);
- strcpy(opt->color_match_context, def->color_match_context);
- strcpy(opt->color_match_selected, def->color_match_selected);
- strcpy(opt->color_selected, def->color_selected);
- strcpy(opt->color_sep, def->color_sep);
+ color_set(opt->color_context, def->color_context);
+ color_set(opt->color_filename, def->color_filename);
+ color_set(opt->color_function, def->color_function);
+ color_set(opt->color_lineno, def->color_lineno);
+ color_set(opt->color_match_context, def->color_match_context);
+ color_set(opt->color_match_selected, def->color_match_selected);
+ color_set(opt->color_selected, def->color_selected);
+ color_set(opt->color_sep, def->color_sep);
}
void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct
grep_opt *opt)
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 60/68] prefer memcpy to strcpy
When we already know the length of a string (e.g., because
we just malloc'd to fit it), it's nicer to use memcpy than
strcpy, as it makes it more obvious that we are not going to
overflow the buffer (because the size we pass matches the
size in the allocation).
This also eliminates calls to strcpy, which make auditing
the code base harder.
Signed-off-by: Jeff King
---
compat/nedmalloc/nedmalloc.c | 5 +++--
fast-import.c| 5 +++--
revision.c | 2 +-
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
index 609ebba..a0a16eb 100644
--- a/compat/nedmalloc/nedmalloc.c
+++ b/compat/nedmalloc/nedmalloc.c
@@ -957,8 +957,9 @@ char *strdup(const char *s1)
{
char *s2 = 0;
if (s1) {
- s2 = malloc(strlen(s1) + 1);
- strcpy(s2, s1);
+ size_t len = strlen(s1) + 1;
+ s2 = malloc(len);
+ memcpy(s2, s1, len);
}
return s2;
}
diff --git a/fast-import.c b/fast-import.c
index 895c6b4..cf6d8bc 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -644,8 +644,9 @@ static void *pool_calloc(size_t count, size_t size)
static char *pool_strdup(const char *s)
{
- char *r = pool_alloc(strlen(s) + 1);
- strcpy(r, s);
+ size_t len = strlen(s) + 1;
+ char *r = pool_alloc(len);
+ memcpy(r, s, len);
return r;
}
diff --git a/revision.c b/revision.c
index af2a18e..2236463 100644
--- a/revision.c
+++ b/revision.c
@@ -38,7 +38,7 @@ char *path_name(const struct name_path *path, const char
*name)
}
n = xmalloc(len);
m = n + len - (nlen + 1);
- strcpy(m, name);
+ memcpy(m, name, nlen + 1);
for (p = path; p; p = p->up) {
if (p->elem_len) {
m -= p->elem_len + 1;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 24/68] http-push: replace strcat with xsnprintf
We account for these strcats in our initial allocation, but
the code is confusing to follow and verify. Let's remember
our original allocation length, and then xsnprintf can
verify that we don't exceed it.
Note that we can't just use xstrfmt here (which would be
even cleaner) because the code tries to grow the buffer only
when necessary.
Signed-off-by: Jeff King
---
http-push.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/http-push.c b/http-push.c
index 1f3788f..37baff8 100644
--- a/http-push.c
+++ b/http-push.c
@@ -786,21 +786,21 @@ xml_start_tag(void *userData, const char *name, const
char **atts)
{
struct xml_ctx *ctx = (struct xml_ctx *)userData;
const char *c = strchr(name, ':');
- int new_len;
+ int old_namelen, new_len;
if (c == NULL)
c = name;
else
c++;
- new_len = strlen(ctx->name) + strlen(c) + 2;
+ old_namelen = strlen(ctx->name);
+ new_len = old_namelen + strlen(c) + 2;
if (new_len > ctx->len) {
ctx->name = xrealloc(ctx->name, new_len);
ctx->len = new_len;
}
- strcat(ctx->name, ".");
- strcat(ctx->name, c);
+ xsnprintf(ctx->name + old_namelen, ctx->len - old_namelen, ".%s", c);
free(ctx->cdata);
ctx->cdata = NULL;
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 66/68] fsck: use for_each_loose_file_in_objdir
Since 27e1e22 (prune: factor out loose-object directory
traversal, 2014-10-15), we now have a generic callback
system for iterating over the loose object directories. This
is used by prune, count-objects, etc.
We did not convert git-fsck at the time because it
implemented an inode-sorting scheme that was not part of the
generic code. Now that the inode-sorting code is gone, we
can reuse the generic code. The result is shorter,
hopefully more readable, and drops some unchecked sprintf
calls.
Signed-off-by: Jeff King
---
builtin/fsck.c | 69 --
1 file changed, 23 insertions(+), 46 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 73c3596..2fe6a31 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -365,45 +365,6 @@ static int fsck_obj_buffer(const unsigned char *sha1, enum
object_type type,
return fsck_obj(obj);
}
-static inline int is_loose_object_file(struct dirent *de,
- char *name, unsigned char *sha1)
-{
- if (strlen(de->d_name) != 38)
- return 0;
- memcpy(name + 2, de->d_name, 39);
- return !get_sha1_hex(name, sha1);
-}
-
-static void fsck_dir(int i, char *path)
-{
- DIR *dir = opendir(path);
- struct dirent *de;
- char name[100];
-
- if (!dir)
- return;
-
- if (verbose)
- fprintf(stderr, "Checking directory %s\n", path);
-
- sprintf(name, "%02x", i);
- while ((de = readdir(dir)) != NULL) {
- unsigned char sha1[20];
-
- if (is_dot_or_dotdot(de->d_name))
- continue;
- if (is_loose_object_file(de, name, sha1)) {
- if (fsck_sha1(sha1))
- errors_found |= ERROR_OBJECT;
- continue;
- }
- if (starts_with(de->d_name, "tmp_obj_"))
- continue;
- fprintf(stderr, "bad sha1 file: %s/%s\n", path, de->d_name);
- }
- closedir(dir);
-}
-
static int default_refs;
static void fsck_handle_reflog_sha1(const char *refname, unsigned char *sha1)
@@ -491,9 +452,28 @@ static void get_default_heads(void)
}
}
+static int fsck_loose(const unsigned char *sha1, const char *path, void *data)
+{
+ if (fsck_sha1(sha1))
+ errors_found |= ERROR_OBJECT;
+ return 0;
+}
+
+static int fsck_cruft(const char *basename, const char *path, void *data)
+{
+ if (!starts_with(basename, "tmp_obj_"))
+ fprintf(stderr, "bad sha1 file: %s\n", path);
+ return 0;
+}
+
+static int fsck_subdir(int nr, const char *path, void *progress)
+{
+ display_progress(progress, nr + 1);
+ return 0;
+}
+
static void fsck_object_dir(const char *path)
{
- int i;
struct progress *progress = NULL;
if (verbose)
@@ -501,12 +481,9 @@ static void fsck_object_dir(const char *path)
if (show_progress)
progress = start_progress(_("Checking object directories"),
256);
- for (i = 0; i < 256; i++) {
- static char dir[4096];
- sprintf(dir, "%s/%02x", path, i);
- fsck_dir(i, dir);
- display_progress(progress, i+1);
- }
+
+ for_each_loose_file_in_objdir(path, fsck_loose, fsck_cruft, fsck_subdir,
+ progress);
stop_progress(&progress);
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 68/68] name-rev: use strip_suffix to avoid magic numbers
The manual size computations here are correct, but using
strip_suffix makes that obvious, and hopefully communicates
the intent of the code more clearly.
Signed-off-by: Jeff King
---
builtin/name-rev.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 8a3a0cd..0377fc1 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -55,16 +55,15 @@ copy_data:
parents;
parents = parents->next, parent_number++) {
if (parent_number > 1) {
- int len = strlen(tip_name);
+ size_t len;
char *new_name;
- if (len > 2 && !strcmp(tip_name + len - 2, "^0"))
- len -= 2;
+ strip_suffix(tip_name, "^0", &len);
if (generation > 0)
- new_name = xstrfmt("%.*s~%d^%d", len, tip_name,
+ new_name = xstrfmt("%.*s~%d^%d", (int)len,
tip_name,
generation, parent_number);
else
- new_name = xstrfmt("%.*s^%d", len, tip_name,
+ new_name = xstrfmt("%.*s^%d", (int)len,
tip_name,
parent_number);
name_rev(parents->item, new_name, 0,
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 52/68] daemon: use cld->env_array when re-spawning
This avoids an ugly strcat into a fixed-size buffer. It's
not wrong (the buffer is plenty large enough for an IPv6
address plus some minor formatting), but it takes some
effort to verify that.
Unfortunately we are still stuck with some fixed-size
buffers to hold the output of inet_ntop. But at least we now
pass very easy-to-verify parameters, rather than doing a
manual computation to account for other data in the buffer.
As a side effect, this also fixes the case where we might
pass an uninitialized portbuf buffer through the
environment. This probably couldn't happen in practice, as
it would mean that addr->sa_family was neither AF_INET nor
AF_INET6 (and that is all we are listening on).
Signed-off-by: Jeff King
---
daemon.c | 26 ++
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/daemon.c b/daemon.c
index 5218a3f..56679a1 100644
--- a/daemon.c
+++ b/daemon.c
@@ -811,8 +811,6 @@ static char **cld_argv;
static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
{
struct child_process cld = CHILD_PROCESS_INIT;
- char addrbuf[300] = "REMOTE_ADDR=", portbuf[300];
- char *env[] = { addrbuf, portbuf, NULL };
if (max_connections && live_children >= max_connections) {
kill_some_child();
@@ -826,27 +824,23 @@ static void handle(int incoming, struct sockaddr *addr,
socklen_t addrlen)
}
if (addr->sa_family == AF_INET) {
+ char buf[128] = "";
struct sockaddr_in *sin_addr = (void *) addr;
- inet_ntop(addr->sa_family, &sin_addr->sin_addr, addrbuf + 12,
- sizeof(addrbuf) - 12);
- snprintf(portbuf, sizeof(portbuf), "REMOTE_PORT=%d",
- ntohs(sin_addr->sin_port));
+ inet_ntop(addr->sa_family, &sin_addr->sin_addr, buf,
sizeof(buf));
+ argv_array_pushf(&cld.env_array, "REMOTE_ADDR=%s", buf);
+ argv_array_pushf(&cld.env_array, "REMOTE_PORT=%d",
+ntohs(sin_addr->sin_port));
#ifndef NO_IPV6
} else if (addr->sa_family == AF_INET6) {
+ char buf[128] = "";
struct sockaddr_in6 *sin6_addr = (void *) addr;
-
- char *buf = addrbuf + 12;
- *buf++ = '['; *buf = '\0'; /* stpcpy() is cool */
- inet_ntop(AF_INET6, &sin6_addr->sin6_addr, buf,
- sizeof(addrbuf) - 13);
- strcat(buf, "]");
-
- snprintf(portbuf, sizeof(portbuf), "REMOTE_PORT=%d",
- ntohs(sin6_addr->sin6_port));
+ inet_ntop(AF_INET6, &sin6_addr->sin6_addr, buf, sizeof(buf));
+ argv_array_pushf(&cld.env_array, "REMOTE_ADDR=[%s]", buf);
+ argv_array_pushf(&cld.env_array, "REMOTE_PORT=%d",
+ntohs(sin6_addr->sin6_port));
#endif
}
- cld.env = (const char **)env;
cld.argv = (const char **)cld_argv;
cld.in = incoming;
cld.out = dup(incoming);
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 44/68] merge-recursive: convert malloc / strcpy to strbuf
This would be a fairly routine use of xstrfmt, except that
we need to remember the length of the result to pass to
cache_name_pos. So just use a strbuf, which makes this
simple.
As a bonus, this gets rid of confusing references to
"pathlen+1". The "1" is for the trailing slash we added, but
that is automatically accounted for in the strbuf's len
parameter.
Signed-off-by: Jeff King
---
merge-recursive.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 44d85be..a5e74d8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -630,25 +630,24 @@ static char *unique_path(struct merge_options *o, const
char *path, const char *
static int dir_in_way(const char *path, int check_working_copy)
{
- int pos, pathlen = strlen(path);
- char *dirpath = xmalloc(pathlen + 2);
+ int pos;
+ struct strbuf dirpath = STRBUF_INIT;
struct stat st;
- strcpy(dirpath, path);
- dirpath[pathlen] = '/';
- dirpath[pathlen+1] = '\0';
+ strbuf_addstr(&dirpath, path);
+ strbuf_addch(&dirpath, '/');
- pos = cache_name_pos(dirpath, pathlen+1);
+ pos = cache_name_pos(dirpath.buf, dirpath.len);
if (pos < 0)
pos = -1 - pos;
if (pos < active_nr &&
- !strncmp(dirpath, active_cache[pos]->name, pathlen+1)) {
- free(dirpath);
+ !strncmp(dirpath.buf, active_cache[pos]->name, dirpath.len)) {
+ strbuf_release(&dirpath);
return 1;
}
- free(dirpath);
+ strbuf_release(&dirpath);
return check_working_copy && !lstat(path, &st) && S_ISDIR(st.st_mode);
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 41/68] init: use strbufs to store paths
The init code predates strbufs, and uses PATH_MAX-sized
buffers along with many manual checks on intermediate sizes
(some of which make magic assumptions, such as that init
will not create a path inside .git longer than 50
characters).
We can simplify this greatly by using strbufs, which drops
some hard-to-verify strcpy calls. Note that we need to
update probe_utf8_pathname_composition, too, as it assumes
we are passing a buffer large enough to append its probe
filenames (it now just takes a strbuf, which also gets rid
of the confusing "len" parameter, which was not the length of
"path" but rather the offset to start writing).
Some of the conversion makes new calls to git_path_buf.
While we're in the area, let's also convert existing calls
to git_path to the safer git_path_buf (our existing calls
were passed to pretty tame functions, and so were not a
problem, but it's easy to be consistent and safe here).
Note that we had an explicit test that "git init" rejects
long template directories. This comes from 32d1776 (init: Do
not segfault on big GIT_TEMPLATE_DIR environment variable,
2009-04-18). We can drop the test_must_fail here, as we now
accept this and need only confirm that we don't segfault,
which was the original point of the test.
Signed-off-by: Jeff King
---
builtin/init-db.c| 174 ---
compat/precompose_utf8.c | 12 ++--
compat/precompose_utf8.h | 2 +-
git-compat-util.h| 2 +-
t/t0001-init.sh | 4 +-
5 files changed, 87 insertions(+), 107 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index e7d0e31..cf6a3c8 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -36,10 +36,11 @@ static void safe_create_dir(const char *dir, int share)
die(_("Could not make %s writable by group"), dir);
}
-static void copy_templates_1(char *path, int baselen,
-char *template, int template_baselen,
+static void copy_templates_1(struct strbuf *path, struct strbuf *template,
DIR *dir)
{
+ size_t path_baselen = path->len;
+ size_t template_baselen = template->len;
struct dirent *de;
/* Note: if ".git/hooks" file exists in the repository being
@@ -49,77 +50,64 @@ static void copy_templates_1(char *path, int baselen,
* with the way the namespace under .git/ is organized, should
* be really carefully chosen.
*/
- safe_create_dir(path, 1);
+ safe_create_dir(path->buf, 1);
while ((de = readdir(dir)) != NULL) {
struct stat st_git, st_template;
- int namelen;
int exists = 0;
+ strbuf_setlen(path, path_baselen);
+ strbuf_setlen(template, template_baselen);
+
if (de->d_name[0] == '.')
continue;
- namelen = strlen(de->d_name);
- if ((PATH_MAX <= baselen + namelen) ||
- (PATH_MAX <= template_baselen + namelen))
- die(_("insanely long template name %s"), de->d_name);
- memcpy(path + baselen, de->d_name, namelen+1);
- memcpy(template + template_baselen, de->d_name, namelen+1);
- if (lstat(path, &st_git)) {
+ strbuf_addstr(path, de->d_name);
+ strbuf_addstr(template, de->d_name);
+ if (lstat(path->buf, &st_git)) {
if (errno != ENOENT)
- die_errno(_("cannot stat '%s'"), path);
+ die_errno(_("cannot stat '%s'"), path->buf);
}
else
exists = 1;
- if (lstat(template, &st_template))
- die_errno(_("cannot stat template '%s'"), template);
+ if (lstat(template->buf, &st_template))
+ die_errno(_("cannot stat template '%s'"),
template->buf);
if (S_ISDIR(st_template.st_mode)) {
- DIR *subdir = opendir(template);
- int baselen_sub = baselen + namelen;
- int template_baselen_sub = template_baselen + namelen;
+ DIR *subdir = opendir(template->buf);
if (!subdir)
- die_errno(_("cannot opendir '%s'"), template);
- path[baselen_sub++] =
- template[template_baselen_sub++] = '/';
- path[baselen_sub] =
- template[template_baselen_sub] = 0;
- copy_templates_1(path, baselen_sub,
-template, template_baselen_sub,
-subdir);
+ die_errno(_("cannot opendir '%s'"),
template->buf);
+ strbuf_addch(path, '/');
+
[PATCH 37/68] remote-ext: simplify git pkt-line generation
We format a pkt-line into a heap buffer, which requires
manual computation of the required size, and uses some bare
sprintf calls. We could use a strbuf instead, which would
take care of the computation for us. But it's even easier
still to use packet_write(). Besides handling the formatting
and writing for us, it fixes two things:
1. Our manual max-size check used 0x, while technically
LARGE_PACKET_MAX is slightly smaller than this.
2. Our packet will now be output as part of
GIT_TRACE_PACKET debugging.
Unfortunately packet_write() does not let us build up the
buffer progressively, so we do have to repeat ourselves a
little depending on the "vhost" setting, but the end result
is still far more readable than the original.
Since there were no tests covering this feature at all,
we'll add a few into t5802.
Signed-off-by: Jeff King
---
builtin/remote-ext.c | 34 +-
t/t5802-connect-helper.sh | 28
2 files changed, 33 insertions(+), 29 deletions(-)
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 3b8c22c..e3cd25d 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -1,6 +1,7 @@
#include "builtin.h"
#include "transport.h"
#include "run-command.h"
+#include "pkt-line.h"
/*
* URL syntax:
@@ -142,36 +143,11 @@ static const char **parse_argv(const char *arg, const
char *service)
static void send_git_request(int stdin_fd, const char *serv, const char *repo,
const char *vhost)
{
- size_t bufferspace;
- size_t wpos = 0;
- char *buffer;
-
- /*
-* Request needs 12 bytes extra if there is vhost ( \0host=\0) and
-* 6 bytes extra ( \0) if there is no vhost.
-*/
- if (vhost)
- bufferspace = strlen(serv) + strlen(repo) + strlen(vhost) + 12;
+ if (!vhost)
+ packet_write(stdin_fd, "%s %s%c", serv, repo, 0);
else
- bufferspace = strlen(serv) + strlen(repo) + 6;
-
- if (bufferspace > 0x)
- die("Request too large to send");
- buffer = xmalloc(bufferspace);
-
- /* Make the packet. */
- wpos = sprintf(buffer, "%04x%s %s%c", (unsigned)bufferspace,
- serv, repo, 0);
-
- /* Add vhost if any. */
- if (vhost)
- sprintf(buffer + wpos, "host=%s%c", vhost, 0);
-
- /* Send the request */
- if (write_in_full(stdin_fd, buffer, bufferspace) < 0)
- die_errno("Failed to send request");
-
- free(buffer);
+ packet_write(stdin_fd, "%s %s%chost=%s%c", serv, repo, 0,
+vhost, 0);
}
static int run_child(const char *arg, const char *service)
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index 878faf2..b7a7f9d 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -69,4 +69,32 @@ test_expect_success 'update backfilled tag without primary
transfer' '
test_cmp expect actual
'
+
+test_expect_success 'set up fake git-daemon' '
+ mkdir remote &&
+ git init --bare remote/one.git &&
+ mkdir remote/host &&
+ git init --bare remote/host/two.git &&
+ write_script fake-daemon <<-\EOF &&
+ git daemon --inetd \
+ --informative-errors \
+ --export-all \
+ --base-path="$TRASH_DIRECTORY/remote" \
+ --interpolated-path="$TRASH_DIRECTORY/remote/%H%D" \
+ "$TRASH_DIRECTORY/remote"
+ EOF
+ export TRASH_DIRECTORY &&
+ PATH=$TRASH_DIRECTORY:$PATH
+'
+
+test_expect_success 'ext command can connect to git daemon (no vhost)' '
+ rm -rf dst &&
+ git clone "ext::fake-daemon %G/one.git" dst
+'
+
+test_expect_success 'ext command can connect to git daemon (vhost)' '
+ rm -rf dst &&
+ git clone "ext::fake-daemon %G/two.git %Vhost" dst
+'
+
test_done
--
2.6.0.rc3.454.g204ad51
--
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
Fwd: diff not finding difference
This is a weird one: [file-1 begin] abcd efg hijklmnop [file-1 end] [file-2 begin] blah blah blah / abdc boo ya! [file-2 end] Do a diff between these and it won't find any difference. Same with the following two lines, each in a different file: sabc fed ghi jkl abc def ghi jkl I first noticed this on the command line git and then in VS2013. The original problem was like my first example. The files were much longer, but all that git would see is the addition of the line of ..., but not the removal of the original line. I've tried some other simple file changes with similar results. Something seems to be definitely broken in git diff. :( Command line version of git is 1.9.5 msysgit.0. A -- 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
[PATCH 30/68] ref-filter: drop sprintf and strcpy calls
The ref-filter code comes from for-each-ref, and inherited a
number of raw sprintf and strcpy calls. These are generally
all safe, as we custom-size the buffers, or are formatting
numbers into sufficiently large buffers. But we can make the
resulting code even simpler and more obviously correct by
using some of our helper functions.
Signed-off-by: Jeff King
---
ref-filter.c | 70 +++-
1 file changed, 22 insertions(+), 48 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index f38dee4..1f71870 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -192,9 +192,7 @@ static int grab_objectname(const char *name, const unsigned
char *sha1,
struct atom_value *v)
{
if (!strcmp(name, "objectname")) {
- char *s = xmalloc(41);
- strcpy(s, sha1_to_hex(sha1));
- v->s = s;
+ v->s = xstrdup(sha1_to_hex(sha1));
return 1;
}
if (!strcmp(name, "objectname:short")) {
@@ -219,10 +217,8 @@ static void grab_common_values(struct atom_value *val, int
deref, struct object
if (!strcmp(name, "objecttype"))
v->s = typename(obj->type);
else if (!strcmp(name, "objectsize")) {
- char *s = xmalloc(40);
- sprintf(s, "%lu", sz);
v->ul = sz;
- v->s = s;
+ v->s = xstrfmt("%lu", sz);
}
else if (deref)
grab_objectname(name, obj->sha1, v);
@@ -246,11 +242,8 @@ static void grab_tag_values(struct atom_value *val, int
deref, struct object *ob
v->s = tag->tag;
else if (!strcmp(name, "type") && tag->tagged)
v->s = typename(tag->tagged->type);
- else if (!strcmp(name, "object") && tag->tagged) {
- char *s = xmalloc(41);
- strcpy(s, sha1_to_hex(tag->tagged->sha1));
- v->s = s;
- }
+ else if (!strcmp(name, "object") && tag->tagged)
+ v->s = xstrdup(sha1_to_hex(tag->tagged->sha1));
}
}
@@ -268,32 +261,22 @@ static void grab_commit_values(struct atom_value *val,
int deref, struct object
if (deref)
name++;
if (!strcmp(name, "tree")) {
- char *s = xmalloc(41);
- strcpy(s, sha1_to_hex(commit->tree->object.sha1));
- v->s = s;
+ v->s = xstrdup(sha1_to_hex(commit->tree->object.sha1));
}
- if (!strcmp(name, "numparent")) {
- char *s = xmalloc(40);
+ else if (!strcmp(name, "numparent")) {
v->ul = commit_list_count(commit->parents);
- sprintf(s, "%lu", v->ul);
- v->s = s;
+ v->s = xstrfmt("%lu", v->ul);
}
else if (!strcmp(name, "parent")) {
- int num = commit_list_count(commit->parents);
- int i;
struct commit_list *parents;
- char *s = xmalloc(41 * num + 1);
- v->s = s;
- for (i = 0, parents = commit->parents;
-parents;
-parents = parents->next, i = i + 41) {
+ struct strbuf s = STRBUF_INIT;
+ for (parents = commit->parents; parents; parents =
parents->next) {
struct commit *parent = parents->item;
- strcpy(s+i, sha1_to_hex(parent->object.sha1));
- if (parents->next)
- s[i+40] = ' ';
+ if (parents != commit->parents)
+ strbuf_addch(&s, ' ');
+ strbuf_addstr(&s,
sha1_to_hex(parent->object.sha1));
}
- if (!i)
- *s = '\0';
+ v->s = strbuf_detach(&s, NULL);
}
}
}
@@ -700,7 +683,6 @@ static void populate_value(struct ref_array_item *ref)
else if (!strcmp(formatp, "track") &&
(starts_with(name, "upstream") ||
starts_with(name, "push"))) {
- char buf[40];
if (stat_tracking_info(branch, &num_ours,
&num_theirs, NULL))
@@ -708,17 +690,13 @@ static void populate_value(struct ref_array_item *ref)
if (!num_ours && !num_theirs)
[PATCH 31/68] help: drop prepend function in favor of xstrfmt
This function predates xstrfmt, and its functionality is a
subset. Let's just use xstrfmt.
Signed-off-by: Jeff King
---
builtin/help.c | 14 ++
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/builtin/help.c b/builtin/help.c
index 3422e73..fba8c01 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -295,16 +295,6 @@ static int is_git_command(const char *s)
is_in_cmdlist(&other_cmds, s);
}
-static const char *prepend(const char *prefix, const char *cmd)
-{
- size_t pre_len = strlen(prefix);
- size_t cmd_len = strlen(cmd);
- char *p = xmalloc(pre_len + cmd_len + 1);
- memcpy(p, prefix, pre_len);
- strcpy(p + pre_len, cmd);
- return p;
-}
-
static const char *cmd_to_page(const char *git_cmd)
{
if (!git_cmd)
@@ -312,9 +302,9 @@ static const char *cmd_to_page(const char *git_cmd)
else if (starts_with(git_cmd, "git"))
return git_cmd;
else if (is_git_command(git_cmd))
- return prepend("git-", git_cmd);
+ return xstrfmt("git-%s", git_cmd);
else
- return prepend("git", git_cmd);
+ return xstrfmt("git%s", git_cmd);
}
static void setup_man_path(void)
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 64/68] fsck: drop inode-sorting code
Fsck tries to access loose objects in order of inode number,
with the hope that this would make cold cache access faster
on a spinning disk. This dates back to 7e8c174 (fsck-cache:
sort entries by inode number, 2005-05-02), which predates
the invention of packfiles.
These days, there's not much point in trying to optimize
cold cache for a large number of loose objects. You are much
better off to simply pack the objects, which will reduce the
disk footprint _and_ provide better locality of data access.
So while you can certainly construct pathological cases
where this code might help, it is not worth the trouble
anymore.
Signed-off-by: Jeff King
---
builtin/fsck.c | 70 ++
1 file changed, 2 insertions(+), 68 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index a019f4a..73c3596 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -39,14 +39,6 @@ static int show_dangling = 1;
#define ERROR_REACHABLE 02
#define ERROR_PACK 04
-#ifdef NO_D_INO_IN_DIRENT
-#define SORT_DIRENT 0
-#define DIRENT_SORT_HINT(de) 0
-#else
-#define SORT_DIRENT 1
-#define DIRENT_SORT_HINT(de) ((de)->d_ino)
-#endif
-
static int fsck_config(const char *var, const char *value, void *cb)
{
if (strcmp(var, "fsck.skiplist") == 0) {
@@ -373,64 +365,6 @@ static int fsck_obj_buffer(const unsigned char *sha1, enum
object_type type,
return fsck_obj(obj);
}
-/*
- * This is the sorting chunk size: make it reasonably
- * big so that we can sort well..
- */
-#define MAX_SHA1_ENTRIES (1024)
-
-struct sha1_entry {
- unsigned long ino;
- unsigned char sha1[20];
-};
-
-static struct {
- unsigned long nr;
- struct sha1_entry *entry[MAX_SHA1_ENTRIES];
-} sha1_list;
-
-static int ino_compare(const void *_a, const void *_b)
-{
- const struct sha1_entry *a = _a, *b = _b;
- unsigned long ino1 = a->ino, ino2 = b->ino;
- return ino1 < ino2 ? -1 : ino1 > ino2 ? 1 : 0;
-}
-
-static void fsck_sha1_list(void)
-{
- int i, nr = sha1_list.nr;
-
- if (SORT_DIRENT)
- qsort(sha1_list.entry, nr,
- sizeof(struct sha1_entry *), ino_compare);
- for (i = 0; i < nr; i++) {
- struct sha1_entry *entry = sha1_list.entry[i];
- unsigned char *sha1 = entry->sha1;
-
- sha1_list.entry[i] = NULL;
- if (fsck_sha1(sha1))
- errors_found |= ERROR_OBJECT;
- free(entry);
- }
- sha1_list.nr = 0;
-}
-
-static void add_sha1_list(unsigned char *sha1, unsigned long ino)
-{
- struct sha1_entry *entry = xmalloc(sizeof(*entry));
- int nr;
-
- entry->ino = ino;
- hashcpy(entry->sha1, sha1);
- nr = sha1_list.nr;
- if (nr == MAX_SHA1_ENTRIES) {
- fsck_sha1_list();
- nr = 0;
- }
- sha1_list.entry[nr] = entry;
- sha1_list.nr = ++nr;
-}
-
static inline int is_loose_object_file(struct dirent *de,
char *name, unsigned char *sha1)
{
@@ -459,7 +393,8 @@ static void fsck_dir(int i, char *path)
if (is_dot_or_dotdot(de->d_name))
continue;
if (is_loose_object_file(de, name, sha1)) {
- add_sha1_list(sha1, DIRENT_SORT_HINT(de));
+ if (fsck_sha1(sha1))
+ errors_found |= ERROR_OBJECT;
continue;
}
if (starts_with(de->d_name, "tmp_obj_"))
@@ -573,7 +508,6 @@ static void fsck_object_dir(const char *path)
display_progress(progress, i+1);
}
stop_progress(&progress);
- fsck_sha1_list();
}
static int fsck_head_link(void)
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 34/68] read_remotes_file: simplify string handling
The main motivation for this cleanup is to switch our
line-reading to a strbuf, which removes the use of a
fixed-size buffer (which limited the size of remote URLs).
Since we have the strbuf, we can make use of strbuf_rtrim().
While we're here, we can also simplify the parsing of each
line. First, we can use skip_prefix() to avoid some magic
numbers.
But second, we can avoid splitting the parsing and actions
for each line into two stages. Right now we figure out which
type of line we have, set an int to a magic number,
skip any intermediate whitespace, and then act on
the resulting value based on the magic number.
Instead, let's factor the whitespace skipping into a
function. That lets us avoid the magic numbers and keep the
actions close to the parsing.
Signed-off-by: Jeff King
---
remote.c | 55 ++-
1 file changed, 18 insertions(+), 37 deletions(-)
diff --git a/remote.c b/remote.c
index 22a60fc..a01f13a 100644
--- a/remote.c
+++ b/remote.c
@@ -54,9 +54,6 @@ static const char *pushremote_name;
static struct rewrites rewrites;
static struct rewrites rewrites_push;
-#define BUF_SIZE (2048)
-static char buffer[BUF_SIZE];
-
static int valid_remote(const struct remote *remote)
{
return (!!remote->url) || (!!remote->foreign_vcs);
@@ -243,50 +240,34 @@ static void add_instead_of(struct rewrite *rewrite, const
char *instead_of)
rewrite->instead_of_nr++;
}
+static const char *skip_spaces(const char *s)
+{
+ while (isspace(*s))
+ s++;
+ return s;
+}
+
static void read_remotes_file(struct remote *remote)
{
+ struct strbuf buf = STRBUF_INIT;
FILE *f = fopen(git_path("remotes/%s", remote->name), "r");
if (!f)
return;
remote->origin = REMOTE_REMOTES;
- while (fgets(buffer, BUF_SIZE, f)) {
- int value_list;
- char *s, *p;
-
- if (starts_with(buffer, "URL:")) {
- value_list = 0;
- s = buffer + 4;
- } else if (starts_with(buffer, "Push:")) {
- value_list = 1;
- s = buffer + 5;
- } else if (starts_with(buffer, "Pull:")) {
- value_list = 2;
- s = buffer + 5;
- } else
- continue;
-
- while (isspace(*s))
- s++;
- if (!*s)
- continue;
+ while (strbuf_getline(&buf, f, '\n') != EOF) {
+ const char *v;
- p = s + strlen(s);
- while (isspace(p[-1]))
- *--p = 0;
+ strbuf_rtrim(&buf);
- switch (value_list) {
- case 0:
- add_url_alias(remote, xstrdup(s));
- break;
- case 1:
- add_push_refspec(remote, xstrdup(s));
- break;
- case 2:
- add_fetch_refspec(remote, xstrdup(s));
- break;
- }
+ if (skip_prefix(buf.buf, "URL:", &v))
+ add_url_alias(remote, xstrdup(skip_spaces(v)));
+ else if (skip_prefix(buf.buf, "Push:", &v))
+ add_push_refspec(remote, xstrdup(skip_spaces(v)));
+ else if (skip_prefix(buf.buf, "Pull:", &v))
+ add_fetch_refspec(remote, xstrdup(skip_spaces(v)));
}
+ strbuf_release(&buf);
fclose(f);
}
--
2.6.0.rc3.454.g204ad51
--
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
[PATCH 36/68] upload-archive: convert sprintf to strbuf
When we report an error to the client, we format it into a
fixed-size buffer using vsprintf(). This can't actually
overflow in practice, since we only format a very tame
subset of strings (mostly strerror() output). However, it's
hard to tell immediately, so let's just use a strbuf so
readers do not have to wonder.
We do add an allocation here, but the performance is not
important; the next step is to call die() anyway.
Signed-off-by: Jeff King
---
builtin/upload-archive.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 32ab94c..dbfe14f 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -49,15 +49,14 @@ int cmd_upload_archive_writer(int argc, const char **argv,
const char *prefix)
__attribute__((format (printf, 1, 2)))
static void error_clnt(const char *fmt, ...)
{
- char buf[1024];
+ struct strbuf buf = STRBUF_INIT;
va_list params;
- int len;
va_start(params, fmt);
- len = vsprintf(buf, fmt, params);
+ strbuf_vaddf(&buf, fmt, params);
va_end(params);
- send_sideband(1, 3, buf, len, LARGE_PACKET_MAX);
- die("sent error to the client: %s", buf);
+ send_sideband(1, 3, buf.buf, buf.len, LARGE_PACKET_MAX);
+ die("sent error to the client: %s", buf.buf);
}
static ssize_t process_input(int child_fd, int band)
--
2.6.0.rc3.454.g204ad51
--
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: diff not finding difference
On Thu, Sep 24, 2015 at 5:09 PM, Jack Adrian Zappa wrote: > This is a weird one: > > [file-1 begin] > > abcd efg hijklmnop > > [file-1 end] > > [file-2 begin] > > blah blah blah > / > abdc boo ya! > > [file-2 end] > > Do a diff between these and it won't find any difference. > > Same with the following two lines, each in a different file: > sabc fed ghi jkl > abc def ghi jkl > > I first noticed this on the command line git and then in VS2013. The > original problem was like my first example. The files were much > longer, but all that git would see is the addition of the line of > ..., but not the removal of the original line. > > I've tried some other simple file changes with similar results. > Something seems to be definitely broken in git diff. :( > > Command line version of git is 1.9.5 msysgit.0. > > > A BTW, I've upgraded to git version 2.5.3.windows.1 and the problem still persists. A -- 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
[PATCH 0/2] Another squash on run-command: add an asynchronous parallel child processor
Here is another proposal for squashing. It's basically both squash proposals by Junio, plus another issues I think should be done right from the beginning. [added by sb:] * If you do not die() in start_failure_fn or return_value_fn, you don't want to write to stderr directly as you would destroy the fine ordering of the processes output. So make the err strbuf available in both these functions, and make sure the strbuf is appended to the buffered output in both cases. Stefan Beller (2): SQUASH??? SQUASH for "fetch_populated_submodules: use new parallel job processing" -- 2.5.0.273.g6fa2560.dirty -- 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
[PATCH 2/2] SQUASH for "fetch_populated_submodules: use new parallel job processing"
This fixes the function signature in the first user of the async run processor.
Signed-off-by: Stefan Beller
---
submodule.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/submodule.c b/submodule.c
index d7c7a6e..2c4396b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -630,13 +630,15 @@ struct submodule_parallel_fetch {
int get_next_submodule(void *data, struct child_process *cp,
struct strbuf *err);
-void handle_submodule_fetch_start_err(void *data, struct child_process *cp,
struct strbuf *err)
+void handle_submodule_fetch_start_err(void *data, struct child_process *cp,
+ struct strbuf *err)
{
struct submodule_parallel_fetch *spf = data;
spf->result = 1;
}
-void handle_submodule_fetch_finish( void *data, struct child_process *cp, int
retvalue)
+void handle_submodule_fetch_finish(void *data, struct child_process *cp,
+ struct strbuf *err, int retvalue)
{
struct submodule_parallel_fetch *spf = data;
--
2.5.0.273.g6fa2560.dirty
--
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
[PATCH 1/2] SQUASH???
This is a mixed bag of squashes.
* pp_start_one() stuffed unborn child to the list of children when
start_command() failed and start_failure() did not die(); return
to the caller without corrupting children[] list in this case.
* make poll(2) timeout used in pp_buffer_stderr() configurable by
the scheduling loop.
* allow slow-start of the whole process, so that we do not spawn
tons of processes before starting to read from any of them, to
give a better first-byte latency.
* fix the semantics of the value returned from pp_start_one() and
adjust the scheduling loop for it.
[added by sb:]
* If you do not die() in start_failure_fn or return_value_fn, you
don't want to write to stderr directly as you would destroy the fine
ordering of the processes output. So make the err strbuf available in
both these functions, and make sure the strbuf is appended to the
buffered output in both cases
Signed-off-by: Junio C Hamano
Signed-off-by: Stefan Beller
---
run-command.c | 43 ++-
run-command.h | 1 +
2 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/run-command.c b/run-command.c
index 494e1f8..0d22291 100644
--- a/run-command.c
+++ b/run-command.c
@@ -907,6 +907,7 @@ void default_start_failure(void *data,
void default_return_value(void *data,
struct child_process *cp,
+ struct strbuf *err,
int result)
{
int i;
@@ -977,7 +978,7 @@ static void set_nonblocking(int fd)
"output will be degraded");
}
-/* returns 1 if a process was started, 0 otherwise */
+/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */
static int pp_start_one(struct parallel_processes *pp)
{
int i;
@@ -991,26 +992,30 @@ static int pp_start_one(struct parallel_processes *pp)
if (!pp->get_next_task(pp->data,
&pp->children[i].process,
&pp->children[i].err))
- return 1;
+ return 0;
- if (start_command(&pp->children[i].process))
+ if (start_command(&pp->children[i].process)) {
pp->start_failure(pp->data,
&pp->children[i].process,
&pp->children[i].err);
+ strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
+ strbuf_reset(&pp->children[i].err);
+ return -1;
+ }
set_nonblocking(pp->children[i].process.err);
pp->nr_processes++;
pp->children[i].in_use = 1;
pp->pfd[i].fd = pp->children[i].process.err;
- return 0;
+ return 1;
}
-static void pp_buffer_stderr(struct parallel_processes *pp)
+static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
{
int i;
- while ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {
+ while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {
if (errno == EINTR)
continue;
pp_cleanup(pp);
@@ -1069,7 +1074,8 @@ static void pp_collect_finished(struct parallel_processes
*pp)
error("waitpid is confused (%s)",
pp->children[i].process.argv[0]);
- pp->return_value(pp->data, &pp->children[i].process, code);
+ pp->return_value(pp->data, &pp->children[i].process,
+&pp->children[i].err, code);
argv_array_clear(&pp->children[i].process.args);
argv_array_clear(&pp->children[i].process.env_array);
@@ -,15 +1117,26 @@ int run_processes_parallel(int n, void *data,
return_value_fn return_value)
{
struct parallel_processes pp;
- pp_init(&pp, n, data, get_next_task, start_failure, return_value);
+ pp_init(&pp, n, data, get_next_task, start_failure, return_value);
while (1) {
- while (pp.nr_processes < pp.max_processes &&
- !pp_start_one(&pp))
- ; /* nothing */
- if (!pp.nr_processes)
+ int no_more_task, cnt;
+ int output_timeout = 100;
+ int spawn_cap = 4;
+
+ for (cnt = spawn_cap, no_more_task = 0;
+cnt && pp.nr_processes < pp.max_processes;
+cnt--) {
+ if (!pp_start_one(&pp)) {
+ no_more_task = 1;
+ break;
+ }
+ }
+
+ if (no_more_task && !pp.nr_processes)
break;
- pp_buffer_stderr(&pp);
+ pp_buffer_stderr(&pp, output_timeout);
+
pp_output(&pp);
pp_collect_finished(&pp);
}
diff --git a/run-command.h b/run-comman
[RFC/PATCH v1] Add Travis CI support
From: Lars Schneider Hi, I recently broke a few tests... In order to avoid that in the future I configured Travis CI for Git. With this patch Travis can run all Git tests including the "git-p4" and "Git-LFS" tests. The tests are executed on "Ubuntu 12.04 LTS Server Edition 64 bit" and on "OS X Mavericks" using gcc and clang. My idea is that the owner of "https://github.com/git/git"; enables this account for Travis (it's free!). Then we would automatically get the test state for all official branches. Every contributor can enable Travis for their respective GitHub accounts. Then they would know if their patches pass all tests in advance, too. You can see the state of my branches here: https://travis-ci.org/larsxschneider/git/branches It's pretty red. The reason is that maint, master, and next have a failure on OS X (test "t9815-git-p4-submit-fail.sh" does not pass). Furthmore pu does not pass on either Linux or OS X (which is propably OK since it is pu). You can also inspect the build/test logs. Here for instance the log for the next branch compiled on Linux with gcc: https://travis-ci.org/larsxschneider/git/jobs/82032861 Cheers, Lars Lars Schneider (1): Add Travis CI support .travis.yml | 28 1 file changed, 28 insertions(+) create mode 100644 .travis.yml -- 2.5.1 -- 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
[RFC/PATCH v1] Add Travis CI support
From: Lars Schneider
The tests are executed on "Ubuntu 12.04 LTS Server Edition 64 bit" and
on "OS X Mavericks" using gcc and clang.
Perforce and Git-LFS are installed and therefore available for the
respective tests.
Signed-off-by: Lars Schneider
---
.travis.yml | 28
1 file changed, 28 insertions(+)
create mode 100644 .travis.yml
diff --git a/.travis.yml b/.travis.yml
new file mode 100644
index 000..056cc99
--- /dev/null
+++ b/.travis.yml
@@ -0,0 +1,28 @@
+language: c
+
+os:
+ - linux
+ - osx
+
+compiler:
+ - clang
+ - gcc
+
+before_script:
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then wget -q
https://package.perforce.com/perforce.pubkey -O - | sudo apt-key add -; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then echo 'deb
http://package.perforce.com/apt/ubuntu precise release' | sudo tee -a
/etc/apt/sources.list; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then sudo apt-get update
-qq; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then sudo apt-get install
perforce-server; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then wget -q
https://packagecloud.io/gpg.key -O - | sudo apt-key add -; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then sudo apt-get install -y
apt-transport-https; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then echo 'deb
https://packagecloud.io/github/git-lfs/debian/ wheezy main' | sudo tee -a
/etc/apt/sources.list; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then sudo apt-get update
-qq; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then sudo apt-get install
git-lfs; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'osx' ]; then brew update; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'osx' ]; then brew install git-lfs; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'osx' ]; then brew tap homebrew/binary;
fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'osx' ]; then sed -i.bak
's/b42758ebe7b54e672b513c34c88f399d0da7b4de1fd23b9f56d222a4f1f3bae5/e987475bfc54129d8d54a0d54363db3ecf6e6852a00daa0c6ffc20b8df1e0e63/'
/usr/local/Library/Taps/homebrew/homebrew-binary/perforce-server.rb; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'osx' ]; then brew install perforce; fi"
+ - "if [ ${TRAVIS_OS_NAME:-'linux'} = 'osx' ]; then brew install
perforce-server; fi"
+
+install: make configure
--
2.5.1
--
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
[PATCH] t5561: get rid of racy appending to logfile
The definition of log_div() appended information to the web server's
logfile to make the test more readable. However, log_div() was called
right after a request is served (which is done by git-http-backend);
the web server waits for the git-http-backend process to exit before
it writes to the log file. When the duration between serving a request
and exiting was long, the log_div() output was written before the last
request's log, and the test failed. (This duration could become
especially long for PROFILE=GEN builds.)
To get rid of this behavior, we should not change the logfile at all.
This commit removes log_div() and its calls. The additional information
is kept in the test (for readability reasons) but filtered out before
comparing it to the actual logfile.
Signed-off-by: Stephan Beyer
Acked-by: Jeff King
---
SubmittingPatches says that when there is consensus the patch has to
be resent to Junio and cc'ed to the list. Here it is (although I
don't know if there is consensus, but, hey, it's a rather trivial patch,
so it should be okay).
See
http://git.661346.n2.nabble.com/t5561-failing-after-make-PROFILE-GEN-td7640150.html
for the original thread.
Compared to the last version, I only added the Acked-by line.
t/t5560-http-backend-noserver.sh | 4
t/t5561-http-backend.sh | 8 +---
t/t556x_common | 12
3 files changed, 1 insertion(+), 23 deletions(-)
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index aa73eea..9fafcf1 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -44,10 +44,6 @@ POST() {
test_cmp exp act
}
-log_div() {
- return 0
-}
-
. "$TEST_DIRECTORY"/t556x_common
expect_aliased() {
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 19afe96..73dcb29 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -29,15 +29,9 @@ POST() {
test_cmp exp act
}
-log_div() {
- echo >>"$HTTPD_ROOT_PATH"/access.log
- echo "### $1" >>"$HTTPD_ROOT_PATH"/access.log
- echo "###" >>"$HTTPD_ROOT_PATH"/access.log
-}
-
. "$TEST_DIRECTORY"/t556x_common
-cat >exp
Re: [PATCH 54/68] drop strcpy in favor of raw sha1_to_hex
On Thu, Sep 24, 2015 at 5:08 PM, Jeff King wrote: > In some cases where we strcpy() the result of sha1_to_hex(), > there's no need; the result goes directly into a printf > statement, and we can simply pass the return value from > sha1_to_hex() directly. > > When this code was originally written, sha1_to_hex used a > single buffer, and it was not safe to use it twice within a > single expression. That changed as of dcb3450 (sha1_to_hex() > usage cleanup, 2006-05-03), but this code ewas never s/ewas/was/ > updated. > > History-dug-by: Eric Sunshine > Signed-off-by: Jeff King -- 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
