git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: "Jeff King" <peff@peff.net>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Leho Kraav" <leho@conversionready.com>,
	git@vger.kernel.org
Subject: Re: [PATCHv2 5/7] versioncmp: cope with common part overlapping with prerelease suffix
Date: Mon, 12 Dec 2016 13:27:21 -0800	[thread overview]
Message-ID: <xmqqshps96ti.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20161208142401.1329-6-szeder.dev@gmail.com> ("SZEDER Gábor"'s message of "Thu, 8 Dec 2016 15:23:59 +0100")

SZEDER Gábor <szeder.dev@gmail.com> writes:

> diff --git a/versioncmp.c b/versioncmp.c
> index a55c23ad5..f86ac562e 100644
> --- a/versioncmp.c
> +++ b/versioncmp.c
> @@ -26,12 +26,15 @@ static int initialized;
>  
>  /*
>   * off is the offset of the first different character in the two strings
> - * s1 and s2. If either s1 or s2 contains a prerelease suffix starting
> - * at that offset, it will be forced to be on top.
> + * s1 and s2. If either s1 or s2 contains a prerelease suffix containing
> + * that offset, then that string will be forced to be on top.
>   *
> - * If both s1 and s2 contain a (different) suffix at that position,
> + * If both s1 and s2 contain a (different) suffix around that position,
>   * their order is determined by the order of those two suffixes in the
>   * configuration.
> + * If any of the strings contains more than one different suffixes around
> + * that position, then that string is sorted according to the contained
> + * suffix which comes first in the configuration.
>   *
>   * Return non-zero if *diff contains the return value for versioncmp()
>   */
> @@ -44,10 +47,21 @@ static int swap_prereleases(const char *s1,
>  
>  	for (i = 0; i < prereleases->nr; i++) {
>  		const char *suffix = prereleases->items[i].string;
> -		if (i1 == -1 && starts_with(s1 + off, suffix))
> -			i1 = i;
> -		if (i2 == -1 && starts_with(s2 + off, suffix))
> -			i2 = i;
> +		int j, start, suffix_len = strlen(suffix);
> +		if (suffix_len < off)
> +			start = off - suffix_len + 1;
> +		else
> +			start = 0;

Now that this function has to rewind the beginning of the comparison
earlier than the given 'off', it makes me wonder if it still makes
sense for the caller to compute it in the first place.

> +		for (j = start; j <= off; j++)
> +			if (i1 == -1 && starts_with(s1 + j, suffix)) {
> +				i1 = i;
> +				break;
> +			}
> +		for (j = start; j <= off; j++)
> +			if (i2 == -1 && starts_with(s2 + j, suffix)) {
> +				i2 = i;
> +				break;
> +			}
>  	}
>  	if (i1 == -1 && i2 == -1)
>  		return 0;

  reply	other threads:[~2016-12-12 21:27 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-05 22:42 2.10.0: multiple versionsort.prereleasesuffix buggy? Leho Kraav (Conversion Ready)
2016-09-05 23:21 ` Jeff King
2016-09-06  1:07   ` SZEDER Gábor
2016-09-06  4:07     ` Jeff King
2016-09-06 19:45       ` SZEDER Gábor
2016-09-07 15:12         ` [PATCH 0/5] Fix version sort prerelease reordering bug SZEDER Gábor
2016-09-07 15:12           ` [PATCH 1/5] t7004-tag: delete unnecessary tags with test_when_finished SZEDER Gábor
2016-09-07 15:12           ` [PATCH 2/5] t7004-tag: use test_config helper SZEDER Gábor
2016-09-07 15:12           ` [PATCH 3/5] t7004-tag: add version sort tests to show prerelease reordering issues SZEDER Gábor
2016-09-07 15:12           ` [PATCH 4/5] versioncmp: pass full tagnames to swap_prereleases() SZEDER Gábor
2016-09-08 17:49             ` Junio C Hamano
2016-09-08 20:37               ` SZEDER Gábor
2016-09-08 21:31                 ` Junio C Hamano
2016-09-07 15:12           ` [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix SZEDER Gábor
2016-09-07 15:48             ` SZEDER Gábor
2016-09-09 10:43               ` Duy Nguyen
2016-10-05  1:33               ` SZEDER Gábor
2016-10-05 17:01                 ` Junio C Hamano
2016-10-05 21:26                   ` SZEDER Gábor
2016-10-05 22:15                     ` Junio C Hamano
2016-10-06  0:40                       ` Jacob Keller
2016-10-06  5:48                         ` Duy Nguyen
2016-12-08 14:23                 ` [PATCHv2 0/7] Fix and generalize version sort reordering SZEDER Gábor
2016-12-08 14:23                   ` [PATCHv2 1/7] t7004-tag: delete unnecessary tags with test_when_finished SZEDER Gábor
2016-12-08 14:23                   ` [PATCHv2 2/7] t7004-tag: use test_config helper SZEDER Gábor
2016-12-08 14:23                   ` [PATCHv2 3/7] t7004-tag: add version sort tests to show prerelease reordering issues SZEDER Gábor
2016-12-08 14:23                   ` [PATCHv2 4/7] versioncmp: pass full tagnames to swap_prereleases() SZEDER Gábor
2016-12-08 14:23                   ` [PATCHv2 5/7] versioncmp: cope with common part overlapping with prerelease suffix SZEDER Gábor
2016-12-12 21:27                     ` Junio C Hamano [this message]
2016-12-13  0:27                       ` SZEDER Gábor
2016-12-13  6:39                         ` Junio C Hamano
2016-12-08 14:24                   ` [PATCHv2 6/7] versioncmp: use earliest-longest contained suffix to determine sorting order SZEDER Gábor
2016-12-08 14:48                     ` [PATCHv2 6.5/7] squash! " SZEDER Gábor
2016-12-08 14:24                   ` [PATCHv2 7/7] versioncmp: generalize version sort suffix reordering SZEDER Gábor
2016-12-08 19:36                     ` Junio C Hamano
2016-12-14 17:08                   ` [PATCHv2 0/7] Fix and generalize version sort reordering Jeff King
2016-12-14 17:36                     ` Junio C Hamano
2016-12-20  8:50                     ` SZEDER Gábor
2016-12-20 16:49                       ` Jeff King
2016-09-06  7:12     ` 2.10.0: multiple versionsort.prereleasesuffix buggy? Leho Kraav (Conversion Ready)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqshps96ti.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=leho@conversionready.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).