Re: [PATCH] Add explanatory comment for transport-helpers refs mapping.

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 17, 2012, 16:04 UTC
Message-ID: <20120717160429.GG3071@burratino>
In-Reply-To: <13702454.DmcNg44yyH@flobuntu>

Hi,

Florian Achleitner wrote:

Show 20 quoted lines

> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -484,8 +484,18 @@ static int fetch_with_import(struct transport *transport,
>  		if (posn->status & REF_STATUS_UPTODATE)
>  			continue;
>  		if (data->refspecs)
> +			/*
> +			 * If the remote-helper advertised the refpec capability, we
> +			 * retrieve the local, private ref from it. The imported data is
> +			 * expected there. (see Documentation/git-remote-helpers.*).
> +			 */
>  			private = apply_refspecs(data->refspecs, data->refspec_nr, posn-
>>name);
>  		else
> +			/*
> +			 * else, the default refspec *:* is implied. The remote-helper has
> +			 * to import the remote heads directly to the local heads.
> +			 * remote-helpers using 'import' should have the refspec capability.
> +			 */
>  			private = xstrdup(posn->name);

What is _exactly_ the information the reader would want to know here? Looking at this code:

		char *private;
		posn = to_fetch[i];
		if (posn->status & REF_STATUS_UPTODATE)
			continue;
		if (!data->refspecs)
			private = xstrdup(...);
		else
			private = apply_refspecs(...);
		if (!private)
			continue;
		read_ref(private, posn->old_sha1);
		free(private);

Despite the misleading "old_sha1" name, this loop runs after fast-import has finished, and the values being read into to_fetch[]::old_sha1 are object names representing the result.

Callers such as builtin/fetch.c then use these values to write feedback to the terminal, to populate FETCH_HEAD, and to determine what new value peer_ref should get.

Shouldn't the comment say something about these SHA-1s not actually being old? Something like:

	/*
	 * If the remote helper advertised the "refspec" capability,
	 * it will have the written result of the import to the refs
	 * named on the right hand side of the first refspec matching
	 * each ref we were fetching.
	 *
	 * (If no "refspec" capability was specified, for historical
	 * reasons we default to *:*.)
	 *
	 * Store the result in to_fetch[i].old_sha1.  Callers such
	 * as "git fetch" can use the value to write feedback to the
	 * terminal, populate FETCH_HEAD, and determine what new value
	 * should be written to peer_ref if the update is a
	 * fast-forward or this is a forced update.
	 */
	for (i = 0; ...

Hmm? Jonathan