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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 25, 2009, 03:41 UTC
Message-ID: <7v1vks2l8q.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1256373092-15126-2-git-send-email-felipe.contreras@gmail.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.

Show 7 quoted lines

> 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.

Show 10 quoted lines

> 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.

Show 5 quoted lines

> 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.

Show 9 quoted lines

> 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?

Show 9 quoted lines

> @@ -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.

Show 13 quoted lines

> 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.

Show 10 quoted lines

> 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.