Am 27.10.23 um 13:16 schrieb Lior Zeltzer:

The reproduction ,as I wrote in the code, should be done with few threads in parallel
Each working on a list of ~10 repos with each repo containing a lot of refs/tags (~1000)
All this should be against gerrit (my gerrit is 3.8.0)

Also read the notes below regarding the code that was moved between 2.31.8 and 2.32.0 in ls-remote.c file
I can elaborate more, in a zoom meeting.

10x
Lior.


-----Original Message-----
From: Bagas Sanjaya <redacted>
Sent: Friday, October 27, 2023 4:08 AM
To: Lior Zeltzer <redacted>; Git Mailing List <redacted>
Cc: Andrzej Hunt <redacted>; Ævar Arnfjörð Bjarmason <redacted>; Junio C Hamano <redacted>
Subject: [EXT] Re: ls-remote bug

External Email

----------------------------------------------------------------------
On Tue, Oct 24, 2023 at 10:55:24AM +0000, Lior Zeltzer wrote:

quoted

quoted

uname -a

Linux dc3lp-veld0045 3.10.0-1160.21.1.el7.x86_64 #1 SMP Tue Mar 16
18:28:22 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Gerrit version :
3.8.0

Bug description :
When running ls-remote : sometime data gets cut in the middle

Reproducing :
You need a few files with a few repo names (I used 4 files with 10
repos each) Call then l1..l4 And the code below just cd into each of
them does ls-remote twice and compares the data Doing it in parallel
on all lists.
Data received in both ls-remotes should be the same , if not, it
prints *** Repos should contain a lot of tags and refs

What repo did you find this regression? Did you mean linux.git (Linux kernel)?

quoted

Note :
1.  without stderr redirection (2>&1) all works well 2. On local repos
(not through gerrit) all works well

I compared various git vers and found the bug to be between 2.31.8 and
2.32.0 Comparing ls-remote.c file between those vers gave me :

Lines :
if (transport_disconnect(transport))
		return 1;

moved to end of sub

copying ls-remote.c from 2.31.8 to 2.32.0 - fixed the bug

This partly undoes 68ffe095a2 (transport: also free remote_refs in
transport_disconnect(), 2021-03-21).  With that patch connections are
kept open during ref sorting and printing.

Perhaps the other side gets tired of waiting and aborts?  Maybe
splitting transport_disconnect() into two functions -- one for
disconnecting and and for cleaning up -- would make sense? And
disconnecting as soon as possible?  Just guessing -- didn't actually
reproduce the bug.  Still, demo patch below.

quoted

Code reproducing bug :

#!/proj/mislcad/areas/DAtools/tools/perl/5.10.1/bin/perl -w use
strict; use Cwd qw(cwd);

my $count = 4;
for my $f (1..$count) {
  my $child = fork();
  if (!$child) {
    my $curr = cwd();
   
    my @repos = `cat l$f`;
    foreach my $repo (@repos) {
      chomp $repo;
      print "$repo\n";
      chdir($repo);
      my $remote_tags_str = `git ls-remote  2>&1`;
      my $remote_tags_str2 = `git ls-remote  2>&1 `;
      chdir($curr);
      if ( $remote_tags_str ne $remote_tags_str2) {
         print "***\n";
      }
    }
 
    exit(0);
  }
}
while (wait != -1) {}
1;

I tried reproducing this regression by:

$ cd /path/to/git.git
$ git ls-remote 2>&1 > /tmp/root.list
$ cd builtin/
$ git ls-remote 2>&1 > /tmp/builtin.list $ cd ../ $ git diff --no-index /tmp/root.list /tmp/builtin.list ```

And indeed, the diff was empty (which meant that both listings are same).

Confused...

--
An old man doll... just what I always wanted! - Clara

---
 builtin/ls-remote.c |  8 +++++---
 builtin/remote.c    |  3 ++-
 transport.c         | 15 +++++++++++++--
 transport.h         |  2 ++
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index fc76575430..4c1daa0f92 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c

@@ -128,6 +128,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
 		repo_set_hash_algo(the_repository, hash_algo);
 	}
+	if (transport_disconnect_raw(transport))
+		status = 1;

 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);

@@ -154,12 +156,12 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		if (show_symref_target && ref->symref)
 			printf("ref: %s\t%s\n", ref->symref, ref->refname);
 		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
-		status = 0; /* we found something */
+		if (status != 1)
+			status = 0; /* we found something */
 	}

 	ref_array_clear(&ref_array);
-	if (transport_disconnect(transport))
-		status = 1;
+	transport_clear(transport);
 	transport_ls_refs_options_release(&transport_options);
 	return status;
 }

diff --git a/builtin/remote.c b/builtin/remote.c
index d91bbe728d..055a221942 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c

@@ -1000,6 +1000,7 @@ static int get_remote_ref_states(const char *name,
 		transport = transport_get(states->remote, states->remote->url_nr > 0 ?
 			states->remote->url[0] : NULL);
 		remote_refs = transport_get_remote_refs(transport, NULL);
+		transport_disconnect_raw(transport);

 		states->queried = 1;
 		if (query & GET_REF_STATES)

@@ -1008,7 +1009,7 @@ static int get_remote_ref_states(const char *name,
 			get_head_names(remote_refs, states);
 		if (query & GET_PUSH_REF_STATES)
 			get_push_ref_states(remote_refs, states);
-		transport_disconnect(transport);
+		transport_clear(transport);
 	} else {
 		for_each_ref(append_ref_to_tracked_list, states);
 		string_list_sort(&states->tracked);

diff --git a/transport.c b/transport.c
index 219af8fd50..c71dab75e9 100644
--- a/transport.c
+++ b/transport.c

@@ -1584,17 +1584,28 @@ int transport_connect(struct transport *transport, const char *name,
 		die(_("operation not supported by protocol"));
 }

-int transport_disconnect(struct transport *transport)
+int transport_disconnect_raw(struct transport *transport)
 {
 	int ret = 0;
 	if (transport->vtable->disconnect)
 		ret = transport->vtable->disconnect(transport);
+	return ret;
+}
+
+void transport_clear(struct transport *transport)
+{
 	if (transport->got_remote_refs)
 		free_refs((void *)transport->remote_refs);
 	clear_bundle_list(transport->bundles);
 	free(transport->bundles);
 	free(transport);
-	return ret;
+}
+
+int transport_disconnect(struct transport *transport)
+{
+	int ret = transport_disconnect_raw(transport);
+	transport_clear(transport);
+	return 0;
 }

 /*

diff --git a/transport.h b/transport.h
index 6393cd9823..fd75905568 100644
--- a/transport.h
+++ b/transport.h

@@ -320,6 +320,8 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs);
 void transport_unlock_pack(struct transport *transport, unsigned int flags);

 int transport_disconnect(struct transport *transport);
+void transport_clear(struct transport *transport);
+int transport_disconnect_raw(struct transport *transport);
 char *transport_anonymize_url(const char *url);
 void transport_take_over(struct transport *transport,
 			 struct child_process *child);
--

2.42.0