On Fri, Jun 14, 2013 at 8:12 AM, Phil Hord <phil.hord@xxxxxxxxx> wrote:
> On Fri, Jun 14, 2013 at 4:56 AM, Ramkumar Ramachandra
> <artagnon@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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
Having said all that, I see now that 2/2 in this series is really
unrelated to this commit and is not the rest of the autostash
implementation, so I am more confused than before. Was the rest of
'autostash' already implemented in some other series? I guess that's
what you meant by "rr/rebase-autostash" which is NOT "this commit"
after all. I am sorry for my confusion, though a clearer commit
message would have helped me in the first place.
Phil
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html