Re: [GSoC PATCH v3 1/1] Unify SMTP auth error handling

From: Yuting Zheng <05zyt30@gmail.com>
Date: Mar 14, 2025, 12:55 UTC
Message-ID: <CAMvj1+pn_+8PRXCUds0NHrRPBWh1uUzOOeNGhXTmHRTg_DGqHg@mail.gmail.com>
In-Reply-To: <xmqqsengn1ms.fsf@gitster.g>

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.

Show 5 quoted lines

> 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.

Show 7 quoted lines

> 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.

Show 7 quoted lines

> 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.