Hi,
more comments:
On Sat, 17 Jan 2009, Ray Chuan wrote:
> diff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c
> index 7c64609..25b655d 100644
> --- a/src/git-1.6.1/http-push.c
> +++ b/src/git-1.6.1/http-push.c
> @@ -1189,6 +1189,7 @@ static struct remote_lock *lock_remote(const
> char *path, long timeout)
> struct strbuf in_buffer = STRBUF_INIT;
> char *url;
> char *ep;
> + char ep_old;
> char timeout_header[25];
> struct remote_lock *lock = NULL;
> struct curl_slist *dav_headers = NULL;
> @@ -1198,9 +1199,18 @@ static struct remote_lock *lock_remote(const
> char *path, long timeout)
> sprintf(url, "%s%s", remote->url, path);
>
> /* Make sure leading directories exist for the remote ref */
> - ep = strchr(url + strlen(remote->url) + 1, '/');
> - while (ep) {
> - *ep = 0;
> + ep = url + strlen(remote->url) + 1;
> + int has_fs = 0;
decl-after-statement.
And name-not-meaningful. What does "has_fs" stand for?
> + while (1) {
> + ep = strchr(ep + 1, '/');
> + if(ep) {
> + ep++;
> + ep_old=*ep;
Okay, you succeeded in fooling me. It took fully five minutes until I
realized that ep_old is not the old value of ep, but of *ep.
And now I know what has_fs does, but the name is an even bigger puzzle.
Almost as big as the puzzle why you did not do a much less intrusive
change:
- after the "while (ep) {" you could say "char saved_character = ep[1];
- then you replace the "*ep = 0" by "ep[1] = '\0';"
- at the end of the loop, you replace the "*ep = '/'" with "ep[1] =
saved_character;"
That way, not only would the patch be much smaller, it would also not have
been as difficult to review as it was.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
