Re: [PATCH] Use 'fast-forward' all over the place

[Junio C Hamano <gitster@pobox.com>]

This is the latter half of my review; please see

    From:	Junio C Hamano <gitster@pobox.com>
    Subject: Re: [PATCH] Use 'fast-forward' all over the place
    Date:	Sat, 24 Oct 2009 10:50:05 -0700
    Message-ID: <7v3a587kc2.fsf@alter.siamese.dyndns.org>

for the other half.

Felipe Contreras <felipe.contreras@gmail.com> writes:

> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c

Ok.  We may want to consider moving this command to contrib/examples/ or
removing it, but that is a separate issue and can be done after we apply
this patch.

> diff --git a/builtin-fetch.c b/builtin-fetch.c
> @@ -269,7 +269,7 @@ static int update_local_ref(struct ref *ref,
>  		strcpy(quickref, find_unique_abbrev(current->object.sha1, DEFAULT_ABBREV));
>  		strcat(quickref, "..");
>  		strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
> -		r = s_update_ref("fast forward", ref, 1);
> +		r = s_update_ref("fast-forward", ref, 1);

This is creating a message in the reflog; I do not think we have any
script/program in-tree that relies on the exact wording of this one, so we
are probably safe ourselves, but I do not know about third-party scripts.

I'd be surprised if there are somebody who reads the reflog and acts
differently upon seeing that we fast-forwarded, but we'd never know until
we cook this in 'next' and see people complain.

The other hunk to this file is an end-user message from Porcelain, so it
should be Ok.

> diff --git a/builtin-merge.c b/builtin-merge.c
> diff --git a/builtin-push.c b/builtin-push.c
> @@ -159,7 +159,7 @@ static int do_push(const char *repo, int flags)
>  		error("failed to push some refs to '%s'", url[i]);
>  		if (nonfastforward && advice_push_nonfastforward) {
>  			printf("To prevent you from losing history, non-fast-forward updates were rejected\n"
> -			       "Merge the remote changes before pushing again.  See the 'non-fast forward'\n"
> +			       "Merge the remote changes before pushing again.  See the 'non-fast-forward'\n"
>  			       "section of 'git push --help' for details.\n");
>  		}

Ok, except that I think we will be seeing merge conflicts with changes
that are pending for 1.7.0 with this hunk---I am not looking forward to
it, but I'll survive with help from rerere.

> diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
> diff --git a/builtin-remote.c b/builtin-remote.c
> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> diff --git a/contrib/examples/git-merge.sh b/contrib/examples/git-merge.sh
> diff --git a/contrib/examples/git-resolve.sh b/contrib/examples/git-resolve.sh

Ok.

> diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
> @@ -315,8 +315,8 @@ generate_update_branch_email()
>  	# "remotes/" will be ignored as well.
>  
>  	# List all of the revisions that were removed by this update, in a
> -	# fast forward update, this list will be empty, because rev-list O
> -	# ^N is empty.  For a non fast forward, O ^N is the list of removed
> +	# fast-forward update, this list will be empty, because rev-list O
> +	# ^N is empty.  For a non fast-forward, O ^N is the list of removed

Wasn't non-fast-forward a single compound word of three?

> @@ -411,7 +411,7 @@ generate_update_branch_email()
>  	# revision because the base is effectively a random revision at this
>  	# point - the user will be interested in what this revision changed
>  	# - including the undoing of previous revisions in the case of
> -	# non-fast forward updates.
> +	# non-fast-forward updates.
>  	echo ""
>  	echo "Summary of changes:"
>  	git diff-tree --stat --summary --find-copies-harder $oldrev..$newrev

... like this hunk, that is.

> diff --git a/git-gui/lib/branch_create.tcl b/git-gui/lib/branch_create.tcl
> index 3817771..f1235c7 100644
> --- a/git-gui/lib/branch_create.tcl
> +++ b/git-gui/lib/branch_create.tcl
> @@ -77,7 +77,7 @@ constructor dialog {} {
>  		-variable @opt_merge
>  	pack $w.options.merge.no -side left
>  	radiobutton $w.options.merge.ff \
> -		-text [mc "Fast Forward Only"] \
> +		-text [mc "Fast-forward Only"] \
>  		-value ff \
>  		-variable @opt_merge
>  	pack $w.options.merge.ff -side left

Please leave git-gui out; (1) it is not managed by me and the patch should
be fed to Shawn separately, and (2) updating [mc] keystrings must need
matching updates to the translation file and the templates.

I also think this is a label string and the capitalization must be kept,
i.e. "Fast-Forward Only".

> diff --git a/git-merge-octopus.sh b/git-merge-octopus.sh
> diff --git a/git-pull.sh b/git-pull.sh
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> diff --git a/git-rebase.sh b/git-rebase.sh

Ok.

> diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
> @@ -51,7 +51,7 @@ check_cache_at () {
>  }
>  
>  cat >bozbar-old <<\EOF
> -This is a sample file used in two-way fast forward merge
> +This is a sample file used in two-way fast-forward merge
>  tests.  Its second line ends with a magic word bozbar
>  which will be modified by the merged head to gnusto.
>  It has some extra lines so that external tools can

Doesn't changing this change the actual test blob used?  Do you know if
the test still passes when git is not broken?

The rest of the patches to t/ directory looked Ok.

> diff --git a/transport.c b/transport.c
> diff --git a/unpack-trees.c b/unpack-trees.c

Ok.

Thanks.