Re: [PATCH 1/1] rebase --stat: fix when rebasing to an unrelated history
Hi Junio,
On Thu, 29 Nov 2018, Junio C Hamano wrote:
Show 16 quoted lines
> "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.
Show 7 quoted lines
> 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).
Indeed, my commit message is a bit too close to what I fixed, and not enough about what needed to be fixed.
Show 8 quoted lines
> > 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.
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.
Show 13 quoted lines
> > @@ -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 ;-)
Show 19 quoted lines
> > 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
Johannes Schindelin via GitGitGadget