Re: [PATCH] branch: remove unused parameter to create_branch()

From: Tobias Klauser <tklauser@distanz.ch>
Date: Nov 4, 2016, 16:52 UTC
Message-ID: <20161104165236.GC819@distanz.ch>
In-Reply-To: <20161104163012.5r3uivnub3bdkqgr@sigill.intra.peff.net>

On 2016-11-04 at 17:30:12 +0100, Jeff King <peff@peff.net> wrote:

Show 18 quoted lines

> On Fri, Nov 04, 2016 at 04:19:49PM +0100, Tobias Klauser wrote:
> 
> > The name parameter to create_branch() has been unused since commit
> > 55c4a673070f ("Prevent force-updating of the current branch"). Remove
> > the parameter and adjust the callers accordingly. Also remove the
> > parameter from the function's documentation comment.
> 
> This seemed eerily familiar, and it turns out I wrote this as a
> preparatory step for a different topic a while back, but never finished
> it.
> 
> So clearly a good change, though we might want to explain a bit more why
> it's correct that the parameter is unused. Here's what I wrote:
> 
>   This function used to have the caller pass in the current value of
>   HEAD, in order to make sure we didn't clobber HEAD.  In 55c4a6730,
>   that logic moved to validate_new_branchname(), which just resolves
>   HEAD itself. The parameter to create_branch is now unused.

Ah, I didn't know about the history of this parameter. It clearly makes sense to explain this in the patch description.

> I also ended up reformatting the documentation comment, but that's
> purely optional. My patch is below for reference. Feel free to grab any
> bits of it that you agree with.

I like your documentation comment much better as IMO it's easier to read and to identify the individual parameters.

Given these facts, I guess it's better if my patch is dropped and yours is applied instead :)

Thanks a lot!