* Re: [PATCH 1/5] am: Fix filename in safe_to_abort() error message
From: Paul Tan @ 2016-12-08 10:21 UTC (permalink / raw)
To: Stephan Beyer
Cc: Git List, Christian Couder, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20161207215133.13433-1-s-beyer@gmx.net>
Hi Stephan,
On Thu, Dec 8, 2016 at 5:51 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> diff --git a/builtin/am.c b/builtin/am.c
> index 6981f42ce..7cf40e6f2 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)
>
> if (read_state_file(&sb, state, "abort-safety", 1) > 0) {
> if (get_oid_hex(sb.buf, &abort_safety))
> - die(_("could not parse %s"), am_path(state, "abort_safety"));
> + die(_("could not parse %s"), am_path(state, "abort-safety"));
Ah, this is obviously correct. Sorry for the oversight.
> } else
> oidclr(&abort_safety);
>
> --
> 2.11.0.27.g4eed97c
Thanks,
Paul
^ permalink raw reply
* Re: [PATCHv6 5/7] worktree: add function to check if worktrees are in use
From: Duy Nguyen @ 2016-12-08 10:40 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <20161208014623.7588-6-sbeller@google.com>
On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> worktree.c | 24 ++++++++++++++++++++++++
> worktree.h | 7 +++++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/worktree.c b/worktree.c
> index 75db689672..2559f33846 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -406,3 +406,27 @@ const struct worktree *find_shared_symref(const char *symref,
>
> return existing;
> }
> +
> +static int uses_worktree_internal(struct worktree **worktrees)
> +{
> + int i;
> + for (i = 0; worktrees[i]; i++)
> + ; /* nothing */
> +
> + free_worktrees(worktrees);
Ayy.. caller allocates, callee frees. This might become a new
maintenance nightmare. Elsewhere I believe we (Junio and me) discussed
the possibility of returning the number of worktrees from
get_worktrees() too. get_worktrees() would take an "int *", if not
NULL, we return the number of worktrees in that pointer.
It's probably a better approach, although I'm afraid it'll add a bit
more work on you.
Alternatively, we could add a new flag to get_worktrees() to tell it
to return all worktrees if there is a least one linked worktree, or
return NULL if there's only main worktree. I'm not sure if this is
clever or very stupid.
> + return i > 1;
> +}
> +
> +int uses_worktrees(void)
"has" may be a better verb than "uses". maybe "has_linked_worktrees"
since we always have and use the main worktree.
> +{
> + return uses_worktree_internal(get_worktrees(0));
> +}
> +
> +int submodule_uses_worktrees(const char *path)
> +{
> + struct worktree **worktrees = get_submodule_worktrees(path, 0);
> + if (!worktrees)
> + return 0;
> +
> + return uses_worktree_internal(worktrees);
> +}
> diff --git a/worktree.h b/worktree.h
> index 157fbc4a66..76027b1fd2 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -33,6 +33,13 @@ extern struct worktree **get_worktrees(unsigned flags);
> extern struct worktree **get_submodule_worktrees(const char *path,
> unsigned flags);
>
> +/*
> + * Returns 1 if more than one worktree exists.
> + * Returns 0 if only the main worktree exists.
> + */
> +extern int uses_worktrees(void);
> +extern int submodule_uses_worktrees(const char *path);
> +
> /*
> * Return git dir of the worktree. Note that the path may be relative.
> * If wt is NULL, git dir of current worktree is returned.
> --
> 2.11.0.rc2.30.gc512cbd.dirty
>
--
Duy
^ permalink raw reply
* Re: [PATCHv6 5/7] worktree: add function to check if worktrees are in use
From: Duy Nguyen @ 2016-12-08 10:51 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <CACsJy8ANNz6FsJ4_5MOhj2Qqd+wHHu5UpVOAobqEiHU2KM26eg@mail.gmail.com>
On Thu, Dec 8, 2016 at 5:40 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> Alternatively, we could add a new flag to get_worktrees() to tell it
> to return all worktrees if there is a least one linked worktree, or
> return NULL if there's only main worktree. I'm not sure if this is
> clever or very stupid.
No, this may be better. Add a flag to say "returns linked worktrees
_only_". Which means when you're in a "normal" repo, get_worktrees()
with this flag returns NULL. When you're in a multiple-worktree repo,
it returns all linked worktrees (no main worktree). I think I might
have a use for this flag in addition to this uses_worktrees() here.
uses_worktrees() look quite simple with that flag
int uses_worktrees(void)
{
struct worktree **worktrees = get_worktrees(WT_LINKED_ONLY);
int retval = worktrees != NULL;
free_worktrees(worktrees);
return retval;
}
--
Duy
^ permalink raw reply
* Re: [PATCHv6 7/7] submodule: add absorb-git-dir function
From: Duy Nguyen @ 2016-12-08 10:56 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <20161208014623.7588-8-sbeller@google.com>
On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
> diff --git a/dir.c b/dir.c
> index 8b74997c66..cc5729f733 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2774,3 +2774,15 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
> free(real_work_tree);
> free(real_git_dir);
> }
> +
> +/*
> + * Migrate the git directory of the given path from old_git_dir to new_git_dir.
> + */
> +void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
> +{
> + if (rename(old_git_dir, new_git_dir) < 0)
> + die_errno(_("could not migrate git directory from '%s' to '%s'"),
> + old_git_dir, new_git_dir);
> +
> + connect_work_tree_and_git_dir(path, new_git_dir);
> +}
Thank you!
--
Duy
^ permalink raw reply
* Re: [PATCHv5 5/5] submodule: add embed-git-dir function
From: Duy Nguyen @ 2016-12-08 11:01 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <20161207210157.18932-6-sbeller@google.com>
On Thu, Dec 8, 2016 at 4:01 AM, Stefan Beller <sbeller@google.com> wrote:
> +/*
> + * Migrate the git directory of the given `path` from `old_git_dir` to
> + * `new_git_dir`. If an error occurs, append it to `err` and return the
> + * error code.
> + */
> +int relocate_gitdir(const char *path, const char *old_git_dir,
> + const char *new_git_dir, const char *displaypath,
> + struct strbuf *err)
> +{
> + int ret = 0;
> +
> + printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
> + displaypath, old_git_dir, new_git_dir);
I'm going to nag you about _() until your fingers automatically type
"_(" after "(" :-D
--
Duy
^ permalink raw reply
* Re: [PATCH 01/17] mv: convert to using pathspec struct interface
From: Duy Nguyen @ 2016-12-08 11:04 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20161208003604.GK116201@google.com>
On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams <bmwill@google.com> wrote:
>> > @@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix,
>> > {
>> > int i;
>> > const char **result;
>> > + struct pathspec ps;
>> > ALLOC_ARRAY(result, count + 1);
>> > - COPY_ARRAY(result, pathspec, count);
>> > - result[count] = NULL;
>> > +
>> > + /* Create an intermediate copy of the pathspec based on the flags */
>> > for (i = 0; i < count; i++) {
>> > - int length = strlen(result[i]);
>> > + int length = strlen(pathspec[i]);
>> > int to_copy = length;
>> > + char *it;
>> > while (!(flags & KEEP_TRAILING_SLASH) &&
>> > - to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
>> > + to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
>> > to_copy--;
>> > - if (to_copy != length || flags & DUP_BASENAME) {
>> > - char *it = xmemdupz(result[i], to_copy);
>> > - if (flags & DUP_BASENAME) {
>> > - result[i] = xstrdup(basename(it));
>> > - free(it);
>> > - } else
>> > - result[i] = it;
>> > - }
>> > +
>> > + it = xmemdupz(pathspec[i], to_copy);
>> > + if (flags & DUP_BASENAME) {
>> > + result[i] = xstrdup(basename(it));
>> > + free(it);
>> > + } else
>> > + result[i] = it;
>> > + }
>> > + result[count] = NULL;
>> > +
>> > + parse_pathspec(&ps,
>> > + PATHSPEC_ALL_MAGIC &
>> > + ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
>> > + PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
>> > + prefix, result);
>> > + assert(count == ps.nr);
>> > +
>> > + /* Copy the pathspec and free the old intermediate strings */
>> > + for (i = 0; i < count; i++) {
>> > + const char *match = xstrdup(ps.items[i].match);
>> > + free((char *) result[i]);
>> > + result[i] = match;
>>
>> Sigh.. it looks so weird that we do all the parsing (in a _copy_
>> pathspec function) then remove struct pathspec and return the plain
>> string. I guess we can't do anything more until we rework cmd_mv code
>> to handle pathspec natively.
>>
>> At the least I think we should rename this function to something else.
>> But if you have time I really wish we could kill this function. I
>> haven't stared at cmd_mv() long and hard, but it looks to me that we
>> combining two separate functionalities in the same function here.
>>
>> If "mv" takes n arguments, then the first <n-1> arguments may be
>> pathspec, the last one is always a plain path. The "dest_path =
>> internal_copy_pathspec..." could be as simple as "dest_path =
>> prefix_path(argv[argc - 1])". the special treatment for this last
>> argument [1] can live here. Then, we can do parse_pathspec for the
>> <n-1> arguments in cmd_mv(). It's still far from perfect, because
>> cmd_mv can't handle pathspec properly, but it reduces the messy mess
>> in internal_copy_pathspec a bit, I hope.
>>
>
> Actually, after looking at this a bit more it seems like we could
> technically use prefix_path for both source and dest (based on how the
> current code is structured) since the source's provied must all exist (as
> in no wildcards are allowed). We could drop using the pathspec struct
> completely in addition to renaming the function (to what I'm still
> unsure).
Yeah that sounds good too (with a caveat: I'm not a heavy user of
git-mv nor touching this code a lot, I might be missing something).
It'll take some looong time before somebody starts converting it to
use pathspec properly, I guess. prefix_path() would keep the code
clean meanwhile.
> I agree that this code should probably be rewritten and made a
> bit cleaner, I don't know if that fits in the scope of this series or
> should be done as a followup patch. If you think it fits here then I
> can try and find some time to do the rework.
--
Duy
^ permalink raw reply
* Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Duy Nguyen @ 2016-12-08 11:05 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20161208000357.GJ116201@google.com>
On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/07, Duy Nguyen wrote:
>> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
>> > Convert 'create_simplify()' to use the pathspec struct interface from
>> > using the '_raw' entry in the pathspec.
>>
>> It would be even better to kill this create_simplify() and let
>> simplify_away() handle struct pathspec directly.
>>
>> There is a bug in this code, that might have been found if we
>> simpify_away() handled pathspec directly: the memcmp() in
>> simplify_away() will not play well with :(icase) magic. My bad. If
>> :(icase) is used, the easiest/safe way is simplify nothing. Later on
>> maybe we can teach simplify_away() to do strncasecmp instead. We could
>> ignore exclude patterns there too (although not excluding is not a
>> bug).
>
> So are you implying that the simplify struct needs to be killed? That
> way the pathspec struct itself is being passed around instead?
Yes. simplify struct was a thing when pathspec was an array of char *.
At this point I think it can retire (when we have time to retire it)
--
Duy
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Johannes Sixt @ 2016-12-08 11:32 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git, sbeller, peff, jacob.keller
In-Reply-To: <20161207222927.GB116201@google.com>
Am 07.12.2016 um 23:29 schrieb Brandon Williams:
> Instead of assuming root is "/"
> I'll need to extract what root is from an absolute path. Aside from
> what root looks like, do most other path constructs behave similarly in
> unix and windows? (like ".." and "." as examples)
Yes, .. and . work the same way, except that they cannot appear in the
\\server\share part. I also think that .. does not cancel these parts.
As long as you use is_absolute_path() and do not simplify path
components before offset_1st_component(), you should be on the safe side.
> Since I don't really have a windows machine to test things it might be
> slightly difficult to get everything correct quickly but hopefully we can
> get this working :)
I'll lend a hand, of course, as time permits.
-- Hannes
^ permalink raw reply
* Serious bug with Git-2.11.0-64-bit and Git-LFS
From: Nick Warr @ 2016-12-08 11:46 UTC (permalink / raw)
To: git
Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with
1.5.3) cloning from our locally hosted gitlab CE server via HTTPS.
When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the
first time the clone will finish the checkout of the git part, then
when it starts downloading the LFS parts it will reliably finish with
a smudge filter error.
This leaves the repo in an unstable condition, you can then fetch the
lfs part without issue, but checking out the lfs files or trying a git
reset --hard will continue to spit out the same error. As you can see,
the actual error is not particularly useful.
fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi
Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs
failed
Unknown command ""
Possibly it's due to the file extension being all capital letters, we
did manage to change the error by recommitting the file with a
lowercase extension, but it failed on the next file (which also had a
capital letter extension).
This has happened on multiple fresh windows 10 64 bit installs,
different machines and target directories (to hopefully remove the
possibility of file permissions) where cloning is taking place.
The solution is to back level to Git 2.10.2 and the error disappears.
More than willing to provide any further information,
Nick Warr
^ permalink raw reply
* Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR
From: Johannes Schindelin @ 2016-12-08 11:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Robbie Iannucci
In-Reply-To: <20161207194105.25780-4-gitster@pobox.com>
Hi Junio,
On Wed, 7 Dec 2016, Junio C Hamano wrote:
> The "libify sequencer" topic stopped passing the die_on_error option
> to hold_locked_index(), and this lost an error message from "git
> merge --ff-only $commit" when there are competing updates in
> progress.
Sorry for the breakage.
When libifying the code, I tried to be careful to retain the error
messages when not dying, and mistakenly assumed that hold_locked_index()
would do the same.
Ciao,
Dscho
^ permalink raw reply
* [PATCH v2] commit: make --only --allow-empty work without paths
From: Andreas Krey @ 2016-12-08 13:50 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <xmqqd1h63xqn.fsf@gitster.mtv.corp.google.com>
--only is implied when paths are present, and required
them unless --amend. But with --allow-empty it should
be allowed as well - it is the only way to create an
empty commit in the presence of staged changes.
Also remove the post-fact cleverness indication;
it's in the man page anyway.
Signed-off-by: Andreas Krey <a.krey@gmx.de>
---
Ok, I've removed the clever message, as Junio suggested.
I don't know what else to do to make it acceptable. :-)
We're going to deploy it internally anyway, but I think
it belongs in git.git as well (aka 'Can I has "will queue"?').
Documentation/git-commit.txt | 3 ++-
builtin/commit.c | 4 +---
t/t7501-commit.sh | 9 +++++++++
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f2ab0ee2e..4f8f20a36 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -265,7 +265,8 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)
If this option is specified together with `--amend`, then
no paths need to be specified, which can be used to amend
the last commit without committing changes that have
- already been staged.
+ already been staged. If used together with `--allow-empty`
+ paths are also not required, and an empty commit will be created.
-u[<mode>]::
--untracked-files[=<mode>]::
diff --git a/builtin/commit.c b/builtin/commit.c
index 8976c3d29..276c74034 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1206,10 +1206,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
if (also + only + all + interactive > 1)
die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
- if (argc == 0 && (also || (only && !amend)))
+ if (argc == 0 && (also || (only && !amend && !allow_empty)))
die(_("No paths with --include/--only does not make sense."));
- if (argc == 0 && only && amend)
- only_include_assumed = _("Clever... amending the last one with dirty index.");
if (argc > 0 && !also && !only)
only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths...");
if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a67..0d8d89309 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -155,6 +155,15 @@ test_expect_success 'amend --only ignores staged contents' '
git diff --exit-code
'
+test_expect_success 'allow-empty --only ignores staged contents' '
+ echo changed-again >file &&
+ git add file &&
+ git commit --allow-empty --only -m "empty" &&
+ git cat-file blob HEAD:file >file.actual &&
+ test_cmp file.expect file.actual &&
+ git diff --exit-code
+'
+
test_expect_success 'set up editor' '
cat >editor <<-\EOF &&
#!/bin/sh
--
2.11.0.10.g1e1b186.dirty
^ permalink raw reply related
* Re: Serious bug with Git-2.11.0-64-bit and Git-LFS
From: Lars Schneider @ 2016-12-08 13:57 UTC (permalink / raw)
To: Nick Warr; +Cc: git
In-Reply-To: <CABW+60x0PSw7uNQZg4SeN7EAbNpraR_HWvnVFz1-fVLYX=B8RQ@mail.gmail.com>
> On 08 Dec 2016, at 12:46, Nick Warr <nick.warr@bossastudios.com> wrote:
>
> Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with
> 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS.
>
> When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the
> first time the clone will finish the checkout of the git part, then
> when it starts downloading the LFS parts it will reliably finish with
> a smudge filter error.
>
> This leaves the repo in an unstable condition, you can then fetch the
> lfs part without issue, but checking out the lfs files or trying a git
> reset --hard will continue to spit out the same error. As you can see,
> the actual error is not particularly useful.
>
> fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi
> Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs
> failed
> Unknown command ""
>
> Possibly it's due to the file extension being all capital letters, we
> did manage to change the error by recommitting the file with a
> lowercase extension, but it failed on the next file (which also had a
> capital letter extension).
>
> This has happened on multiple fresh windows 10 64 bit installs,
> different machines and target directories (to hopefully remove the
> possibility of file permissions) where cloning is taking place.
>
> The solution is to back level to Git 2.10.2 and the error disappears.
>
> More than willing to provide any further information,
Hi Nick,
debris_junk.FBX is not stored properly in Git LFS.
I explained the problem in detail here:
https://github.com/git-lfs/git-lfs/issues/1729
You should add the file properly to GitLFS to fix the problem.
However, I think this is a regression in GitLFS and I hope it will
be fixed in the next version.
No change/fix in Git is required.
Cheers,
Lars
^ permalink raw reply
* Re: Serious bug with Git-2.11.0-64-bit and Git-LFS
From: Nick Warr @ 2016-12-08 14:00 UTC (permalink / raw)
To: Lars Schneider; +Cc: git
In-Reply-To: <06520F42-BD49-4349-83B3-74DCA1E260CD@gmail.com>
That looks pretty much like the error we're dealing with, any reason
why going back a point version on Git (not git-lfs) would resolve the
issue however?
On 8 December 2016 at 13:57, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>> On 08 Dec 2016, at 12:46, Nick Warr <nick.warr@bossastudios.com> wrote:
>>
>> Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with
>> 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS.
>>
>> When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the
>> first time the clone will finish the checkout of the git part, then
>> when it starts downloading the LFS parts it will reliably finish with
>> a smudge filter error.
>>
>> This leaves the repo in an unstable condition, you can then fetch the
>> lfs part without issue, but checking out the lfs files or trying a git
>> reset --hard will continue to spit out the same error. As you can see,
>> the actual error is not particularly useful.
>>
>> fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi
>> Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs
>> failed
>> Unknown command ""
>>
>> Possibly it's due to the file extension being all capital letters, we
>> did manage to change the error by recommitting the file with a
>> lowercase extension, but it failed on the next file (which also had a
>> capital letter extension).
>>
>> This has happened on multiple fresh windows 10 64 bit installs,
>> different machines and target directories (to hopefully remove the
>> possibility of file permissions) where cloning is taking place.
>>
>> The solution is to back level to Git 2.10.2 and the error disappears.
>>
>> More than willing to provide any further information,
>
> Hi Nick,
>
> debris_junk.FBX is not stored properly in Git LFS.
> I explained the problem in detail here:
> https://github.com/git-lfs/git-lfs/issues/1729
>
> You should add the file properly to GitLFS to fix the problem.
> However, I think this is a regression in GitLFS and I hope it will
> be fixed in the next version.
>
> No change/fix in Git is required.
>
> Cheers,
> Lars
^ permalink raw reply
* [PATCHv2 1/7] t7004-tag: delete unnecessary tags with test_when_finished
From: SZEDER Gábor @ 2016-12-08 14:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Nguyễn Thái Ngọc Duy, Leho Kraav, git,
SZEDER Gábor
In-Reply-To: <20161208142401.1329-1-szeder.dev@gmail.com>
The '--force is moot with a non-existing tag name' test creates two
new tags, which are then deleted right after the test is finished,
outside the test_expect_success block, allowing 'git tag -d's output to
pollute the test output.
Use test_when_finished to delete those tags.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/t7004-tag.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 8b0f71a2a..396cffeeb 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -122,11 +122,11 @@ test_expect_success '--force can create a tag with the name of one existing' '
tag_exists mytag'
test_expect_success '--force is moot with a non-existing tag name' '
+ test_when_finished git tag -d newtag forcetag &&
git tag newtag >expect &&
git tag --force forcetag >actual &&
test_cmp expect actual
'
-git tag -d newtag forcetag
# deleting tags:
--
2.11.0.78.g5a2d011
^ permalink raw reply related
* [PATCHv2 3/7] t7004-tag: add version sort tests to show prerelease reordering issues
From: SZEDER Gábor @ 2016-12-08 14:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Nguyễn Thái Ngọc Duy, Leho Kraav, git,
SZEDER Gábor
In-Reply-To: <20161208142401.1329-1-szeder.dev@gmail.com>
Version sort with prerelease reordering sometimes puts tagnames in the
wrong order, when the common part of two compared tagnames ends with
the leading character(s) of one or more configured prerelease
suffixes. Add tests that demonstrate these issues.
The unrelated '--format should list tags as per format given' test
later uses tags matching the same prefix as the version sort tests,
thus was affected by the new tags added for the new tests in this
patch. Change that test to perform its checks on a different set of
tags.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/t7004-tag.sh | 36 ++++++++++++++++++++++++++++++------
1 file changed, 30 insertions(+), 6 deletions(-)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 920a1b4b2..6445aae29 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1538,6 +1538,32 @@ test_expect_success 'reverse version sort with prerelease reordering' '
test_cmp expect actual
'
+test_expect_failure 'version sort with prerelease reordering and common leading character' '
+ test_config versionsort.prereleaseSuffix -before &&
+ git tag foo1.7-before1 &&
+ git tag foo1.7 &&
+ git tag foo1.7-after1 &&
+ git tag -l --sort=version:refname "foo1.7*" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.7-before1
+ foo1.7
+ foo1.7-after1
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_failure 'version sort with prerelease reordering, multiple suffixes and common leading character' '
+ test_config versionsort.prereleaseSuffix -before &&
+ git config --add versionsort.prereleaseSuffix -after &&
+ git tag -l --sort=version:refname "foo1.7*" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.7-before1
+ foo1.7-after1
+ foo1.7
+ EOF
+ test_cmp expect actual
+'
+
run_with_limited_stack () {
(ulimit -s 128 && "$@")
}
@@ -1566,13 +1592,11 @@ EOF"
test_expect_success '--format should list tags as per format given' '
cat >expect <<-\EOF &&
- refname : refs/tags/foo1.10
- refname : refs/tags/foo1.3
- refname : refs/tags/foo1.6
- refname : refs/tags/foo1.6-rc1
- refname : refs/tags/foo1.6-rc2
+ refname : refs/tags/v1.0
+ refname : refs/tags/v1.0.1
+ refname : refs/tags/v1.1.3
EOF
- git tag -l --format="refname : %(refname)" "foo*" >actual &&
+ git tag -l --format="refname : %(refname)" "v1*" >actual &&
test_cmp expect actual
'
--
2.11.0.78.g5a2d011
^ permalink raw reply related
* [PATCHv2 5/7] versioncmp: cope with common part overlapping with prerelease suffix
From: SZEDER Gábor @ 2016-12-08 14:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Nguyễn Thái Ngọc Duy, Leho Kraav, git,
SZEDER Gábor
In-Reply-To: <20161208142401.1329-1-szeder.dev@gmail.com>
Version sort with prerelease reordering sometimes puts tagnames in the
wrong order, when the common part of two compared tagnames overlaps
with the leading character(s) of one or more configured prerelease
suffixes. Note the position of "v2.1.0-beta-1":
$ git -c versionsort.prereleaseSuffix=-beta \
tag -l --sort=version:refname v2.1.*
v2.1.0-beta-2
v2.1.0-beta-3
v2.1.0
v2.1.0-RC1
v2.1.0-RC2
v2.1.0-beta-1
v2.1.1
v2.1.2
The reason is that when comparing a pair of tagnames, first
versioncmp() looks for the first different character in a pair of
tagnames, and then the swap_prereleases() helper function looks for a
configured prerelease suffix _starting at_ that character. Thus, when
in the above example the sorting algorithm happens to compare the
tagnames "v2.1.0-beta-1" and "v2.1.0-RC2", swap_prereleases() tries to
match the suffix "-beta" against "beta-1" to no avail, and the two
tagnames erroneously end up being ordered lexicographically.
To fix this issue change swap_prereleases() to look for configured
prerelease suffixes _containing_ the position of that first different
character.
Care must be taken, when a configured suffix is longer than the
tagnames' common part up to the first different character, to avoid
reading memory before the beginning of the tagnames. Add a test that
uses an exceptionally long prerelease suffix to check for this, in the
hope that in case of a regression the illegal memory access causes a
segfault in 'git tag' on one of the commonly used platforms (the test
happens to pass successfully on my Linux system with the safety check
removed), or at least makes valgrind complain.
Under some circumstances it's possible that more than one prerelease
suffixes can be found in the same tagname around that first different
character. With this simple bugfix patch such a tagname is sorted
according to the contained suffix that comes first in the
configuration for now. This is less than ideal in some cases, and the
following patch will take care of those.
Reported-by: Leho Kraav <leho@conversionready.com>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
Documentation/config.txt | 8 ++++++--
t/t7004-tag.sh | 9 +++++++--
versioncmp.c | 28 +++++++++++++++++++++-------
3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb6790d..c1a9616e9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3008,8 +3008,12 @@ versionsort.prereleaseSuffix::
This variable can be specified multiple times, once per suffix. The
order of suffixes in the config file determines the sorting order
(e.g. if "-pre" appears before "-rc" in the config file then 1.0-preXX
-is sorted before 1.0-rcXX). The sorting order between different
-suffixes is undefined if they are in multiple config files.
+is sorted before 1.0-rcXX).
+If more than one suffixes match the same tagname, then that tagname will
+be sorted according to the matching suffix which comes first in the
+configuration.
+The sorting order between different suffixes is undefined if they are
+in multiple config files.
web.browser::
Specify a web browser that may be used by some commands.
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 6445aae29..c7aaace8c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1538,7 +1538,7 @@ test_expect_success 'reverse version sort with prerelease reordering' '
test_cmp expect actual
'
-test_expect_failure 'version sort with prerelease reordering and common leading character' '
+test_expect_success 'version sort with prerelease reordering and common leading character' '
test_config versionsort.prereleaseSuffix -before &&
git tag foo1.7-before1 &&
git tag foo1.7 &&
@@ -1552,7 +1552,7 @@ test_expect_failure 'version sort with prerelease reordering and common leading
test_cmp expect actual
'
-test_expect_failure 'version sort with prerelease reordering, multiple suffixes and common leading character' '
+test_expect_success 'version sort with prerelease reordering, multiple suffixes and common leading character' '
test_config versionsort.prereleaseSuffix -before &&
git config --add versionsort.prereleaseSuffix -after &&
git tag -l --sort=version:refname "foo1.7*" >actual &&
@@ -1564,6 +1564,11 @@ test_expect_failure 'version sort with prerelease reordering, multiple suffixes
test_cmp expect actual
'
+test_expect_success 'version sort with very long prerelease suffix' '
+ test_config versionsort.prereleaseSuffix -very-looooooooooooooooooooooooong-prerelease-suffix &&
+ git tag -l --sort=version:refname
+'
+
run_with_limited_stack () {
(ulimit -s 128 && "$@")
}
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;
+ 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;
--
2.11.0.78.g5a2d011
^ permalink raw reply related
* [PATCHv2 7/7] versioncmp: generalize version sort suffix reordering
From: SZEDER Gábor @ 2016-12-08 14:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Nguyễn Thái Ngọc Duy, Leho Kraav, git,
SZEDER Gábor
In-Reply-To: <20161208142401.1329-1-szeder.dev@gmail.com>
The 'versionsort.prereleaseSuffix' configuration variable, as its name
suggests, is supposed to only deal with tagnames with prerelease
suffixes, and allows sorting those prerelease tags in a user-defined
order before the suffixless main release tag, instead of sorting them
simply lexicographically.
However, the previous changes in this series resulted in an
interesting and useful property of version sort:
- The empty string as a configured suffix matches all tagnames,
including tagnames without any suffix, but
- tagnames containing a "real" configured suffix are still ordered
according to that real suffix, because any longer suffix takes
precedence over the empty string.
Exploiting this property we can easily generalize suffix reordering
and specify the order of tags with given suffixes not only before but
even after a main release tag by using the empty suffix to denote the
position of the main release tag, without any algorithm changes:
$ git -c versionsort.prereleaseSuffix=-alpha \
-c versionsort.prereleaseSuffix=-beta \
-c versionsort.prereleaseSuffix="" \
-c versionsort.prereleaseSuffix=-gamma \
-c versionsort.prereleaseSuffix=-delta \
tag -l --sort=version:refname 'v3.0*'
v3.0-alpha1
v3.0-beta1
v3.0
v3.0-gamma1
v3.0-delta1
Since 'versionsort.prereleaseSuffix' is not a fitting name for a
configuration variable to control this more general suffix reordering,
introduce the new variable 'versionsort.suffix'. Still keep the old
configuration variable name as a deprecated alias, though, to avoid
suddenly breaking setups already using it. Ignore the old variable if
both old and new configuration variables are set, but emit a warning
so users will be aware of it and can fix their configuration. Extend
the documentation to describe and add a test to check this more
general behavior.
Note: since the empty suffix matches all tagnames, tagnames with
suffixes not included in the configuration are listed together with
the suffixless main release tag, ordered lexicographically right after
that, i.e. before tags with suffixes listed in the configuration
following the empty suffix.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
Documentation/config.txt | 36 ++++++++++++++++++++++++++----------
Documentation/git-tag.txt | 4 ++--
t/t7004-tag.sh | 35 +++++++++++++++++++++++++++++++++++
versioncmp.c | 9 ++++++++-
4 files changed, 71 insertions(+), 13 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 58009c70c..ae85d4b9a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2999,16 +2999,32 @@ user.signingKey::
This option is passed unchanged to gpg's --local-user parameter,
so you may specify a key using any method that gpg supports.
-versionsort.prereleaseSuffix::
- When version sort is used in linkgit:git-tag[1], prerelease
- tags (e.g. "1.0-rc1") may appear after the main release
- "1.0". By specifying the suffix "-rc" in this variable,
- "1.0-rc1" will appear before "1.0".
-+
-This variable can be specified multiple times, once per suffix. The
-order of suffixes in the config file determines the sorting order
-(e.g. if "-pre" appears before "-rc" in the config file then 1.0-preXX
-is sorted before 1.0-rcXX).
+versionsort.prereleaseSuffix (deprecated)::
+ Deprecated alias for `versionsort.suffix`. Ignored if
+ `versionsort.suffix` is set.
+
+versionsort.suffix::
+ Even when version sort is used in linkgit:git-tag[1], tagnames
+ with the same base version but different suffixes are still sorted
+ lexicographically, resulting e.g. in prerelease tags appearing
+ after the main release (e.g. "1.0-rc1" after "1.0"). This
+ variable can be specified to determine the sorting order of tags
+ with different suffixes.
++
+By specifying a single suffix in this variable, any tagname containing
+that suffix will appear before the corresponding main release. E.g. if
+the variable is set to "-rc", then all "1.0-rcX" tags will appear before
+"1.0". If specified multiple times, once per suffix, then the order of
+suffixes in the configuration will determine the sorting order of tagnames
+with those suffixes. E.g. if "-pre" appears before "-rc" in the
+configuration, then all "1.0-preX" tags will be listed before any
+"1.0-rcX" tags. The placement of the main release tag relative to tags
+with various suffixes can be determined by specifying the empty suffix
+among those other suffixes. E.g. if the suffixes "-rc", "", "-ck" and
+"-bfs" appear in the configuration in this order, then all "v4.8-rcX" tags
+are listed first, followed by "v4.8", then "v4.8-ckX" and finally
+"v4.8-bfsX".
++
If more than one suffixes match the same tagname, then that tagname will
be sorted according to the suffix which starts at the earliest position in
the tagname. If more than one different matching suffixes start at
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 7ecca8e24..19990850d 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -101,8 +101,8 @@ OPTIONS
multiple times, in which case the last key becomes the primary
key. Also supports "version:refname" or "v:refname" (tag
names are treated as versions). The "version:refname" sort
- order can also be affected by the
- "versionsort.prereleaseSuffix" configuration variable.
+ order can also be affected by the "versionsort.suffix"
+ configuration variable.
The keys supported are the same as those in `git for-each-ref`.
Sort order defaults to the value configured for the `tag.sort`
variable if it exists, or lexicographic order otherwise. See
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e2efe312d..bdd28dad1 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1595,6 +1595,41 @@ test_expect_success 'version sort with prerelease reordering, multiple suffixes
test_cmp expect actual
'
+test_expect_success 'version sort with general suffix reordering' '
+ test_config versionsort.suffix -alpha &&
+ git config --add versionsort.suffix -beta &&
+ git config --add versionsort.suffix "" &&
+ git config --add versionsort.suffix -gamma &&
+ git config --add versionsort.suffix -delta &&
+ git tag foo1.10-alpha &&
+ git tag foo1.10-beta &&
+ git tag foo1.10-gamma &&
+ git tag foo1.10-delta &&
+ git tag foo1.10-unlisted-suffix &&
+ git tag -l --sort=version:refname "foo1.10*" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.10-alpha
+ foo1.10-beta
+ foo1.10
+ foo1.10-unlisted-suffix
+ foo1.10-gamma
+ foo1.10-delta
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'versionsort.suffix overrides versionsort.prereleaseSuffix' '
+ test_config versionsort.suffix -before &&
+ test_config versionsort.prereleaseSuffix -after &&
+ git tag -l --sort=version:refname "foo1.7*" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.7-before1
+ foo1.7
+ foo1.7-after1
+ EOF
+ test_cmp expect actual
+'
+
test_expect_success 'version sort with very long prerelease suffix' '
test_config versionsort.prereleaseSuffix -very-looooooooooooooooooooooooong-prerelease-suffix &&
git tag -l --sort=version:refname
diff --git a/versioncmp.c b/versioncmp.c
index ae0a9199b..62c8d69dc 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -145,8 +145,15 @@ int versioncmp(const char *s1, const char *s2)
}
if (!initialized) {
+ const struct string_list *deprecated_prereleases;
initialized = 1;
- prereleases = git_config_get_value_multi("versionsort.prereleasesuffix");
+ prereleases = git_config_get_value_multi("versionsort.suffix");
+ deprecated_prereleases = git_config_get_value_multi("versionsort.prereleasesuffix");
+ if (prereleases) {
+ if (deprecated_prereleases)
+ warning("ignoring versionsort.prereleasesuffix because versionsort.suffix is set");
+ } else
+ prereleases = deprecated_prereleases;
}
if (prereleases && swap_prereleases(s1, s2, (const char *) p1 - s1 - 1,
&diff))
--
2.11.0.78.g5a2d011
^ permalink raw reply related
* [PATCHv2 6/7] versioncmp: use earliest-longest contained suffix to determine sorting order
From: SZEDER Gábor @ 2016-12-08 14:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Nguyễn Thái Ngọc Duy, Leho Kraav, git,
SZEDER Gábor
In-Reply-To: <20161208142401.1329-1-szeder.dev@gmail.com>
When comparing tagnames, it is possible that a tagname contains more
than one of the configured prerelease suffixes around the first
different character. After fixing a bug in the previous commit such a
tagname is sorted according to the contained suffix which comes first
in the configuration. This is, however, not quite the right thing to
do in the following corner cases:
1. $ git -c versionsort.suffix=-bar
-c versionsort.suffix=-foo-baz
-c versionsort.suffix=-foo-bar
tag -l --sort=version:refname 'v1*'
v1.0-foo-bar
v1.0-foo-baz
The suffix of the tagname 'v1.0-foo-bar' is clearly '-foo-bar',
so it should be listed last. However, as it also contains '-bar'
around the first different character, it is listed first instead,
because that '-bar' suffix comes first the configuration.
2. One of the configured suffixes starts with the other:
$ git -c versionsort.prereleasesuffix=-pre \
-c versionsort.prereleasesuffix=-prerelease \
tag -l --sort=version:refname 'v2*'
v2.0-prerelease1
v2.0-pre1
v2.0-pre2
Here the tagname 'v2.0-prerelease1' should be the last. When
comparing 'v2.0-pre1' and 'v2.0-prerelease1' the first different
characters are '1' and 'r', respectively. Since this first
different character must be part of the configured suffix, the
'-pre' suffix is not recognized in the first tagname. OTOH, the
'-prerelease' suffix is properly recognized in
'v2.0-prerelease1', thus it is listed first.
Improve version sort in these corner cases, and
- look for a configured prerelease suffix containing the first
different character or ending right before it, so the '-pre'
suffixes are recognized in case (2). This also means that
when comparing tagnames 'v2.0-pre1' and 'v2.0-pre2',
swap_prereleases() would find the '-pre' suffix in both, but then
it will return "undecided" and the caller will do the right thing
by sorting based in '1' and '2'.
- If the tagname contains more than one suffix, then give precedence
to the contained suffix that starts at the earliest offset in the
tagname to address (1).
- If there are more than one suffixes starting at that earliest
position, then give precedence to the longest of those suffixes,
thus ensuring that in (2) the tagname 'v2.0-prerelease1' won't be
sorted based on the '-pre' suffix.
Add tests for these corner cases and adjust the documentation
accordingly.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
Documentation/config.txt | 6 ++++--
t/t7004-tag.sh | 31 +++++++++++++++++++++++++++++++
versioncmp.c | 33 +++++++++++++++++++++++++--------
3 files changed, 60 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index c1a9616e9..58009c70c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3010,8 +3010,10 @@ order of suffixes in the config file determines the sorting order
(e.g. if "-pre" appears before "-rc" in the config file then 1.0-preXX
is sorted before 1.0-rcXX).
If more than one suffixes match the same tagname, then that tagname will
-be sorted according to the matching suffix which comes first in the
-configuration.
+be sorted according to the suffix which starts at the earliest position in
+the tagname. If more than one different matching suffixes start at
+that earliest position, then that tagname will be sorted according to the
+longest of those suffixes.
The sorting order between different suffixes is undefined if they are
in multiple config files.
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index c7aaace8c..e2efe312d 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1564,6 +1564,37 @@ test_expect_success 'version sort with prerelease reordering, multiple suffixes
test_cmp expect actual
'
+test_expect_success 'version sort with prerelease reordering, multiple suffixes match the same tag' '
+ test_config versionsort.prereleaseSuffix -bar &&
+ git config --add versionsort.prereleaseSuffix -foo-baz &&
+ git config --add versionsort.prereleaseSuffix -foo-bar &&
+ git tag foo1.8-foo-bar &&
+ git tag foo1.8-foo-baz &&
+ git tag foo1.8 &&
+ git tag -l --sort=version:refname "foo1.8*" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.8-foo-baz
+ foo1.8-foo-bar
+ foo1.8
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'version sort with prerelease reordering, multiple suffixes match starting at the same position' '
+ test_config versionsort.prereleaseSuffix -pre &&
+ git config --add versionsort.prereleaseSuffix -prerelease &&
+ git tag foo1.9-pre1 &&
+ git tag foo1.9-pre2 &&
+ git tag foo1.9-prerelease1 &&
+ git tag -l --sort=version:refname "foo1.9*" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.9-pre1
+ foo1.9-pre2
+ foo1.9-prerelease1
+ EOF
+ test_cmp expect actual
+'
+
test_expect_success 'version sort with very long prerelease suffix' '
test_config versionsort.prereleaseSuffix -very-looooooooooooooooooooooooong-prerelease-suffix &&
git tag -l --sort=version:refname
diff --git a/versioncmp.c b/versioncmp.c
index f86ac562e..ae0a9199b 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -27,14 +27,18 @@ 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 containing
- * that offset, then that string will be forced to be on top.
+ * that offset or a suffix ends right before that offset, then that
+ * string will be forced to be on top.
*
* 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.
+ * suffix which starts at the earliest offset in that string.
+ * If more than one different contained suffixes start at that earliest
+ * offset, then that string is sorted according to the longest of those
+ * suffixes.
*
* Return non-zero if *diff contains the return value for versioncmp()
*/
@@ -44,27 +48,40 @@ static int swap_prereleases(const char *s1,
int *diff)
{
int i, i1 = -1, i2 = -1;
+ int start_at1 = off, start_at2 = off, match_len1 = -1, match_len2 = -1;
for (i = 0; i < prereleases->nr; i++) {
const char *suffix = prereleases->items[i].string;
- int j, start, suffix_len = strlen(suffix);
+ int j, start, end, suffix_len = strlen(suffix);
if (suffix_len < off)
- start = off - suffix_len + 1;
+ start = off - suffix_len;
else
start = 0;
- for (j = start; j <= off; j++)
- if (i1 == -1 && starts_with(s1 + j, suffix)) {
+ end = match_len1 < suffix_len ? start_at1 : start_at1-1;
+ for (j = start; j <= end; j++)
+ if (starts_with(s1 + j, suffix)) {
i1 = i;
+ start_at1 = j;
+ match_len1 = suffix_len;
break;
}
- for (j = start; j <= off; j++)
- if (i2 == -1 && starts_with(s2 + j, suffix)) {
+ end = match_len2 < suffix_len ? start_at2 : start_at2-1;
+ for (j = start; j <= end; j++)
+ if (starts_with(s2 + j, suffix)) {
i2 = i;
+ start_at2 = j;
+ match_len2 = suffix_len;
break;
}
}
if (i1 == -1 && i2 == -1)
return 0;
+ if (i1 == i2)
+ /* Found the same suffix in both, e.g. "-rc" in "v1.0-rcX"
+ * and "v1.0-rcY": the caller should decide based on "X"
+ * and "Y". */
+ return 0;
+
if (i1 >= 0 && i2 >= 0)
*diff = i1 - i2;
else if (i1 >= 0)
--
2.11.0.78.g5a2d011
^ permalink raw reply related
* [PATCHv2 2/7] t7004-tag: use test_config helper
From: SZEDER Gábor @ 2016-12-08 14:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Nguyễn Thái Ngọc Duy, Leho Kraav, git,
SZEDER Gábor
In-Reply-To: <20161208142401.1329-1-szeder.dev@gmail.com>
... instead of setting and then manually unsetting configuration
variables, on one occasion even outside the test_expect_success block.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/t7004-tag.sh | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 396cffeeb..920a1b4b2 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -297,11 +297,9 @@ EOF
'
test_expect_success 'listing tags in column with column.*' '
- git config column.tag row &&
- git config column.ui dense &&
+ test_config column.tag row &&
+ test_config column.ui dense &&
COLUMNS=40 git tag -l >actual &&
- git config --unset column.ui &&
- git config --unset column.tag &&
cat >expected <<\EOF &&
a1 aa1 cba t210 t211
v0.2.1 v1.0 v1.0.1 v1.1.3
@@ -314,9 +312,8 @@ test_expect_success 'listing tag with -n --column should fail' '
'
test_expect_success 'listing tags -n in column with column.ui ignored' '
- git config column.ui "row dense" &&
+ test_config column.ui "row dense" &&
COLUMNS=40 git tag -l -n >actual &&
- git config --unset column.ui &&
cat >expected <<\EOF &&
a1 Foo
aa1 Foo
@@ -1200,11 +1197,10 @@ test_expect_success GPG,RFC1991 \
'
# try to sign with bad user.signingkey
-git config user.signingkey BobTheMouse
test_expect_success GPG \
'git tag -s fails if gpg is misconfigured (bad key)' \
- 'test_must_fail git tag -s -m tail tag-gpg-failure'
-git config --unset user.signingkey
+ 'test_config user.signingkey BobTheMouse &&
+ test_must_fail git tag -s -m tail tag-gpg-failure'
# try to produce invalid signature
test_expect_success GPG \
@@ -1484,7 +1480,7 @@ test_expect_success 'reverse lexical sort' '
'
test_expect_success 'configured lexical sort' '
- git config tag.sort "v:refname" &&
+ test_config tag.sort "v:refname" &&
git tag -l "foo*" >actual &&
cat >expect <<-\EOF &&
foo1.3
@@ -1495,6 +1491,7 @@ test_expect_success 'configured lexical sort' '
'
test_expect_success 'option override configured sort' '
+ test_config tag.sort "v:refname" &&
git tag -l --sort=-refname "foo*" >actual &&
cat >expect <<-\EOF &&
foo1.6
@@ -1509,13 +1506,12 @@ test_expect_success 'invalid sort parameter on command line' '
'
test_expect_success 'invalid sort parameter in configuratoin' '
- git config tag.sort "v:notvalid" &&
+ test_config tag.sort "v:notvalid" &&
test_must_fail git tag -l "foo*"
'
test_expect_success 'version sort with prerelease reordering' '
- git config --unset tag.sort &&
- git config versionsort.prereleaseSuffix -rc &&
+ test_config versionsort.prereleaseSuffix -rc &&
git tag foo1.6-rc1 &&
git tag foo1.6-rc2 &&
git tag -l --sort=version:refname "foo*" >actual &&
@@ -1530,6 +1526,7 @@ test_expect_success 'version sort with prerelease reordering' '
'
test_expect_success 'reverse version sort with prerelease reordering' '
+ test_config versionsort.prereleaseSuffix -rc &&
git tag -l --sort=-version:refname "foo*" >actual &&
cat >expect <<-\EOF &&
foo1.10
--
2.11.0.78.g5a2d011
^ permalink raw reply related
* [PATCHv2 0/7] Fix and generalize version sort reordering
From: SZEDER Gábor @ 2016-12-08 14:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Nguyễn Thái Ngọc Duy, Leho Kraav, git,
SZEDER Gábor
In-Reply-To: <20161005033353.Horde.33pf2naqnF4HgwPWSy9DaHV@webmail.informatik.kit.edu>
> After having slept on this a couple of times
After having slept on it a few (erm...) more times...
> I think the longest
> matching prerelease suffix should determine the sorting order.
>
> A release tag usually consists of an optional prefix, e.g. 'v' or
> 'snapshot-', followed by the actual version number, followed by an
> optional suffix. In the contrived example quoted above this suffix
> is '-foo-bar', thus it feels wrong to match '-bar' against it, when
> the user explicitly configured '-foo-bar' as prerelease suffix as
> well.
At the risk of overthinking it: I think it would be more correct to
say that the earliest starting matching prerelease suffix should
determine the sorting order in the first place.
> Then here is a more realistic case for sorting based on the longest
> matching suffix, where
>
> - a longer suffix starts with the shorter one,
> - and the longer suffix comes after the shorter one in the
> configuration.
>
> With my patches it looks like this:
>
> $ git -c versionsort.prereleasesuffix=-pre \
> -c versionsort.prereleasesuffix=-prerelease \
> tag -l --sort=version:refname
> v1.0.0-prerelease1
> v1.0.0-pre1
> v1.0.0-pre2
> v1.0.0
And when there happen to be more than one matching suffixes starting
at the same earliest position, then we should pick the longest of
them. The new patch 6/7 implements this behavior.
Changes since v1:
- Documentation, in-code comment and commit message updates.
- Use different tagnames and suffixes in the tests, keeping the new
tests simpler and changes to existing tests smaller.
- Added a test of questionable value to try to check that we don't
read memory before the tagname strings in case of an unusually long
suffix.
- Removed unnecessary {}.
- Added two patches on top to address the corner cases and to
introduce 'versionsort.suffix' for the resulting more general
suffix reordering behavior.
The interdiff at the bottom is between v1 and only the first five
patches of v2, i.e. not including the two new patches. I think it's
easier to judge the changes this way.
SZEDER Gábor (7):
t7004-tag: delete unnecessary tags with test_when_finished
t7004-tag: use test_config helper
t7004-tag: add version sort tests to show prerelease reordering issues
versioncmp: pass full tagnames to swap_prereleases()
versioncmp: cope with common part overlapping with prerelease suffix
versioncmp: use earliest-longest contained suffix to determine sorting
order
versioncmp: generalize version sort suffix reordering
Documentation/config.txt | 44 ++++++++++++----
Documentation/git-tag.txt | 4 +-
t/t7004-tag.sh | 132 +++++++++++++++++++++++++++++++++++++++-------
versioncmp.c | 68 +++++++++++++++++-------
4 files changed, 196 insertions(+), 52 deletions(-)
--
2.11.0.78.g5a2d011
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb6790d..c1a9616e9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3008,8 +3008,12 @@ versionsort.prereleaseSuffix::
This variable can be specified multiple times, once per suffix. The
order of suffixes in the config file determines the sorting order
(e.g. if "-pre" appears before "-rc" in the config file then 1.0-preXX
-is sorted before 1.0-rcXX). The sorting order between different
-suffixes is undefined if they are in multiple config files.
+is sorted before 1.0-rcXX).
+If more than one suffixes match the same tagname, then that tagname will
+be sorted according to the matching suffix which comes first in the
+configuration.
+The sorting order between different suffixes is undefined if they are
+in multiple config files.
web.browser::
Specify a web browser that may be used by some commands.
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index f0cfe1fa3..c7aaace8c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1511,14 +1511,14 @@ test_expect_success 'invalid sort parameter in configuratoin' '
'
test_expect_success 'version sort with prerelease reordering' '
- test_config versionsort.prereleaseSuffix -beta &&
- git tag foo1.6-beta1 &&
- git tag foo1.6-beta2 &&
+ test_config versionsort.prereleaseSuffix -rc &&
+ git tag foo1.6-rc1 &&
+ git tag foo1.6-rc2 &&
git tag -l --sort=version:refname "foo*" >actual &&
cat >expect <<-\EOF &&
foo1.3
- foo1.6-beta1
- foo1.6-beta2
+ foo1.6-rc1
+ foo1.6-rc2
foo1.6
foo1.10
EOF
@@ -1526,54 +1526,49 @@ test_expect_success 'version sort with prerelease reordering' '
'
test_expect_success 'reverse version sort with prerelease reordering' '
- test_config versionsort.prereleaseSuffix -beta &&
+ test_config versionsort.prereleaseSuffix -rc &&
git tag -l --sort=-version:refname "foo*" >actual &&
cat >expect <<-\EOF &&
foo1.10
foo1.6
- foo1.6-beta2
- foo1.6-beta1
+ foo1.6-rc2
+ foo1.6-rc1
foo1.3
EOF
test_cmp expect actual
'
test_expect_success 'version sort with prerelease reordering and common leading character' '
- test_config versionsort.prereleaseSuffix -beta &&
- git tag foo1.6-after1 &&
- git tag -l --sort=version:refname "foo*" >actual &&
+ test_config versionsort.prereleaseSuffix -before &&
+ git tag foo1.7-before1 &&
+ git tag foo1.7 &&
+ git tag foo1.7-after1 &&
+ git tag -l --sort=version:refname "foo1.7*" >actual &&
cat >expect <<-\EOF &&
- foo1.3
- foo1.6-beta1
- foo1.6-beta2
- foo1.6
- foo1.6-after1
- foo1.10
+ foo1.7-before1
+ foo1.7
+ foo1.7-after1
EOF
test_cmp expect actual
'
-# Capitalization of suffixes is important here, because "-RC" would normally
-# be sorted before "-beta" and the config settings should override that.
test_expect_success 'version sort with prerelease reordering, multiple suffixes and common leading character' '
- test_config versionsort.prereleaseSuffix -beta &&
- git config --add versionsort.prereleaseSuffix -RC &&
- git tag foo1.6-RC1 &&
- git tag foo1.6-RC2 &&
- git tag -l --sort=version:refname "foo*" >actual &&
+ test_config versionsort.prereleaseSuffix -before &&
+ git config --add versionsort.prereleaseSuffix -after &&
+ git tag -l --sort=version:refname "foo1.7*" >actual &&
cat >expect <<-\EOF &&
- foo1.3
- foo1.6-beta1
- foo1.6-beta2
- foo1.6-RC1
- foo1.6-RC2
- foo1.6
- foo1.6-after1
- foo1.10
+ foo1.7-before1
+ foo1.7-after1
+ foo1.7
EOF
test_cmp expect actual
'
+test_expect_success 'version sort with very long prerelease suffix' '
+ test_config versionsort.prereleaseSuffix -very-looooooooooooooooooooooooong-prerelease-suffix &&
+ git tag -l --sort=version:refname
+'
+
run_with_limited_stack () {
(ulimit -s 128 && "$@")
}
diff --git a/versioncmp.c b/versioncmp.c
index 87b49a622..f86ac562e 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -26,16 +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 or the character at that offset is part of a
- * prerelease suffix, then that string 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, the
- * order is determined by config file.
- *
- * Note that we don't have to deal with the situation when both s1 and
- * s2 contain the same suffix because the common part is already
- * consumed by the caller.
+ * 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()
*/
@@ -53,18 +52,16 @@ static int swap_prereleases(const char *s1,
start = off - suffix_len + 1;
else
start = 0;
- for (j = start; j <= off; j++) {
+ for (j = start; j <= off; j++)
if (i1 == -1 && starts_with(s1 + j, suffix)) {
i1 = i;
break;
}
- }
- for (j = start; j <= off; j++) {
+ for (j = start; j <= off; j++)
if (i2 == -1 && starts_with(s2 + j, suffix)) {
i2 = i;
break;
}
- }
}
if (i1 == -1 && i2 == -1)
return 0;
^ permalink raw reply related
* [PATCHv2 4/7] versioncmp: pass full tagnames to swap_prereleases()
From: SZEDER Gábor @ 2016-12-08 14:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Nguyễn Thái Ngọc Duy, Leho Kraav, git,
SZEDER Gábor
In-Reply-To: <20161208142401.1329-1-szeder.dev@gmail.com>
The swap_prereleases() helper function is responsible for finding
configured prerelease suffixes in a pair of tagnames to be compared,
but this function currently gets to see only the parts of those two
tagnames starting at the first different character. To fix some
issues related to the common part of two tagnames overlapping with
leading part of a prerelease suffix, this helper function must see
both full tagnames.
In preparation for the fix in the following patch, refactor
swap_prereleases() and its caller to pass two full tagnames and an
additional offset indicating the position of the first different
character.
While updating the comment describing that function, remove the
sentence about not dealing with both tagnames having the same suffix.
Currently it doesn't add much value (we know that there is a different
character, so it's obvious that it can't possibly be the same suffix
in both), and at the end of this patch series it won't even be true
anymore.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
versioncmp.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/versioncmp.c b/versioncmp.c
index 80bfd109f..a55c23ad5 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -25,32 +25,28 @@ static const struct string_list *prereleases;
static int initialized;
/*
- * p1 and p2 point to the first different character in two strings. If
- * either p1 or p2 starts with a prerelease suffix, it will be forced
- * to be on top.
+ * 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.
*
- * If both p1 and p2 start with (different) suffix, the order is
- * determined by config file.
- *
- * Note that we don't have to deal with the situation when both p1 and
- * p2 start with the same suffix because the common part is already
- * consumed by the caller.
+ * If both s1 and s2 contain a (different) suffix at that position,
+ * their order is determined by the order of those two suffixes in the
+ * configuration.
*
* Return non-zero if *diff contains the return value for versioncmp()
*/
-static int swap_prereleases(const void *p1_,
- const void *p2_,
+static int swap_prereleases(const char *s1,
+ const char *s2,
+ int off,
int *diff)
{
- const char *p1 = p1_;
- const char *p2 = p2_;
int i, i1 = -1, i2 = -1;
for (i = 0; i < prereleases->nr; i++) {
const char *suffix = prereleases->items[i].string;
- if (i1 == -1 && starts_with(p1, suffix))
+ if (i1 == -1 && starts_with(s1 + off, suffix))
i1 = i;
- if (i2 == -1 && starts_with(p2, suffix))
+ if (i2 == -1 && starts_with(s2 + off, suffix))
i2 = i;
}
if (i1 == -1 && i2 == -1)
@@ -121,7 +117,8 @@ int versioncmp(const char *s1, const char *s2)
initialized = 1;
prereleases = git_config_get_value_multi("versionsort.prereleasesuffix");
}
- if (prereleases && swap_prereleases(p1 - 1, p2 - 1, &diff))
+ if (prereleases && swap_prereleases(s1, s2, (const char *) p1 - s1 - 1,
+ &diff))
return diff;
state = result_type[state * 3 + (((c2 == '0') + (isdigit (c2) != 0)))];
--
2.11.0.78.g5a2d011
^ permalink raw reply related
* Re: Serious bug with Git-2.11.0-64-bit and Git-LFS
From: Lars Schneider @ 2016-12-08 14:18 UTC (permalink / raw)
To: Nick Warr; +Cc: git
In-Reply-To: <CABW+60za0shXucPgg_jGYt4f8QbkLzLmS5GRf8czE67Taqd+zw@mail.gmail.com>
> On 08 Dec 2016, at 15:00, Nick Warr <nick.warr@bossastudios.com> wrote:
>
> That looks pretty much like the error we're dealing with, any reason
> why going back a point version on Git (not git-lfs) would resolve the
> issue however?
Going back to GitLFS 1.4.* would make the error disappear. However, I think
you should fix your repository. Try this:
git lfs clone <YOUR REPO URL>
cd <YOUR REPO>
git rm --cached "workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi Effects/Effects/Debris/Meshes/debris_junk.FBX"
git add --force "workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi Effects/Effects/Debris/Meshes/debris_junk.FBX"
git commit -m "Move files properly to GitLFS"
git push
Afterwards you should be able to use the latest version of Git and GitLFS
without trouble.
Cheers,
Lars
PS: Top posting is not that popular in the Git community ;-)
>
> On 8 December 2016 at 13:57, Lars Schneider <larsxschneider@gmail.com> wrote:
>>
>>> On 08 Dec 2016, at 12:46, Nick Warr <nick.warr@bossastudios.com> wrote:
>>>
>>> Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with
>>> 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS.
>>>
>>> When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the
>>> first time the clone will finish the checkout of the git part, then
>>> when it starts downloading the LFS parts it will reliably finish with
>>> a smudge filter error.
>>>
>>> This leaves the repo in an unstable condition, you can then fetch the
>>> lfs part without issue, but checking out the lfs files or trying a git
>>> reset --hard will continue to spit out the same error. As you can see,
>>> the actual error is not particularly useful.
>>>
>>> fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi
>>> Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs
>>> failed
>>> Unknown command ""
>>>
>>> Possibly it's due to the file extension being all capital letters, we
>>> did manage to change the error by recommitting the file with a
>>> lowercase extension, but it failed on the next file (which also had a
>>> capital letter extension).
>>>
>>> This has happened on multiple fresh windows 10 64 bit installs,
>>> different machines and target directories (to hopefully remove the
>>> possibility of file permissions) where cloning is taking place.
>>>
>>> The solution is to back level to Git 2.10.2 and the error disappears.
>>>
>>> More than willing to provide any further information,
>>
>> Hi Nick,
>>
>> debris_junk.FBX is not stored properly in Git LFS.
>> I explained the problem in detail here:
>> https://github.com/git-lfs/git-lfs/issues/1729
>>
>> You should add the file properly to GitLFS to fix the problem.
>> However, I think this is a regression in GitLFS and I hope it will
>> be fixed in the next version.
>>
>> No change/fix in Git is required.
>>
>> Cheers,
>> Lars
^ permalink raw reply
* [REGRESSION 2.10.2] problematic "empty auth" changes
From: Johannes Schindelin @ 2016-12-08 14:47 UTC (permalink / raw)
To: David Turner; +Cc: git
Hi Dave,
I got a couple of bug reports that claim that 2.10.2 regressed on using
network credentials. That is, users regularly hit Enter twice when being
asked for user name and password while fetching via https://, and cURL
automatically used to fall back to using the login credentials (i.e.
authenticating via the Domain controller).
Turns out those claims are correct: hitting Enter twice (or using URLs
with empty user name/password such as https://:tfs:8080/) work in 2.10.1
and yield "Authentication failed" in 2.10.2.
I tracked this down to 5275c3081c (http: http.emptyauth should allow empty
(not just NULL) usernames, 2016-10-04) which all of a sudden disallowed
empty user names (and now only handles things correctly when
http.emptyAuth is set to true specifically).
This smells like a real bad regression to me, certainly given the time I
had to spend to figure this out (starting from not exactly helpful bug
reports, due to being very specific to their setups being private).
I am *really* tempted to change the default of http.emptyAuth to true, *at
least* for Windows (where it is quite common to use your login credentials
to authenticate to corporate servers).
Before I do anything rash, though: Do you see any downside to that?
Ciao,
Dscho
^ permalink raw reply
* [PATCHv2 6.5/7] squash! versioncmp: use earliest-longest contained suffix to determine sorting order
From: SZEDER Gábor @ 2016-12-08 14:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Nguyễn Thái Ngọc Duy, Leho Kraav, git,
SZEDER Gábor
In-Reply-To: <20161208142401.1329-7-szeder.dev@gmail.com>
As the number of identical steps to be done for both tagnames grows,
extract them into a helper function, with the additional benefit that
the conditionals near the end of swap_prereleases() will use more
meaningful variable names.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
I was not particularly happy with the amount of code duplication this
series added to swap_prereleases() in patches 5 and 6, hence this bit
of refactoring. Which I'm not particularly happy with either,
because, well, look at that diffstat.
I don't have strong feelings either way, but here it is, take it or
leave it.
versioncmp.c | 62 ++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 37 insertions(+), 25 deletions(-)
diff --git a/versioncmp.c b/versioncmp.c
index 62c8d69dc..601ceddef 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -24,6 +24,29 @@
static const struct string_list *prereleases;
static int initialized;
+struct suffix_match {
+ int conf_pos;
+ int start;
+ int len;
+};
+
+static void find_better_matching_suffix(const char *tagname, const char *suffix,
+ int suffix_len, int start, int conf_pos,
+ struct suffix_match *match)
+{
+ /* A better match either starts earlier or starts at the same offset
+ * but is longer. */
+ int end = match->len < suffix_len ? match->start : match->start-1;
+ int i;
+ for (i = start; i <= end; i++)
+ if (starts_with(tagname + i, suffix)) {
+ match->conf_pos = conf_pos;
+ match->start = i;
+ match->len = suffix_len;
+ break;
+ }
+}
+
/*
* 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 containing
@@ -47,46 +70,35 @@ static int swap_prereleases(const char *s1,
int off,
int *diff)
{
- int i, i1 = -1, i2 = -1;
- int start_at1 = off, start_at2 = off, match_len1 = -1, match_len2 = -1;
+ int i;
+ struct suffix_match match1 = { -1, off, -1 };
+ struct suffix_match match2 = { -1, off, -1 };
for (i = 0; i < prereleases->nr; i++) {
const char *suffix = prereleases->items[i].string;
- int j, start, end, suffix_len = strlen(suffix);
+ int start, suffix_len = strlen(suffix);
if (suffix_len < off)
start = off - suffix_len;
else
start = 0;
- end = match_len1 < suffix_len ? start_at1 : start_at1-1;
- for (j = start; j <= end; j++)
- if (starts_with(s1 + j, suffix)) {
- i1 = i;
- start_at1 = j;
- match_len1 = suffix_len;
- break;
- }
- end = match_len2 < suffix_len ? start_at2 : start_at2-1;
- for (j = start; j <= end; j++)
- if (starts_with(s2 + j, suffix)) {
- i2 = i;
- start_at2 = j;
- match_len2 = suffix_len;
- break;
- }
+ find_better_matching_suffix(s1, suffix, suffix_len, start,
+ i, &match1);
+ find_better_matching_suffix(s2, suffix, suffix_len, start,
+ i, &match2);
}
- if (i1 == -1 && i2 == -1)
+ if (match1.conf_pos == -1 && match2.conf_pos == -1)
return 0;
- if (i1 == i2)
+ if (match1.conf_pos == match2.conf_pos)
/* Found the same suffix in both, e.g. "-rc" in "v1.0-rcX"
* and "v1.0-rcY": the caller should decide based on "X"
* and "Y". */
return 0;
- if (i1 >= 0 && i2 >= 0)
- *diff = i1 - i2;
- else if (i1 >= 0)
+ if (match1.conf_pos >= 0 && match2.conf_pos >= 0)
+ *diff = match1.conf_pos - match2.conf_pos;
+ else if (match1.conf_pos >= 0)
*diff = -1;
- else /* if (i2 >= 0) */
+ else /* if (match2.conf_pos >= 0) */
*diff = 1;
return 1;
}
--
2.11.0.78.g5a2d011
^ permalink raw reply related
* Re: [PATCH 4/5] Make sequencer abort safer
From: Johannes Schindelin @ 2016-12-08 15:28 UTC (permalink / raw)
To: Stephan Beyer; +Cc: git, Christian Couder, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20161207215133.13433-4-s-beyer@gmx.net>
Hi,
On Wed, 7 Dec 2016, Stephan Beyer wrote:
> diff --git a/sequencer.c b/sequencer.c
> index 30b10ba14..c9b560ac1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
> static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
> static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
> static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
Is it required by law to have a four-letter infix, or can we have a nicer
variable name (e.g. git_path_current_file)?
Ciao,
Dscho
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox