[PATCH] git-rebase.sh: Display error output from git-checkout when detaching HEAD fails.

[PATCH] git-rebase.sh: Display error output from git-checkout when detaching HEAD fails.

[Robert Shearman]

The "git checkout" command executed could fail if, for example, upstream contains a file that would overrwrite a local, untracked file. The output redirection didn't work as stderr was redirected to /dev/null, as was stdout. This appears to be not what was intended so the order of redirections is fixed so that stderr is redirected to stdout instead.
---
 git-rebase.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index e2d85ee..0da2210 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -376,7 +376,7 @@ fi
 
 # Detach HEAD and reset the tree
 echo "First, rewinding head to replay your work on top of it..."
-git checkout "$onto^0" >/dev/null 2>&1 ||
+git checkout "$onto^0" 2>&1 >/dev/null ||
 	die "could not detach HEAD"
 # git reset --hard "$onto^0"
 
-- 
1.5.6.2.225.g4596.dirty

Re: [PATCH] git-rebase.sh: Display error output from git-checkout when detaching HEAD fails.

[Junio C Hamano]

Robert Shearman <robertshearman@gmail.com> writes:

> The "git checkout" command executed could fail if, for example, upstream contains a file that would overrwrite a local, untracked file. The output redirection didn't work as stderr was redirected to /dev/null, as was stdout. This appears to be not what was intended so the order of redirections is fixed so that stderr is redirected to stdout instead.

Very long lines, lacks sign-off.

> ---
>  git-rebase.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-rebase.sh b/git-rebase.sh
> index e2d85ee..0da2210 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -376,7 +376,7 @@ fi
>  
>  # Detach HEAD and reset the tree
>  echo "First, rewinding head to replay your work on top of it..."
> -git checkout "$onto^0" >/dev/null 2>&1 ||

I think this very much is done deliberately by somebody who knows the
shell to discard everything.

> +git checkout "$onto^0" 2>&1 >/dev/null ||

And if it is beneficial to show the error, you just do not touch fd #2,
like this:

	git checkout "$onto^0" >/dev/null

As I do not see any reason to send the error message to stdout like you
did.

I also suspect that this part of the script predates 6124aee (add a quiet
option to git-checkout, 2007-02-01) where the command learned to be more
quiet during the normal operation.  Perhaps you can replace the line with

	git checkout -q "$onto^0"

and be done with it.  I haven't tested it, though.

Re: [PATCH] git-rebase.sh: Display error output from git-checkout when detaching HEAD fails.

[Rob Shearman]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

2008/7/11 Junio C Hamano <gitster@pobox.com>:
> Robert Shearman <robertshearman@gmail.com> writes:
>> diff --git a/git-rebase.sh b/git-rebase.sh
>> index e2d85ee..0da2210 100755
>> --- a/git-rebase.sh
>> +++ b/git-rebase.sh
>> @@ -376,7 +376,7 @@ fi
>>
>>  # Detach HEAD and reset the tree
>>  echo "First, rewinding head to replay your work on top of it..."
>> -git checkout "$onto^0" >/dev/null 2>&1 ||
>
> I think this very much is done deliberately by somebody who knows the
> shell to discard everything.

Why wasn't "git checkout "$onto^0" &> /dev/null" used then? Then only
reason I can come up with would be portability, but it seems
surprising to me.

>> +git checkout "$onto^0" 2>&1 >/dev/null ||
>
> And if it is beneficial to show the error, you just do not touch fd #2,
> like this:
>
>        git checkout "$onto^0" >/dev/null

Absolutely. I was just trying to fix the statement to what I thought
was the original intent.

> As I do not see any reason to send the error message to stdout like you
> did.
>
> I also suspect that this part of the script predates 6124aee (add a quiet
> option to git-checkout, 2007-02-01) where the command learned to be more
> quiet during the normal operation.  Perhaps you can replace the line with
>
>        git checkout -q "$onto^0"
>
> and be done with it.  I haven't tested it, though.

I just tested it and it solves the original issue whilst not
displaying unnecessary messages during a rebase. For reference, the
attached script reproduces the issue that I was trying to solve.

Should I resend the patch (like the following) now that it is
effectively completely your work?

-git checkout "$onto^0" >/dev/null 2>&1 ||
+git checkout -q "$onto^0" ||

-- 
Rob Shearman

[-- Attachment #2: rebase_clash.sh --]
[-- Type: application/x-sh, Size: 356 bytes --]

Re: [PATCH] git-rebase.sh: Display error output from git-checkout when detaching HEAD fails.

[Junio C Hamano]

"Rob Shearman" <robertshearman@gmail.com> writes:

> 2008/7/11 Junio C Hamano <gitster@pobox.com>:
> ...
>> I think this very much is done deliberately by somebody who knows the
>> shell to discard everything.
>
> Why wasn't "git checkout "$onto^0" &> /dev/null" used then? Then only
> reason I can come up with would be portability,...

Yes, we are quite old fashioned when it comes to shell scripting.

>> ...  Perhaps you can replace the line with
>>
>>        git checkout -q "$onto^0"
>>
>> and be done with it.  I haven't tested it, though.
>
> I just tested it and it solves the original issue whilst not
> displaying unnecessary messages during a rebase. For reference, the
> attached script reproduces the issue that I was trying to solve.
>
> Should I resend the patch (like the following) now that it is
> effectively completely your work?

Your choice.

The issues you might want to consider when making that choice are:

 - Resending something that is trivial may seem waste of time on your
   part;

 - Bringing the issue up is more than half of solving it, and you deserve
   the credit.  By resending with a clear commit log message you assure
   this;

 - I am handling many patches for git.git that are readily applicable
   immediately after I read them in my mbox, while creating a commit out
   of this discussion is something I need to "work on".  When I get around
   to do some git work tonight, I may not even recall this dialogue, and
   the solution may be forgotten.

 - I am lazy and may forget to pass --author="Rob Shearman <your@email>"
   even if I remember this conversation and when I make a commit myself;

I just made a commit out of this discussion before I forget, so no need to
resend for this one, but in general I'd appreciate a resend in general ;-)

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 14 Jul 2008 14:05:35 -0700
Subject: [PATCH] git-rebase: report checkout failure

When detaching the HEAD to the base commit, the "git checkout" command
could fail if, for example, upstream contains a file that would overrwrite
a local, untracked file.  Unconditionally discarding the standard error
stream was done to squelch the progress and notices back when checkout
did not have -q option, but there is no reason to keep doing it anymore.

Noticed by Robert Shearman.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-rebase.sh |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index e2d85ee..7825f88 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -376,9 +376,7 @@ fi
 
 # Detach HEAD and reset the tree
 echo "First, rewinding head to replay your work on top of it..."
-git checkout "$onto^0" >/dev/null 2>&1 ||
-	die "could not detach HEAD"
-# git reset --hard "$onto^0"
+git checkout -q "$onto^0" || die "could not detach HEAD"
 
 # If the $onto is a proper descendant of the tip of the branch, then
 # we just fast forwarded.
-- 
1.5.6.3.473.gc658e