{"thread":{"id":"9773","subject":"[PATCH] Functions for updating refs.","startedAt":"2007-09-04T13:39:44Z","lastAt":"2007-09-05T00:16:59Z","messageCount":10,"participants":["Carlos Rica","Johannes Schindelin","Johannes Sixt","Karl Hasselström","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"52437","messageId":"46DD6020.4050401@gmail.com","threadId":"9773","inReplyTo":null,"subject":"[PATCH] Functions for updating refs.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-09-04T13:39:44Z","receivedAt":"2007-09-04T13:39:44Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"Signed-off-by: Carlos Rica \n---\n\n They are designed to be reused also from other builtins,\n like the recently changed builtin-tag.c and the upcoming\n builtin-reset.c, and perhaps also from builtin-fetch--tool.c.\n\n builtin-update-ref.c | 8 ++------\n refs.c | 32 ++++++++++++++++++++++++++++++++\n refs.h | 8 ++++++++\n send-pack.c | 12 ++++--------\n 4 files changed, 46 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin-update-ref.c b/builtin-update-ref.c\nindex 8339cf1..bd7fe4d 100644\n--- a/builtin-update-ref.c\n+++ b/builtin-update-ref.c\n@@ -62,10 +62,6 @@ 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-\treturn 0;\n+\treturn update_ref_or_die(msg, refname, sha1,\n+\t\t\t\toldval ? oldsha1 : NULL, ref_flags);\n }\ndiff --git a/refs.c b/refs.c\nindex 09a2c87..4fd5065 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1455,3 +1455,35 @@ 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_or_die(const char *action, const char *refname,\n+\t\t\t\tconst unsigned char *sha1,\n+\t\t\t\tconst unsigned char *oldval, int flags)\n+{\n+\tstatic struct ref_lock *lock;\n+\tlock = lock_any_ref_for_update(refname, oldval, flags);\n+\tif (!lock)\n+\t\tdie(\"Cannot lock the ref '%s'.\", refname);\n+\tif (write_ref_sha1(lock, sha1, action) < 0)\n+\t\tdie(\"Cannot update the ref '%s'.\", refname);\n+\treturn 0;\n+}\n+\n+int update_ref_or_error(const char *action, const char *refname,\n+\t\t\t\tconst unsigned char *sha1,\n+\t\t\t\tconst unsigned char *oldval, int quiet)\n+{\n+\tstatic struct ref_lock *lock;\n+\tlock = lock_any_ref_for_update(refname, oldval, 0);\n+\tif (!lock) {\n+\t\tif (!quiet)\n+\t\t\terror(\"Cannot lock the ref '%s'.\", refname);\n+\t\treturn 1;\n+\t}\n+\tif (write_ref_sha1(lock, sha1, action) < 0) {\n+\t\tif (!quiet)\n+\t\t\terror(\"Cannot update the ref '%s'.\", refname);\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\ndiff --git a/refs.h b/refs.h\nindex f234eb7..3d0100e 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -64,4 +64,12 @@ 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+int update_ref_or_die(const char *action, const char *refname,\n+\t\t\t\tconst unsigned char *sha1,\n+\t\t\t\tconst unsigned char *oldval, int flags);\n+int update_ref_or_error(const char *action, const char *refname,\n+\t\t\t\tconst unsigned char *sha1,\n+\t\t\t\tconst unsigned char *oldval, int quiet);\n+\n #endif /* REFS_H */\ndiff --git a/send-pack.c b/send-pack.c\nindex 9fc8a81..1907684 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -313,14 +313,10 @@ 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_or_error(\"update by push\",\n+\t\t\t\t\t\t\trs.dst, ref->new_sha1,\n+\t\t\t\t\t\t\tNULL, 0);\n \t\t\t\tfree(rs.dst);\n \t\t\t}\n \t\t}\n-- \n1.5.0\n"},{"id":"52438","messageId":"Pine.LNX.4.64.0709041444070.28586@racer.site","threadId":"9773","inReplyTo":"46DD6020.4050401@gmail.com","subject":"Re: [PATCH] Functions for updating refs.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-04T13:45:21Z","receivedAt":"2007-09-04T13:45:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 4 Sep 2007, Carlos Rica wrote:\n\n> Signed-off-by: Carlos Rica \n> ---\n> \n> They are designed to be reused also from other builtins,\n> like the recently changed builtin-tag.c and the upcoming\n> builtin-reset.c, and perhaps also from builtin-fetch--tool.c.\n\nThis should go into the commit message.\n\n> +int update_ref_or_die(const char *action, const char *refname,\n> +\t\t\t\tconst unsigned char *sha1,\n> +\t\t\t\tconst unsigned char *oldval, int flags)\n\nShould this not be \"void\"? And should it not use update_ref_or_error()?\n\nOtherwise I like it.\n\nCiao,\nDscho\n"},{"id":"52448","messageId":"46DD6B93.10005@eudaptics.com","threadId":"9773","inReplyTo":"Pine.LNX.4.64.0709041444070.28586@racer.site","subject":"Re: [PATCH] Functions for updating refs.","fromName":"Johannes Sixt","fromEmail":"j.sixt@eudaptics.com","sentAt":"2007-09-04T14:28:35Z","receivedAt":"2007-09-04T14:28:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Schindelin schrieb:\n> On Tue, 4 Sep 2007, Carlos Rica wrote:\n>> +int update_ref_or_die(const char *action, const char *refname,\n>> +\t\t\t\tconst unsigned char *sha1,\n>> +\t\t\t\tconst unsigned char *oldval, int flags)\n> \n> Should this not be \"void\"? And should it not use update_ref_or_error()?\n\nIt should not use *_error() directly because then it would print two error \nmessages in a row.\n\n-- Hannes\n"},{"id":"52461","messageId":"Pine.LNX.4.64.0709041557590.28586@racer.site","threadId":"9773","inReplyTo":"46DD6B93.10005@eudaptics.com","subject":"Re: [PATCH] Functions for updating refs.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-04T14:58:30Z","receivedAt":"2007-09-04T14:58:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 4 Sep 2007, Johannes Sixt wrote:\n\n> Johannes Schindelin schrieb:\n> > On Tue, 4 Sep 2007, Carlos Rica wrote:\n> > > +int update_ref_or_die(const char *action, const char *refname,\n> > > +\t\t\t\tconst unsigned char *sha1,\n> > > +\t\t\t\tconst unsigned char *oldval, int flags)\n> > \n> > Should this not be \"void\"? And should it not use update_ref_or_error()?\n> \n> It should not use *_error() directly because then it would print two error\n> messages in a row.\n\nWell, my idea was to let _error() print the message, and just die().\n\nCiao,\nDscho\n"},{"id":"52464","messageId":"46DD7506.3090503@eudaptics.com","threadId":"9773","inReplyTo":"Pine.LNX.4.64.0709041557590.28586@racer.site","subject":"Re: [PATCH] Functions for updating refs.","fromName":"Johannes Sixt","fromEmail":"j.sixt@eudaptics.com","sentAt":"2007-09-04T15:08:54Z","receivedAt":"2007-09-04T15:08:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Schindelin schrieb:\n> Hi,\n> \n> On Tue, 4 Sep 2007, Johannes Sixt wrote:\n> \n>> Johannes Schindelin schrieb:\n>>> On Tue, 4 Sep 2007, Carlos Rica wrote:\n>>>> +int update_ref_or_die(const char *action, const char *refname,\n>>>> +\t\t\t\tconst unsigned char *sha1,\n>>>> +\t\t\t\tconst unsigned char *oldval, int flags)\n>>> Should this not be \"void\"? And should it not use update_ref_or_error()?\n>> It should not use *_error() directly because then it would print two error\n>> messages in a row.\n> \n> Well, my idea was to let _error() print the message, and just die().\n\nHow do you avoid that die() prints an error, too?\n\n-- Hannes\n"},{"id":"52469","messageId":"20070904152901.GA31299@diana.vm.bytemark.co.uk","threadId":"9773","inReplyTo":"46DD7506.3090503@eudaptics.com","subject":"Re: [PATCH] Functions for updating refs.","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2007-09-04T15:29:01Z","receivedAt":"2007-09-04T15:29:01Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2007-09-04 17:08:54 +0200, Johannes Sixt wrote:\n\n> Johannes Schindelin schrieb:\n>\n> > Well, my idea was to let _error() print the message, and just die().\n>\n> How do you avoid that die() prints an error, too?\n\nMake a generic function that takes a function pointer argument, then\ncall it with die or error as appropriate.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"52471","messageId":"Pine.LNX.4.64.0709041632410.28586@racer.site","threadId":"9773","inReplyTo":"46DD7506.3090503@eudaptics.com","subject":"Re: [PATCH] Functions for updating refs.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-04T15:33:38Z","receivedAt":"2007-09-04T15:33:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 4 Sep 2007, Johannes Sixt wrote:\n\n> Johannes Schindelin schrieb:\n> \n> > Well, my idea was to let _error() print the message, and just die().\n> \n> How do you avoid that die() prints an error, too?\n\nGood point. Colour me convinced.\n\nCiao,\nDscho\n"},{"id":"52499","messageId":"7v642qnwr7.fsf@gitster.siamese.dyndns.org","threadId":"9773","inReplyTo":"46DD6020.4050401@gmail.com","subject":"Re: [PATCH] Functions for updating refs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-04T17:52:44Z","receivedAt":"2007-09-04T17:52:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Rica writes:\n\n> diff --git a/refs.c b/refs.c\n> index 09a2c87..4fd5065 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1455,3 +1455,35 @@ 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_or_die(const char *action, const char *refname,\n> +\t\t\t\tconst unsigned char *sha1,\n> +\t\t\t\tconst unsigned char *oldval, int flags)\n> +{\n> +\tstatic struct ref_lock *lock;\n> +\tlock = lock_any_ref_for_update(refname, oldval, flags);\n> +\tif (!lock)\n> +\t\tdie(\"Cannot lock the ref '%s'.\", refname);\n> +\tif (write_ref_sha1(lock, sha1, action) < 0)\n> +\t\tdie(\"Cannot update the ref '%s'.\", refname);\n> +\treturn 0;\n> +}\n> +\n> +int update_ref_or_error(const char *action, const char *refname,\n> +\t\t\t\tconst unsigned char *sha1,\n> +\t\t\t\tconst unsigned char *oldval, int quiet)\n> +{\n> +\tstatic struct ref_lock *lock;\n> +\tlock = lock_any_ref_for_update(refname, oldval, 0);\n> +\tif (!lock) {\n> +\t\tif (!quiet)\n> +\t\t\terror(\"Cannot lock the ref '%s'.\", refname);\n> +\t\treturn 1;\n> +\t}\n> +\tif (write_ref_sha1(lock, sha1, action) < 0) {\n> +\t\tif (!quiet)\n> +\t\t\terror(\"Cannot update the ref '%s'.\", refname);\n> +\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> +}\n\nThis makes me wonder three things:\n\n - Why doesn't \"or_error\" side allow \"flags\" as \"or_die\" one?\n Could the 'quiet' option become part of \"flags\" perhaps?\n\n - They look quite similar. Is it a good idea to refactor them\n further, or they are so small it does not matter?\n\n - Why isn't lock released with unlock_ref()?\n"},{"id":"52529","messageId":"1b46aba20709041632x60ee4d3eweae9f5217d2f3b86@mail.gmail.com","threadId":"9773","inReplyTo":"7v642qnwr7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Functions for updating refs.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-09-04T23:32:32Z","receivedAt":"2007-09-04T23:32:32Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"2007/9/4, Junio C Hamano :\n> Carlos Rica writes:\n>\n> > diff --git a/refs.c b/refs.c\n> > index 09a2c87..4fd5065 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -1455,3 +1455,35 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)\n> > {\n> > return do_for_each_reflog(\"\", fn, cb_data);\n> > }\n> > +\n> > +int update_ref_or_die(const char *action, const char *refname,\n> > + const unsigned char *sha1,\n> > + const unsigned char *oldval, int flags)\n> > +{\n> > + static struct ref_lock *lock;\n> > + lock = lock_any_ref_for_update(refname, oldval, flags);\n> > + if (!lock)\n> > + die(\"Cannot lock the ref '%s'.\", refname);\n> > + if (write_ref_sha1(lock, sha1, action) < 0)\n> > + die(\"Cannot update the ref '%s'.\", refname);\n> > + return 0;\n> > +}\n> > +\n> > +int update_ref_or_error(const char *action, const char *refname,\n> > + const unsigned char *sha1,\n> > + const unsigned char *oldval, int quiet)\n> > +{\n> > + static struct ref_lock *lock;\n> > + lock = lock_any_ref_for_update(refname, oldval, 0);\n> > + if (!lock) {\n> > + if (!quiet)\n> > + error(\"Cannot lock the ref '%s'.\", refname);\n> > + return 1;\n> > + }\n> > + if (write_ref_sha1(lock, sha1, action) < 0) {\n> > + if (!quiet)\n> > + error(\"Cannot update the ref '%s'.\", refname);\n> > + return 1;\n> > + }\n> > + return 0;\n> > +}\n>\n> This makes me wonder three things:\n>\n> - Why doesn't \"or_error\" side allow \"flags\" as \"or_die\" one?\n> Could the 'quiet' option become part of \"flags\" perhaps?\n\nI saw that the only code that needed the flags was the\nbuiltin-update-ref.c, and it also needed to die(). The\nothers I saw only want that parameter set to 0.\nbuiltin-tag.c was doing die() also, not using flags, though.\n\n> - They look quite similar. Is it a good idea to refactor them\n> further, or they are so small it does not matter?\n\nI would like to know how to refactor it, however the code I saw\nsometimes needs to call die(), others to error() and others\nneed to get only a value of success or not.\n\nThe function die() returns 128 and terminates the program,\nprepending \"fatal: \" in the message, while error() doesn't exit\nand prepend \"error: \", so they were very different and I\nresolved to separate them.\n\nAnother option is returning different error codes, so\nthe caller could decide what to output in each case, but\nI thought that these functions were only useful to unify those\ninstructions with those error messages for this common\noperation that many builtins use, specially when they come\nfrom scripts who call to \"git update-ref\".\n\n>\n> - Why isn't lock released with unlock_ref()?\n\nI inspected this some weeks ago, and I finally came to think\nthat it is released in the write_ref_sha1 call after the lock.\nBut the code was complex, can someone confirm this?\n"},{"id":"52533","messageId":"7vd4wym0ec.fsf@gitster.siamese.dyndns.org","threadId":"9773","inReplyTo":"1b46aba20709041632x60ee4d3eweae9f5217d2f3b86@mail.gmail.com","subject":"Re: [PATCH] Functions for updating refs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-05T00:16:59Z","receivedAt":"2007-09-05T00:16:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlos Rica\" writes:\n>> - Why doesn't \"or_error\" side allow \"flags\" as \"or_die\" one?\n>> Could the 'quiet' option become part of \"flags\" perhaps?\n>\n> I saw that the only code that needed the flags was the\n> builtin-update-ref.c, and it also needed to die(). The\n> others I saw only want that parameter set to 0.\n> builtin-tag.c was doing die() also, not using flags, though.\n\nOk, when other built-ins start using these functions, they might\nwant to pass different flags, but it is easy enough for us to\nextend the interface later.\n\n>> - They look quite similar. Is it a good idea to refactor them\n>> further, or they are so small it does not matter?\n>\n> The function die() returns 128 and terminates the program,\n> prepending \"fatal: \" in the message, while error() doesn't exit\n> and prepend \"error: \", so they were very different and I\n> resolved to separate them.\n\nFair enough.\n\n>> - Why isn't lock released with unlock_ref()?\n>\n> I inspected this some weeks ago, and I finally came to think\n> that it is released in the write_ref_sha1 call after the lock.\n\nAh, that's right!\n"}]}