git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: git@vger.kernel.org, "brian m . carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH v2 2/3] setup: do not use invalid `repository_format`
Date: Tue, 22 Jan 2019 02:07:25 -0500	[thread overview]
Message-ID: <20190122070725.GA28555@sigill.intra.peff.net> (raw)
In-Reply-To: <20190117063114.1901775-1-martin.agren@gmail.com>

On Thu, Jan 17, 2019 at 07:31:14AM +0100, Martin Ågren wrote:

> > I also get why read_repository_format() doing this in patch 3 would be a
> > problem:
> >
> >   +       if (format->version == -1) {
> >   +               clear_repository_format(format);
> >   +               format->version = -1;
> >   +       }
> >
> > but doesn't that point out that clear_repository_format() should be
> > setting hash_algo to GIT_HASH_SHA1 as the default (and likewise "bare =
> > -1", etc, that is done in that function)?
> 
> Something like the below on top of this series (then rebased). (The last
> hunk below is a revert of this patch.)

Yes, that's exactly what I had in mind. Usually our clear() functions
put the struct back into some default state from which it can be used
gain. But the state after clear() here (without the patch below) is
something that nobody is ever expected to look at.

Granted, the only function which fills it in is read_...(), and it sets
those defaults itself. But it just seems to me if we're going to have to
put _something_ in the struct to initialize or clear it, it might as
well be those.

> I'd like to think of the situation before this patch above as a
> situation where the API promises something and the user uses the API
> beyond that. The next patch in this series changes the internals of the
> API in a way that is consistent with the promise made, but which ends up
> affecting an over-eager user.

As with many parts of Git, there really isn't a clear promise. :) I
don't think you're wrong at all about the current state of things. I'm
mostly basing my comments on "what would I _expect_ the promise to be
based on our general patterns". If that's far from what we promise now,
then it's a hassle to convert. But I think it's actually pretty close.

> What this patch above does is to make the user do what the API promise
> allows them to do, i.e., no more shortcuts. What you're saying is, why
> isn't the promise stronger? So the user won't have to think as much?
> 
> So in particular, why doesn't `clear...()` and the error path in
> `read_...()` impose sane, usable defaults? My first concern is that it
> means we need to make a stronger promise, which might then be hard to
> back away from, if we want to. Maybe we'll never want to...

I'm not too worried about that personally. I think the more likely
problem is that the API is misunderstood and misused. ;)

> My second concern is, what should we be falling back to, going forward?
> At some point, the hash indicated by `REPOSITORY_FORMAT_INIT` will be
> SHA-256. Before that, and as soon as we support both hashes, what if we
> pick up SHA-256 before stumbling on some other piece of the config --
> should we now reset the struct to indicate SHA-1, or rather keep the
> SHA-256 value, which by itself is valid? (The same could be argued now,
> for something other than hash functions, but the SHA-1/256 example might
> be more obvious in the context of this patch.)

I'd think this would _always_ be sha-1. Because it's not about "what's
the default for this program running". It's about "what have I read from
this on-disk repo config". And the rule there is "if they don't say
otherwise, it is sha1". That won't change even in a sha256 world,
because we'll maintain backwards-compatibility with legacy repositories
forever.

Now if your next question is: "does any caller misuse this as more than
looking at the repo format", I don't know the answer for sure. That
would be worth poking at (or perhaps having just poked yourself, you
might have an idea already).

> My third worry is that we should then equip `clear_...()` or at least
> the error path of `read_...()` with some logic to keep "as much as
> possible" of what we've picked up and reset the rest, all the while
> making sure we don't end up with something self-contradicting or stupid.
> After all, we'll have promised the users that they can ignore any errors
> and just run ahead.

I think clear() should always throw everything away. Saving partial bits
from the error path of read() is harder. My gut says "no", but I agree
that's a trickier question. I think the real-world thing here is: we're
reading repo config and see an extensions.* field that says "use
sha256". But then we encounter an error, or don't otherwise have a
version. What do we do?

