[PATCH 0/2] Rebasing pull with autostash

It's trivial to support the feature :)

Ramkumar Ramachandra (2):
  pull: respect rebase.autostash
  pull: clarify the large && { ... } form

 git-pull.sh     |  7 +++++--
 t/t5520-pull.sh | 11 +++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

-- 
1.8.3.1.379.gb74074e.dirty

Remove the large && { ... } form, as the block can be confused with a
function block.  Use a simple if-condition instead.  No functional
changes.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 git-pull.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index fb01763..babf009 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -195,7 +195,7 @@ error_on_no_merge_candidates () {
 	exit 1
 }
 
-test true = "$rebase" && {
+if test true = "$rebase"; then
 	if ! git rev-parse -q --verify HEAD >/dev/null
 	then
 		# On an unborn branch

@@ -220,7 +220,8 @@ test true = "$rebase" && {
 			break
 		fi
 	done
-}
+fi
+
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity $progress $dry_run $recurse_submodules --update-head-ok "$@" || exit 1
 test -z "$dry_run" || exit 0

-- 
1.8.3.1.379.gb74074e.dirty

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.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 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
 	oldremoteref= &&

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 6af6c63..08afd25 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -102,6 +102,17 @@ test_expect_success 'pull.rebase' '
 	test new = $(git show HEAD:file2)
 '
 
+test_expect_success 'pull --rebase respects rebase.autostash' '
+	git reset --hard before-rebase &&
+	test_config pull.rebase true &&
+	test_config rebase.autostash true &&
+	echo dirty >>file2
+	git pull . copy &&
+	test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
+	test new = $(git show HEAD:file2) &&
+	grep dirty file2
+'
+
 test_expect_success 'branch.to-rebase.rebase' '
 	git reset --hard before-rebase &&
 	test_config branch.to-rebase.rebase true &&

-- 
1.8.3.1.379.gb74074e.dirty

On Fri, Jun 14, 2013 at 4:56 AM, Ramkumar Ramachandra
[off-list ref] 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.

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

On Fri, Jun 14, 2013 at 8:12 AM, Phil Hord [off-list ref] wrote:

On Fri, Jun 14, 2013 at 4:56 AM, Ramkumar Ramachandra
[off-list ref] wrote:

quoted

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.

quoted

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 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

On Fri, Jun 14, 2013 at 08:12:56AM -0400, Phil Hord wrote:

On Fri, Jun 14, 2013 at 4:56 AM, Ramkumar Ramachandra
[off-list ref] wrote:

quoted

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:

It seems to me that Ram's message is already in the imperative.  The
only (slight) issue is that rr/rebase-autostash will become hard to find
once Junio cleans up feature branches that have graduated.  Since that
branch has graduated to master, it would be clearer to refer to commit
5879477 (rebase: implement --[no-]autostash and rebase.autostash,
2013-05-12).  Is something like this clearer?

    "git pull" currently cannot be used with the "autostash" feature
    added to "git rebase" by commit 5879477 (rebase: implement
    --[no-]autostash and rebase.autostash, 2013-05-12) because it
    unconditionally calls requre_clean_worktree early on, which results
    in:

        # dirty worktree or index
        $ git pull
        Cannot pull with rebase: Your index contains uncommitted changes.
        Please commit or stash them.

    Remove this restriction by skipping the call to
    require_clean_worktree if the "rebase.autostash" configuration
    variable is set.

On Fri, Jun 14, 2013 at 8:29 AM, John Keeping [off-list ref] wrote:

On Fri, Jun 14, 2013 at 08:12:56AM -0400, Phil Hord wrote:

quoted

On Fri, Jun 14, 2013 at 4:56 AM, Ramkumar Ramachandra
[off-list ref] wrote:

quoted

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:

It seems to me that Ram's message is already in the imperative.  The
only (slight) issue is that rr/rebase-autostash will become hard to find
once Junio cleans up feature branches that have graduated.  Since that
branch has graduated to master, it would be clearer to refer to commit
5879477 (rebase: implement --[no-]autostash and rebase.autostash,
2013-05-12).  Is something like this clearer?

    "git pull" currently cannot be used with the "autostash" feature
    added to "git rebase" by commit 5879477 (rebase: implement
    --[no-]autostash and rebase.autostash, 2013-05-12) because it
    unconditionally calls requre_clean_worktree early on, which results
    in:

        # dirty worktree or index
        $ git pull
        Cannot pull with rebase: Your index contains uncommitted changes.
        Please commit or stash them.

    Remove this restriction by skipping the call to
    require_clean_worktree if the "rebase.autostash" configuration
    variable is set.

Yes, thanks.   I was mislead by my poor understanding all the players
involved.  This disambiguates things nicely.

Phil

John Keeping wrote:

Is something like this clearer?
[...]

Yes, thanks.