Re: [PATCH 1/2] pull: respect rebase.autostash

From: Phil Hord <phil.hord@gmail.com>
Date: Jun 14, 2013, 12:12 UTC
Message-ID: <CABURp0pVzQ2rVB-B2L+uCMtPV7QFPSgkZnHitCsrPCrR9LJOVA@mail.gmail.com>
In-Reply-To: <1371200178-9927-2-git-send-email-artagnon@gmail.com>

On Fri, Jun 14, 2013 at 4:56 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 14 quoted lines

> If a rebasing pull is requested, pull unconditionally runs
> require_clean_worktree() resulting in:
>
>   # dirty worktree or index
>   $ git pull
>   Cannot pull with rebase: Your index contains uncommitted changes.
>   Please commit or stash them.
>
> It does this to inform the user early on that a rebase cannot be run on
> a dirty worktree, and that a stash is required.  However,
> rr/rebase-autostash lifts this limitation on rebase by providing a way
> to automatically stash using the rebase.autostash configuration
> variable.  Read this variable in pull, and take advantage of this
> feature.

This commit message does not tell me what this commit does. It mostly describes the current situation. Then it refers to something called "rr/rebase-autostash" which will lose meaning in the future when this commit is no longer current on the list. A better way to refer to this commit is to say "this commit". However, even this is not the norm for this project. The norm here is to avoid such noise by speaking in the imperative mood. That is, do not tell me what this commit does; instead, tell the code what to do. See Documentation/SubmittingPatches:

  Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour.  Try to make sure your explanation can be understood
  without external resources. Instead of giving a URL to a mailing list
  archive, summarize the relevant points of the discussion.

Show 26 quoted lines

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  git-pull.sh     |  2 ++
>  t/t5520-pull.sh | 11 +++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/git-pull.sh b/git-pull.sh
> index 638aabb..fb01763 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -44,6 +44,7 @@ merge_args= edit=
>  curr_branch=$(git symbolic-ref -q HEAD)
>  curr_branch_short="${curr_branch#refs/heads/}"
>  rebase=$(git config --bool branch.$curr_branch_short.rebase)
> +autostash=$(git config --bool rebase.autostash)
>  if test -z "$rebase"
>  then
>         rebase=$(git config --bool pull.rebase)
> @@ -203,6 +204,7 @@ test true = "$rebase" && {
>                         die "$(gettext "updating an unborn branch with changes added to the index")"
>                 fi
>         else
> +               test true = "$autostash" ||
>                 require_clean_work_tree "pull with rebase" "Please commit or stash them."
>         fi

This commit does not seem useful on its own.  All it does is prevent
the safety check for a clean work tree when the autostash flag is set.
 I understand that this is necessary for the rest of the change to be
useful, but I do not see any reason for it to be split into two
commits like this.  I think it would be more understandable if this
were squashed together with the rest of the change, both now for
reviews and in the future when someone tries to understand this change
in retrospect.

In particular, the commit message suggests that this commit will perform the autostash if this variable is set, but it does not do that yet. I think if you squash these two together it will be more concise and understandable.

Thanks, Phil