Re: [PATCH v2 14/22] i18n: rebase-interactive: mark strings for translation
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 27, 2016, 16:58 UTC
- Message-ID
- <xmqqk2ifsajn.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <57481CA0.5080801@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
Show 21 quoted lines
>>> 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"