[PATCH] (trivial) add helpful "use --soft" for bare reset

[PATCH] (trivial) add helpful "use --soft" for bare reset

[David Fries]

Add a helpful "use --soft" message for git-reset (mixed) in a bare
repository.  This tells the user what they can do, instead of just what
they can't.

Signed-off-by: David Fries <David@Fries.net>
---
When I was first learning how to use git and I needed to reset my bare
repository I would make it a full repository just so I could use
git-reset, a message like the above would have saved me a lot of
effort back then.

I'm not the only one, I received an e-mail yesterday confused on what
to do, found my patch via google "it saved my bacon."

 builtin/reset.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 98bca04..dd0cc1e 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -332,7 +332,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		setup_work_tree();
 
 	if (reset_type == MIXED && is_bare_repository())
-		die(_("%s reset is not allowed in a bare repository"),
+		die(_("%s reset is not allowed in a bare repository, use --soft"),
 		    _(reset_type_names[reset_type]));
 
 	/* Soft reset does not touch the index file nor the working tree
-- 
1.7.2.3

Re: [PATCH] (trivial) add helpful "use --soft" for bare reset

[Junio C Hamano]

David Fries <david@fries.net> writes:

> Add a helpful "use --soft" message for git-reset (mixed) in a bare
> repository.  This tells the user what they can do, instead of just what
> they can't.
> ...
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 98bca04..dd0cc1e 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -332,7 +332,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  		setup_work_tree();
>  
>  	if (reset_type == MIXED && is_bare_repository())
> -		die(_("%s reset is not allowed in a bare repository"),
> +		die(_("%s reset is not allowed in a bare repository, use --soft"),
>  		    _(reset_type_names[reset_type]));
>  
>  	/* Soft reset does not touch the index file nor the working tree

If we want to give a short and concise advice in this codepath, it should
be that "git reset" of any variant must not be used in a bare repository,
which is what we currently say.

The _intent_ of "reset --soft" is to KEEP the current index and the
contents of the working tree, so that next commit using the index and the
working tree will be done immediately on top of the commit you are
resetting to. The implementation detail of how "reset --soft" keeps the
index and the working tree happens to be by not touching them and updating
the branch pointed by the HEAD pointer.

If we wanted to be strict, we would have at least checked that the index
and the working tree existed (iow, made sure we had something worth
keeping) before proceeding, but we were too lazy to check the sanity of
the end user here. An assumption made in early days of git where everybody
knew what was going on is that nobody sane would use "reset" of any flavor
in a bare repository as the command is about changing the base of the next
commit _made_ in that repository, and _making_ a new commit on top of the
updated HEAD would very much mean it is not a bare repository.

And that assumption was the only reason why the command would run inside a
bare repository as a substitute for "update-ref HEAD $the_right_commit".
IOW, "reset --soft" is allowed not because we thought that it made sense
to use the command in a bare repository, nor because we wanted to
encourage it as a way to flip the branch that happens to be the primary
one pointed at by HEAD pointer, but because we (as the implementors) could
get away with an implementation that does not forbid its use, as (1)
nobody sane would have done so anyway, and (2) the implementation happened
to be harmless.

Take a step back and think a bit _why_ you wanted to flip the branch to
point at a different commit. Did you push an incorrect commit to the
repository? The right way to fix that mistake is by pushing the correct
one, possibly with --force. You can also repoint a ref at a different
commit with "update-ref $the_ref $right_commit". It will let you correct a
branch that is not the primary branch pointed at with the HEAD pointer in
the bare repository, unlike "reset --soft".

Encouraging "reset --soft" as a way to run "update-ref HEAD" in a bare
repository leads new users in a wrong direction. The next person who reads
your updated error message would say "I cannot use reset --soft to update
a branch that is not the primary branch in a bare repository. Let's add an
option to name which branch to update, usable only when the command is
used in a bare repository with --soft option". Only because you did not
teach the right way "update-ref refs/heads/$that_branch $right_commit", we
would end up with yet another option to "reset" command that is applicable
only in a very narrow special case (i.e. "in bare and only with --soft").

I do not think we want to go that route.

Re: [PATCH] (trivial) add helpful "use --soft" for bare reset

[David Fries]

Instead of,
fatal: mixed reset is not allowed in a bare repository
print,
fatal: mixed reset is not allowed in a bare repository, see update-ref
To tell the user what they can do, instead of just what they can't.

Signed-off-by: David Fries <David@Fries.net>
---
 builtin/reset.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 98bca04..d0d4d66 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -332,7 +332,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		setup_work_tree();
 
 	if (reset_type == MIXED && is_bare_repository())
-		die(_("%s reset is not allowed in a bare repository"),
+		die(_("%s reset is not allowed in a bare repository, see update-ref"),
 		    _(reset_type_names[reset_type]));
 
 	/* Soft reset does not touch the index file nor the working tree
-- 
1.7.2.3

Re: [PATCH] (trivial) add helpful "use --soft" for bare reset

[Junio C Hamano]

David Fries <david@fries.net> writes:

> diff --git a/builtin/reset.c b/builtin/reset.c
> index 98bca04..d0d4d66 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -332,7 +332,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  		setup_work_tree();
>  
>  	if (reset_type == MIXED && is_bare_repository())
> -		die(_("%s reset is not allowed in a bare repository"),
> +		die(_("%s reset is not allowed in a bare repository, see update-ref"),
>  		    _(reset_type_names[reset_type]));

There still is one thing that worries me remains here, even though I may
be worried too much. I tend to think that giving an incorrect advice is
far worse than not giving one.

Are you absolutely sure that the user wanted to update only the tip of a
ref without affecting the index nor the working tree, when a mixed reset
is issued in a bare repository, and there is _no other_ explanation why
the user issued a forbidden command?

For example, I have ~/git.git and /pub/scm/git/git.git on a single machine
somewhere. The former is with a working tree and the latter is a bare
repository. I usually live in ~/git.git/ on that machine, but sometimes I
would do things like:

	$ git repack -a -d ;# working area
	$ cd /pub/scm/git/git.git ;# clean up from time to time ...
        $ git repack -a -d ;# ... the public one as well

I may disconnect from my screen session to the machine after I do this,
and I may have forgotten where I was the next time I come back to the
machine.  After I reconnect to the same screen session, I may say "git
reset" to get back to a known state, which is what I often want to do in
the working area repository ~/git.git, mistakenly thinking that I am in my
usual ~/git.git directory.

In such a scenario, the mistake is not that I used a wrong command "reset"
in an attempt to update the tip of the branch. The mistake is that I tried
to use the right command to update the index, but I did it in a wrong
place. "Did you mean to do that somewhere else?" would be a much more
appropriate advice in that case.

Re: [PATCH] (trivial) add helpful "use --soft" for bare reset

[David Fries]

On Thu, Jun 30, 2011 at 01:06:26PM -0700, Junio C Hamano wrote:
> David Fries <david@fries.net> writes:
> 
> > diff --git a/builtin/reset.c b/builtin/reset.c
> > index 98bca04..d0d4d66 100644
> > --- a/builtin/reset.c
> > +++ b/builtin/reset.c
> > @@ -332,7 +332,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
> >  		setup_work_tree();
> >  
> >  	if (reset_type == MIXED && is_bare_repository())
> > -		die(_("%s reset is not allowed in a bare repository"),
> > +		die(_("%s reset is not allowed in a bare repository, see update-ref"),
> >  		    _(reset_type_names[reset_type]));


Let me go back and respond to your first reply,

> Take a step back and think a bit _why_ you wanted to flip the branch to
> point at a different commit. Did you push an incorrect commit to the
> repository? The right way to fix that mistake is by pushing the correct
> one, possibly with --force. You can also repoint a ref at a different
> commit with "update-ref $the_ref $right_commit". It will let you correct a
> branch that is not the primary branch pointed at with the HEAD pointer in
> the bare repository, unlike "reset --soft".

Back when I knew less it was a push, rebase, push failed, reset the
bare repository so it would work.  I didn't know about push -f, but
that's what you call a learning curve, learn something by figuring out
how to get something done, then later figure out there's a much easier
way.

But at work push -f no longer works, it's administratively denied from
remote for certain branches, the kind that you generally never want to
rewind.  But on occasion we do.  The options are to administratively
change permissions, push -f, change back, or login to the server,
clone, push -f, or manipulate the bare repository directly.  Modifying
the bare repository is the quickest and git-update-ref works, it just
isn't in the porcelain commands, so less likely to be known.

> There still is one thing that worries me remains here, even though I may
> be worried too much. I tend to think that giving an incorrect advice is
> far worse than not giving one.
> 
> Are you absolutely sure that the user wanted to update only the tip of a
> ref without affecting the index nor the working tree, when a mixed reset
> is issued in a bare repository, and there is _no other_ explanation why
> the user issued a forbidden command?

I guess I think of reset as moving the current branch and needing to
know how much collateral damage it can do, modify the branch location,
modify index, modify working files.  I'm not concerned about a user
actually being in a bare repository thinking they're not, because
resetting the index or working directory can loose information that
you can't get back by looking at the ref-log until gc runs, and
nothing woring on the index or working directory will work so they'll
figure it out soon enough.

> For example, I have ~/git.git and /pub/scm/git/git.git on a single machine
> somewhere. The former is with a working tree and the latter is a bare
> repository. I usually live in ~/git.git/ on that machine, but sometimes I
> would do things like:
> 
> 	$ git repack -a -d ;# working area
> 	$ cd /pub/scm/git/git.git ;# clean up from time to time ...
>         $ git repack -a -d ;# ... the public one as well
> 
> I may disconnect from my screen session to the machine after I do this,
> and I may have forgotten where I was the next time I come back to the
> machine.  After I reconnect to the same screen session, I may say "git
> reset" to get back to a known state, which is what I often want to do in
> the working area repository ~/git.git, mistakenly thinking that I am in my
> usual ~/git.git directory.

But you are doing a `git-reset some_ref` to move to some other commit,
or `git-reset` to discard any index changes and leave the branch where
it is?  The first makes it clear you want to move where the branch is
pointing, the second does nothing as far as the commit the branch is
on.  For the first case they are either in the repository they
intended to or not, but either way they are intending to move the
current branch.  In the second case they're not even trying to move
the branch.

> In such a scenario, the mistake is not that I used a wrong command "reset"
> in an attempt to update the tip of the branch. The mistake is that I tried
> to use the right command to update the index, but I did it in a wrong
> place. "Did you mean to do that somewhere else?" would be a much more
> appropriate advice in that case.

Yes, your message would be appropriate in that case, but there's no
way for git to guess that.  Besides if it's a knowledgeable user just
executing the wrong command in the wrong repository, any error message
is enough for them pause, pay attention, figure out that's not what
they expected, realize what they did, and go on their merry way.  The
message is for someone who's used to resetting their non-bare
repository, goes to the bare repository and left to hang as it doesn't
work and man git-reset(1) doesn't offer any hints (if they look).

My worry is git-update-ref's behavior is unexpected when someone's
used to git-commit, git-branch etc, as in
`git-branch new_branch old_branch^` vs
`git-update-ref new_branch old_branch^` 
as someone has to know to add refs/heads to git-update-ref where they
don't with git-branch.

The point of the patch is trying to help out a user who is learning,
and provide something to point them in the right direction.  

-- 
David Fries <david@fries.net>    PGP pub CB1EE8F0
http://fries.net/~david/

Re: [PATCH] (trivial) add helpful "use --soft" for bare reset

[Junio C Hamano]

David Fries <david@fries.net> writes:

> But at work push -f no longer works, it's administratively denied from
> remote for certain branches, the kind that you generally never want to
> rewind.  But on occasion we do.  The options are to administratively
> change permissions, push -f, change back, or login to the server,
> clone, push -f, or manipulate the bare repository directly.  Modifying
> the bare repository is the quickest and git-update-ref works, it just
> isn't in the porcelain commands, so less likely to be known.

That is expected. Whoever denies "administratively" a push that rewinds
the history would have the authority to grant exception to the policy, and
ability to help you rewind the history. You would need to work with
them. If "them" happens to be "you", then you are expected to have that
authority and ability, which may include to have direct write access to
the repository and to know update-ref.

> ....  I'm not concerned about a user
> actually being in a bare repository thinking they're not, because
> resetting the index or working directory can loose information that
> you can't get back by looking at the ref-log until gc runs, and
> nothing woring on the index or working directory will work so they'll
> figure it out soon enough.

You may not be concerned, but I am; otherwise the message you are
responding to wouldn't have been written. And I would agree they'll figure
it out soon enough with the current error message, that does not have an
advice to look at update-ref, which is irrelevant. I am mostly worried
about the additional wrong (for their situation) advice throwing them off
track.

>> In such a scenario, the mistake is not that I used a wrong command "reset"
>> in an attempt to update the tip of the branch. The mistake is that I tried
>> to use the right command to update the index, but I did it in a wrong
>> place. "Did you mean to do that somewhere else?" would be a much more
>> appropriate advice in that case.
>
> Yes, your message would be appropriate in that case, but there's no
> way for git to guess that.

That is exactly my point. If we cannot guess reliably, I do not want to
see us giving an inappropriate advice that does not apply to the user's
situation, potentially leading the user in the wrong direction.