From: Michael Haggerty <mhagger@alum.mit.edu>
To: Jeff King <peff@peff.net>
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"Stefan Beller" <sbeller@google.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Brandon Williams" <bmwill@google.com>,
git@vger.kernel.org
Subject: Re: [PATCH v2 08/11] t1404: demonstrate two problems with reference transactions
Date: Sun, 10 Sep 2017 07:07:27 +0200 [thread overview]
Message-ID: <cdfea8a5-fc86-095d-7f5f-89a8f922cac9@alum.mit.edu> (raw)
In-Reply-To: <20170909111753.pidf26f5koaewyho@sigill.intra.peff.net>
On 09/09/2017 01:17 PM, Jeff King wrote:
> On Fri, Sep 08, 2017 at 03:51:50PM +0200, Michael Haggerty wrote:
> [...]
> So if we get what we want, we execute ":" which should be a successful
> exit code.
I think the `:` is superfluous even if we care about the exit code of
the `case`. I'll remove it.
>> + undefined)
>> + # This is not correct; it means the deletion has happened
>> + # already even though update-ref should not have been
>> + # able to acquire the lock yet.
>> + echo "$prefix/foo deleted prematurely" &&
>> + break
>> + ;;
>
> But if we don't, we hit a "break". But we're not in a loop, so the break
> does nothing. Is the intent to give a false value to the switch so that
> we fail the &&-chain? If so, I'd think "false" would be the right thing
> to use. It's more to the point, and from a few limited tests, it looks
> like "break" will return "0" even outside a loop (bash writes a
> complaint to stderr, but dash doesn't).
>
> Or did you just forget that you're not writing C and that ";;" is the
> correct way to spell "break" here? :)
An earlier version of the patch used a loop and needed the `break`. But
when I removed the loop, I probably didn't notice the now-unneeded
breaks because of what you said. I'll take them out.
>> [...]
>> + esac >out &&
>> [...]
>> + test_must_be_empty out &&
>
> The return value of "break" _doesn't_ matter, because you end up using
> the presence of the error message.
>
> I think we could write this as just:
>
> case "$sha1" in
> $D)
> # good
> ;;
> undefined)
> echo >&2 this is bad
> false
> ;;
> esac &&
>
> I'm OK with it either way (testing the exit code or testing the output),
> but either way the "break" calls are doing nothing and can be dropped, I
> think.
Yes, using the exit code to decide success is simpler. I'll make that
change, too.
Thanks for your comments.
Michael
next prev parent reply other threads:[~2017-09-10 5:07 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-08 13:51 [PATCH v2 00/11] Implement transactions for the packed ref store Michael Haggerty
2017-09-08 13:51 ` [PATCH v2 01/11] packed-backend: don't adjust the reference count on lock/unlock Michael Haggerty
2017-09-08 13:51 ` [PATCH v2 02/11] struct ref_transaction: add a place for backends to store data Michael Haggerty
2017-09-08 13:51 ` [PATCH v2 03/11] packed_ref_store: implement reference transactions Michael Haggerty
2017-09-08 13:51 ` [PATCH v2 04/11] packed_delete_refs(): implement method Michael Haggerty
2017-09-08 13:51 ` [PATCH v2 05/11] files_pack_refs(): use a reference transaction to write packed refs Michael Haggerty
2017-09-08 13:51 ` [PATCH v2 06/11] prune_refs(): also free the linked list Michael Haggerty
2017-09-08 13:51 ` [PATCH v2 07/11] files_initial_transaction_commit(): use a transaction for packed refs Michael Haggerty
2017-09-08 13:51 ` [PATCH v2 08/11] t1404: demonstrate two problems with reference transactions Michael Haggerty
2017-09-09 11:17 ` Jeff King
2017-09-10 5:07 ` Michael Haggerty [this message]
2017-09-08 13:51 ` [PATCH v2 09/11] files_ref_store: use a transaction to update packed refs Michael Haggerty
2017-09-08 13:51 ` [PATCH v2 10/11] packed-backend: rip out some now-unused code Michael Haggerty
2017-09-08 13:51 ` [PATCH v2 11/11] files_transaction_finish(): delete reflogs before references Michael Haggerty
2017-09-09 11:18 ` [PATCH v2 00/11] Implement transactions for the packed ref store Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cdfea8a5-fc86-095d-7f5f-89a8f922cac9@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=avarab@gmail.com \
--cc=bmwill@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).