Re: [PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1

Jonathan Nieder [off-list ref] writes:

When v2.18.0-rc0~10^2~1 (refspec: consolidate ref-prefix generation
logic, 2018-05-16) factored out the ref-prefix generation code for
reuse, it left out the 'if (!item->exact_sha1)' test in the original
ref-prefix generation code. As a result, fetches by SHA-1 generate
ref-prefixes as though the SHA-1 being fetched were an abbreviated ref
name:

 $ GIT_TRACE_PACKET=1 bin-wrappers/git -c protocol.version=2 \
	fetch origin 12039e008f9a4e3394f3f94f8ea897785cb09448
[...]
 packet:        fetch> ref-prefix 12039e008f9a4e3394f3f94f8ea897785cb09448
 packet:        fetch> ref-prefix refs/12039e008f9a4e3394f3f94f8ea897785cb09448
 packet:        fetch> ref-prefix refs/tags/12039e008f9a4e3394f3f94f8ea897785cb09448
 packet:        fetch> ref-prefix refs/heads/12039e008f9a4e3394f3f94f8ea897785cb09448
 packet:        fetch> ref-prefix refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448
 packet:        fetch> ref-prefix refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448/HEAD
 packet:        fetch> 0000

If there is another ref name on the command line or the object being
fetched is already available locally, then that's mostly harmless.
But otherwise, we error out with

 fatal: no matching remote head

since the server did not send any refs we are interested in.  Filter
out the exact_sha1 refspecs to avoid this.

This patch adds a test to check this behavior that notices another
behavior difference between protocol v0 and v2 in the process.  Add a
NEEDSWORK comment to clear it up.

Thanks.

I wonder if there is a more effective way to smoke out other bugs
remaining in proto v2.  When the fetch-by-SHA1 feature was added
originally, we certainly would have added a test or two to make sure
it won't break.  The root cause of this breakage is that we lack the
ability to easily exercise proto v2 on these existing tests that
were written back in the proto v0 days.  It there were such a way
(like, a common set of tests that are run with all supported
protos), we would have caught the breakge even before the topic hit
'next'.

Junio C Hamano wrote:

Jonathan Nieder [off-list ref] writes:

quoted

This patch adds a test to check this behavior that notices another
behavior difference between protocol v0 and v2 in the process.  Add a
NEEDSWORK comment to clear it up.

Thanks.

I wonder if there is a more effective way to smoke out other bugs
remaining in proto v2.  When the fetch-by-SHA1 feature was added
originally, we certainly would have added a test or two to make sure
it won't break.  The root cause of this breakage is that we lack the
ability to easily exercise proto v2 on these existing tests that
were written back in the proto v0 days.  It there were such a way
(like, a common set of tests that are run with all supported
protos), we would have caught the breakge even before the topic hit
'next'.

I had a similar thought.

I am not sure I agree about the root cause, but root causes are
generally slippery to define.  Because this bug had significant
internal impact, we came up with a few next steps:

- shore up protocol v2 test coverage, as you described

- arrange for long refactoring series we submit to be divided up for
  the team to review, to avoid reviewer fatigue.  Hopefully this will
  make us a better example for other submitters of long series.  We're
  open to cooperating with others --- maybe we can set up a volunteer
  reviewer brigade to get a more diverse set of eyes on each series
  --- though organizing that is harder.

- improve telemetry for our internal deployment, to get earlier notice
  when Git is producing more errors.  I suspect other installations
  may want something like this too --- e.g. I think this is one of the
  benefits of what Jeff Hostetler is starting to build with json-writer.

- help internal users triage errors from Git (like those decision
  trees parents have to help decide when to bring a child to the
  doctor), so that we get earlier notice and can roll back and report
  upstream more quickly when they've run into a Git bug

Or in other words, please expect more in this area soon, and feel free
to pester me if the test coverage doesn't arrive. :)

Thanks,
Jonathan