threads / patch / 36478

patchgit-rebase: fix probable reflog typo

Subject: [PATCH] git-rebase: fix probable reflog typo

## tl;dr

6 messages between Apr 23, 2014 and Apr 24, 2014. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Felipe Contreras· Apr 23, 2014, 00:14 UTC · lore

Commit 26cd160 (rebase -i: use a better reflog message) tried to produce a better reflog message, however, it seems a statement was introduced by mistake.

'comment_for_reflog start' basically overides the GIT_REFLOG_ACTION we just set.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--interactive.sh | 2 --
 1 file changed, 2 deletions(-)
Show changes to git-rebase--interactive.sh +0 −2
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 43631b4..5f1d8c9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -893,8 +893,6 @@ then
 	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
 	output git checkout "$switch_to" -- ||
 	die "Could not checkout $switch_to"
-
-	comment_for_reflog start
 fi
 
 orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?"
-- 
1.9.2+fc1.1.g5c924db
Matthieu Moy· Apr 23, 2014, 11:36 UTC · re: Felipe Contreras · lore

Re: [PATCH] git-rebase: fix probable reflog typo

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 6 quoted lines
> Commit 26cd160 (rebase -i: use a better reflog message) tried to produce
> a better reflog message, however, it seems a statement was introduced by
> mistake.
>
> 'comment_for_reflog start' basically overides the GIT_REFLOG_ACTION we
> just set.

I think this is more complex than this. To give a bit more context, the code you're changing is:

comment_for_reflog start
if test ! -z "$switch_to"
then
	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
	output git checkout "$switch_to" -- ||
	die "Could not checkout $switch_to"
	comment_for_reflog start
fi

The GIT_REFLOG_ACTION= changes the reflog message for the git checkout command.

Since we use the construct
GIT_REFLOG_ACTION="..." shell_function

the shell may keep $GIT_REFLOG_ACTION set after calling the function (I don't remember what POSIX says, but dash does it: $ f () { echo $X; } $ X=42 f 42 $ echo $X 42 $ X=y f y $ echo $X y ).

So, one needs to reset $GIT_REFLOG_ACTION to what it used to be if is it to be used later. However, it seems to me that the "comment_for_reflog start" is used only for this checkout command. If so, there's no need for the "comment_for_reflog start" before the if statement either.

In any case, this should be clarified with at least a comment in the code.

Show 13 quoted lines
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 43631b4..5f1d8c9 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -893,8 +893,6 @@ then
>  	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
>  	output git checkout "$switch_to" -- ||
>  	die "Could not checkout $switch_to"
> -
> -	comment_for_reflog start
>  fi
>  
>  orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?"
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Felipe Contreras· Apr 23, 2014, 21:22 UTC · re: Matthieu Moy · lore

Re: [PATCH] git-rebase: fix probable reflog typo

Matthieu Moy wrote:
Show 8 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Commit 26cd160 (rebase -i: use a better reflog message) tried to produce
> > a better reflog message, however, it seems a statement was introduced by
> > mistake.
> >
> > 'comment_for_reflog start' basically overides the GIT_REFLOG_ACTION we
> > just set.
> So, one needs to reset $GIT_REFLOG_ACTION to what it used to be if is it
> to be used later. However, it seems to me that the "comment_for_reflog
> start" is used only for this checkout command. If so, there's no need
> for the "comment_for_reflog start" before the if statement either.

Exactly. But if this variable is only meant for this command, it should be `VAR=VAL command`, that would make it clear without the need of a comment.

-- 
Felipe Contreras
Matthieu Moy· Apr 24, 2014, 06:21 UTC · re: Felipe Contreras · lore

Re: [PATCH] git-rebase: fix probable reflog typo

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 17 quoted lines
> Matthieu Moy wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> > Commit 26cd160 (rebase -i: use a better reflog message) tried to produce
>> > a better reflog message, however, it seems a statement was introduced by
>> > mistake.
>> >
>> > 'comment_for_reflog start' basically overides the GIT_REFLOG_ACTION we
>> > just set.
>
>> So, one needs to reset $GIT_REFLOG_ACTION to what it used to be if is it
>> to be used later. However, it seems to me that the "comment_for_reflog
>> start" is used only for this checkout command. If so, there's no need
>> for the "comment_for_reflog start" before the if statement either.
>
> Exactly. But if this variable is only meant for this command, it should be
> `VAR=VAL command`, that would make it clear without the need of a comment.

I don't understand. Are you suggesting to replace the shell function "output" with an external command? If not, which command would you want to call here?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Felipe Contreras· Apr 24, 2014, 07:22 UTC · re: Matthieu Moy · lore

Re: [PATCH] git-rebase: fix probable reflog typo

Matthieu Moy wrote:
Show 22 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> > Matthieu Moy wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >> 
> >> > Commit 26cd160 (rebase -i: use a better reflog message) tried to produce
> >> > a better reflog message, however, it seems a statement was introduced by
> >> > mistake.
> >> >
> >> > 'comment_for_reflog start' basically overides the GIT_REFLOG_ACTION we
> >> > just set.
> >
> >> So, one needs to reset $GIT_REFLOG_ACTION to what it used to be if is it
> >> to be used later. However, it seems to me that the "comment_for_reflog
> >> start" is used only for this checkout command. If so, there's no need
> >> for the "comment_for_reflog start" before the if statement either.
> >
> > Exactly. But if this variable is only meant for this command, it should be
> > `VAR=VAL command`, that would make it clear without the need of a comment.
> 
> I don't understand. Are you suggesting to replace the shell function
> "output" with an external command? If not, which command would you want
> to call here?

Recently some code was changed to do 'test_must_fail env VAR=VAL command', why can't we do the same?

-- 
Felipe Contreras
Matthieu Moy· Apr 24, 2014, 08:40 UTC · re: Felipe Contreras · lore

Re: [PATCH] git-rebase: fix probable reflog typo

Felipe Contreras <felipe.contreras@gmail.com> writes:
> Recently some code was changed to do 'test_must_fail env VAR=VAL command', why
> can't we do the same?
I guess we can.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

← back to recent threads