Re: [PATCH 1/3] http-push: append slash if possible for directories
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jan 17, 2009, 06:02 UTC
- Message-ID
- <alpine.DEB.1.00.0901170654540.3586@pacific.mpi-cbg.de>
- In-Reply-To
- <be6fef0d0901161853y6784310dgf94dadb2218001a1@mail.gmail.com>
Hi,
more comments:
On Sat, 17 Jan 2009, Ray Chuan wrote:
Show 23 quoted lines
> 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?
Show 5 quoted lines
> + 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