[PATCH 00/86] replace prefixcmp() with has_prefix()
Here is a big patch series to replace prefixcmp() with a new has_prefix() function. So the first patch of this series introduces has_prefix() and the last patch removes prefixcmp(). Except in a few cases, I used a script that does basically the following to generate the commits in between: === #!/bin/bash perl -pi -e 's/!prefixcmp\(/has_prefix\(/g' "$1" perl -pi -e 's/prefixcmp\(/!has_prefix\(/g' "$1" git commit -m "$1: replace prefixcmd() with has_prefix()" "$1" === The few special cases are the following ones: - remote*: replace prefixcmd() with has_prefix() - transport*: replace prefixcmd() with has_prefix() - environment: replace prefixcmd() with has_prefix() In first 2 cases above, I processed a few files at the same time instead of just one. In the case of "environment", I removed " != 0" after "!has_prefix(...)" as it is not necessary and makes it more difficult to understand the logic. Of course it's possible to squash many of the commits together if it is prefered. Christian Couder (86): strbuf: add has_prefix() to be used instead of prefixcmp() diff: replace prefixcmd() with has_prefix() fast-import: replace prefixcmd() with has_prefix() remote*: replace prefixcmd() with has_prefix() daemon: replace prefixcmd() with has_prefix() pretty: replace prefixcmd() with has_prefix() revision: replace prefixcmd() with has_prefix() transport*: replace prefixcmd() with has_prefix() config: replace prefixcmd() with has_prefix() sha1_name: replace prefixcmd() with has_prefix() wt-status: replace prefixcmd() with has_prefix() upload-pack: replace prefixcmd() with has_prefix() test-line-buffer: replace prefixcmd() with has_prefix() parse-options: replace prefixcmd() with has_prefix() fetch-pack: replace prefixcmd() with has_prefix() git: replace prefixcmd() with has_prefix() tag: replace prefixcmd() with has_prefix() sequencer: replace prefixcmd() with has_prefix() commit: replace prefixcmd() with has_prefix() http: replace prefixcmd() with has_prefix() imap-send: replace prefixcmd() with has_prefix() help: replace prefixcmd() with has_prefix() log-tree: replace prefixcmd() with has_prefix() merge-recursive: replace prefixcmd() with has_prefix() notes: replace prefixcmd() with has_prefix() refs: replace prefixcmd() with has_prefix() setup: replace prefixcmd() with has_prefix() bisect: replace prefixcmd() with has_prefix() branch: replace prefixcmd() with has_prefix() http-push: replace prefixcmd() with has_prefix() send-pack: replace prefixcmd() with has_prefix() http-backend: replace prefixcmd() with has_prefix() notes-utils: replace prefixcmd() with has_prefix() pkt-line: replace prefixcmd() with has_prefix() alias: replace prefixcmd() with has_prefix() attr: replace prefixcmd() with has_prefix() connect: replace prefixcmd() with has_prefix() pager: replace prefixcmd() with has_prefix() convert: replace prefixcmd() with has_prefix() environment: replace prefixcmd() with has_prefix() shell: replace prefixcmd() with has_prefix() pathspec: replace prefixcmd() with has_prefix() submodule: replace prefixcmd() with has_prefix() test-string-list: replace prefixcmd() with has_prefix() builtin/apply: replace prefixcmd() with has_prefix() builtin/archive: replace prefixcmd() with has_prefix() builtin/branch: replace prefixcmd() with has_prefix() builtin/checkout: replace prefixcmd() with has_prefix() builtin/clean: replace prefixcmd() with has_prefix() builtin/clone: replace prefixcmd() with has_prefix() builtin/column: replace prefixcmd() with has_prefix() builtin/commit: replace prefixcmd() with has_prefix() builtin/describe: replace prefixcmd() with has_prefix() builtin/fast-export: replace prefixcmd() with has_prefix() builtin/fetch-pack: replace prefixcmd() with has_prefix() builtin/fetch: replace prefixcmd() with has_prefix() builtin/fmt-merge-msg: replace prefixcmd() with has_prefix() builtin/for-each-ref: replace prefixcmd() with has_prefix() builtin/fsck: replace prefixcmd() with has_prefix() builtin/help: replace prefixcmd() with has_prefix() builtin/index-pack: replace prefixcmd() with has_prefix() builtin/init-db: replace prefixcmd() with has_prefix() builtin/log: replace prefixcmd() with has_prefix() builtin/ls-remote: replace prefixcmd() with has_prefix() builtin/mailinfo: replace prefixcmd() with has_prefix() builtin/merge-recursive: replace prefixcmd() with has_prefix() builtin/merge: replace prefixcmd() with has_prefix() builtin/name-rev: replace prefixcmd() with has_prefix() builtin/notes: replace prefixcmd() with has_prefix() builtin/pack-objects: replace prefixcmd() with has_prefix() builtin/prune: replace prefixcmd() with has_prefix() builtin/receive-pack: replace prefixcmd() with has_prefix() builtin/reflog: replace prefixcmd() with has_prefix() builtin/remote: replace prefixcmd() with has_prefix() builtin/rev-parse:
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
Christian Couder writes: > Christian Couder (86): > strbuf: add has_prefix() to be used instead of prefixcmp() > diff: replace prefixcmd() with has_prefix() > fast-import: replace prefixcmd() with has_prefix() [...] > builtin/update-ref: replace prefixcmd() with has_prefix() > builtin/upload-archive: replace prefixcmd() with has_prefix() > strbuf: remove prefixcmp() as it has been replaced with has_prefix() All of your subjects except for the first and last say "prefixcm*d*". :-) -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
On 2013-11-09 08:05, Christian Couder wrote: Here is a big patch series to replace prefixcmp() with a new has_prefix() function. Seems like totally useless codechurn to me. Besides, prefixcmp() ties in nicely with strcmp() and memcmp() (and returns 0 on a match just like its namesakes), whereas your function must return non-zero on match and thus can't be used as a qsort() callback. Granted, prefixcmp() lends itself poorly to that as well, but at least it's consistent with the other *cmp() functions. So -1 on this whole series. -- Andreas Ericsson andreas.erics...@op5.se OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231 Considering the successes of the wars on alcohol, poverty, drugs and terror, I think we should give some serious thought to declaring war on peace. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
On Sat, Nov 9, 2013 at 3:24 PM, Thomas Rast wrote: > Christian Couder writes: > >> Christian Couder (86): >> strbuf: add has_prefix() to be used instead of prefixcmp() >> diff: replace prefixcmd() with has_prefix() >> fast-import: replace prefixcmd() with has_prefix() > [...] >> builtin/update-ref: replace prefixcmd() with has_prefix() >> builtin/upload-archive: replace prefixcmd() with has_prefix() >> strbuf: remove prefixcmp() as it has been replaced with has_prefix() > > All of your subjects except for the first and last say "prefixcm*d*". :-) Yeah, sorry about that. Junio already sent me, with some others in cc, an email about this and I replied to all asking Junio if he wants me to resend with a fixed subject, but unfortunately the mailing list was not among the recipient of his email and my reply. Thanks and sorry again, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
On Mon, Nov 11, 2013 at 05:09:17PM +0100, Andreas Ericsson wrote: > On 2013-11-09 08:05, Christian Couder wrote: > >Here is a big patch series to replace prefixcmp() with a new > >has_prefix() function. > > > > Seems like totally useless codechurn to me. Besides, prefixcmp() > ties in nicely with strcmp() and memcmp() (and returns 0 on a > match just like its namesakes), whereas your function must return > non-zero on match and thus can't be used as a qsort() callback. > Granted, prefixcmp() lends itself poorly to that as well, but at > least it's consistent with the other *cmp() functions. I think you missed the earlier threads that let up to this, but the summary is that prefixcmp does not really make any sense as a qsort callback. Consider that comparing "foobar" and "foo" would yield 0, but "foo" and "foobar" would be positive. I am ambivalent on the code churn, but if we do apply it, we should probably leave off the final patch (dropping prefixcmp) for a cycle to let topics in flight catch up to the change. Just diffing "master" and "next", I see some new uses of prefixcmp which will need adjusted, along with spots where the patches themselves will cause textual conflicts. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
Jeff King writes: > I am ambivalent on the code churn, but if we do apply it, we should > probably leave off the final patch (dropping prefixcmp) for a cycle to > let topics in flight catch up to the change. Just diffing "master" and > "next", I see some new uses of prefixcmp which will need adjusted, along > with spots where the patches themselves will cause textual conflicts. Yes, I did that check too (but between 'maint' and 'pu'). I think it is a good idea to stop using whatever_cmp() name for things that are not *cmp() functions in the longer term, but smooth migration is a bit tricky (but not as tricky as end-user visible transitions). Even though we already added has_suffix() for tail matches, it is not too late to rethink, as it is not in 'master' yet. One thing I noticed is that it is probably misnamed, or at least in a way that invites confusion. Can people tell which one of these is correct without looking at existing callsites? has_suffix(filename, "txt"); has_suffix(filename, ".txt"); The semantics of the function we have is the latter and is better called endswith(), I suspect. And the corresponding function to check for head matches should probably be called beginswith(). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
On Tue, Nov 12, 2013 at 08:53:45AM -0800, Junio C Hamano wrote: > Even though we already added has_suffix() for tail matches, it is > not too late to rethink, as it is not in 'master' yet. > > One thing I noticed is that it is probably misnamed, or at least in > a way that invites confusion. Can people tell which one of these is > correct without looking at existing callsites? > > has_suffix(filename, "txt"); > has_suffix(filename, ".txt"); To me, it is obviously the latter. My name for "thing at the end of a file after the dot" is "extension", not "suffix". I thought that was universal, but if there are people who find it confusing, it is worth changing. After all, the point is to make the code more readable. > The semantics of the function we have is the latter and is better > called endswith(), I suspect. And the corresponding function to > check for head matches should probably be called beginswith(). Those are OK to me. "has_suffix" would be my first choice, but if it is confusing to others, your suggestions are fine. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
From: Junio C Hamano > > Even though we already added has_suffix() for tail matches, it is > not too late to rethink, as it is not in 'master' yet. > > One thing I noticed is that it is probably misnamed, or at least in > a way that invites confusion. Can people tell which one of these is > correct without looking at existing callsites? > > has_suffix(filename, "txt"); > has_suffix(filename, ".txt"); > > The semantics of the function we have is the latter and is better > called endswith(), I suspect. And the corresponding function to > check for head matches should probably be called beginswith(). I don't know if has_suffix() is confusing for a native speaker. After a look at some languages, Python has "startwith()" and "endswith()", and Java has "startWith()" and "endsWith()". So I agree that it is a good name. But while we are at it, why not "ends_with()" and "begins_with()"? To me using an underscore seems more consistent with what we are doing in Git. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
Christian Couder writes: > From: Junio C Hamano >> >> Even though we already added has_suffix() for tail matches, it is >> not too late to rethink, as it is not in 'master' yet. >> >> One thing I noticed is that it is probably misnamed, or at least in >> a way that invites confusion. Can people tell which one of these is >> correct without looking at existing callsites? >> >> has_suffix(filename, "txt"); >> has_suffix(filename, ".txt"); >> >> The semantics of the function we have is the latter and is better >> called endswith(), I suspect. And the corresponding function to >> check for head matches should probably be called beginswith(). > > I don't know if has_suffix() is confusing for a native speaker. > > After a look at some languages, Python has "startwith()" and > "endswith()", and Java has "startWith()" and "endsWith()". > > But while we are at it, why not > "ends_with()" and "begins_with()"? To me using an underscore seems > more consistent with what we are doing in Git. Sure. I do not think Peff and I were discussing at that level yet to debate between camelCase and words_with_underscore. We were mainly talking about what words to be used, which needs to come before the final appearance. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
From: Junio C Hamano > > Christian Couder writes: >> >> After a look at some languages, Python has "startwith()" and >> "endswith()", and Java has "startWith()" and "endsWith()". >> >> But while we are at it, why not >> "ends_with()" and "begins_with()"? To me using an underscore seems >> more consistent with what we are doing in Git. > > Sure. > > I do not think Peff and I were discussing at that level yet to > debate between camelCase and words_with_underscore. We were mainly > talking about what words to be used, which needs to come before the > final appearance. Ok. By the way Ruby has "start_with?" and "end_with?". So the thing we can discuss, if we go this way are: 1) with an "s" after the verb or not? 2) should we use "start" or "begin"? 3) with an underscore, nothing or camelCase My preference is: 1) with an "s" 2) "start" 3) underscore so that gives: starts_with() and ends_with() Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
On Wed, Nov 13, 2013 at 07:47:03AM +0100, Christian Couder wrote: > My preference is: > > 1) with an "s" > 2) "start" > 3) underscore > > so that gives: starts_with() and ends_with() FWIW, that looks good to me, too. Whether there is confusion over the meaning of "suffix" or not, it makes sense, all other things being equal, to use the same terms as other popular languages. Like you, I prefer "with an s", but we are deep in bikeshedding territory now. I can live with anything. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
From: Jeff King > > On Wed, Nov 13, 2013 at 07:47:03AM +0100, Christian Couder wrote: > >> My preference is: >> >> 1) with an "s" >> 2) "start" >> 3) underscore >> >> so that gives: starts_with() and ends_with() > > FWIW, that looks good to me, too. Whether there is confusion over the > meaning of "suffix" or not, it makes sense, all other things being > equal, to use the same terms as other popular languages. > > Like you, I prefer "with an s", but we are deep in bikeshedding > territory now. I can live with anything. :) When I prepared a new version of my patch series, this time to rename suffixcmp() to ends_with(), it appeared that we already have a static ends_with() function in vcs-svn/fast_export.c with another slightly different implementation :-) I will send a new version that will remove this redundant implementation. Cheers, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
On Sun, Nov 17, 2013 at 9:52 AM, Christian Couder wrote: > From: Jeff King >> >> On Wed, Nov 13, 2013 at 07:47:03AM +0100, Christian Couder wrote: >> >>> My preference is: >>> >>> 1) with an "s" >>> 2) "start" >>> 3) underscore >>> >>> so that gives: starts_with() and ends_with() >> >> FWIW, that looks good to me, too. Whether there is confusion over the >> meaning of "suffix" or not, it makes sense, all other things being >> equal, to use the same terms as other popular languages. >> >> Like you, I prefer "with an s", but we are deep in bikeshedding >> territory now. I can live with anything. :) > > When I prepared a new version of my patch series, this time to rename > suffixcmp() to ends_with(), it appeared that we already have a static > ends_with() function in vcs-svn/fast_export.c with another slightly > different implementation :-) > > I will send a new version that will remove this redundant > implementation. There is also a new version of my 86 patch long series to replace prefixcmp() with starts_with() that I am ready to send, but I hesitate to spam the whole list :-) I can put it somewhere like GitHub where people can see everything and perhaps send only a few patches to the list, including the first and the last. @Junio, how would you like me to proceed? Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/86] replace prefixcmp() with has_prefix()
Christian Couder writes: > There is also a new version of my 86 patch long series to replace > prefixcmp() with starts_with() that I am ready to send, but I hesitate > to spam the whole list :-) > I can put it somewhere like GitHub where people can see everything and > perhaps send only a few patches to the list, including the first and > the last. > @Junio, how would you like me to proceed? Let's hold this off for now. The other half of this series is already in 'next' and I do not want to bother with the "revert, rebranch and remerge" dance this close to the 1.8.5 final. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
