Re: [PATCH v3 1/4] git-credential-store: support multiple credential files

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Mar 14, 2015 at 01:33:28PM -0400, Jeff King wrote:

> On Sat, Mar 14, 2015 at 04:15:53PM +0800, Paul Tan wrote:
> 
> > Even though in this case the store_credential() function is not used
> > anywhere else, from my personal API design experience I think that
> > cementing the rule of "the first file in the list is the default" in
> > the behavior of the function is not a good thing. For example, in the
> > future, we may wish to keep the precedence ordering the same, but if
> > none of the credential files exist, we create the XDG file by default
> > instead. It's a balance of flexibility, but in this case I think
> > putting the default filename in a separate argument is a good thing.
> 
> Yeah, I see your line of reasoning. I think this is probably a case of
> YAGNI, but it is really a matter of personal preference. It's not a big
> deal either way.

By the way, I hope this (and the other comment) do not come off as "you
are wrong, but I do not feel like arguing with you". I really do think
these are a matter of taste, and while we often express issues of taste
in reviews, it is ultimately up to the patch submitter (who is, after
all, doing most of the work) to have the final say on minor issues like
this.  Sometimes the response to taste issue is "oh, I didn't think to
do that, thanks for the suggestion" and sometimes it is "nah, I like it
better my way". And both are OK.

Of course there are also taste issues where we insist (e.g., consistent
whitespace), but I do not think this is one of them.  :)

Maybe that was all obvious, but since you are new to the list, I wanted
to make sure I was clear.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]