Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 7, 2013, 03:32 UTC
Message-ID: <7vk3of817v.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1365239012-15079-3-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Also, add a new option: 'auto', so if there's more than one patch, the
> cover letter is generated, otherwise it's not.

Very sensible goal.

> This has the slight disadvantage that a piece of code will always be run
> even if the user doesn't want a cover letter, and thus waste a few
> cycles.

I am not sure what overhead you are referring to.

We need to count how many we are emitting with or without the cover letter, and count is stored in "total". Even when the user said "auto" (which I personally think should become the default in the longer term, but that is a separate issue), we shouldn't have to spend any extra cost if you moved the code that does anything heavy for cover letter generation after we know what that "total" is, no?

I think the reason you did not move the "find the branch for use in the cover letter" code down was because it uses the rev->pending interface, which you cannot read out of after you count the commits, but that logic to use rev->pending predates the introduction of a more modern rev->cmdline mechanism, which is already used by the find_branch_name() function in the same codepath, and it is not clobbered by prepare_revision_walk().

So perhaps by moving that code down after we know what value "total" has, and rewriting "what was the positive commit the user gave us" logic using rev->cmdline, you do not have to do unnecessary work at all.