Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages
Michael Haggerty writes: > Junio, I incorporated your feedback (which so far has only affected > commit messages). I also rebased the patch series to the current > master. I pushed the result to GitHub [1]. I'll refrain from spamming > the list with v3 yet. Thanks; let us know when you are ready ;-) I finished reading the remainder of the v2, and I think I sent out what I found worth commenting on (either positive or negative). I think the next thing to convert to the transaction API would be the "ok we know the set of updates from the pusher; let's update all of them" in receive-pack? In a sense that is of a lot more real-world impact than the update-ref plumbing. Side note: honestly speaking, I was dissapointed to see that the ref updates by the receive-pack process was not included in the series when I saw the cover letter that said this was a series about transactional updates to refs. Anyway... There are a few things that need to be thought through. Making the update in receive-pack all-or-none is a behaviour change, even though it may be a good one. We may want to allow the user a way to ask for the traditional "reject only the ones that cannot be updated". It probably goes like this: - On the wire, a new "ref-update-aon" capability is advertised from receive-pack to send-pack and can be requested in the opposite direction. - On the "git push" side, a new "--all-or-none" option, and optionally a new "push.allOrNone" configuration, is used to request the "ref-update-aon" capability over the wire. - On the receive-pack side, a new "receive.allOrNone" configuration can be used to always update refs in all-or-none fashion, no matter what the pusher says. - The receive-pack uses the ref transaction to update the refs in all-or-none fashion if it has receive.allOrNone, or both sides agree to use ref-update-aon in the capability exchange. If not, it updates the refs in some-may-succeed-some-may-fail fashion, one by one. Or something like that. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages
On 04/01/2014 12:37 AM, Michael Haggerty wrote: > On 03/31/2014 11:51 PM, Junio C Hamano wrote: >> Michael Haggerty writes: >> >>> Make (most of) the error messages for invalid input have the same >>> format [1]: >>> >>> $COMMAND [SP $REFNAME]: $MESSAGE >>> >>> Update the tests accordingly. >>> >>> [1] A few error messages are left with their old form, because >>> $COMMAND and $REFNAME aren't passed all the way down the call >>> stack. Maybe those sites should be changed some day, too. >>> >>> Signed-off-by: Michael Haggerty >>> --- >> >> Up to this point, modulo nits that have been pointed out separately, >> the series looked reasonably well done. > > Thanks for the feedback! Would you like me to expand the commit > messages to answer the questions that you asked about the previous > patches? And if so, do you want a v3 sent to the list already or should > I wait for you to review patches 19-27 first? Junio, I incorporated your feedback (which so far has only affected commit messages). I also rebased the patch series to the current master. I pushed the result to GitHub [1]. I'll refrain from spamming the list with v3 yet. Michael [1] Branch "ref-transactions" at https://github.com/mhagger/git -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages
On 03/31/2014 11:51 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> Make (most of) the error messages for invalid input have the same >> format [1]: >> >> $COMMAND [SP $REFNAME]: $MESSAGE >> >> Update the tests accordingly. >> >> [1] A few error messages are left with their old form, because >> $COMMAND and $REFNAME aren't passed all the way down the call >> stack. Maybe those sites should be changed some day, too. >> >> Signed-off-by: Michael Haggerty >> --- > > Up to this point, modulo nits that have been pointed out separately, > the series looked reasonably well done. Thanks for the feedback! Would you like me to expand the commit messages to answer the questions that you asked about the previous patches? And if so, do you want a v3 sent to the list already or should I wait for you to review patches 19-27 first? Cheers, Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages
Michael Haggerty writes:
> Make (most of) the error messages for invalid input have the same
> format [1]:
>
> $COMMAND [SP $REFNAME]: $MESSAGE
>
> Update the tests accordingly.
>
> [1] A few error messages are left with their old form, because
> $COMMAND and $REFNAME aren't passed all the way down the call
> stack. Maybe those sites should be changed some day, too.
>
> Signed-off-by: Michael Haggerty
> ---
Up to this point, modulo nits that have been pointed out separately,
the series looked reasonably well done.
Thanks.
> builtin/update-ref.c | 24
> t/t1400-update-ref.sh | 32
> 2 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index b49a5b0..bbc04b2 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -202,19 +202,19 @@ static const char *parse_cmd_update(struct strbuf
> *input, const char *next)
>
> update->ref_name = parse_refname(input, &next);
> if (!update->ref_name)
> - die("update line missing ");
> + die("update: missing ");
>
> if (parse_next_sha1(input, &next, update->new_sha1,
> "update", update->ref_name,
> PARSE_SHA1_ALLOW_EMPTY))
> - die("update %s missing ", update->ref_name);
> + die("update %s: missing ", update->ref_name);
>
> update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
> "update", update->ref_name,
> PARSE_SHA1_OLD);
>
> if (*next != line_termination)
> - die("update %s has extra input: %s", update->ref_name, next);
> + die("update %s: extra input: %s", update->ref_name, next);
>
> return next;
> }
> @@ -227,17 +227,17 @@ static const char *parse_cmd_create(struct strbuf
> *input, const char *next)
>
> update->ref_name = parse_refname(input, &next);
> if (!update->ref_name)
> - die("create line missing ");
> + die("create: missing ");
>
> if (parse_next_sha1(input, &next, update->new_sha1,
> "create", update->ref_name, 0))
> - die("create %s missing ", update->ref_name);
> + die("create %s: missing ", update->ref_name);
>
> if (is_null_sha1(update->new_sha1))
> - die("create %s given zero ", update->ref_name);
> + die("create %s: zero ", update->ref_name);
>
> if (*next != line_termination)
> - die("create %s has extra input: %s", update->ref_name, next);
> + die("create %s: extra input: %s", update->ref_name, next);
>
> return next;
> }
> @@ -250,19 +250,19 @@ static const char *parse_cmd_delete(struct strbuf
> *input, const char *next)
>
> update->ref_name = parse_refname(input, &next);
> if (!update->ref_name)
> - die("delete line missing ");
> + die("delete: missing ");
>
> if (parse_next_sha1(input, &next, update->old_sha1,
> "delete", update->ref_name, PARSE_SHA1_OLD)) {
> update->have_old = 0;
> } else {
> if (is_null_sha1(update->old_sha1))
> - die("delete %s given zero ",
> update->ref_name);
> + die("delete %s: zero ", update->ref_name);
> update->have_old = 1;
> }
>
> if (*next != line_termination)
> - die("delete %s has extra input: %s", update->ref_name, next);
> + die("delete %s: extra input: %s", update->ref_name, next);
>
> return next;
> }
> @@ -275,7 +275,7 @@ static const char *parse_cmd_verify(struct strbuf *input,
> const char *next)
>
> update->ref_name = parse_refname(input, &next);
> if (!update->ref_name)
> - die("verify line missing ");
> + die("verify: missing ");
>
> if (parse_next_sha1(input, &next, update->old_sha1,
> "verify", update->ref_name, PARSE_SHA1_OLD)) {
> @@ -286,7 +286,7 @@ static const char *parse_cmd_verify(struct strbuf *input,
> const char *next)
> }
>
> if (*next != line_termination)
> - die("verify %s has extra input: %s", update->ref_name, next);
> + die("verify %s: extra input: %s", update->ref_name, next);
>
> return next;
> }
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 1db0689..48ccc4d 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -371,7 +371,7 @@ test_expect_success 'stdin fails on junk after quoted
> argument' '
> test_expect_success 'stdin fails create with no ref' '
> echo "create " >stdin &&
> test_must_fail git update-ref --stdin err &&
> - grep "fatal: create line missing " err
> + grep "fatal: create: missing " err
> '
>
> test
