Tay Ray Chuan <rctay89@xxxxxxxxx> writes: > After 753bc91 ("Remove the requirement opaquelocktoken uri scheme"), > lock tokens are in the URI forms in which they are received from the > server, eg. 'opaquelocktoken:', 'urn:uuid:'. > > However, "start_put" (and consequently "start_move"), which attempts to > create a unique temporary file using the UUID of the lock token, > inadvertently uses the lock token in its URI form. These file > operations on the server may not be successful (specifically, in > Windows), due to the colon ':' character from the URI form of the lock > token in the file path. > > This patch uses a hash of the lock token instead, guaranteeing only > "safe" characters (a-f, 0-9) are used in the file path. > > The token's hash is generated when the lock token is received from the > server in handle_new_lock_ctx, minimizing the number of times of > hashing. > > Signed-off-by: Tay Ray Chuan <rctay89@xxxxxxxxx> Thanks, very clearly written. > diff --git a/http-push.c b/http-push.c > index eefd64c..0a252dd 100644 > --- a/http-push.c > +++ b/http-push.c > @@ -153,6 +153,7 @@ struct remote_lock > char *url; > char *owner; > char *token; > + char *token_sha1_hex; At this point, this new field is only used as a unique suffix, and "sha1_hex" is a implementation detail of the mechanism to guarantee the uniqueness. Naming things for what they are is preferred over naming things for how they are crafted. Hence: char tmpfile_suffix[41]; would be a better definition here. > @@ -558,7 +559,7 @@ static void start_put(struct transfer_request *request) > > append_remote_object_url(&buf, remote->url, hex, 0); > strbuf_addstr(&buf, "_"); > - strbuf_addstr(&buf, request->lock->token); > + strbuf_addstr(&buf, request->lock->token_sha1_hex, 41); And replace these two strbuf_addstr() with: strbuf_add(&buf, request->lock->tmpfile_suffix, 41); > @@ -1130,6 +1131,8 @@ static void handle_lockprop_ctx(struct xml_ctx *ctx, int tag_closed) > static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed) > { > struct remote_lock *lock = (struct remote_lock *)ctx->userData; > + git_SHA_CTX sha_ctx; > + unsigned char lock_token_sha1[20]; > > if (tag_closed && ctx->cdata) { > if (!strcmp(ctx->name, DAV_ACTIVELOCK_OWNER)) { > @@ -1142,6 +1145,12 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed) > } else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) { > lock->token = xmalloc(strlen(ctx->cdata) + 1); > strcpy(lock->token, ctx->cdata); > + > + git_SHA1_Init(&sha_ctx); > + git_SHA1_Update(&sha_ctx, lock->token, strlen(lock->token)); > + git_SHA1_Final(lock_token_sha1, &sha_ctx); > + > + lock->token_sha1_hex = sha1_to_hex(lock_token_sha1); The last one is wrong because string returned by sha1_to_hex() is volatile. lock->tmpfile_suffix[0] = '_'; memcpy(lock->tmpfile_suffix + 1, sha1_to_hex(sha1)); Other than that, I think this is a good patch. -- 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