If that's an undefined setup (and I think it is -- if you're using
extensions.* you're supposed to always set the version field), then I
don't know that it really matters that much. But throwing the whole
thing away (even if it means a buggy code path is more likely to use
sha1) seems OK to me.

> Maybe I'm worrying way too much, and I shouldn't be so afraid of making
> a stronger promise here and now because of vague slippery-slope thinking.
> 
> Thanks for pushing back and forcing me to articulate my thinking.

For the record, I can live with it either way. There are so many funky
little setup corner cases in the code already, and we don't even really
have a real-world case to dissect at this point. So the right thing may
also just be to finish this patch series as quickly as possible and move
on to something more useful. :)

-Peff

  reply	other threads:[~2019-01-22  7:07 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-18  7:25 [PATCH 0/3] setup: add `clear_repository_format()` Martin Ågren
2018-12-18  7:25 ` [PATCH 1/3] setup: drop return value from `read_repository_format()` Martin Ågren
2018-12-19 15:27   ` Jeff King
2018-12-19 21:42     ` Martin Ågren
2018-12-20  0:17     ` brian m. carlson
2018-12-20  2:52       ` Jeff King
2018-12-20  3:45         ` brian m. carlson
2018-12-20 14:53           ` Jeff King
2018-12-18  7:25 ` [PATCH 2/3] setup: do not use invalid `repository_format` Martin Ågren
2018-12-19  0:18   ` brian m. carlson
2018-12-19 21:43     ` Martin Ågren
2018-12-19 15:38   ` Jeff King
2018-12-19 21:46     ` Martin Ågren
2018-12-19 23:17       ` Jeff King
2018-12-20  0:21     ` brian m. carlson
2018-12-18  7:25 ` [PATCH 3/3] setup: add `clear_repository_format()` Martin Ågren
2018-12-19 15:48   ` Jeff King
2018-12-19 21:49     ` Martin Ågren
2019-01-14 18:34 ` [PATCH v2 0/3] " Martin Ågren
2019-01-14 18:34   ` [PATCH v2 1/3] setup: free old value before setting `work_tree` Martin Ågren
2019-01-14 18:34   ` [PATCH v2 2/3] setup: do not use invalid `repository_format` Martin Ågren
2019-01-15 19:31     ` Jeff King
2019-01-17  6:31       ` Martin Ågren
2019-01-22  7:07         ` Jeff King [this message]
2019-01-22 13:34           ` Martin Ågren
2019-01-22 21:45             ` [PATCH v3 0/2] setup: fix memory leaks with `struct repository_format` Martin Ågren
2019-01-22 21:45               ` [PATCH v3 1/2] setup: free old value before setting `work_tree` Martin Ågren
2019-01-22 21:45               ` [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format` Martin Ågren
2019-01-23  5:57                 ` Jeff King
2019-01-24  0:14                   ` brian m. carlson
2019-01-25 19:25                     ` Martin Ågren
2019-01-25 19:24                   ` Martin Ågren
2019-01-25 19:51                     ` Jeff King
2019-02-25 19:21                       ` Martin Ågren
2019-02-26 17:46                         ` Jeff King
2019-02-28 20:36                           ` [PATCH v4 0/2] " Martin Ågren
2019-02-28 20:36                             ` [PATCH v4 1/2] setup: free old value before setting `work_tree` Martin Ågren
2019-02-28 20:36                             ` [PATCH v4 2/2] setup: fix memory leaks with `struct repository_format` Martin Ågren
2019-03-06  4:56                             ` [PATCH v4 0/2] " Jeff King
2019-01-14 18:34   ` [PATCH v2 3/3] setup: add `clear_repository_format()` 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=20190122070725.GA28555@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=martin.agren@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: [PATCH v2 2/3] setup: do not use invalid `repository_format`' \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git