{"thread":{"id":"9780","subject":"[PATCH] Function for updating refs.","startedAt":"2007-09-05T01:38:24Z","lastAt":"2007-09-05T12:03:50Z","messageCount":3,"participants":["Carlos Rica","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"52536","messageId":"46DE0890.5020709@gmail.com","threadId":"9780","inReplyTo":null,"subject":"[PATCH] Function for updating refs.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-09-05T01:38:24Z","receivedAt":"2007-09-05T01:38:24Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"A function intended to be called from builtins updating refs\nby locking them before write, specially those that came from\nscripts using \"git update-ref\".\n\nSigned-off-by: Carlos Rica <jasampler@gmail.com>\n---\n builtin-fetch--tool.c |   21 ++++++++-------------\n builtin-update-ref.c  |    8 ++------\n refs.c                |   28 ++++++++++++++++++++++++++++\n refs.h                |    6 ++++++\n send-pack.c           |   11 +++--------\n 5 files changed, 47 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\nindex e2f8ede..a192fd7 100644\n--- a/builtin-fetch--tool.c\n+++ b/builtin-fetch--tool.c\n@@ -31,24 +31,19 @@ static void show_new(enum object_type type, unsigned char *sha1_new)\n \t\tfind_unique_abbrev(sha1_new, DEFAULT_ABBREV));\n }\n\n-static int update_ref(const char *action,\n+static int update_ref_env(const char *action,\n \t\t      const char *refname,\n \t\t      unsigned char *sha1,\n \t\t      unsigned char *oldval)\n {\n \tchar msg[1024];\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstatic struct ref_lock *lock;\n\n \tif (!rla)\n \t\trla = \"(reflog update)\";\n-\tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n-\tlock = lock_any_ref_for_update(refname, oldval, 0);\n-\tif (!lock)\n-\t\treturn 1;\n-\tif (write_ref_sha1(lock, sha1, msg) < 0)\n-\t\treturn 1;\n-\treturn 0;\n+\tif (snprintf(msg, sizeof(msg), \"%s: %s\", rla, action) >= sizeof(msg))\n+\t\terror(\"reflog message too long: %.*s...\", 50, msg);\n+\treturn update_ref(msg, refname, sha1, oldval, 0, QUIET_ON_ERR);\n }\n\n static int update_local_ref(const char *name,\n@@ -88,7 +83,7 @@ static int update_local_ref(const char *name,\n \t\tfprintf(stderr, \"* %s: storing %s\\n\",\n \t\t\tname, note);\n \t\tshow_new(type, sha1_new);\n-\t\treturn update_ref(msg, name, sha1_new, NULL);\n+\t\treturn update_ref_env(msg, name, sha1_new, NULL);\n \t}\n\n \tif (!hashcmp(sha1_old, sha1_new)) {\n@@ -102,7 +97,7 @@ static int update_local_ref(const char *name,\n \tif (!strncmp(name, \"refs/tags/\", 10)) {\n \t\tfprintf(stderr, \"* %s: updating with %s\\n\", name, note);\n \t\tshow_new(type, sha1_new);\n-\t\treturn update_ref(\"updating tag\", name, sha1_new, NULL);\n+\t\treturn update_ref_env(\"updating tag\", name, sha1_new, NULL);\n \t}\n\n \tcurrent = lookup_commit_reference(sha1_old);\n@@ -117,7 +112,7 @@ static int update_local_ref(const char *name,\n \t\tfprintf(stderr, \"* %s: fast forward to %s\\n\",\n \t\t\tname, note);\n \t\tfprintf(stderr, \"  old..new: %s..%s\\n\", oldh, newh);\n-\t\treturn update_ref(\"fast forward\", name, sha1_new, sha1_old);\n+\t\treturn update_ref_env(\"fast forward\", name, sha1_new, sha1_old);\n \t}\n \tif (!force) {\n \t\tfprintf(stderr,\n@@ -131,7 +126,7 @@ static int update_local_ref(const char *name,\n \t\t\"* %s: forcing update to non-fast forward %s\\n\",\n \t\tname, note);\n \tfprintf(stderr, \"  old...new: %s...%s\\n\", oldh, newh);\n-\treturn update_ref(\"forced-update\", name, sha1_new, sha1_old);\n+\treturn update_ref_env(\"forced-update\", name, sha1_new, sha1_old);\n }\n\n static int append_fetch_head(FILE *fp,\ndiff --git a/builtin-update-ref.c b/builtin-update-ref.c\nindex 8339cf1..2ed98e8 100644\n--- a/builtin-update-ref.c\n+++ b/builtin-update-ref.c\n@@ -8,7 +8,6 @@ static const char git_update_ref_usage[] =\n int cmd_update_ref(int argc, const char **argv, const char *prefix)\n {\n \tconst char *refname=NULL, *value=NULL, *oldval=NULL, *msg=NULL;\n-\tstruct ref_lock *lock;\n \tunsigned char sha1[20], oldsha1[20];\n \tint i, delete, ref_flags;\n\n@@ -62,10 +61,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \tif (oldval && *oldval && get_sha1(oldval, oldsha1))\n \t\tdie(\"%s: not a valid old SHA1\", oldval);\n\n-\tlock = lock_any_ref_for_update(refname, oldval ? oldsha1 : NULL, ref_flags);\n-\tif (!lock)\n-\t\tdie(\"%s: cannot lock the ref\", refname);\n-\tif (write_ref_sha1(lock, sha1, msg) < 0)\n-\t\tdie(\"%s: cannot update the ref\", refname);\n+\tupdate_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n+\t\t\t\t\t\tref_flags, DIE_ON_ERR);\n \treturn 0;\n }\ndiff --git a/refs.c b/refs.c\nindex 09a2c87..1343f0d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1455,3 +1455,31 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)\n {\n \treturn do_for_each_reflog(\"\", fn, cb_data);\n }\n+\n+int update_ref(const char *action, const char *refname,\n+\t\tconst unsigned char *sha1, const unsigned char *oldval,\n+\t\tint flags, enum action_on_err onerr)\n+{\n+\tstatic struct ref_lock *lock;\n+\tlock = lock_any_ref_for_update(refname, oldval, flags);\n+\tif (!lock) {\n+\t\tconst char *str = \"Cannot lock the ref '%s'.\";\n+\t\tswitch (onerr) {\n+\t\tcase MSG_ON_ERR: error(str, refname); break;\n+\t\tcase DIE_ON_ERR: die(str, refname); break;\n+\t\tcase QUIET_ON_ERR: break;\n+\t\t}\n+\t\treturn 1;\n+\t}\n+\tif (write_ref_sha1(lock, sha1, action) < 0) {\n+\t\tconst char *str = \"Cannot update the ref '%s'.\";\n+\t\tswitch (onerr) {\n+\t\tcase MSG_ON_ERR: error(str, refname); break;\n+\t\tcase DIE_ON_ERR: die(str, refname); break;\n+\t\tcase QUIET_ON_ERR: break;\n+\t\t}\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\ndiff --git a/refs.h b/refs.h\nindex f234eb7..6eb98a4 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -64,4 +64,10 @@ extern int rename_ref(const char *oldref, const char *newref, const char *logmsg\n /** resolve ref in nested \"gitlink\" repository */\n extern int resolve_gitlink_ref(const char *name, const char *refname, unsigned char *result);\n\n+/** lock a ref and then write its file */\n+enum action_on_err { MSG_ON_ERR, DIE_ON_ERR, QUIET_ON_ERR };\n+int update_ref(const char *action, const char *refname,\n+\t\tconst unsigned char *sha1, const unsigned char *oldval,\n+\t\tint flags, enum action_on_err onerr);\n+\n #endif /* REFS_H */\ndiff --git a/send-pack.c b/send-pack.c\nindex 9fc8a81..c59eea4 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -313,14 +313,9 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha\n \t\t\t\t\tif (delete_ref(rs.dst, NULL)) {\n \t\t\t\t\t\terror(\"Failed to delete\");\n \t\t\t\t\t}\n-\t\t\t\t} else {\n-\t\t\t\t\tlock = lock_any_ref_for_update(rs.dst, NULL, 0);\n-\t\t\t\t\tif (!lock)\n-\t\t\t\t\t\terror(\"Failed to lock\");\n-\t\t\t\t\telse\n-\t\t\t\t\t\twrite_ref_sha1(lock, ref->new_sha1,\n-\t\t\t\t\t\t\t       \"update by push\");\n-\t\t\t\t}\n+\t\t\t\t} else\n+\t\t\t\t\tupdate_ref(\"update by push\", rs.dst,\n+\t\t\t\t\t\tref->new_sha1, NULL, 0, 0);\n \t\t\t\tfree(rs.dst);\n \t\t\t}\n \t\t}\n-- \n1.5.0\n"},{"id":"52542","messageId":"7vy7flk2z5.fsf@gitster.siamese.dyndns.org","threadId":"9780","inReplyTo":"46DE0890.5020709@gmail.com","subject":"Re: [PATCH] Function for updating refs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-05T07:04:14Z","receivedAt":"2007-09-05T07:04:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Rica <jasampler@gmail.com> writes:\n\n> A function intended to be called from builtins updating refs\n> by locking them before write, specially those that came from\n> scripts using \"git update-ref\".\n>\n> Signed-off-by: Carlos Rica <jasampler@gmail.com>\n\nThanks.  Very nice.\n\nI have two comments but I think they are very minor details I\ncan and should fix in my inbox and apply, instead of asking you\nto update and resend.\n\n> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\n> index e2f8ede..a192fd7 100644\n> --- a/builtin-fetch--tool.c\n> +++ b/builtin-fetch--tool.c\n> @@ -31,24 +31,19 @@ static void show_new(enum object_type type, unsigned char *sha1_new)\n>  \t\tfind_unique_abbrev(sha1_new, DEFAULT_ABBREV));\n>  }\n>\n> -static int update_ref(const char *action,\n> +static int update_ref_env(const char *action,\n>  \t\t      const char *refname,\n>  \t\t      unsigned char *sha1,\n>  \t\t      unsigned char *oldval)\n>  {\n>  \tchar msg[1024];\n>  \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n> -\tstatic struct ref_lock *lock;\n>\n>  \tif (!rla)\n>  \t\trla = \"(reflog update)\";\n> -\tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n> -\tlock = lock_any_ref_for_update(refname, oldval, 0);\n> -\tif (!lock)\n> -\t\treturn 1;\n> -\tif (write_ref_sha1(lock, sha1, msg) < 0)\n> -\t\treturn 1;\n> -\treturn 0;\n> +\tif (snprintf(msg, sizeof(msg), \"%s: %s\", rla, action) >= sizeof(msg))\n> +\t\terror(\"reflog message too long: %.*s...\", 50, msg);\n\nThe original I did was sloppy and did not detect this situation;\nthanks for fixing it.  You do not refuse the primary operation,\nwhich is to update the ref, so this should be a warning instead\nof an error, I think.\n\n> diff --git a/send-pack.c b/send-pack.c\n> index 9fc8a81..c59eea4 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -313,14 +313,9 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha\n>  \t\t\t\t\tif (delete_ref(rs.dst, NULL)) {\n>  \t\t\t\t\t\terror(\"Failed to delete\");\n>  \t\t\t\t\t}\n> -\t\t\t\t} else {\n> -\t\t\t\t\tlock = lock_any_ref_for_update(rs.dst, NULL, 0);\n> -\t\t\t\t\tif (!lock)\n> -\t\t\t\t\t\terror(\"Failed to lock\");\n> -\t\t\t\t\telse\n> -\t\t\t\t\t\twrite_ref_sha1(lock, ref->new_sha1,\n> -\t\t\t\t\t\t\t       \"update by push\");\n> -\t\t\t\t}\n\nThis removal makes \"struct ref_lock *lock\" (not shown in the\ncontext) unused.  I will remove the declaration.\n"},{"id":"52581","messageId":"1b46aba20709050503r6027ada3u79ca8f5cb662775f@mail.gmail.com","threadId":"9780","inReplyTo":"7vy7flk2z5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Function for updating refs.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-09-05T12:03:50Z","receivedAt":"2007-09-05T12:03:50Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"2007/9/5, Junio C Hamano <gitster@pobox.com>:\n> Carlos Rica <jasampler@gmail.com> writes:\n> > -     snprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n> > -     lock = lock_any_ref_for_update(refname, oldval, 0);\n> > -     if (!lock)\n> > -             return 1;\n> > -     if (write_ref_sha1(lock, sha1, msg) < 0)\n> > -             return 1;\n> > -     return 0;\n> > +     if (snprintf(msg, sizeof(msg), \"%s: %s\", rla, action) >= sizeof(msg))\n> > +             error(\"reflog message too long: %.*s...\", 50, msg);\n>\n> The original I did was sloppy and did not detect this situation;\n> thanks for fixing it.  You do not refuse the primary operation,\n> which is to update the ref, so this should be a warning instead\n> of an error, I think.\n\nYes, it is. Also, I tested what would happen when the lock fails. I tried to\nlock an already locked ref, and it died printing the message\ndie(\"unable to create '%s.lock': %s\", path, strerror(errno));  from\nlockfile.c. I think this is interesting. There are other failing\nreasons that could\nmake the update_ref function to print its error, but I haven't tested them.\n\n> > diff --git a/send-pack.c b/send-pack.c\n> > ...\n> This removal makes \"struct ref_lock *lock\" (not shown in the\n> context) unused.  I will remove the declaration.\n\nThank you. Also in builtin-update-ref.c the main function could return\ndirectly that value returned from the call to update_ref().\n"}]}