| Message ID | 680385e4bd5c34a5fcf9651a776c52db61557652.1543317195.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix git rebase --stat -i <unrelated-history> | expand |
"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.
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
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > 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> Ah, that's excellent. Thanks.
diff --git a/builtin/rebase.c b/builtin/rebase.c index 5b3e5baec8..9e4b0b564f 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1483,7 +1483,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) 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), oid_to_hex(&options.onto->object.oid)); /* We want color (if set), but no pager */ @@ -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); diffcore_std(&opts); diff_flush(&opts); } 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" fi test -n "$interactive_rebase" && run_specific_rebase diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh index 38bd876cab..a1ee912118 100755 --- a/t/t3406-rebase-message.sh +++ b/t/t3406-rebase-message.sh @@ -91,4 +91,14 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' test_i18ngrep "Invalid whitespace option" err ' +test_expect_success 'rebase -i onto unrelated history' ' + git init unrelated && + test_commit -C unrelated 1 && + git -C unrelated remote add -f origin "$PWD" && + git -C unrelated branch --set-upstream-to=origin/master && + git -C unrelated -c core.editor=true rebase -i -v --stat >actual && + test_i18ngrep "Changes from (empty)" actual && + test_i18ngrep "5 files changed" actual +' + test_done