Re: [PATCH 1/1] rebase --stat: fix when rebasing to an unrelated history

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> The built-in version of the `git rebase` command blindly translated that
> shell script code, assuming that there is no need to test whether there
> *was* a merge base, and due to its better error checking, exited with a
> fatal error (because it tried to read the object with hash 00000000...
> as a tree).

And the scripted version produced a wrong result because it did not
check the lack of merge base and fed an empty string (presumably, as
that is what you would get from mb=$(merge-base A B) when A and B
are unrelated) to later steps?  If that is the case, then it *is* a
very significant thing to record here.  As it was not merely "failed
to stop due to lack of error checking", but a lot worse.  It was
producing a wrong result.

A more faithful reimplementation would not have exited with fatal
error, but would have produced the same wrong result, so "a better
error checking caused the reimplementation die where the original
did not die" may be correct but is much less important than the fact
that "the logic used in the original to produce diffstat forgot that
there may not be a merge base and would not have worked correctly in
such a case, and that is what we are correcting" is more important.

Please descibe the issue as such, if that is the case (I cannot read
from the description in the proposed log message if that is the
case---but I came to the conclusion that it is what you wanted to
fix reading the code).

>  		if (options.flags & REBASE_VERBOSE)
>  			printf(_("Changes from %s to %s:\n"),
> -				oid_to_hex(&merge_base),
> +				is_null_oid(&merge_base) ?
> +				"(empty)" : oid_to_hex(&merge_base),

I am not sure (empty) is a good idea for two reasons.  It is going
to be inserted into an translated string without translation.  Also
it is too similar to the fallback string used by some printf
implementations when NULL was given to %s by mistake.

If there weren't i18n issues, I would suggest to use "empty merge
base" or "empty tree" instead, but the old one would have shown
0{40}, so perhaps empty_tree_oid_hex() is a good substitute?

> @@ -1494,8 +1495,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
>  		opts.detect_rename = DIFF_DETECT_RENAME;
>  		diff_setup_done(&opts);
> -		diff_tree_oid(&merge_base, &options.onto->object.oid,
> -			      "", &opts);
> +		diff_tree_oid(is_null_oid(&merge_base) ?
> +			      the_hash_algo->empty_tree : &merge_base,
> +			      &options.onto->object.oid, "", &opts);

The original would have run "git diff '' $onto", and died with an
error message, so both the original and the reimplementation were
failing.  Just in different ways ;-)

> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index b97ffdc9dd..be3b241676 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -718,10 +718,12 @@ if test -n "$diffstat"
>  then
>  	if test -n "$verbose"
>  	then
> -		echo "$(eval_gettext "Changes from \$mb to \$onto:")"
> +		mb_display="${mb:-(empty)}"
> +		echo "$(eval_gettext "Changes from \$mb_display to \$onto:")"
>  	fi
> +	mb_tree="${mb:-$(git hash-object -t tree /dev/null)}"
>  	# We want color (if set), but no pager
> -	GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
> +	GIT_PAGER='' git diff --stat --summary "$mb_tree" "$onto"

Code fix for diff-tree invocation, both in the builtin/ version and
this one, looks correct.