Re: [PATCH v2 14/22] i18n: rebase-interactive: mark strings for translation

[Junio C Hamano <gitster@pobox.com>]

Vasco Almeida <vascomalmeida@sapo.pt> writes:

>>>  is_empty_commit() {
>>> -	tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null ||
>>> -		die "$1: not a commit that can be picked")
>>> -	ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null ||
>>> +	sha1=$1
>>> +	tree=$(git rev-parse -q --verify "$sha1"^{tree} 2>/dev/null ||
>>> +		die "$(eval_gettext "\$sha1: not a commit that can be picked")")
>>> +	ptree=$(git rev-parse -q --verify "$sha1"^^{tree} 2>/dev/null ||
>>>  		ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)
>>>  	test "$tree" = "$ptree"
>>>  }
>> 
>> Both of the two callsites of this function use the variable $sha1,
>> and at least one of them uses that variable after this function
>> returns, but they pass it as the first parameter to this function,
>> so the assignment added by this patch does not break them, which is
>> good.
>> 
> I didn't know that. I can change sha1=$1 to local_sha1=$1 or _sha1=$1 (I
> don't know what is the convention here) if that is safer, avoiding using
> the bash-ism "local" keyword, and preventing future distractions.

I do not think it is necessary to do any of the changes for this
one, which is only used locally in this file.  They just need to be
careful when they add or modify the callers.

Even if it is renamed $local_sha1, they will still need to be
careful anyway, because the only way they _can_ break or be broken
by your patch is when they somehow decide to pass something that is
not called $sha1 to this function, i.e. they would be changing the
callsite---which they must be doing for a reason and would also
involve change to the code what happens after this function returns.
If they start using $local_sha1 there, they would be broken if you
renamed it today, and if they use $sha1 there, they would be broken
if you didn't---so they have to be careful and check what this one
clobbers _anyway_.  

IOW, a solution that clobbers _some_ variable cannot be made safer
by renaming.  The callers always have to be careful not to be
affected.

You could do something like this, though.  I _think_ the original
that calls "die" inside $() is broken anyway, so it probably is a
good change regardless.

 git-rebase--interactive.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9d2bfb7..b5113d6 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -222,8 +222,10 @@ has_action () {
 }
 
 is_empty_commit() {
-	tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null ||
-		die "$1: not a commit that can be picked")
+	tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null) || {
+		sha1=$1
+		die "$(eval_gettext "\$sha1: not a commit that can be picked")"
+	}
 	ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null ||
 		ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)
 	test "$tree" = "$ptree"