git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: "Martin Ågren" <martin.agren@gmail.com>,
	"Rafael Ascensão" <rafa.almas@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	David Turner <novalis@novalis.org>,
	Jonathan Nieder <jrnieder@gmail.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH] refs: handle null-oid for pseudorefs
Date: Mon, 7 May 2018 09:39:06 +0200	[thread overview]
Message-ID: <1291dbc0-31f9-213a-ae30-1a6349ea1626@alum.mit.edu> (raw)
In-Reply-To: <20180506133549.8536-1-martin.agren@gmail.com>

On 05/06/2018 03:35 PM, Martin Ågren wrote:
> According to the documentation on `git update-ref`, it is possible to
> "specify 40 '0' or an empty string as <oldvalue> to make sure that the
> ref you are creating does not exist." But in the code for pseudorefs, we
> do not implement this. If we fail to read the old ref, we immediately
> die. A failure to read would actually be a good thing if we have been
> given the null-oid.
> 
> With the null-oid, allow -- and even require -- the ref-reading to fail.
> This implements the "make sure that the ref ... does not exist" part of
> the documentation.
> 
> Since we have a `strbuf err` for collecting errors, let's use it and
> signal an error to the caller instead of dying hard.
> 
> Reported-by: Rafael Ascensão <rafa.almas@gmail.com>
> Helped-by: Rafael Ascensão <rafa.almas@gmail.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>

Thanks for the patch. This looks good to me. But it it seems that the
test coverage related to pseudorefs is still not great. Ideally, all of
the following combinations should be tested:

Pre-update value   | ref-update old OID   | Expected result
-------------------|----------------------|----------------
missing            | missing              | accept *
missing            | value                | reject
set                | missing              | reject *
set                | correct value        | accept
set                | wrong value          | reject

I think your test only covers the lines with asterisks. Are the other
scenarios already covered by other tests? If not, how about adding them?
That would give us confidence that the new code works in all circumstances.

Michael

  parent reply	other threads:[~2018-05-07  7:39 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-04 16:28 git update-ref fails to create reference. (bug) Rafael Ascensão
2018-05-04 18:26 ` Martin Ågren
2018-05-05 19:08   ` Rafael Ascensão
2018-05-06 13:35     ` [PATCH] refs: handle null-oid for pseudorefs Martin Ågren
2018-05-06 15:37       ` David Turner
2018-05-07  7:39       ` Michael Haggerty [this message]
2018-05-07 10:05         ` Martin Ågren
2018-05-10 19:29           ` [PATCH v2 0/3] refs: handle zero oid " Martin Ågren
2018-05-10 19:29             ` [PATCH v2 1/3] refs.c: refer to "object ID", not "sha1", in error messages Martin Ågren
2018-05-10 19:29             ` [PATCH v2 2/3] t1400: add tests around adding/deleting pseudorefs Martin Ågren
2018-05-10 19:29             ` [PATCH v2 3/3] refs: handle zero oid for pseudorefs Martin Ågren

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=1291dbc0-31f9-213a-ae30-1a6349ea1626@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=martin.agren@gmail.com \
    --cc=novalis@novalis.org \
    --cc=rafa.almas@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    /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).