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

> On Fri, Jun 14, 2013 at 4:56 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> 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.
>
>
>
>>
>> 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