git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: Jeff King <peff@peff.net>
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: Thu, 17 Jan 2019 07:31:14 +0100	[thread overview]
Message-ID: <20190117063114.1901775-1-martin.agren@gmail.com> (raw)
In-Reply-To: <20190115193112.GE4886@sigill.intra.peff.net>

On Tue, 15 Jan 2019 at 20:31, Jeff King <peff@peff.net> wrote:
>
> On Mon, Jan 14, 2019 at 07:34:56PM +0100, Martin Ågren wrote:
>
> > This can be observed by adding this to the end of
> > `read_repository_format()`:
> >
> >       if (format->version == -1)
> >               format->hash_algo = 0; /* no-one should peek at this! */
> >
> > Check that `version` is non-negative before using `hash_algo`.

> I'm still somewhat confused about how this breaks. If we move
> ".git/config" out of the way, then we have no version indicator and
> presumably we should guess GIT_HASH_SHA1. Which is what's happening if
> we fail to call repo_set_hash_algo(), no?  In other words, wouldn't
> repo_set_hash_algo() always be a noop in that case?
>
> I get why adding the code snippet above would cause that assumption to
> break, but I am just not sure why we would add that code snippet. ;)
>
> 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.)

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.

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...

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.)

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.

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.

Martin


diff --git a/cache.h b/cache.h
index 3ef63d27c4..acd86e9f9f 100644
--- a/cache.h
+++ b/cache.h
@@ -974,15 +974,21 @@ struct repository_format {
 
 /**
  * Always use this to initialize a `struct repository_format`
- * to a well-defined state before calling `read_repository()`.
+ * to a well-defined, default state before calling
+ * `read_repository()`.
  */
-#define REPOSITORY_FORMAT_INIT { 0 }
+#define REPOSITORY_FORMAT_INIT (struct repository_format){ \
+				 .version = -1, \
+				 .is_bare = -1, \
+				 .hash_algo = GIT_HASH_SHA1, \
+				 .unknown_extensions = STRING_LIST_INIT_DUP, \
+			       }
 
 /*
  * Read the repository format characteristics from the config file "path" into
  * "format" struct. Returns the numeric version. On error, -1 is returned,
  * format->version is set to -1, and all other fields in the struct are
- * undefined.
+ * set to the default configuration (REPOSITORY_FORMAT_INIT).
  */
 int read_repository_format(struct repository_format *format, const char *path);
 
diff --git a/setup.c b/setup.c
index 70d9007ae5..f3ea479ad9 100644
--- a/setup.c
+++ b/setup.c
@@ -511,15 +511,9 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 int read_repository_format(struct repository_format *format, const char *path)
 {
 	clear_repository_format(format);
-	format->version = -1;
-	format->is_bare = -1;
-	format->hash_algo = GIT_HASH_SHA1;
-	string_list_init(&format->unknown_extensions, 1);
 	git_config_from_file(check_repo_format, path, format);
-	if (format->version == -1) {
+	if (format->version == -1)
 		clear_repository_format(format);
-		format->version = -1;
-	}
 	return format->version;
 }
 
@@ -528,7 +522,7 @@ void clear_repository_format(struct repository_format *format)
 	string_list_clear(&format->unknown_extensions, 0);
 	free(format->work_tree);
 	free(format->partial_clone);
-	memset(format, 0, sizeof(*format));
+	*format = REPOSITORY_FORMAT_INIT;
 }
 
 int verify_repository_format(const struct repository_format *format,
@@ -1152,7 +1146,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
 			setup_git_env(gitdir);
 		}
-		if (startup_info->have_repository && repo_fmt.version > -1)
+		if (startup_info->have_repository)
 			repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
 	}
 

  reply	other threads:[~2019-01-17  6:31 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 [this message]
2019-01-22  7:07         ` Jeff King
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=20190117063114.1901775-1-martin.agren@gmail.com \
    --to=martin.agren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --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