Git development
 help / color / mirror / Atom feed
* [PATCH 11/23] commit-reach.c: allow get_merge_bases_many_0 to handle any repo
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit-reach.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/commit-reach.c b/commit-reach.c
index 81015830cb..b3b1f62aba 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -216,7 +216,8 @@ static int remove_redundant(struct repository *r, struct commit **array, int cnt
 	return filled;
 }
 
-static struct commit_list *get_merge_bases_many_0(struct commit *one,
+static struct commit_list *get_merge_bases_many_0(struct repository *r,
+						  struct commit *one,
 						  int n,
 						  struct commit **twos,
 						  int cleanup)
@@ -226,7 +227,7 @@ static struct commit_list *get_merge_bases_many_0(struct commit *one,
 	struct commit_list *result;
 	int cnt, i;
 
-	result = merge_bases_many(the_repository, one, n, twos);
+	result = merge_bases_many(r, one, n, twos);
 	for (i = 0; i < n; i++) {
 		if (one == twos[i])
 			return result;
@@ -249,7 +250,7 @@ static struct commit_list *get_merge_bases_many_0(struct commit *one,
 	clear_commit_marks(one, all_flags);
 	clear_commit_marks_many(n, twos, all_flags);
 
-	cnt = remove_redundant(the_repository, rslt, cnt);
+	cnt = remove_redundant(r, rslt, cnt);
 	result = NULL;
 	for (i = 0; i < cnt; i++)
 		commit_list_insert_by_date(rslt[i], &result);
@@ -261,19 +262,19 @@ struct commit_list *get_merge_bases_many(struct commit *one,
 					 int n,
 					 struct commit **twos)
 {
-	return get_merge_bases_many_0(one, n, twos, 1);
+	return get_merge_bases_many_0(the_repository, one, n, twos, 1);
 }
 
 struct commit_list *get_merge_bases_many_dirty(struct commit *one,
 					       int n,
 					       struct commit **twos)
 {
-	return get_merge_bases_many_0(one, n, twos, 0);
+	return get_merge_bases_many_0(the_repository, one, n, twos, 0);
 }
 
 struct commit_list *get_merge_bases(struct commit *one, struct commit *two)
 {
-	return get_merge_bases_many_0(one, 1, &two, 1);
+	return get_merge_bases_many_0(the_repository, one, 1, &two, 1);
 }
 
 /*
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 10/23] commit-reach.c: allow remove_redundant to handle any repo
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit-reach.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/commit-reach.c b/commit-reach.c
index a53b31e6a2..81015830cb 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -156,7 +156,7 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in)
 	return ret;
 }
 
-static int remove_redundant(struct commit **array, int cnt)
+static int remove_redundant(struct repository *r, struct commit **array, int cnt)
 {
 	/*
 	 * Some commit in the array may be an ancestor of
@@ -174,7 +174,7 @@ static int remove_redundant(struct commit **array, int cnt)
 	ALLOC_ARRAY(filled_index, cnt - 1);
 
 	for (i = 0; i < cnt; i++)
-		parse_commit(array[i]);
+		repo_parse_commit(r, array[i]);
 	for (i = 0; i < cnt; i++) {
 		struct commit_list *common;
 		uint32_t min_generation = array[i]->generation;
@@ -190,7 +190,7 @@ static int remove_redundant(struct commit **array, int cnt)
 			if (array[j]->generation < min_generation)
 				min_generation = array[j]->generation;
 		}
-		common = paint_down_to_common(the_repository, array[i], filled,
+		common = paint_down_to_common(r, array[i], filled,
 					      work, min_generation);
 		if (array[i]->object.flags & PARENT2)
 			redundant[i] = 1;
@@ -249,7 +249,7 @@ static struct commit_list *get_merge_bases_many_0(struct commit *one,
 	clear_commit_marks(one, all_flags);
 	clear_commit_marks_many(n, twos, all_flags);
 
-	cnt = remove_redundant(rslt, cnt);
+	cnt = remove_redundant(the_repository, rslt, cnt);
 	result = NULL;
 	for (i = 0; i < cnt; i++)
 		commit_list_insert_by_date(rslt[i], &result);
@@ -370,7 +370,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)
 			p->item->object.flags &= ~STALE;
 		}
 	}
-	num_head = remove_redundant(array, num_head);
+	num_head = remove_redundant(the_repository, array, num_head);
 	for (i = 0; i < num_head; i++)
 		tail = &commit_list_insert(array[i], tail)->next;
 	free(array);
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 09/23] commit-reach.c: allow merge_bases_many to handle any repo
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit-reach.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/commit-reach.c b/commit-reach.c
index 67c2e43d5e..a53b31e6a2 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -95,7 +95,9 @@ static struct commit_list *paint_down_to_common(struct repository *r,
 	return result;
 }
 
-static struct commit_list *merge_bases_many(struct commit *one, int n, struct commit **twos)
+static struct commit_list *merge_bases_many(struct repository *r,
+					    struct commit *one, int n,
+					    struct commit **twos)
 {
 	struct commit_list *list = NULL;
 	struct commit_list *result = NULL;
@@ -110,14 +112,14 @@ static struct commit_list *merge_bases_many(struct commit *one, int n, struct co
 			return commit_list_insert(one, &result);
 	}
 
-	if (parse_commit(one))
+	if (repo_parse_commit(r, one))
 		return NULL;
 	for (i = 0; i < n; i++) {
-		if (parse_commit(twos[i]))
+		if (repo_parse_commit(r, twos[i]))
 			return NULL;
 	}
 
-	list = paint_down_to_common(the_repository, one, n, twos, 0);
+	list = paint_down_to_common(r, one, n, twos, 0);
 
 	while (list) {
 		struct commit *commit = pop_commit(&list);
@@ -224,7 +226,7 @@ static struct commit_list *get_merge_bases_many_0(struct commit *one,
 	struct commit_list *result;
 	int cnt, i;
 
-	result = merge_bases_many(one, n, twos);
+	result = merge_bases_many(the_repository, one, n, twos);
 	for (i = 0; i < n; i++) {
 		if (one == twos[i])
 			return result;
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 08/23] commit-reach.c: allow paint_down_to_common to handle any repo
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

As the function is file local and not widely used, migrate it all at once.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit-reach.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/commit-reach.c b/commit-reach.c
index 9f79ce0a22..67c2e43d5e 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -30,7 +30,8 @@ static int queue_has_nonstale(struct prio_queue *queue)
 }
 
 /* all input commits in one and twos[] must have been parsed! */
-static struct commit_list *paint_down_to_common(struct commit *one, int n,
+static struct commit_list *paint_down_to_common(struct repository *r,
+						struct commit *one, int n,
 						struct commit **twos,
 						int min_generation)
 {
@@ -83,7 +84,7 @@ static struct commit_list *paint_down_to_common(struct commit *one, int n,
 			parents = parents->next;
 			if ((p->object.flags & flags) == flags)
 				continue;
-			if (parse_commit(p))
+			if (repo_parse_commit(r, p))
 				return NULL;
 			p->object.flags |= flags;
 			prio_queue_put(&queue, p);
@@ -116,7 +117,7 @@ static struct commit_list *merge_bases_many(struct commit *one, int n, struct co
 			return NULL;
 	}
 
-	list = paint_down_to_common(one, n, twos, 0);
+	list = paint_down_to_common(the_repository, one, n, twos, 0);
 
 	while (list) {
 		struct commit *commit = pop_commit(&list);
@@ -187,8 +188,8 @@ static int remove_redundant(struct commit **array, int cnt)
 			if (array[j]->generation < min_generation)
 				min_generation = array[j]->generation;
 		}
-		common = paint_down_to_common(array[i], filled, work,
-					      min_generation);
+		common = paint_down_to_common(the_repository, array[i], filled,
+					      work, min_generation);
 		if (array[i]->object.flags & PARENT2)
 			redundant[i] = 1;
 		for (j = 0; j < filled; j++)
@@ -322,7 +323,9 @@ int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit *
 	if (commit->generation > min_generation)
 		return ret;
 
-	bases = paint_down_to_common(commit, nr_reference, reference, commit->generation);
+	bases = paint_down_to_common(the_repository, commit,
+				     nr_reference, reference,
+				     commit->generation);
 	if (commit->object.flags & PARENT2)
 		ret = 1;
 	clear_commit_marks(commit, all_flags);
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 07/23] commit: allow parse_commit* to handle any repo
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

Just like the previous commit, parse_commit and friends are used a lot
and are found in new patches, so we cannot change their signature easily.

Re-introduce these function prefixed with 'repo_' that take a repository
argument and keep the original as a shallow macro.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.c                                      | 18 ++++++++------
 commit.h                                      | 17 +++++++++----
 .../coccinelle/the_repository.pending.cocci   | 24 +++++++++++++++++++
 3 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/commit.c b/commit.c
index dc8a39d52a..7a931d7fd4 100644
--- a/commit.c
+++ b/commit.c
@@ -443,7 +443,10 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
 	return 0;
 }
 
-int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_commit_graph)
+int repo_parse_commit_internal(struct repository *r,
+			       struct commit *item,
+			       int quiet_on_missing,
+			       int use_commit_graph)
 {
 	enum object_type type;
 	void *buffer;
@@ -454,9 +457,9 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com
 		return -1;
 	if (item->object.parsed)
 		return 0;
-	if (use_commit_graph && parse_commit_in_graph(the_repository, item))
+	if (use_commit_graph && parse_commit_in_graph(r, item))
 		return 0;
-	buffer = read_object_file(&item->object.oid, &type, &size);
+	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
 	if (!buffer)
 		return quiet_on_missing ? -1 :
 			error("Could not read %s",
@@ -467,18 +470,19 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com
 			     oid_to_hex(&item->object.oid));
 	}
 
-	ret = parse_commit_buffer(the_repository, item, buffer, size, 0);
+	ret = parse_commit_buffer(r, item, buffer, size, 0);
 	if (save_commit_buffer && !ret) {
-		set_commit_buffer(the_repository, item, buffer, size);
+		set_commit_buffer(r, item, buffer, size);
 		return 0;
 	}
 	free(buffer);
 	return ret;
 }
 
-int parse_commit_gently(struct commit *item, int quiet_on_missing)
+int repo_parse_commit_gently(struct repository *r,
+			     struct commit *item, int quiet_on_missing)
 {
-	return parse_commit_internal(item, quiet_on_missing, 1);
+	return repo_parse_commit_internal(r, item, quiet_on_missing, 1);
 }
 
 void parse_commit_or_die(struct commit *item)
diff --git a/commit.h b/commit.h
index 1d260d62f5..08935f9a19 100644
--- a/commit.h
+++ b/commit.h
@@ -79,12 +79,21 @@ struct commit *lookup_commit_reference_by_name(const char *name);
 struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref_name);
 
 int parse_commit_buffer(struct repository *r, struct commit *item, const void *buffer, unsigned long size, int check_graph);
-int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_commit_graph);
-int parse_commit_gently(struct commit *item, int quiet_on_missing);
-static inline int parse_commit(struct commit *item)
+int repo_parse_commit_internal(struct repository *r, struct commit *item,
+			       int quiet_on_missing, int use_commit_graph);
+int repo_parse_commit_gently(struct repository *r,
+			     struct commit *item,
+			     int quiet_on_missing);
+static inline int repo_parse_commit(struct repository *r, struct commit *item)
 {
-	return parse_commit_gently(item, 0);
+	return repo_parse_commit_gently(r, item, 0);
 }
+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
+#define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)
+#define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)
+#define parse_commit(item) repo_parse_commit(the_repository, item)
+#endif
+
 void parse_commit_or_die(struct commit *item);
 
 struct buffer_slab;
diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
index 46f3a1b23a..b185fe0a1d 100644
--- a/contrib/coccinelle/the_repository.pending.cocci
+++ b/contrib/coccinelle/the_repository.pending.cocci
@@ -40,3 +40,27 @@ expression F;
 - has_object_file_with_flags(
 + repo_has_object_file_with_flags(the_repository,
   E)
+
+@@
+expression E;
+expression F;
+expression G;
+@@
+- parse_commit_internal(
++ repo_parse_commit_internal(the_repository,
+  E, F, G)
+
+@@
+expression E;
+expression F;
+@@
+- parse_commit_gently(
++ repo_parse_commit_gently(the_repository,
+  E, F)
+
+@@
+expression E;
+@@
+- parse_commit(
++ repo_parse_commit(the_repository,
+  E)
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 06/23] object: parse_object to honor its repository argument
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

In 8e4b0b6047 (object.c: allow parse_object to handle
arbitrary repositories, 2018-06-28), we forgot to pass the
repository down to the read_object_file.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 object.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/object.c b/object.c
index e54160550c..003f870484 100644
--- a/object.c
+++ b/object.c
@@ -259,8 +259,8 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)
 	if (obj && obj->parsed)
 		return obj;
 
-	if ((obj && obj->type == OBJ_BLOB && has_object_file(oid)) ||
-	    (!obj && has_object_file(oid) &&
+	if ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||
+	    (!obj && repo_has_object_file(r, oid) &&
 	     oid_object_info(r, oid, NULL) == OBJ_BLOB)) {
 		if (check_object_signature(repl, NULL, 0, NULL) < 0) {
 			error(_("sha1 mismatch %s"), oid_to_hex(oid));
@@ -270,7 +270,7 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)
 		return lookup_object(r, oid->hash);
 	}
 
-	buffer = read_object_file(oid, &type, &size);
+	buffer = repo_read_object_file(r, oid, &type, &size);
 	if (buffer) {
 		if (check_object_signature(repl, buffer, size, type_name(type)) < 0) {
 			free(buffer);
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 05/23] object-store: prepare has_{sha1, object}_file to handle any repo
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .../coccinelle/the_repository.pending.cocci   | 30 +++++++++++++++++++
 object-store.h                                | 22 ++++++++++----
 sha1-file.c                                   | 15 ++++++----
 3 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
index a7ac9e0c46..46f3a1b23a 100644
--- a/contrib/coccinelle/the_repository.pending.cocci
+++ b/contrib/coccinelle/the_repository.pending.cocci
@@ -10,3 +10,33 @@ expression G;
 - read_object_file(
 + repo_read_object_file(the_repository,
   E, F, G)
+
+@@
+expression E;
+@@
+- has_sha1_file(
++ repo_has_sha1_file(the_repository,
+  E)
+
+@@
+expression E;
+expression F;
+@@
+- has_sha1_file_with_flags(
++ repo_has_sha1_file_with_flags(the_repository,
+  E)
+
+@@
+expression E;
+@@
+- has_object_file(
++ repo_has_object_file(the_repository,
+  E)
+
+@@
+expression E;
+expression F;
+@@
+- has_object_file_with_flags(
++ repo_has_object_file_with_flags(the_repository,
+  E)
diff --git a/object-store.h b/object-store.h
index 00a64622e6..2b5e6ff1ed 100644
--- a/object-store.h
+++ b/object-store.h
@@ -212,15 +212,27 @@ int read_loose_object(const char *path,
  * object_info. OBJECT_INFO_SKIP_CACHED is automatically set; pass
  * nonzero flags to also set other flags.
  */
-extern int has_sha1_file_with_flags(const unsigned char *sha1, int flags);
-static inline int has_sha1_file(const unsigned char *sha1)
+int repo_has_sha1_file_with_flags(struct repository *r,
+				  const unsigned char *sha1, int flags);
+static inline int repo_has_sha1_file(struct repository *r,
+				     const unsigned char *sha1)
 {
-	return has_sha1_file_with_flags(sha1, 0);
+	return repo_has_sha1_file_with_flags(r, sha1, 0);
 }
 
+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
+#define has_sha1_file_with_flags(sha1, flags) repo_has_sha1_file_with_flags(the_repository, sha1, flags)
+#define has_sha1_file(sha1) repo_has_sha1_file(the_repository, sha1)
+#endif
+
 /* Same as the above, except for struct object_id. */
-extern int has_object_file(const struct object_id *oid);
-extern int has_object_file_with_flags(const struct object_id *oid, int flags);
+int repo_has_object_file(struct repository *r, const struct object_id *oid);
+int repo_has_object_file_with_flags(struct repository *r,
+				    const struct object_id *oid, int flags);
+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
+#define has_object_file(oid) repo_has_object_file(the_repository, oid)
+#define has_object_file_with_flags(oid, flags) repo_has_object_file_with_flags(the_repository, oid, flags)
+#endif
 
 /*
  * Return true iff an alternate object database has a loose object
diff --git a/sha1-file.c b/sha1-file.c
index c5b704aec5..e77273ccfd 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -1768,24 +1768,27 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
 	return ret;
 }
 
-int has_sha1_file_with_flags(const unsigned char *sha1, int flags)
+int repo_has_sha1_file_with_flags(struct repository *r,
+				  const unsigned char *sha1, int flags)
 {
 	struct object_id oid;
 	if (!startup_info->have_repository)
 		return 0;
 	hashcpy(oid.hash, sha1);
-	return oid_object_info_extended(the_repository, &oid, NULL,
+	return oid_object_info_extended(r, &oid, NULL,
 					flags | OBJECT_INFO_SKIP_CACHED) >= 0;
 }
 
-int has_object_file(const struct object_id *oid)
+int repo_has_object_file(struct repository *r,
+			 const struct object_id *oid)
 {
-	return has_sha1_file(oid->hash);
+	return repo_has_sha1_file(r, oid->hash);
 }
 
-int has_object_file_with_flags(const struct object_id *oid, int flags)
+int repo_has_object_file_with_flags(struct repository *r,
+				    const struct object_id *oid, int flags)
 {
-	return has_sha1_file_with_flags(oid->hash, flags);
+	return repo_has_sha1_file_with_flags(r, oid->hash, flags);
 }
 
 static void check_tree(const void *buf, size_t size)
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 04/23] object-store: prepare read_object_file to deal with any repo
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

As read_object_file is a widely used function (which is also regularly used
in new code in flight between master..pu), changing its signature is painful
is hard, as other series in flight rely on the original signature. It would
burden the maintainer if we'd just change the signature.

Introduce repo_read_object_file which takes the repository argument, and
hide the original read_object_file as a macro behind
NO_THE_REPOSITORY_COMPATIBILITY_MACROS, similar to
e675765235 (diff.c: remove implicit dependency on the_index, 2018-09-21)

Add a coccinelle patch to convert existing callers, but do not apply
the resulting patch to keep the diff of this patch small.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/coccinelle/the_repository.pending.cocci | 12 ++++++++++++
 object-store.h                                  | 10 ++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)
 create mode 100644 contrib/coccinelle/the_repository.pending.cocci

diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
new file mode 100644
index 0000000000..a7ac9e0c46
--- /dev/null
+++ b/contrib/coccinelle/the_repository.pending.cocci
@@ -0,0 +1,12 @@
+// This file is used for the ongoing refactoring of
+// bringing the index or repository struct in all of
+// our code base.
+
+@@
+expression E;
+expression F;
+expression G;
+@@
+- read_object_file(
++ repo_read_object_file(the_repository,
+  E, F, G)
diff --git a/object-store.h b/object-store.h
index 3d98a682b2..00a64622e6 100644
--- a/object-store.h
+++ b/object-store.h
@@ -165,10 +165,16 @@ extern void *read_object_file_extended(struct repository *r,
 				       const struct object_id *oid,
 				       enum object_type *type,
 				       unsigned long *size, int lookup_replace);
-static inline void *read_object_file(const struct object_id *oid, enum object_type *type, unsigned long *size)
+static inline void *repo_read_object_file(struct repository *r,
+					  const struct object_id *oid,
+					  enum object_type *type,
+					  unsigned long *size)
 {
-	return read_object_file_extended(the_repository, oid, type, size, 1);
+	return read_object_file_extended(r, oid, type, size, 1);
 }
+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
+#define read_object_file(oid, type, size) repo_read_object_file(the_repository, oid, type, size)
+#endif
 
 /* Read and unpack an object file into memory, write memory to an object file */
 int oid_object_info(struct repository *r, const struct object_id *, unsigned long *);
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 03/23] object-store: allow read_object_file_extended to read from any repo
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

read_object_file_extended is not widely used, so migrate it all at once.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 object-store.h |  5 +++--
 sha1-file.c    | 11 ++++++-----
 streaming.c    |  2 +-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/object-store.h b/object-store.h
index 63b7605a3e..3d98a682b2 100644
--- a/object-store.h
+++ b/object-store.h
@@ -161,12 +161,13 @@ void sha1_file_name(struct repository *r, struct strbuf *buf, const unsigned cha
 
 void *map_sha1_file(struct repository *r, const unsigned char *sha1, unsigned long *size);
 
-extern void *read_object_file_extended(const struct object_id *oid,
+extern void *read_object_file_extended(struct repository *r,
+				       const struct object_id *oid,
 				       enum object_type *type,
 				       unsigned long *size, int lookup_replace);
 static inline void *read_object_file(const struct object_id *oid, enum object_type *type, unsigned long *size)
 {
-	return read_object_file_extended(oid, type, size, 1);
+	return read_object_file_extended(the_repository, oid, type, size, 1);
 }
 
 /* Read and unpack an object file into memory, write memory to an object file */
diff --git a/sha1-file.c b/sha1-file.c
index 856e000ee1..c5b704aec5 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -1403,7 +1403,8 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type,
  * deal with them should arrange to call read_object() and give error
  * messages themselves.
  */
-void *read_object_file_extended(const struct object_id *oid,
+void *read_object_file_extended(struct repository *r,
+				const struct object_id *oid,
 				enum object_type *type,
 				unsigned long *size,
 				int lookup_replace)
@@ -1413,10 +1414,10 @@ void *read_object_file_extended(const struct object_id *oid,
 	const char *path;
 	struct stat st;
 	const struct object_id *repl = lookup_replace ?
-		lookup_replace_object(the_repository, oid) : oid;
+		lookup_replace_object(r, oid) : oid;
 
 	errno = 0;
-	data = read_object(the_repository, repl->hash, type, size);
+	data = read_object(r, repl->hash, type, size);
 	if (data)
 		return data;
 
@@ -1428,11 +1429,11 @@ void *read_object_file_extended(const struct object_id *oid,
 		die(_("replacement %s not found for %s"),
 		    oid_to_hex(repl), oid_to_hex(oid));
 
-	if (!stat_sha1_file(the_repository, repl->hash, &st, &path))
+	if (!stat_sha1_file(r, repl->hash, &st, &path))
 		die(_("loose object %s (stored in %s) is corrupt"),
 		    oid_to_hex(repl), path);
 
-	if ((p = has_packed_and_bad(the_repository, repl->hash)) != NULL)
+	if ((p = has_packed_and_bad(r, repl->hash)) != NULL)
 		die(_("packed object %s (stored in %s) is corrupt"),
 		    oid_to_hex(repl), p->pack_name);
 
diff --git a/streaming.c b/streaming.c
index d1e6b2dce6..c843a1230f 100644
--- a/streaming.c
+++ b/streaming.c
@@ -490,7 +490,7 @@ static struct stream_vtbl incore_vtbl = {
 
 static open_method_decl(incore)
 {
-	st->u.incore.buf = read_object_file_extended(oid, type, &st->size, 0);
+	st->u.incore.buf = read_object_file_extended(the_repository, oid, type, &st->size, 0);
 	st->u.incore.read_ptr = 0;
 	st->vtbl = &incore_vtbl;
 
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 02/23] packfile: allow has_packed_and_bad to handle arbitrary repositories
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

has_packed_and_bad is not widely used, so just migrate it all at once.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 packfile.c  | 5 +++--
 packfile.h  | 2 +-
 sha1-file.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/packfile.c b/packfile.c
index 841b36182f..bc2e0f5043 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1131,12 +1131,13 @@ void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1)
 	p->num_bad_objects++;
 }
 
-const struct packed_git *has_packed_and_bad(const unsigned char *sha1)
+const struct packed_git *has_packed_and_bad(struct repository *r,
+					    const unsigned char *sha1)
 {
 	struct packed_git *p;
 	unsigned i;
 
-	for (p = the_repository->objects->packed_git; p; p = p->next)
+	for (p = r->objects->packed_git; p; p = p->next)
 		for (i = 0; i < p->num_bad_objects; i++)
 			if (hasheq(sha1,
 				   p->bad_object_sha1 + the_hash_algo->rawsz * i))
diff --git a/packfile.h b/packfile.h
index 442625723d..7a62d72231 100644
--- a/packfile.h
+++ b/packfile.h
@@ -146,7 +146,7 @@ extern int packed_object_info(struct repository *r,
 			      off_t offset, struct object_info *);
 
 extern void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);
-extern const struct packed_git *has_packed_and_bad(const unsigned char *sha1);
+extern const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);
 
 /*
  * Iff a pack file in the given repository contains the object named by sha1,
diff --git a/sha1-file.c b/sha1-file.c
index b8ce21cbaf..856e000ee1 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -1432,7 +1432,7 @@ void *read_object_file_extended(const struct object_id *oid,
 		die(_("loose object %s (stored in %s) is corrupt"),
 		    oid_to_hex(repl), path);
 
-	if ((p = has_packed_and_bad(repl->hash)) != NULL)
+	if ((p = has_packed_and_bad(the_repository, repl->hash)) != NULL)
 		die(_("packed object %s (stored in %s) is corrupt"),
 		    oid_to_hex(repl), p->pack_name);
 
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 01/23] sha1_file: allow read_object to read objects in arbitrary repositories
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller, Junio C Hamano
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>

Allow read_object (a file local functon in sha1_file) to
handle arbitrary repositories by passing the repository down
to oid_object_info_extended.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1-file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sha1-file.c b/sha1-file.c
index dd0b6aa873..b8ce21cbaf 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -1361,7 +1361,9 @@ int oid_object_info(struct repository *r,
 	return type;
 }
 
-static void *read_object(const unsigned char *sha1, enum object_type *type,
+static void *read_object(struct repository *r,
+			 const unsigned char *sha1,
+			 enum object_type *type,
 			 unsigned long *size)
 {
 	struct object_id oid;
@@ -1373,7 +1375,7 @@ static void *read_object(const unsigned char *sha1, enum object_type *type,
 
 	hashcpy(oid.hash, sha1);
 
-	if (oid_object_info_extended(the_repository, &oid, &oi, 0) < 0)
+	if (oid_object_info_extended(r, &oid, &oi, 0) < 0)
 		return NULL;
 	return content;
 }
@@ -1414,7 +1416,7 @@ void *read_object_file_extended(const struct object_id *oid,
 		lookup_replace_object(the_repository, oid) : oid;
 
 	errno = 0;
-	data = read_object(repl->hash, type, size);
+	data = read_object(the_repository, repl->hash, type, size);
 	if (data)
 		return data;
 
@@ -1755,7 +1757,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
 
 	if (has_loose_object(oid))
 		return 0;
-	buf = read_object(oid->hash, &type, &len);
+	buf = read_object(the_repository, oid->hash, &type, &len);
 	if (!buf)
 		return error(_("cannot read sha1_file for %s"), oid_to_hex(oid));
 	hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", type_name(type), len) + 1;
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 00/23] sb/more-repo-in-api
From: Stefan Beller @ 2018-12-15  0:09 UTC (permalink / raw)
  To: git; +Cc: Stefan Beller

I realized next has not been rewound, so I can resend sb/more-repo-in-api,
which I hereby do. The changes are minimal and address the only comment
by Jonathan so far.

Thanks,
Stefan

Stefan Beller (23):
  sha1_file: allow read_object to read objects in arbitrary repositories
  packfile: allow has_packed_and_bad to handle arbitrary repositories
  object-store: allow read_object_file_extended to read from any repo
  object-store: prepare read_object_file to deal with any repo
  object-store: prepare has_{sha1, object}_file to handle any repo
  object: parse_object to honor its repository argument
  commit: allow parse_commit* to handle any repo
  commit-reach.c: allow paint_down_to_common to handle any repo
  commit-reach.c: allow merge_bases_many to handle any repo
  commit-reach.c: allow remove_redundant to handle any repo
  commit-reach.c: allow get_merge_bases_many_0 to handle any repo
  commit-reach: prepare get_merge_bases to handle any repo
  commit-reach: prepare in_merge_bases[_many] to handle any repo
  commit: prepare get_commit_buffer to handle any repo
  commit: prepare repo_unuse_commit_buffer to handle any repo
  commit: prepare logmsg_reencode to handle arbitrary repositories
  pretty: prepare format_commit_message to handle arbitrary repositories
  submodule: use submodule repos for object lookup
  submodule: don't add submodule as odb for push
  commit-graph: convert remaining functions to handle any repo
  commit: prepare free_commit_buffer and release_commit_memory for any
    repo
  path.h: make REPO_GIT_PATH_FUNC repository agnostic
  t/helper/test-repository: celebrate independence from the_repository

 builtin/fsck.c                                |   3 +-
 builtin/log.c                                 |   6 +-
 builtin/rev-list.c                            |   3 +-
 cache.h                                       |   2 +
 commit-graph.c                                |  40 +++--
 commit-reach.c                                |  73 +++++----
 commit-reach.h                                |  38 +++--
 commit.c                                      |  41 ++---
 commit.h                                      |  43 +++++-
 .../coccinelle/the_repository.pending.cocci   | 144 ++++++++++++++++++
 object-store.h                                |  35 ++++-
 object.c                                      |   8 +-
 packfile.c                                    |   5 +-
 packfile.h                                    |   2 +-
 path.h                                        |   2 +-
 pretty.c                                      |  28 ++--
 pretty.h                                      |   7 +-
 sha1-file.c                                   |  34 +++--
 streaming.c                                   |   2 +-
 submodule.c                                   |  78 +++++++---
 t/helper/test-repository.c                    |  10 ++
 21 files changed, 454 insertions(+), 150 deletions(-)
 create mode 100644 contrib/coccinelle/the_repository.pending.cocci

 git range-diff origin/sb/more-repo-in-api... >>0000-cover-letter.patch
 
1:  99017ffac8 ! 1:  f24b120287 submodule: use submodule repos for object lookup
    @@ -40,12 +40,13 @@
     - * attempt to lookup both the left and right commits and put them into the
     - * left and right pointers.
     +/*
    -+ * Initialize 'out' based on the provided submodule path.
    ++ * Initialize a repository struct for a submodule based on the provided 'path'.
     + *
     + * Unlike repo_submodule_init, this tolerates submodules not present
     + * in .gitmodules. This function exists only to preserve historical behavior,
     + *
    -+ * Returns 0 on success, -1 when the submodule is not present.
    ++ * Returns the repository struct on success,
    ++ * NULL when the submodule is not present.
       */
     -static void show_submodule_header(struct diff_options *o, const char *path,
     +static struct repository *open_submodule(const char *path)
    @@ -59,6 +60,7 @@
     +		return NULL;
     +	}
     +
    ++	/* Mark it as a submodule */
     +	out->submodule_prefix = xstrdup(path);
     +
     +	strbuf_release(&sb);
2:  809765861c = 2:  25190d6174 submodule: don't add submodule as odb for push
3:  4a7735da72 = 3:  965421aab2 commit-graph: convert remaining functions to handle any repo
4:  aeeb1ba49e = 4:  bf31f32723 commit: prepare free_commit_buffer and release_commit_memory for any repo
5:  5ffebe9463 = 5:  c4e54e6b0d path.h: make REPO_GIT_PATH_FUNC repository agnostic
6:  9c89920c46 = 6:  a7ed0c57ba t/helper/test-repository: celebrate independence from the_repository

^ permalink raw reply

* [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree
From: Stefan Beller @ 2018-12-14 23:59 UTC (permalink / raw)
  To: gitster; +Cc: git, sbeller
In-Reply-To: <20181214235945.41191-1-sbeller@google.com>

74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by
ensure-core-worktree, 2018-08-13) missed to update the BUG message.
Fix it.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d38113a31a..31ac30cf2f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)
 	struct repository subrepo;
 
 	if (argc != 2)
-		BUG("submodule--helper connect-gitdir-workingtree <name> <path>");
+		BUG("submodule--helper ensure-core-worktree <path>");
 
 	path = argv[1];
 
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 4/4] submodule deinit: unset core.worktree
From: Stefan Beller @ 2018-12-14 23:59 UTC (permalink / raw)
  To: gitster; +Cc: git, sbeller
In-Reply-To: <20181214235945.41191-1-sbeller@google.com>

When a submodule is deinit'd, the working tree is gone, so the setting of
core.worktree is bogus. Unset it. As we covered the only other case in
which a submodule loses its working tree in the earlier step
(i.e. switching branches of top-level project to move to a commit that did
not have the submodule), this makes the code always maintain
core.worktree correctly unset when there is no working tree
for a submodule.

This re-introduces 984cd77ddb (submodule deinit: unset core.worktree,
2018-06-18), which was reverted as part of f178c13fda (Revert "Merge
branch 'sb/submodule-core-worktree'", 2018-09-07)

The whole series was reverted as the offending commit e98317508c
(submodule: ensure core.worktree is set after update, 2018-06-18)
was relied on by other commits such as 984cd77ddb.

Keep the offending commit reverted, but its functionality came back via
4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such
that we can reintroduce 984cd77ddb now.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 2 ++
 t/lib-submodule-update.sh   | 2 +-
 t/t7400-submodule-basic.sh  | 5 +++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 31ac30cf2f..672b74db89 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const char *prefix,
 		if (!(flags & OPT_QUIET))
 			printf(format, displaypath);
 
+		submodule_unset_core_worktree(sub);
+
 		strbuf_release(&sb_rm);
 	}
 
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 51d4555549..5b56b23166 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -235,7 +235,7 @@ reset_work_tree_to_interested () {
 	then
 		mkdir -p submodule_update/.git/modules/sub1/modules &&
 		cp -r submodule_update_repo/.git/modules/sub1/modules/sub2 submodule_update/.git/modules/sub1/modules/sub2
-		GIT_WORK_TREE=. git -C submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree
+		# core.worktree is unset for sub2 as it is not checked out
 	fi &&
 	# indicate we are interested in the submodule:
 	git -C submodule_update config submodule.sub1.url "bogus" &&
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 76a7cb0af7..aba2d4d6ee 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the whole submodule section
 	rmdir init
 '
 
+test_expect_success 'submodule deinit should unset core.worktree' '
+	test_path_is_file .git/modules/example/config &&
+	test_must_fail git config -f .git/modules/example/config core.worktree
+'
+
 test_expect_success 'submodule deinit from subdirectory' '
 	git submodule update --init &&
 	git config submodule.example.foo bar &&
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 2/4] submodule: unset core.worktree if no working tree is present
From: Stefan Beller @ 2018-12-14 23:59 UTC (permalink / raw)
  To: gitster; +Cc: git, sbeller
In-Reply-To: <20181214235945.41191-1-sbeller@google.com>

When a submodules work tree is removed, we should unset its core.worktree
setting as the worktree is no longer present. This is not just in line
with the conceptual view of submodules, but it fixes an inconvenience
for looking at submodules that are not checked out:

    git clone --recurse-submodules git://github.com/git/git && cd git &&
    git checkout --recurse-submodules v2.13.0
    git -C .git/modules/sha1collisiondetection log
    fatal: cannot chdir to '../../../sha1collisiondetection': \
        No such file or directory

With this patch applied, the final call to git log works instead of dying
in its setup, as the checkout will unset the core.worktree setting such
that following log will be run in a bare repository.

This patch covers all commands that are in the unpack machinery, i.e.
checkout, read-tree, reset. A follow up patch will address
"git submodule deinit", which will also make use of the new function
submodule_unset_core_worktree(), which is why we expose it in this patch.

This patch was authored as 4fa4f90ccd (submodule: unset core.worktree if
no working tree is present, 2018-06-12), which was reverted as part of
f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'",
2018-09-07). The revert was needed as the nearby commit e98317508c
(submodule: ensure core.worktree is set after update, 2018-06-18) is
faulty and at the time of 7e25437d35 (Merge branch
'sb/submodule-core-worktree', 2018-07-18) we could not revert the faulty
commit only, as they were depending on each other: If core.worktree is
unset, we have to have ways to ensure that it is set again once
the working tree reappears again.

Now that 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17),
specifically 74d4731da1 (submodule--helper: replace
connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13) is
present, we already check and ensure core.worktree is set when
populating a new work tree, such that we can re-introduce the commits
that unset core.worktree when removing the worktree.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c               | 14 ++++++++++++++
 submodule.h               |  2 ++
 t/lib-submodule-update.sh |  3 ++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 6415cc5580..d393e947e6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1561,6 +1561,18 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
 	return ret;
 }
 
+void submodule_unset_core_worktree(const struct submodule *sub)
+{
+	char *config_path = xstrfmt("%s/modules/%s/config",
+				    get_git_common_dir(), sub->name);
+
+	if (git_config_set_in_file_gently(config_path, "core.worktree", NULL))
+		warning(_("Could not unset core.worktree setting in submodule '%s'"),
+			  sub->path);
+
+	free(config_path);
+}
+
 static const char *get_super_prefix_or_empty(void)
 {
 	const char *s = get_super_prefix();
@@ -1726,6 +1738,8 @@ int submodule_move_head(const char *path,
 
 			if (is_empty_dir(path))
 				rmdir_or_warn(path);
+
+			submodule_unset_core_worktree(sub);
 		}
 	}
 out:
diff --git a/submodule.h b/submodule.h
index a680214c01..9e18e9b807 100644
--- a/submodule.h
+++ b/submodule.h
@@ -131,6 +131,8 @@ int submodule_move_head(const char *path,
 			const char *new_head,
 			unsigned flags);
 
+void submodule_unset_core_worktree(const struct submodule *sub);
+
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
  * a submodule by clearing any repo-specific environment variables, but
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 016391723c..51d4555549 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() {
 			git branch -t remove_sub1 origin/remove_sub1 &&
 			$command remove_sub1 &&
 			test_superproject_content origin/remove_sub1 &&
-			! test -e sub1
+			! test -e sub1 &&
+			test_must_fail git config -f .git/modules/sub1/config core.worktree
 		)
 	'
 	# ... absorbing a .git directory along the way.
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 1/4] submodule update: add regression test with old style setups
From: Stefan Beller @ 2018-12-14 23:59 UTC (permalink / raw)
  To: gitster; +Cc: git, sbeller
In-Reply-To: <20181214235945.41191-1-sbeller@google.com>

As f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'",
2018-09-07) was produced shortly before a release, nobody asked for
a regression test to be included. Add a regression test that makes sure
that the invocation of `git submodule update` on old setups doesn't
produce errors as pointed out in f178c13fda.

The place to add such a regression test may look odd in t7412, but
that is the best place as there we setup old style submodule setups
explicitly.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
index ce74c12da2..1cfa150768 100755
--- a/t/t7412-submodule-absorbgitdirs.sh
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' '
 	GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \
 		core.worktree "../../../nested" &&
 	# make sure this re-setup is correct
-	git status --ignore-submodules=none
+	git status --ignore-submodules=none &&
+
+	# also make sure this old setup does not regress
+	git submodule update --init --recursive >out 2>err &&
+	test_must_be_empty out &&
+	test_must_be_empty err
 '
 
 test_expect_success 'absorb the git dir in a nested submodule' '
-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply related

* [PATCH 0/4] submodules: unset core.worktree when no working tree present
From: Stefan Beller @ 2018-12-14 23:59 UTC (permalink / raw)
  To: gitster; +Cc: git, sbeller
In-Reply-To: <xmqqefas8ss4.fsf@gitster-ct.c.googlers.com>

v2:
I reworded the commit messages to explain the patches from the ground up
instead of only linking to the old commits, that got reverted.

> Just pretend that the ealier commits and their reversion never
> happened, and further pretend that we are doing the best thing that
> should happen to our codebase.

I disagree with that first stance (I can freely admit those commits happened),
but agree on the second point, so I explained why the code is the best
for the code base now. So I kept those pointers in there, too, to make it
easier for future code archeologists. 

v1:

A couple days before the 2.19 release we had a bug report about
broken submodules[1] and reverted[2] the commits leading up to them.

The behavior of said bug fixed itself by taking a different approach[3],
specifically by a weaker enforcement of having `core.worktree` set in a
submodule [4].

The revert [2] was overly broad as we neared the release, such that we wanted
to rather keep the known buggy behavior of always having `core.worktree` set,
rather than figuring out how to fix the new bug of having 'git submodule update'
not working in old style repository setups.

This series re-introduces those reverted patches, with no changes in code,
but with drastically changed commit messages, as those focus on why it is safe
to re-introduce them instead of explaining the desire for the change.

[1] https://public-inbox.org/git/2659750.rG6xLiZASK@twilight
[2] f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07)
[3] 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17)
[4] 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)
Stefan Beller (4):
  submodule update: add regression test with old style setups
  submodule: unset core.worktree if no working tree is present
  submodule--helper: fix BUG message in ensure_core_worktree
  submodule deinit: unset core.worktree

 builtin/submodule--helper.c        |  4 +++-
 submodule.c                        | 14 ++++++++++++++
 submodule.h                        |  2 ++
 t/lib-submodule-update.sh          |  5 +++--
 t/t7400-submodule-basic.sh         |  5 +++++
 t/t7412-submodule-absorbgitdirs.sh |  7 ++++++-
 6 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.20.0.405.gbc1bbc6f85-goog


^ permalink raw reply

* Re: [PATCH v4 4/6] revision: implement sparse algorithm
From: Ævar Arnfjörð Bjarmason @ 2018-12-14 23:32 UTC (permalink / raw)
  To: Derrick Stolee via GitGitGadget
  Cc: git, peff, jrnieder, Junio C Hamano, Derrick Stolee
In-Reply-To: <c44172c35ece7aafec02c7f3c8438ccca4f69023.1544822533.git.gitgitgadget@gmail.com>


On Fri, Dec 14 2018, Derrick Stolee via GitGitGadget wrote:

> Despite these potential drawbacks, the benefits of the algorithm
> are clear. By adding a counter to 'add_children_by_path' and
> 'mark_tree_contents_uninteresting', I measured the number of
> parsed trees for the two algorithms in a variety of repos.

Ah, so we take one or the other. I tested this with:

    diff --git a/revision.c b/revision.c
    index 63bf6230dc..e9c67aa550 100644
    --- a/revision.c
    +++ b/revision.c
    @@ -61,6 +61,8 @@ static void mark_tree_contents_uninteresting(struct repository *r,
            struct tree_desc desc;
            struct name_entry entry;

    +       fprintf(stderr, "MTCU\n");
    +
            if (parse_tree_gently(tree, 1) < 0)
                    return;

    @@ -166,6 +168,8 @@ static void add_children_by_path(struct repository *r,
            struct tree_desc desc;
            struct name_entry entry;

    +       fprintf(stderr, "ACBP\n");
    +
            if (!tree)
                    return;

And tried testing my pushing this branch to my git.git:

    $ for v in true false; do git push --delete avar push/sparse; ./git -c pack.usesparse=$v --exec-path=$PWD push avar HEAD 2>&1 | grep -e MTCU -e ACBP | uniq -c; done
    To github.com:avar/git.git
     - [deleted]               push/sparse
         22 ACBP
    To github.com:avar/git.git
     - [deleted]               push/sparse
        184 MTCU

But snipping around a bit from the commit message:

>When enumerating objects to place in a pack-file during 'git
>pack-objects --revs', we discover the "frontier" of commits
>that we care about and the boundary with commit we find
>uninteresting. From that point, we walk trees to discover which
>trees and blobs are uninteresting. Finally, we walk trees from the
>interesting commits to find the interesting objects that are
>placed in the pack.
>[...]
>These improvements will have even larger benefits in the super-
>large Windows repository. In our experiments, we see the
>"Enumerate objects" phase of pack-objects taking 60-80% of the
>end-to-end time of non-trivial pushes, taking longer than the
>network time to send the pack and the server time to verify the
>pack.

If I change my monkeypatch to:

    diff --git a/revision.c b/revision.c
    index 63bf6230dc..9171ca27c5 100644
    --- a/revision.c
    +++ b/revision.c
    @@ -63,0 +64,3 @@ static void mark_tree_contents_uninteresting(struct repository *r,
    +       fprintf(stderr, "MTCU\n");
    +       sleep(1);
    +
    @@ -168,0 +172,3 @@ static void add_children_by_path(struct repository *r,
    +       fprintf(stderr, "ACBP\n");
    +       sleep(1);
    +

We spend a long time printing those out before we ever get to
"Enumerating objects".

Which was where I was trying to test this, i.e. is this a lot of work we
perform before we print out the progress bar, and regardless of this
optimization should have other progress output there, so we can see this
time we're spending on this?

^ permalink raw reply

* Re: [PATCH 0/4] Expose gpgsig in pretty-print
From: John Passaro @ 2018-12-14 23:13 UTC (permalink / raw)
  To: Michał Górny; +Cc: git, Junio C Hamano, Alexey Shumkin, Jeff King
In-Reply-To: <CAJdN7KgWQyjrfifqNEr5SeHM0F1KzrKyoK0gy0AeTd-jPvMtCw@mail.gmail.com>

On Fri, Dec 14, 2018 at 6:10 PM John Passaro <john.a.passaro@gmail.com> wrote:
>
> On Fri, Dec 14, 2018 at 11:49 AM Michał Górny <mgorny@gentoo.org> wrote:
> >
> > On Fri, 2018-12-14 at 11:07 -0500, John Passaro wrote:
> > > On Thu, Dec 13, 2018 at 11:12 PM Michał Górny <mgorny@gentoo.org> wrote:
> > > >
> > > > On Thu, 2018-12-13 at 16:22 -0500, John Passaro wrote:
> > > > > Currently, users who do not have GPG installed have no way to discern
> > > > > signed from unsigned commits without examining raw commit data. I
> > > > > propose two new pretty-print placeholders to expose this information:
> > > > >
> > > > > %GR: full ("R"aw) contents of gpgsig header
> > > > > %G+: Y/N if the commit has nonempty gpgsig header or not
> > > > >
> > > > > The second is of course much more likely to be used, but having exposed
> > > > > the one, exposing the other too adds almost no complexity.
> > > > >
> > > > > I'm open to suggestion on the names of these placeholders.
> > > > >
> > > > > This commit is based on master but e5a329a279 ("run-command: report exec
> > > > > failure" 2018-12-11) is required for the tests to pass.
> > > > >
> > > > > One note is that this change touches areas of the pretty-format
> > > > > documentation that are radically revamped in aw/pretty-trailers: see
> > > > > 42617752d4 ("doc: group pretty-format.txt placeholders descriptions"
> > > > > 2018-12-08). I have another version of this branch based on that branch
> > > > > as well, so you can use that in case conflicts with aw/pretty-trailers
> > > > > arise.
> > > > >
> > > > > See:
> > > > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig
> > > > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig--based-on-aw-pretty-trailers
> > > > >
> > > > > John Passaro (4):
> > > > >   pretty: expose raw commit signature
> > > > >   t/t7510-signed-commit.sh: test new placeholders
> > > > >   doc, tests: pretty behavior when gpg missing
> > > > >   docs/pretty-formats: add explanation + copy edits
> > > > >
> > > > >  Documentation/pretty-formats.txt |  21 ++++--
> > > > >  pretty.c                         |  36 ++++++++-
> > > > >  t/t7510-signed-commit.sh         | 125 +++++++++++++++++++++++++++++--
> > > > >  3 files changed, 167 insertions(+), 15 deletions(-)
> > > > >
> > > > >
> > > > > base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
> > > > > prerequisite-patch-id: aedfe228fd293714d9cd0392ac22ff1cba7365db
> > > >
> > > > Just a suggestion: since the raw signature is not very useful without
> > > > the commit data to check it against, and the commit data is non-trivial
> > > > to construct (requires mangling raw data anyway), maybe you could either
> > > > add another placeholder to get the data for signature verification, or
> > > > (alternatively or simultaneously) add a placeholder that prints both
> > > > data and signature in the OpenPGP message format (i.e. something you can
> > > > pass straight to 'gpg --verify').
> > > >
> > >
> > > That's a great idea!
> > >
> > > Then I might rename the other new placeholders too:
> > >
> > > %Gs: signed commit signature (blank when unsigned)
> > > %Gp: signed commit payload (i.e. in practice minus the gpgsig header;
> > > also blank when unsigned as well)
> > > %Gq: query/question whether is signed commit ("Y"/"N")
> > >
> > > Thus establishing %G<lowercase> as the gpg-related placeholders that
> > > do not actually require gpg.
> > >
> > > And add a test that %Gp%n%Gs or the like passes gpg --verify.
> > >
> > > I'll put in a v2 later today or tomorrow. Thank you for the feedback!
> > >
> >
> > Technically speaking, '%Gp%n%Gs' sounds a bit odd, given that
> > the payload needs to be preceded by the PGP message header but instead
> > of footer it has the signature's header.  Also note that some lines in
> > the payload may need to be escaped.
>
> It's indeed failing with
> "-----BEGIN PGP SIGNED MESSAGE-----%n%Gp%n%Gs"
>
> This appears to be a misunderstanding on my part of how cleartext GPG
> messages work, as this message seems to fail verification even when I
> construct it manually:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> tree 3820419b50e9827493beedf6a1423e7d9c7edf3b
> parent 356f37356edf1a45c8494870e1c051e2fbe529fa
> author A U Thor <author@example.com> 1112912413 -0700
> committer C O Mitter <committer@example.com> 1112912533 -0700
>
> sixth
> -----BEGIN PGP SIGNATURE-----
>
> iHQEABECADQWIQRz11h0S+chaY7FTocTtvUezd5DDQUCXBQzKRYcY29tbWl0dGVy
> QGV4YW1wbGUuY29tAAoJEBO29R7N3kMN2QYAnA2A/VpD4qMI9rJlIYnyyO32rTXz
> AJ0drWlJsASMcf6AEX6nSQPxWq81Fg==
> =fr2F
> -----END PGP SIGNATURE-----
>
> I got this by running tho following inside the trash directory for t7510,
> which as far as I can tell is roughly equivalent to
> The gpg --verify fails.
> {
>   echo "-----BEGIN PGP SIGNED MESSAGE-----"
>   echo Hash: SHA256
>   echo
>   git cat-file commit sixth-signed | perl -ne '/^(?:gpgsig)? / || print'
>   git cat-file commit sixth-signed | perl -ne 's/^(?:gpgsig)? // && print'
> } | tee commit-as-signed-message | gpg --verify

I should add that when I took out Hash: SHA256 (don't ask), gpg went
from saying "signature digest conflict" to saying "BAD signature". Not
quite as bad but still confusing.
>
> All seems to work fine when I treat %Gs as a detached signature.
> How should the combined message be constructed properly? (Goes
> to usefulness of printing signature payload, and indeed of raw crypto
> data in general.)
>
>
> > --
> > Best regards,
> > Michał Górny

^ permalink raw reply

* Re: [PATCH 0/4] Expose gpgsig in pretty-print
From: John Passaro @ 2018-12-14 23:10 UTC (permalink / raw)
  To: Michał Górny; +Cc: git, Junio C Hamano, Alexey Shumkin, Jeff King
In-Reply-To: <1544806139.7371.1.camel@gentoo.org>

On Fri, Dec 14, 2018 at 11:49 AM Michał Górny <mgorny@gentoo.org> wrote:
>
> On Fri, 2018-12-14 at 11:07 -0500, John Passaro wrote:
> > On Thu, Dec 13, 2018 at 11:12 PM Michał Górny <mgorny@gentoo.org> wrote:
> > >
> > > On Thu, 2018-12-13 at 16:22 -0500, John Passaro wrote:
> > > > Currently, users who do not have GPG installed have no way to discern
> > > > signed from unsigned commits without examining raw commit data. I
> > > > propose two new pretty-print placeholders to expose this information:
> > > >
> > > > %GR: full ("R"aw) contents of gpgsig header
> > > > %G+: Y/N if the commit has nonempty gpgsig header or not
> > > >
> > > > The second is of course much more likely to be used, but having exposed
> > > > the one, exposing the other too adds almost no complexity.
> > > >
> > > > I'm open to suggestion on the names of these placeholders.
> > > >
> > > > This commit is based on master but e5a329a279 ("run-command: report exec
> > > > failure" 2018-12-11) is required for the tests to pass.
> > > >
> > > > One note is that this change touches areas of the pretty-format
> > > > documentation that are radically revamped in aw/pretty-trailers: see
> > > > 42617752d4 ("doc: group pretty-format.txt placeholders descriptions"
> > > > 2018-12-08). I have another version of this branch based on that branch
> > > > as well, so you can use that in case conflicts with aw/pretty-trailers
> > > > arise.
> > > >
> > > > See:
> > > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig
> > > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig--based-on-aw-pretty-trailers
> > > >
> > > > John Passaro (4):
> > > >   pretty: expose raw commit signature
> > > >   t/t7510-signed-commit.sh: test new placeholders
> > > >   doc, tests: pretty behavior when gpg missing
> > > >   docs/pretty-formats: add explanation + copy edits
> > > >
> > > >  Documentation/pretty-formats.txt |  21 ++++--
> > > >  pretty.c                         |  36 ++++++++-
> > > >  t/t7510-signed-commit.sh         | 125 +++++++++++++++++++++++++++++--
> > > >  3 files changed, 167 insertions(+), 15 deletions(-)
> > > >
> > > >
> > > > base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
> > > > prerequisite-patch-id: aedfe228fd293714d9cd0392ac22ff1cba7365db
> > >
> > > Just a suggestion: since the raw signature is not very useful without
> > > the commit data to check it against, and the commit data is non-trivial
> > > to construct (requires mangling raw data anyway), maybe you could either
> > > add another placeholder to get the data for signature verification, or
> > > (alternatively or simultaneously) add a placeholder that prints both
> > > data and signature in the OpenPGP message format (i.e. something you can
> > > pass straight to 'gpg --verify').
> > >
> >
> > That's a great idea!
> >
> > Then I might rename the other new placeholders too:
> >
> > %Gs: signed commit signature (blank when unsigned)
> > %Gp: signed commit payload (i.e. in practice minus the gpgsig header;
> > also blank when unsigned as well)
> > %Gq: query/question whether is signed commit ("Y"/"N")
> >
> > Thus establishing %G<lowercase> as the gpg-related placeholders that
> > do not actually require gpg.
> >
> > And add a test that %Gp%n%Gs or the like passes gpg --verify.
> >
> > I'll put in a v2 later today or tomorrow. Thank you for the feedback!
> >
>
> Technically speaking, '%Gp%n%Gs' sounds a bit odd, given that
> the payload needs to be preceded by the PGP message header but instead
> of footer it has the signature's header.  Also note that some lines in
> the payload may need to be escaped.

It's indeed failing with
"-----BEGIN PGP SIGNED MESSAGE-----%n%Gp%n%Gs"

This appears to be a misunderstanding on my part of how cleartext GPG
messages work, as this message seems to fail verification even when I
construct it manually:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

tree 3820419b50e9827493beedf6a1423e7d9c7edf3b
parent 356f37356edf1a45c8494870e1c051e2fbe529fa
author A U Thor <author@example.com> 1112912413 -0700
committer C O Mitter <committer@example.com> 1112912533 -0700

sixth
-----BEGIN PGP SIGNATURE-----

iHQEABECADQWIQRz11h0S+chaY7FTocTtvUezd5DDQUCXBQzKRYcY29tbWl0dGVy
QGV4YW1wbGUuY29tAAoJEBO29R7N3kMN2QYAnA2A/VpD4qMI9rJlIYnyyO32rTXz
AJ0drWlJsASMcf6AEX6nSQPxWq81Fg==
=fr2F
-----END PGP SIGNATURE-----

I got this by running tho following inside the trash directory for t7510,
which as far as I can tell is roughly equivalent to
The gpg --verify fails.
{
  echo "-----BEGIN PGP SIGNED MESSAGE-----"
  echo Hash: SHA256
  echo
  git cat-file commit sixth-signed | perl -ne '/^(?:gpgsig)? / || print'
  git cat-file commit sixth-signed | perl -ne 's/^(?:gpgsig)? // && print'
} | tee commit-as-signed-message | gpg --verify

All seems to work fine when I treat %Gs as a detached signature.
How should the combined message be constructed properly? (Goes
to usefulness of printing signature payload, and indeed of raw crypto
data in general.)


> --
> Best regards,
> Michał Górny

^ permalink raw reply

* Re: Git blame performance on files with a lot of history
From: Ævar Arnfjörð Bjarmason @ 2018-12-14 22:48 UTC (permalink / raw)
  To: Clement Moyroud; +Cc: git
In-Reply-To: <CABXAcUzoNJ6s3=2xZfWYQUZ_AUefwP=5UVUgMnafKHHtufzbSA@mail.gmail.com>


On Fri, Dec 14 2018, Clement Moyroud wrote:

> My group at work is migrating a CVS repo to Git. The biggest issue we
> face so far is the performance of git blame, especially compared to
> CVS on the same file. One file especially causes us trouble: it's a
> 30k lines file with 25 years of history in 3k+ commits. The complete
> repo has 200k+ commits over that same period of time.

There's a real-world repo with a shape & size very similar to this that
has good performance, gcc.git: https://github.com/gcc-mirror/gcc

    $ wc -l ChangeLog
    20240 ChangeLog
    $ git log --oneline -- ChangeLog | wc -l
    2676
    $ git log --oneline | wc -l
    165309
    $ time git blame ChangeLog >/dev/null

    real    0m1.977s
    user    0m1.909s
    sys     0m0.069s

Its history began in 1997, and the changes to the ChangeLog file by its
nature is fairly evenly spread through that period.

So check out that repo to see if you have similar or worse
performance. Does your work repo show the same problem with a history
produced with 'git fast-export --anonymize', and if so is that something
you'd be OK with sharing?

^ permalink raw reply

* Re: [PATCH v5 1/1] protocol: advertise multiple supported versions
From: Ævar Arnfjörð Bjarmason @ 2018-12-14 22:39 UTC (permalink / raw)
  To: Josh Steadmon
  Cc: git, gitster, sbeller, jonathantanmy, szeder.dev,
	Johannes Schindelin, Jonathan Nieder, Jeff King
In-Reply-To: <20181214215804.GF37614@google.com>


On Fri, Dec 14 2018, Josh Steadmon wrote:

> On 2018.12.14 21:20, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Fri, Nov 16 2018, Josh Steadmon wrote:
>>
>> I started looking at this to address
>> https://public-inbox.org/git/nycvar.QRO.7.76.6.1812141318520.43@tvgsbejvaqbjf.bet/
>> and was about to send a re-roll of my own series, but then...
>>
>> > Currently the client advertises that it supports the wire protocol
>> > version set in the protocol.version config. However, not all services
>> > support the same set of protocol versions. For example, git-receive-pack
>> > supports v1 and v0, but not v2. If a client connects to git-receive-pack
>> > and requests v2, it will instead be downgraded to v0. Other services,
>> > such as git-upload-archive, do not do any version negotiation checks.
>> >
>> > This patch creates a protocol version registry. Individual client and
>> > server programs register all the protocol versions they support prior to
>> > communicating with a remote instance. Versions should be listed in
>> > preference order; the version specified in protocol.version will
>> > automatically be moved to the front of the registry.
>> >
>> > The protocol version registry is passed to remote helpers via the
>> > GIT_PROTOCOL environment variable.
>> >
>> > Clients now advertise the full list of registered versions. Servers
>> > select the first allowed version from this advertisement.
>> >
>> > Additionally, remove special cases around advertising version=0.
>> > Previously we avoided adding version negotiation fields in server
>> > responses if it looked like the client wanted v0. However, including
>> > these fields does not change behavior, so it's better to have simpler
>> > code.
>>
>> ...this paragraph is new in your v5, from the cover letter: "Changes
>> from V4: remove special cases around advertising version=0". However as
>> seen in the code & tests:
>>
>> > [...]
>> >  static void push_ssh_options(struct argv_array *args, struct argv_array *env,
>> >  			     enum ssh_variant variant, const char *port,
>> > -			     enum protocol_version version, int flags)
>> > +			     const struct strbuf *version_advert, int flags)
>> >  {
>> > -	if (variant == VARIANT_SSH &&
>> > -	    version > 0) {
>> > +	if (variant == VARIANT_SSH) {
>> >  		argv_array_push(args, "-o");
>> >  		argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
>> > -		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
>> > -				 version);
>> > +		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
>> > +				 version_advert->buf);
>> >  	}
>> > [...]
>> > --- a/t/t5601-clone.sh
>> > +++ b/t/t5601-clone.sh
>> > @@ -346,7 +346,7 @@ expect_ssh () {
>> >
>> >  test_expect_success 'clone myhost:src uses ssh' '
>> >  	git clone myhost:src ssh-clone &&
>> > -	expect_ssh myhost src
>> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL" myhost src
>> >  '
>> >
>> >  test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '
>> > @@ -357,12 +357,12 @@ test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '
>> >
>> >  test_expect_success 'bracketed hostnames are still ssh' '
>> >  	git clone "[myhost:123]:src" ssh-bracket-clone &&
>> > -	expect_ssh "-p 123" myhost src
>> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
>> >  '
>> >
>> >  test_expect_success 'OpenSSH variant passes -4' '
>> >  	git clone -4 "[myhost:123]:src" ssh-ipv4-clone &&
>> > -	expect_ssh "-4 -p 123" myhost src
>> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL -4 -p 123" myhost src
>> >  '
>> >
>> >  test_expect_success 'variant can be overridden' '
>> > @@ -406,7 +406,7 @@ test_expect_success 'OpenSSH-like uplink is treated as ssh' '
>> >  	GIT_SSH="$TRASH_DIRECTORY/uplink" &&
>> >  	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" &&
>> >  	git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
>> > -	expect_ssh "-p 123" myhost src
>> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
>> >  '
>> >
>> >  test_expect_success 'plink is treated specially (as putty)' '
>> > @@ -446,14 +446,14 @@ test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
>> >  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
>> >  	GIT_SSH_VARIANT=ssh \
>> >  	git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
>> > -	expect_ssh "-p 123" myhost src
>> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
>> >  '
>> >
>> >  test_expect_success 'ssh.variant overrides plink detection' '
>> >  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
>> >  	git -c ssh.variant=ssh \
>> >  		clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
>> > -	expect_ssh "-p 123" myhost src
>> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
>> >  '
>> >
>> >  test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
>> > @@ -488,7 +488,7 @@ test_clone_url () {
>> >  }
>> >
>> >  test_expect_success !MINGW 'clone c:temp is ssl' '
>> > -	test_clone_url c:temp c temp
>> > +	test_clone_url c:temp "-o SendEnv=GIT_PROTOCOL" c temp
>> >  '
>> >
>> >  test_expect_success MINGW 'clone c:temp is dos drive' '
>> > @@ -499,7 +499,7 @@ test_expect_success MINGW 'clone c:temp is dos drive' '
>> >  for repo in rep rep/home/project 123
>> >  do
>> >  	test_expect_success "clone host:$repo" '
>> > -		test_clone_url host:$repo host $repo
>> > +		test_clone_url host:$repo "-o SendEnv=GIT_PROTOCOL" host $repo
>> >  	'
>> >  done
>> >
>> > @@ -507,16 +507,16 @@ done
>> >  for repo in rep rep/home/project 123
>> >  do
>> >  	test_expect_success "clone [::1]:$repo" '
>> > -		test_clone_url [::1]:$repo ::1 "$repo"
>> > +		test_clone_url [::1]:$repo "-o SendEnv=GIT_PROTOCOL" ::1 "$repo"
>> >  	'
>> >  done
>> >  #home directory
>> >  test_expect_success "clone host:/~repo" '
>> > -	test_clone_url host:/~repo host "~repo"
>> > +	test_clone_url host:/~repo "-o SendEnv=GIT_PROTOCOL" host "~repo"
>> >  '
>> >
>> >  test_expect_success "clone [::1]:/~repo" '
>> > -	test_clone_url [::1]:/~repo ::1 "~repo"
>> > +	test_clone_url [::1]:/~repo "-o SendEnv=GIT_PROTOCOL" ::1 "~repo"
>> >  '
>> >
>> >  # Corner cases
>> > @@ -532,22 +532,22 @@ done
>> >  for tcol in "" :
>> >  do
>> >  	test_expect_success "clone ssh://host.xz$tcol/home/user/repo" '
>> > -		test_clone_url "ssh://host.xz$tcol/home/user/repo" host.xz /home/user/repo
>> > +		test_clone_url "ssh://host.xz$tcol/home/user/repo" "-o SendEnv=GIT_PROTOCOL" host.xz /home/user/repo
>> >  	'
>> >  	# from home directory
>> >  	test_expect_success "clone ssh://host.xz$tcol/~repo" '
>> > -	test_clone_url "ssh://host.xz$tcol/~repo" host.xz "~repo"
>> > +	test_clone_url "ssh://host.xz$tcol/~repo" "-o SendEnv=GIT_PROTOCOL" host.xz "~repo"
>> >  '
>> >  done
>> >
>> >  # with port number
>> >  test_expect_success 'clone ssh://host.xz:22/home/user/repo' '
>> > -	test_clone_url "ssh://host.xz:22/home/user/repo" "-p 22 host.xz" "/home/user/repo"
>> > +	test_clone_url "ssh://host.xz:22/home/user/repo" "-o SendEnv=GIT_PROTOCOL -p 22 host.xz" "/home/user/repo"
>> >  '
>> >
>> >  # from home directory with port number
>> >  test_expect_success 'clone ssh://host.xz:22/~repo' '
>> > -	test_clone_url "ssh://host.xz:22/~repo" "-p 22 host.xz" "~repo"
>> > +	test_clone_url "ssh://host.xz:22/~repo" "-o SendEnv=GIT_PROTOCOL -p 22 host.xz" "~repo"
>> >  '
>> >
>> >  #IPv6
>> > @@ -555,7 +555,7 @@ for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] [user@::
>> >  do
>> >  	ehost=$(echo $tuah | sed -e "s/1]:/1]/" | tr -d "[]")
>> >  	test_expect_success "clone ssh://$tuah/home/user/repo" "
>> > -	  test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
>> > +	  test_clone_url ssh://$tuah/home/user/repo '-o SendEnv=GIT_PROTOCOL' $ehost /home/user/repo
>> >  	"
>> >  done
>> >
>> > @@ -564,7 +564,7 @@ for tuah in ::1 [::1] user@::1 user@[::1] [user@::1]
>> >  do
>> >  	euah=$(echo $tuah | tr -d "[]")
>> >  	test_expect_success "clone ssh://$tuah/~repo" "
>> > -	  test_clone_url ssh://$tuah/~repo $euah '~repo'
>> > +	  test_clone_url ssh://$tuah/~repo '-o SendEnv=GIT_PROTOCOL' $euah '~repo'
>> >  	"
>> >  done
>> >
>> > @@ -573,7 +573,7 @@ for tuah in [::1] user@[::1] [user@::1]
>> >  do
>> >  	euah=$(echo $tuah | tr -d "[]")
>> >  	test_expect_success "clone ssh://$tuah:22/home/user/repo" "
>> > -	  test_clone_url ssh://$tuah:22/home/user/repo '-p 22' $euah /home/user/repo
>> > +	  test_clone_url ssh://$tuah:22/home/user/repo '-o SendEnv=GIT_PROTOCOL -p 22' $euah /home/user/repo
>> >  	"
>> >  done
>> >
>> > @@ -582,7 +582,7 @@ for tuah in [::1] user@[::1] [user@::1]
>> >  do
>> >  	euah=$(echo $tuah | tr -d "[]")
>> >  	test_expect_success "clone ssh://$tuah:22/~repo" "
>> > -	  test_clone_url ssh://$tuah:22/~repo '-p 22' $euah '~repo'
>> > +	  test_clone_url ssh://$tuah:22/~repo '-o SendEnv=GIT_PROTOCOL -p 22' $euah '~repo'
>> >  	"
>> >  done
>>
>> ...so now we're unconditionally going to SendEnv=GIT_PROTOCOL to "ssh"
>> invocations. I don't have an issue with this, but given the change in
>> the commit message this seems to have snuck under the radar. You're just
>> talking about always including the version in server responses, nothing
>> about the client always needing SendEnv=GIT_PROTOCOL now even with v0.
>
> Ack. I'll make sure that V6 calls this out in the commit message.
>
>
>> If the server always sends the version now, why don't you need to amend
>> the same t5400-send-pack.sh tests as in my "tests: mark & fix tests
>> broken under GIT_TEST_PROTOCOL_VERSION=1"? There's one that spews out
>> "version" there under my GIT_TEST_PROTOCOL_VERSION=1.
>
> Sorry if I'm being dense, but can you point out more specifically what
> is wrong here? I don't see anything in t5400 that would be affected by
> this; the final test case ("receive-pack de-dupes .have lines") is the
> only one that does any tracing, and it strips out everything that's not
> a ref advertisement before checking the output. Sorry if I'm missing
> something obvious.

I think I'm just misunderstanding this bit:

    Additionally, remove special cases around advertising version=0.
    Previously we avoided adding version negotiation fields in server
    responses if it looked like the client wanted v0. However, including
    these fields does not change behavior, so it's better to have
    simpler code.

I meant that if you pick the series I have up to "tests: add a special
setup that sets protocol.version", which is at c6f33984fc in a WIP
branch in github.com/avar/git.git and run:

    $ GIT_TEST_PROTOCOL_VERSION=1 ./t5400-send-pack.sh -v -i -x

It'll fail with:

    + test_cmp expect refs
    + diff -u expect refs
    --- expect      2018-12-14 22:26:52.485411992 +0000
    +++ refs        2018-12-14 22:26:52.713412148 +0000
    @@ -1,3 +1,4 @@
    +version 1
     5285e1710002a06a379056b0d21357a999f3c642 refs/heads/master
     5285e1710002a06a379056b0d21357a999f3c642 refs/remotes/origin/HEAD
     5285e1710002a06a379056b0d21357a999f3c642 refs/remotes/origin/master
    error: last command exited with $?=1
    not ok 16 - receive-pack de-dupes .have lines

And I read your commit message to mean "v0 clients also support v1
responses with the version in them, so let's just always send it". But I
think this is my confusion (but I still don't know what it *really*
means).

>> I was worried about this breaking GIT_SSH_COMMAND, but then I see due to
>> an interaction with picking "what ssh implementation?" we don't pass "-G
>> -o SendEnv=GIT_PROTOCOL" at all when I have a GIT_SSH_COMMAND, but *do*
>> pass it to my normal /usr/bin/ssh. Is this intended? Now if I have a
>> GIT_SSH_COMMAND that expects to wrap openssh I need to pass "-c
>> ssh.variant=ssh", because "-c ssh.variant=auto" will now omit these new
>> arguments.
>
> I am not an expert on this part of the code, but it seems to be
> intended. Later on in the function, there are BUG()s that state that
> VARIANT_AUTO should not be passed to the push_ssh_options() function.
> And this only changes the behavior when version=0. For protocol versions
> 1 & 2, VARIANT_AUTO never had SendEnv=GIT_PROTOCOL added to the command
> line. Again, sorry if I'm missing some implication here.

I'm wondering if we need to worry about some new odd interactions
between client/servers when using GIT_SSH* wrappers, maybe not, but
e.g. as opposed to 0da0e49ba1 ("ssh: 'auto' variant to select between
'ssh' and 'simple'", 2017-11-20) which noted a similar change had been
tested at Google internally (and I read it as Google using GIT_SSH* on
workstations) your commit doesn't make any mention of this case being
tested / considered.

So do we need to worry about some new interaction here? Maybe not. Just
something for people more experienced with the ssh integration to chime
in on.

^ permalink raw reply

* Re: [PATCH v5 1/1] protocol: advertise multiple supported versions
From: Josh Steadmon @ 2018-12-14 21:58 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, gitster, sbeller, jonathantanmy, szeder.dev,
	Johannes Schindelin, Jonathan Nieder, Jeff King
In-Reply-To: <87imzv273e.fsf@evledraar.gmail.com>

On 2018.12.14 21:20, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Nov 16 2018, Josh Steadmon wrote:
> 
> I started looking at this to address
> https://public-inbox.org/git/nycvar.QRO.7.76.6.1812141318520.43@tvgsbejvaqbjf.bet/
> and was about to send a re-roll of my own series, but then...
> 
> > Currently the client advertises that it supports the wire protocol
> > version set in the protocol.version config. However, not all services
> > support the same set of protocol versions. For example, git-receive-pack
> > supports v1 and v0, but not v2. If a client connects to git-receive-pack
> > and requests v2, it will instead be downgraded to v0. Other services,
> > such as git-upload-archive, do not do any version negotiation checks.
> >
> > This patch creates a protocol version registry. Individual client and
> > server programs register all the protocol versions they support prior to
> > communicating with a remote instance. Versions should be listed in
> > preference order; the version specified in protocol.version will
> > automatically be moved to the front of the registry.
> >
> > The protocol version registry is passed to remote helpers via the
> > GIT_PROTOCOL environment variable.
> >
> > Clients now advertise the full list of registered versions. Servers
> > select the first allowed version from this advertisement.
> >
> > Additionally, remove special cases around advertising version=0.
> > Previously we avoided adding version negotiation fields in server
> > responses if it looked like the client wanted v0. However, including
> > these fields does not change behavior, so it's better to have simpler
> > code.
> 
> ...this paragraph is new in your v5, from the cover letter: "Changes
> from V4: remove special cases around advertising version=0". However as
> seen in the code & tests:
> 
> > [...]
> >  static void push_ssh_options(struct argv_array *args, struct argv_array *env,
> >  			     enum ssh_variant variant, const char *port,
> > -			     enum protocol_version version, int flags)
> > +			     const struct strbuf *version_advert, int flags)
> >  {
> > -	if (variant == VARIANT_SSH &&
> > -	    version > 0) {
> > +	if (variant == VARIANT_SSH) {
> >  		argv_array_push(args, "-o");
> >  		argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
> > -		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
> > -				 version);
> > +		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
> > +				 version_advert->buf);
> >  	}
> > [...]
> > --- a/t/t5601-clone.sh
> > +++ b/t/t5601-clone.sh
> > @@ -346,7 +346,7 @@ expect_ssh () {
> >
> >  test_expect_success 'clone myhost:src uses ssh' '
> >  	git clone myhost:src ssh-clone &&
> > -	expect_ssh myhost src
> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL" myhost src
> >  '
> >
> >  test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '
> > @@ -357,12 +357,12 @@ test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '
> >
> >  test_expect_success 'bracketed hostnames are still ssh' '
> >  	git clone "[myhost:123]:src" ssh-bracket-clone &&
> > -	expect_ssh "-p 123" myhost src
> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
> >  '
> >
> >  test_expect_success 'OpenSSH variant passes -4' '
> >  	git clone -4 "[myhost:123]:src" ssh-ipv4-clone &&
> > -	expect_ssh "-4 -p 123" myhost src
> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL -4 -p 123" myhost src
> >  '
> >
> >  test_expect_success 'variant can be overridden' '
> > @@ -406,7 +406,7 @@ test_expect_success 'OpenSSH-like uplink is treated as ssh' '
> >  	GIT_SSH="$TRASH_DIRECTORY/uplink" &&
> >  	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" &&
> >  	git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
> > -	expect_ssh "-p 123" myhost src
> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
> >  '
> >
> >  test_expect_success 'plink is treated specially (as putty)' '
> > @@ -446,14 +446,14 @@ test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
> >  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
> >  	GIT_SSH_VARIANT=ssh \
> >  	git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
> > -	expect_ssh "-p 123" myhost src
> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
> >  '
> >
> >  test_expect_success 'ssh.variant overrides plink detection' '
> >  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
> >  	git -c ssh.variant=ssh \
> >  		clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
> > -	expect_ssh "-p 123" myhost src
> > +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
> >  '
> >
> >  test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
> > @@ -488,7 +488,7 @@ test_clone_url () {
> >  }
> >
> >  test_expect_success !MINGW 'clone c:temp is ssl' '
> > -	test_clone_url c:temp c temp
> > +	test_clone_url c:temp "-o SendEnv=GIT_PROTOCOL" c temp
> >  '
> >
> >  test_expect_success MINGW 'clone c:temp is dos drive' '
> > @@ -499,7 +499,7 @@ test_expect_success MINGW 'clone c:temp is dos drive' '
> >  for repo in rep rep/home/project 123
> >  do
> >  	test_expect_success "clone host:$repo" '
> > -		test_clone_url host:$repo host $repo
> > +		test_clone_url host:$repo "-o SendEnv=GIT_PROTOCOL" host $repo
> >  	'
> >  done
> >
> > @@ -507,16 +507,16 @@ done
> >  for repo in rep rep/home/project 123
> >  do
> >  	test_expect_success "clone [::1]:$repo" '
> > -		test_clone_url [::1]:$repo ::1 "$repo"
> > +		test_clone_url [::1]:$repo "-o SendEnv=GIT_PROTOCOL" ::1 "$repo"
> >  	'
> >  done
> >  #home directory
> >  test_expect_success "clone host:/~repo" '
> > -	test_clone_url host:/~repo host "~repo"
> > +	test_clone_url host:/~repo "-o SendEnv=GIT_PROTOCOL" host "~repo"
> >  '
> >
> >  test_expect_success "clone [::1]:/~repo" '
> > -	test_clone_url [::1]:/~repo ::1 "~repo"
> > +	test_clone_url [::1]:/~repo "-o SendEnv=GIT_PROTOCOL" ::1 "~repo"
> >  '
> >
> >  # Corner cases
> > @@ -532,22 +532,22 @@ done
> >  for tcol in "" :
> >  do
> >  	test_expect_success "clone ssh://host.xz$tcol/home/user/repo" '
> > -		test_clone_url "ssh://host.xz$tcol/home/user/repo" host.xz /home/user/repo
> > +		test_clone_url "ssh://host.xz$tcol/home/user/repo" "-o SendEnv=GIT_PROTOCOL" host.xz /home/user/repo
> >  	'
> >  	# from home directory
> >  	test_expect_success "clone ssh://host.xz$tcol/~repo" '
> > -	test_clone_url "ssh://host.xz$tcol/~repo" host.xz "~repo"
> > +	test_clone_url "ssh://host.xz$tcol/~repo" "-o SendEnv=GIT_PROTOCOL" host.xz "~repo"
> >  '
> >  done
> >
> >  # with port number
> >  test_expect_success 'clone ssh://host.xz:22/home/user/repo' '
> > -	test_clone_url "ssh://host.xz:22/home/user/repo" "-p 22 host.xz" "/home/user/repo"
> > +	test_clone_url "ssh://host.xz:22/home/user/repo" "-o SendEnv=GIT_PROTOCOL -p 22 host.xz" "/home/user/repo"
> >  '
> >
> >  # from home directory with port number
> >  test_expect_success 'clone ssh://host.xz:22/~repo' '
> > -	test_clone_url "ssh://host.xz:22/~repo" "-p 22 host.xz" "~repo"
> > +	test_clone_url "ssh://host.xz:22/~repo" "-o SendEnv=GIT_PROTOCOL -p 22 host.xz" "~repo"
> >  '
> >
> >  #IPv6
> > @@ -555,7 +555,7 @@ for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] [user@::
> >  do
> >  	ehost=$(echo $tuah | sed -e "s/1]:/1]/" | tr -d "[]")
> >  	test_expect_success "clone ssh://$tuah/home/user/repo" "
> > -	  test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
> > +	  test_clone_url ssh://$tuah/home/user/repo '-o SendEnv=GIT_PROTOCOL' $ehost /home/user/repo
> >  	"
> >  done
> >
> > @@ -564,7 +564,7 @@ for tuah in ::1 [::1] user@::1 user@[::1] [user@::1]
> >  do
> >  	euah=$(echo $tuah | tr -d "[]")
> >  	test_expect_success "clone ssh://$tuah/~repo" "
> > -	  test_clone_url ssh://$tuah/~repo $euah '~repo'
> > +	  test_clone_url ssh://$tuah/~repo '-o SendEnv=GIT_PROTOCOL' $euah '~repo'
> >  	"
> >  done
> >
> > @@ -573,7 +573,7 @@ for tuah in [::1] user@[::1] [user@::1]
> >  do
> >  	euah=$(echo $tuah | tr -d "[]")
> >  	test_expect_success "clone ssh://$tuah:22/home/user/repo" "
> > -	  test_clone_url ssh://$tuah:22/home/user/repo '-p 22' $euah /home/user/repo
> > +	  test_clone_url ssh://$tuah:22/home/user/repo '-o SendEnv=GIT_PROTOCOL -p 22' $euah /home/user/repo
> >  	"
> >  done
> >
> > @@ -582,7 +582,7 @@ for tuah in [::1] user@[::1] [user@::1]
> >  do
> >  	euah=$(echo $tuah | tr -d "[]")
> >  	test_expect_success "clone ssh://$tuah:22/~repo" "
> > -	  test_clone_url ssh://$tuah:22/~repo '-p 22' $euah '~repo'
> > +	  test_clone_url ssh://$tuah:22/~repo '-o SendEnv=GIT_PROTOCOL -p 22' $euah '~repo'
> >  	"
> >  done
> 
> ...so now we're unconditionally going to SendEnv=GIT_PROTOCOL to "ssh"
> invocations. I don't have an issue with this, but given the change in
> the commit message this seems to have snuck under the radar. You're just
> talking about always including the version in server responses, nothing
> about the client always needing SendEnv=GIT_PROTOCOL now even with v0.

Ack. I'll make sure that V6 calls this out in the commit message.


> If the server always sends the version now, why don't you need to amend
> the same t5400-send-pack.sh tests as in my "tests: mark & fix tests
> broken under GIT_TEST_PROTOCOL_VERSION=1"? There's one that spews out
> "version" there under my GIT_TEST_PROTOCOL_VERSION=1.

Sorry if I'm being dense, but can you point out more specifically what
is wrong here? I don't see anything in t5400 that would be affected by
this; the final test case ("receive-pack de-dupes .have lines") is the
only one that does any tracing, and it strips out everything that's not
a ref advertisement before checking the output. Sorry if I'm missing
something obvious.


> I was worried about this breaking GIT_SSH_COMMAND, but then I see due to
> an interaction with picking "what ssh implementation?" we don't pass "-G
> -o SendEnv=GIT_PROTOCOL" at all when I have a GIT_SSH_COMMAND, but *do*
> pass it to my normal /usr/bin/ssh. Is this intended? Now if I have a
> GIT_SSH_COMMAND that expects to wrap openssh I need to pass "-c
> ssh.variant=ssh", because "-c ssh.variant=auto" will now omit these new
> arguments.

I am not an expert on this part of the code, but it seems to be
intended. Later on in the function, there are BUG()s that state that
VARIANT_AUTO should not be passed to the push_ssh_options() function.
And this only changes the behavior when version=0. For protocol versions
1 & 2, VARIANT_AUTO never had SendEnv=GIT_PROTOCOL added to the command
line. Again, sorry if I'm missing some implication here.


Thanks,
Josh

^ permalink raw reply

* Re: Bug in lineendings handling that prevents resetting checking out, rebasing etc
From: Mr&Mrs D @ 2018-12-14 21:51 UTC (permalink / raw)
  To: tboegi; +Cc: git
In-Reply-To: <20181214213246.GA2182@tor.lan>

Thanks for looking to it - git attributes was added in
4b0863a8b834c5804fd3a568ed56ff85b27acdeb

The file in question was added in 17ca75f0a8c25f321f2b63bc3b9c065ff91adc23

So you mean to say that because a gitattributes was added after the
fact this resulted in an illegal state?

But _shouldn't git reset --hard work anyway?_ That's the buggy part.

As for fixing it - not sure what is the best course of action here.
probably issuing `git add --renormalize .` and committing that to the
stable (dev) branch will fix this for future checkouts/rebases but
IIUC won't do nothing for older commits - so checking out a commit
before the fix one, ghit will see this file as changed and then
completely refuse to go back to another branch

This seems a bug - as illegal as the state of the file is, shouldn't
git reset always work?

Thanks! (will be out for a bit but really looking forward to your replies)
On Fri, Dec 14, 2018 at 4:32 PM Torsten Bögershausen <tboegi@web.de> wrote:
>
> On Fri, Dec 14, 2018 at 04:04:15PM -0500, Mr&Mrs D wrote:
> > Hi all,
> >
> > I maintain a python project you can clone from:
> >
> > git@github.com:wrye-bash/wrye-bash.git
> >
> > For reasons unknown git sees a particular file as changed
> > (Mopy/Docs/Bash Readme Template.html, sometimes others too). This file
> > was probably committed to the svn repository this git repo was created
> > from with CRLF line endings. When we moved to git we added a
> > gitattributes file (
> > https://github.com/wrye-bash/wrye-bash/blob/dev/.gitattributes ) and
> > this file was edited to explicitly state htms are text - all to no
> > avail. From time to time - on windows - as in when checking out an old
> > commit - git would see that file as changed. The only workaround that
> > worked for me was
> >
> >     git rm -r . --cached -q && git reset --hard
> >
> > For more details and discussion see this SO question I posted almost
> > five years ago:
> >
> > https://stackoverflow.com/questions/21122094/git-line-endings-cant-stash-reset-and-now-cant-rebase-over-spurious-line-en
> >
> > I used to work in windows and the bug was tolerable as there was that
> > workaround. Now I moved to mac and no workaround works anymore - we
> > have a special page on our wiki  with workarounds for this one btw:
> >
> > https://github.com/wrye-bash/wrye-bash/wiki/%5Bgit%5D-Issues-with-line-endings-preventing-checking,-merge,-etc
> >
> > Well after 5 years and countless hours trying to solve this I reach
> > out to you guys and girls - _this is a full-time bug in git line
> > endings handling_. When someone issues a git reset --hard this should
> > work no matter what - well it does not. So this bug may be really a
> > can of worms.
> >
> > Please someone clone this repo on linux or mac - probably just cloning
> > will have the files appear as changed (by the way hitting refresh on
> > git gui I have different sets of files appear as changed). If not then
> >
> > git checkout utumno-wip
> Thet commit is -excuse me if that sounds too harsh- is messed up.
> git status says
> modified:   Mopy/Docs/Bash Readme Template.html
>
> And if I dig into the EOL stuff, I run
> git ls-files --eol | grep  Readme | less
>
> And find a contradiction here:
> i/crlf  w/crlf  attr/text               Mopy/Docs/Bash Readme Template.html
>
> The attributes say "text" and the file has CRLF "in the repo",
> (techically speaking in the index) and that is an "illegal" condition
> in the repo, and not a bug in Git.
> I didn't try the rebase as such, sice the first problem needs
> to be fixed, before we try to move on.
>
> So, the old commits are problematic/illegal and they are as they are.
> Such a commit can not be fixed, whenever somebody checks it out,
> there will be a problem (or two, or none, depending on the timing,
> the file system...)
>
> We can not fix commits like b1acc012878c9fdd8b4ad610ce7eae0dcbcbcab0.
> We can make new commits, and fix them.
>
> We can fix one branch, and other branches, and merge them together.
> But rebase seems to be problamatic, at least to me.
> What exactly do you want to do?
>
> Can we agree to do a merge of 2 branches?
> Then I can possibly help you out.
>
>
>
>
>
> > git rebase -i dev
> >
> > and then select a commit to edit should be enough to trigger this bug
> >
> > Needless to say I am  well aware of things like `git add --renormalize
> > .` - but renormalizing is not the issue. The issue is that _files show
> > as changed and even a git reset --hard won't convince git that
> > nothing's changed_.
> >
> > $ git reset --hard
> > HEAD is now at e5c16790 Wip proper handling of ini tweaks encoding - TODOs:
> > $ git status
> > interactive rebase in progress; onto 02ae6f26
> > Last commands done (4 commands done):
> >    pick 3a39a0c0 Monkey patch for undecodable inis:
> >    pick e5c16790 Wip proper handling of ini tweaks encoding - TODOs:
> >   (see more in file .git/rebase-merge/done)
> > Next commands to do (19 remaining commands):
> >    edit a3a7b237 Amend last commit and linefixes:  ΕΕΕΕ
> >    edit 432fd314 fFF handle empty or malformed inis
> >   (use "git rebase --edit-todo" to view and edit)
> > You are currently editing a commit while rebasing branch 'utumno-wip'
> > on '02ae6f26'.
> >   (use "git commit --amend" to amend the current commit)
> >   (use "git rebase --continue" once you are satisfied with your changes)
> >
> > Changes not staged for commit:
> >   (use "git add <file>..." to update what will be committed)
> >   (use "git checkout -- <file>..." to discard changes in working directory)
> >
> > modified:   Mopy/Docs/Bash Readme Template.html
> >
> > Untracked files:
> >   (use "git add <file>..." to include in what will be committed)
> >
> > .DS_Store
> > .idea.7z
> >
> > no changes added to commit (use "git add" and/or "git commit -a")
> > $
> >
> > I really hope someone here can debug this
> > Thanks!

^ permalink raw reply

* [PATCH] doc: improve grammar in git-update-index
From: Anthony Sottile @ 2018-12-14 21:25 UTC (permalink / raw)
  To: git; +Cc: Anthony Sottile

Signed-off-by: Anthony Sottile <asottile@umich.edu>
---
 Documentation/git-update-index.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index 1c4d146a4..9c03ca167 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -326,7 +326,7 @@ inefficient `lstat(2)`.  If your filesystem is one of them, you
 can set "assume unchanged" bit to paths you have not changed to
 cause Git not to do this check.  Note that setting this bit on a
 path does not mean Git will check the contents of the file to
-see if it has changed -- it makes Git to omit any checking and
+see if it has changed -- it means Git will skip any checking and
 assume it has *not* changed.  When you make changes to working
 tree files, you have to explicitly tell Git about it by dropping
 "assume unchanged" bit, either before or after you modify them.
-- 
2.17.1


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox