On Fri Mar 14, 2025 at 3:58 AM CST, Junio C Hamano wrote:
Thank you for the thorough review. As a newcomer, I really appreciate you
taking time to help me improve.
> "improves clarity ." is (not well formatted and) a bit subjective
> and does not apply to all three changes the patch is making here,
> does it?
I'll reformat the commit message and split the patch into more detailed
parts.
> Hmph, the interpreter may tolerate the new block-eval "eval {}"
> simple statement that lacks terminating ';' but is this an
> improvement? The original look more kosher from syntactic point of
> view. It seems to be totally unrelated change from the rest of the
> patch.
I'll revert it to the original state.
> We seem to already have the comment added by this hunk, since
> 4d31a44a (git-send-email: use git credential to obtain password,
> 2013-02-12). Am I looking at a wrong version of the source (or a
> wrong version of the patch)?
>
> And curiously we do not seem to have this else clause with the
> comment that is getting removed.
You're correct - this was caused by my failure to rebase before
submission.I'll clean up all duplicate comments.
> As I do not see two evals in our copy of git-send-email.perl source,
> it may be moot at this point to comment on this patch, but if we did
> have a eval block each of the if/else arms, moving the control
> structure around and turning "if eval {} else eval {}" into "eval {
> if ... else ...}" may make it cleaner to see what is going on,
> especially if we plan to extend the choices and add elsif to the
> chain later.
I'll use if/else structure which is more extensible.
> Have a SP between "#" and the comment body.
Understood. I'll rigorously adhere to code style guidelines by adding
space after comment markers.
> I'll stop here, as the patch does not seem to be designed to apply
> to our source tree.
This was caused by my local branch being several commits behind upstream.
I've now synchronized and will resubmit properly.
