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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Thu, 29 Nov 2018, Junio C Hamano wrote:

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

Indeed. But it was only in the `--stat` reporting, so it did not produce
an incorrect rebased history.

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

True.

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

Indeed, my commit message is a bit too close to what I fixed, and not
enough about what needed to be fixed.

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

Oooops.

> Also it is too similar to the fallback string used by some printf
> implementations when NULL was given to %s by mistake.

You mean `(null)`? That was actually intentional, I just thought that
`(empty)` would be different enough...

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

As this is a user-facing message, I'd rather avoid something like
`empty_tree_oid_hex()`, which to every Git user who does not happen to be
a Git developer, too, must sound very cryptic.

But I guess that I should not be so lazy and really use two different
messages here:

	Changes from <merge-base> to <onto>

and if there is no merge base,

	Changes in <onto>

Will fix.

> > @@ -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 ;-)

Right.

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

Okay. I fixed the two things you pointed out, just waiting for the Linux
phase to finish (I don't think there is anything OS-specific in this
patch, so I trust macOS and Windows to pass if Linux passes):
https://git-for-windows.visualstudio.com/git/_build/results?buildId=26116

Ciao,
Dscho