Re: [PATCH v2 2/2] Porcelain scripts: Rewrite cryptic "needs update" error message
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 30, 2010, 21:08 UTC
- Message-ID
- <7vzkuzgcs6.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1285877017-8060-3-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 13 quoted lines
> diff --git a/git-pull.sh b/git-pull.sh
> index 8eb74d4..a15b545 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -201,10 +201,7 @@ test true = "$rebase" && {
> die "updating an unborn branch with changes added to the index"
> fi
> else
> - git update-index --ignore-submodules --refresh &&
> - git diff-files --ignore-submodules --quiet &&
> - git diff-index --ignore-submodules --cached --quiet HEAD -- ||
> - die "refusing to pull with rebase: your working tree is not up-to-date"
> + require_clean_work_tree "pull with rebase"Ok, this wants both the working tree and the index clean, and it will exit with non-zero. The original message mentioned "working tree" but by calling require_clean_work_tree the user won't see that word anymore.
Show 16 quoted lines
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index a27952d..9546975 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -153,14 +153,6 @@ run_pre_rebase_hook () {
> fi
> }
>
> -require_clean_work_tree () {
> - # test if working tree is dirty
> - git rev-parse --verify HEAD > /dev/null &&
> - git update-index --ignore-submodules --refresh &&
> - git diff-files --quiet --ignore-submodules &&
> - git diff-index --cached --quiet HEAD --ignore-submodules -- ||
> - die "Working tree is dirty"
> -}We used to test and exit early if HEAD cannot be read (e.g. empty history), but now require_clean_work_tree() will probably spit a cryptic error from diff-index it makes. Don't you need to have an equivalent check for HEAD somewhere before you make the first call to r-c-w-t?
Show 12 quoted lines
> @@ -557,12 +549,9 @@ do_next () {
> exit "$status"
> fi
> # Run in subshell because require_clean_work_tree can die.
> - if ! (require_clean_work_tree)
> + if ! (require_clean_work_tree "rebase")
> then
> - warn "Commit or stash your changes, and then run"
> - warn
> - warn " git rebase --continue"
> - warn
> + warn "Then run git rebase --continue."We will see something like:
cannot rebase: you have unstaged changes.
M Makefile
cannot rebase: you index contains uncommitted changes.
M Makefile
M hello.c
M goodbye.c
... 147 other paths that make the above scroll away ...
Please commit or stash them.
Then run git rebase --continue.Is this what we really want? Also the messages seem somewhat case-challenged.
Show 9 quoted lines
> exit 1 > fi > ;; > @@ -740,7 +729,7 @@ do > die "Cannot read HEAD" > git update-index --ignore-submodules --refresh && > git diff-files --quiet --ignore-submodules || > - die "Working tree is dirty" > + die "Working tree is dirty. Please commit or stash your changes to proceed."
I wonder if it is a good advice to choose between committing and stashing.
This codepath is for --continue, and by definition when rebase started there wasn't any local modification (otherwise it wouldn't have started and we wouldn't have come this far), so the change must have come from the end user who wanted to amend (or resolve conflicts), thought that s/he included all the changes s/he did in the working tree in the amended commit and told us to continue. We however found some leftover local modifications.
"Please commit or stash" is probably the worst advice we can give in this particular situation. It is likely to be something s/he forgot to add; the existing advice that says "Please commit them first and then ..." you can see a few lines down this part is probably better.
In other codepaths, aborting rebase and cleaning the work area first before attempting a rebase might be a better choice than either committing or stashing. Often, I find that is the least confusing choise, at least.
Show 22 quoted lines
> diff --git a/git-rebase.sh b/git-rebase.sh > index 3335cee..c3ca8d5 100755 > --- a/git-rebase.sh > +++ b/git-rebase.sh > @@ -416,19 +416,7 @@ else > fi > fi > > -# The tree must be really really clean. > -if ! git update-index --ignore-submodules --refresh > /dev/null; then > - echo >&2 "cannot rebase: you have unstaged changes" > - git diff-files --name-status -r --ignore-submodules -- >&2 > - exit 1 > -fi > -diff=$(git diff-index --cached --name-status -r --ignore-submodules HEAD --) > -case "$diff" in > -?*) echo >&2 "cannot rebase: your index contains uncommitted changes" > - echo >&2 "$diff" > - exit 1 > - ;; > -esac > +require_clean_work_tree "rebase"
This side is uncontroversial, except that it has the same "is commit/stash the best advice?" issue.