* Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
From: Junio C Hamano @ 2017-02-27 22:58 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Jeff King, Johannes Schindelin, sunny, Jakub Narębski,
Matthieu Moy
In-Reply-To: <20170227215627.GB17065@hank>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> On 02/27, Junio C Hamano wrote:
>> Thomas Gummerer <t.gummerer@gmail.com> writes:
>>
>> > + test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null || exit 1
>>
>> This silent "exit 1" made me scratch my head, but --error-unmatch
>> would have already given an error message, like
>>
>> error: pathspec 'no such' did not match any file(s) known to git.
>> Did you forget to 'git add'?
>>
>> so that would be OK.
>
> Yeah exactly, the plan was to let --error-unmatch print the error
> message, as it's better at giving a good error message than we could
> easily do here I think.
>
> Maybe this deserves a comment so others won't have the same doubts
> about this line?
Nah, I do not think so. It probably is obvious for those who write
(and read) "--error-unmatch". I was just being slow to realize that
the sole point of that option was to complain ;-)
^ permalink raw reply
* Re: [PATCH 2/2] commit: don't check for space twice when looking for header
From: René Scharfe @ 2017-02-27 22:54 UTC (permalink / raw)
To: Jakub Narębski, Git List; +Cc: Junio C Hamano
In-Reply-To: <1dd0884c-032c-fb04-67f6-8b181fd65eea@gmail.com>
Am 27.02.2017 um 23:27 schrieb Jakub Narębski:
> W dniu 25.02.2017 o 20:27, René Scharfe pisze:
>> Both standard_header_field() and excluded_header_field() check if
>> there's a space after the buffer that's handed to them. We already
>> check in the caller if that space is present. Don't bother calling
>> the functions if it's missing, as they are guaranteed to return 0 in
>> that case, and remove the now redundant checks from them.
>>
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>> commit.c | 18 ++++++++----------
>> 1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/commit.c b/commit.c
>> index 173c6d3818..fab8269731 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
>>
>> static inline int standard_header_field(const char *field, size_t len)
>> {
>> - return ((len == 4 && !memcmp(field, "tree ", 5)) ||
>> - (len == 6 && !memcmp(field, "parent ", 7)) ||
>> - (len == 6 && !memcmp(field, "author ", 7)) ||
>> - (len == 9 && !memcmp(field, "committer ", 10)) ||
>> - (len == 8 && !memcmp(field, "encoding ", 9)));
>> + return ((len == 4 && !memcmp(field, "tree", 4)) ||
>> + (len == 6 && !memcmp(field, "parent", 6)) ||
>> + (len == 6 && !memcmp(field, "author", 6)) ||
>> + (len == 9 && !memcmp(field, "committer", 9)) ||
>> + (len == 8 && !memcmp(field, "encoding", 8)));
>
> I agree (for what it is worth from me) with the rest of changes,
> but I think current code is better self-documenting for this
> function.
Having a function that is given a buffer/length pair and accessing the
byte after it raises questions, though. :)
Nicer than keeping the space would be to use excluded_header_field() for
standard headers as well as a next step, I think -- but that would be a
bit slower.
>
>> }
>>
>> static int excluded_header_field(const char *field, size_t len, const char **exclude)
>> @@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, size_t len, const char **exc
>>
>> while (*exclude) {
>> size_t xlen = strlen(*exclude);
>> - if (len == xlen &&
>> - !memcmp(field, *exclude, xlen) && field[xlen] == ' ')
>> + if (len == xlen && !memcmp(field, *exclude, xlen))
>> return 1;
>> exclude++;
>> }
>> @@ -1357,9 +1356,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(
>> eof = memchr(line, ' ', next - line);
>> if (!eof)
>> eof = next;
>> -
>> - if (standard_header_field(line, eof - line) ||
>> - excluded_header_field(line, eof - line, exclude))
>> + else if (standard_header_field(line, eof - line) ||
>> + excluded_header_field(line, eof - line, exclude))
>> continue;
>>
>> it = xcalloc(1, sizeof(*it));
>>
>
^ permalink raw reply
* Re: [PATCH 1/6] t0006 & t5000: prepare for 64-bit time_t
From: Junio C Hamano @ 2017-02-27 22:55 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <3871d1d9de0315a1a98bce5d8f544a5e18917f25.1488231002.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> This quick fix, however, tests for *long* to be 64-bit or not. What we
> need, though, is a test that says whether *whatever data type we use for
> timestamps* is 64-bit or not.
>
> The same quick fix was used to handle the similar problem where Git's
> source code uses `unsigned long` to represent size, instead of `size_t`.
>
> So let's just add another prerequisite to test specifically whether
> timestamps are represented by a 64-bit data type or not. Later, when we
> will have switched to using `time_t` where appropriate, we can flip that
> prerequisite to test `time_t` instead of `long`.
The changes that move from LONG_IS to TIME_IS in this patch are all
about time (iow, LONG_IS_64BIT prereq is still used in the check on
sizes). The patch looks sensible.
^ permalink raw reply
* Re: [PATCH 2/6] Specify explicitly where we parse timestamps
From: Junio C Hamano @ 2017-02-27 22:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <xmqqwpcbmfai.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> - unsigned long number = strtoul(date, &end, 10);
>> + time_t number = parse_timestamp(date, &end, 10);
>
> This hunk does not belong to this step. Everybody else in this step
obviously I meant "the left half of this hunk" ;-)
> still receives parse_timestamp()'s return value in ulong, not time_t.
> I presume that that will happen in the final step 6/6 (which could
> be a huge patch that exceeds 100k?)
^ permalink raw reply
* Re: [PATCH 2/6] Specify explicitly where we parse timestamps
From: Junio C Hamano @ 2017-02-27 22:37 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <12b60c14dad15e3252e314771b3fe369305bbfc5.1488231002.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Currently, Git's source code represents all timestamps as `unsigned
> long`. In preparation for using `time_t` instead, let's introduce a
> symbol `parse_timestamp` (currently being defined to `strtoul`) where
> appropriate, so that we can later easily switch to use `strtoull()`
> instead.
This definitely is a very good thing to do as a separate step.
> diff --git a/date.c b/date.c
> index a996331f5b3..a8848f6e141 100644
> --- a/date.c
> +++ b/date.c
> ...
> @@ -1066,7 +1066,7 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num,
> time_t now)
> {
> char *end;
> - unsigned long number = strtoul(date, &end, 10);
> + time_t number = parse_timestamp(date, &end, 10);
This hunk does not belong to this step. Everybody else in this step
still receives parse_timestamp()'s return value in ulong, not time_t.
I presume that that will happen in the final step 6/6 (which could
be a huge patch that exceeds 100k?)
^ permalink raw reply
* Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
From: Junio C Hamano @ 2017-02-27 20:32 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Jeff King, Johannes Schindelin, sunny, Jakub Narębski,
Matthieu Moy
In-Reply-To: <20170225213306.2410-5-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> if test -z "$patch_mode"
> then
> - git reset --hard ${GIT_QUIET:+-q}
> + if test $# != 0
> + then
> + git reset ${GIT_QUIET:+-q} -- "$@"
> + git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z --modified "$@")
"ls-files -z" on the command line?
Apparently new tests do not cover the correctness of this codepath.
I wonder if this
git ls-files -z --modified "$@" |
git checkout-index -z --stdin
is what the above "checkout" wanted to do. The "reset" in the
previous step presumably updated the index entries that match
specified pathspec to those of the HEAD, so checking out the paths
that match "$@" from the index would be the same as checking them
out from the HEAD (while updating the index with them).
> + git clean --force ${GIT_QUIET:+-q} -d -- "$@"
> + else
> + git reset --hard ${GIT_QUIET:+-q}
> + fi
Thanks.
^ permalink raw reply
* Re: Reference for quote "creating branch is not the issue, merging is", in context of Subversion/Git
From: Jakub Narębski @ 2017-02-27 22:42 UTC (permalink / raw)
To: Igor Djordjevic, Michael Hüttermann, git
In-Reply-To: <32232f89-a6f8-be30-278a-bdfedae0716a@gmail.com>
W dniu 26.02.2017 o 16:19, Igor Djordjevic pisze:
> Hello Michael,
>
> On 26/02/2017 12:40, Michael Hüttermann wrote:
>> Linus Torvalds made a statement regarding merging/branching and stated
>> (as far as I know) that "creating branch is not the issue, merge is", in
>> context of Subversion/Git.
>> I do not find the origin source for that. Can you please help and point
>> me to a statement or article where Linus elaborated on this?
>
> Could it be that you think of "Tech Talk: Linus Torvalds on Git"[1]
> (held on May 3, 2007)?
>
> To give you some clue, here`s an excerpt from Linus' talk/presentation
> (taken from the transcript[2] containing the whole thing):
>
> "... Subversion for example, talks very loudly about how they do CVS
> right by making branching really cheap. It's probably on their main
> webpage where they probably say branching in subversion is O(1)
> operation, you can do as many cheap branches as you want. Nevermind
> that O(1) is actually with pretty large O I think, but even if it
> takes a millionth of a second to do branching, who cares? It's the
> wrong thing you are measuring. Nobody is interested in branching,
> branches are completely useless unless you merge them, and CVS cannot
> merge anything at all. You can merge things once, but because CVS
> then forgets what you did, you can never ever merge anything again
> without getting horrible horrible conflicts. Merging in subversion is
> a complete disaster. The subversion people kind of acknowledge this
> and they have a plan, and their plan sucks too. It is incredible how
> stupid these people are. They've been looking at the wrong problem
> all the time. Branching is not the issue, merging is..."
>
> This specific branch/merge performance talk starts at 50:20[3], where
> the part quoted above comes at 51:34[4].
Note also that while "creating branch is not the issue, merge is"
remains true, modern Subversion (post 1.5) makes merging easy thanks
to svn:mergeinfo property.
Though it does it in completely different way than Git and other
"graph of commits" VCS-es, because of the "branch is directory"
philosophy, namely that it keeps information about what was merged
in, rather than finding common ancestor(s) and using this information
for resolving merge.
>
> Please note that there`s more context before and after this excerpt
> that puts it all into the meant perspective, so you may really want
> to watch/listen/read the whole thing anyway.
>
> Regards,
> Buga
>
> [1] https://www.youtube.com/watch?v=4XpnKHJAok8
> [2] https://git.wiki.kernel.org/index.php/LinusTalk200705Transcript
> [3] https://youtu.be/4XpnKHJAok8?t=3020
> [4] https://youtu.be/4XpnKHJAok8?t=3094
>
^ permalink raw reply
* Re: [PATCH 05/10] submodule--helper: add is_active command
From: Brandon Williams @ 2017-02-27 18:35 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZKH_e2NLd=A=og452f-1bfFcSoi5=SM5oetu87TT766Q@mail.gmail.com>
On 02/23, Stefan Beller wrote:
> On Thu, Feb 23, 2017 at 3:47 PM, Brandon Williams <bmwill@google.com> wrote:
> > There are a lot of places where an explicit check for
> > submodule."<name>".url is done to see if a submodule exists. In order
> > to more easily facilitate the use of the submodule.active config option
> > to indicate active submodules, add a helper which can be used to query
> > if a submodule is active or not.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > builtin/submodule--helper.c | 11 ++++++++
> > t/t7413-submodule-is-active.sh | 63 ++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 74 insertions(+)
> > create mode 100755 t/t7413-submodule-is-active.sh
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index df0d9c166..dac02604d 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -1128,6 +1128,16 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
> > return 0;
> > }
> >
> > +static int is_active(int argc, const char **argv, const char *prefix)
> > +{
> > + if (argc != 2)
> > + die("submodule--helper is-active takes exactly 1 arguments");
> > +
> > + gitmodules_config();
> > +
> > + return !is_submodule_initialized(argv[1]);
> > +}
> > +
> > #define SUPPORT_SUPER_PREFIX (1<<0)
> >
> > struct cmd_struct {
> > @@ -1147,6 +1157,7 @@ static struct cmd_struct commands[] = {
> > {"init", module_init, 0},
> > {"remote-branch", resolve_remote_submodule_branch, 0},
> > {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> > + {"is-active", is_active, 0},
> > };
> >
> > int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> > diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
> > new file mode 100755
> > index 000000000..683487020
> > --- /dev/null
> > +++ b/t/t7413-submodule-is-active.sh
> > @@ -0,0 +1,63 @@
> > +#!/bin/sh
> > +
> > +test_description='Test submodule--helper is-active
> > +
> > +This test verifies that `git submodue--helper is-active` correclty identifies
> > +submodules which are "active" and interesting to the user.
> > +'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'setup' '
> > + git init sub &&
> > + test_commit -C sub initial &&
> > + git init super &&
> > + test_commit -C super initial &&
> > + git -C super submodule add ../sub sub1 &&
> > + git -C super submodule add ../sub sub2 &&
> > + git -C super commit -a -m "add 2 submodules at sub{1,2}"
> > +'
> > +
> > +test_expect_success 'is-active works with urls' '
> > + git -C super submodule--helper is-active sub1 &&
> > + git -C super submodule--helper is-active sub2 &&
> > +
> > + git -C super config --unset submodule.sub1.URL &&
> > + test_must_fail git -C super submodule--helper is-active sub1 &&
> > + git -C super config submodule.sub1.URL ../sub &&
> > + git -C super submodule--helper is-active sub1
> > +'
> > +
> > +test_expect_success 'is-active works with basic submodule.active config' '
> > + git -C super config --add submodule.active "." &&
> > + git -C super config --unset submodule.sub1.URL &&
> > + git -C super config --unset submodule.sub2.URL &&
>
> I think we'd want to unset only one of them here
>
> > +
> > + git -C super submodule--helper is-active sub1 &&
> > + git -C super submodule--helper is-active sub2 &&
>
> to test 2 different cases of one being active by config setting only and
> the other having both.
Will do.
>
> I could not spot test for having the URL set but the config setting set, not
> including the submodule, e.g.
>
> git -C super config submodule.sub1.URL ../sub &&
> git -C super submodule.active ":(exclude)sub1" &&
>
> which would be expected to not be active, as once the configuration
> is there it takes precedence over any (no)URL setting?
The last test, tests this functionality as the URL settings for both
sub1 and sub2 are in the config. You'll notice in the tests where I
unset the URL config, that they get added back at the end of the test so
that future tests have a clean state to work with.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 2/2] apply: handle assertion failure gracefully
From: Junio C Hamano @ 2017-02-27 22:33 UTC (permalink / raw)
To: René Scharfe; +Cc: Vegard Nossum, git, Christian Couder, Michal Zalewski
In-Reply-To: <f191e3a8-a55b-7030-ebbb-3f46c74fdc94@web.de>
René Scharfe <l.s.r@web.de> writes:
> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>>> diff --git a/apply.c b/apply.c
>>>> index cbf7cc7f2..9219d2737 100644
>>>> --- a/apply.c
>>>> +++ b/apply.c
>>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>>>> if (!old_name)
>>>> return 0;
>>>>
>>>> - assert(patch->is_new <= 0);
>>>
>>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that
>>> line. Its intent was to handle diffs that contain an old name even for
>>> a file that's created. Citing from its commit message: "When we
>>> cannot be sure by parsing the patch that it is not a creation patch,
>>> we shouldn't complain when if there is no such a file." Why not stop
>>> complaining also in case we happen to know for sure that it's a
>>> creation patch? I.e., why not replace the assert() with:
>>>
>>> if (patch->is_new == 1)
>>> goto is_new;
>>>
>>>> previous = previous_patch(state, patch, &status);
>>
>> When the caller does know is_new is true, old_name must be made/left
>> NULL. That is the invariant this assert is checking to catch an
>> error in the calling code.
>
> There are some places in apply.c that set ->is_new to 1, but none of
> them set ->old_name to NULL at the same time.
I thought all of these are flipping ->is_new that used to be -1
(unknown) to (now we know it is new), and sets only new_name without
doing anything to old_name, because they know originally both names
are set to NULL.
> Having to keep these two members in sync sounds iffy anyway. Perhaps
> accessors can help, e.g. a setter which frees old_name when is_new is
> set to 1, or a getter which returns NULL for old_name if is_new is 1.
Definitely, the setter would make it harder to make the mistake.
^ permalink raw reply
* Re: [BUG] branch renamed to 'HEAD'
From: Junio C Hamano @ 2017-02-27 22:28 UTC (permalink / raw)
To: Jeff King; +Cc: Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170227090233.uk7dfruggytgmuw2@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I guess something like the patch below works, but I wonder if there is a
> less-horrible way to accomplish the same thing.
I suspect that a less-horrible would be a lot more intrusive. It
would go like "interpret-branch-name only gives local branch name,
and when it does not show it, the callers that know they do not
necessarily need local branch name would call other at-mark things".
As you pointed out with the @{upstream} that potentially point at a
local branch, it will quickly get more involved, I would think, and
I tend to think that this patch of yours is probably the least evil
one among possible solutions.
Perhaps with s/not_in_refs_heads/not_a_branch_name/ (or swapping
polarity, "is_a_branch_name"), the resulting code may not be too
hard to read?
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] commit: don't check for space twice when looking for header
From: Jakub Narębski @ 2017-02-27 22:27 UTC (permalink / raw)
To: René Scharfe, Git List; +Cc: Junio C Hamano
In-Reply-To: <b1d5c882-38b8-dd2d-2e5f-aafb8dfada81@web.de>
W dniu 25.02.2017 o 20:27, René Scharfe pisze:
> Both standard_header_field() and excluded_header_field() check if
> there's a space after the buffer that's handed to them. We already
> check in the caller if that space is present. Don't bother calling
> the functions if it's missing, as they are guaranteed to return 0 in
> that case, and remove the now redundant checks from them.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> commit.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 173c6d3818..fab8269731 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
>
> static inline int standard_header_field(const char *field, size_t len)
> {
> - return ((len == 4 && !memcmp(field, "tree ", 5)) ||
> - (len == 6 && !memcmp(field, "parent ", 7)) ||
> - (len == 6 && !memcmp(field, "author ", 7)) ||
> - (len == 9 && !memcmp(field, "committer ", 10)) ||
> - (len == 8 && !memcmp(field, "encoding ", 9)));
> + return ((len == 4 && !memcmp(field, "tree", 4)) ||
> + (len == 6 && !memcmp(field, "parent", 6)) ||
> + (len == 6 && !memcmp(field, "author", 6)) ||
> + (len == 9 && !memcmp(field, "committer", 9)) ||
> + (len == 8 && !memcmp(field, "encoding", 8)));
I agree (for what it is worth from me) with the rest of changes,
but I think current code is better self-documenting for this
function.
> }
>
> static int excluded_header_field(const char *field, size_t len, const char **exclude)
> @@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, size_t len, const char **exc
>
> while (*exclude) {
> size_t xlen = strlen(*exclude);
> - if (len == xlen &&
> - !memcmp(field, *exclude, xlen) && field[xlen] == ' ')
> + if (len == xlen && !memcmp(field, *exclude, xlen))
> return 1;
> exclude++;
> }
> @@ -1357,9 +1356,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(
> eof = memchr(line, ' ', next - line);
> if (!eof)
> eof = next;
> -
> - if (standard_header_field(line, eof - line) ||
> - excluded_header_field(line, eof - line, exclude))
> + else if (standard_header_field(line, eof - line) ||
> + excluded_header_field(line, eof - line, exclude))
> continue;
>
> it = xcalloc(1, sizeof(*it));
>
^ permalink raw reply
* [PATCH 1/6] t0006 & t5000: prepare for 64-bit time_t
From: Johannes Schindelin @ 2017-02-27 21:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1488231002.git.johannes.schindelin@gmx.de>
Git's source code refers to timestamps as unsigned longs. On 32-bit
platforms, as well as on Windows, unsigned long is not large enough to
capture dates that are "absurdly far in the future".
It is perfectly valid by the C standard, of course, for the `long` data
type to refer to 32-bit integers. That is why the `time_t` data type
exists: so that it can be 64-bit even if `long` is 32-bit. Git's source
code simply does not use `time_t`, is all.
The earlier quick fix 6b9c38e14cd (t0006: skip "far in the future" test
when unsigned long is not long enough, 2016-07-11) forced the test cases
to be skipped that require a 64-bit (or larger) data type to be used to
represent the time.
This quick fix, however, tests for *long* to be 64-bit or not. What we
need, though, is a test that says whether *whatever data type we use for
timestamps* is 64-bit or not.
The same quick fix was used to handle the similar problem where Git's
source code uses `unsigned long` to represent size, instead of `size_t`.
So let's just add another prerequisite to test specifically whether
timestamps are represented by a 64-bit data type or not. Later, when we
will have switched to using `time_t` where appropriate, we can flip that
prerequisite to test `time_t` instead of `long`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/helper/test-date.c | 5 ++++-
t/t0006-date.sh | 4 ++--
t/t5000-tar-tree.sh | 6 +++---
t/test-lib.sh | 2 ++
4 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index 506054bcd5d..4727bea255c 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -4,7 +4,8 @@ static const char *usage_msg = "\n"
" test-date relative [time_t]...\n"
" test-date show:<format> [time_t]...\n"
" test-date parse [date]...\n"
-" test-date approxidate [date]...\n";
+" test-date approxidate [date]...\n"
+" test-date is64bit\n";
static void show_relative_dates(const char **argv, struct timeval *now)
{
@@ -93,6 +94,8 @@ int cmd_main(int argc, const char **argv)
parse_dates(argv+1, &now);
else if (!strcmp(*argv, "approxidate"))
parse_approxidate(argv+1, &now);
+ else if (!strcmp(*argv, "is64bit"))
+ return sizeof(unsigned long) == 8 ? 0 : 1;
else
usage(usage_msg);
return 0;
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index c0c910867d7..9539b425ffb 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -53,8 +53,8 @@ check_show unix-local "$TIME" '1466000000'
# arbitrary time absurdly far in the future
FUTURE="5758122296 -0400"
-check_show iso "$FUTURE" "2152-06-19 18:24:56 -0400" LONG_IS_64BIT
-check_show iso-local "$FUTURE" "2152-06-19 22:24:56 +0000" LONG_IS_64BIT
+check_show iso "$FUTURE" "2152-06-19 18:24:56 -0400" TIME_IS_64BIT
+check_show iso-local "$FUTURE" "2152-06-19 22:24:56 +0000" TIME_IS_64BIT
check_parse() {
echo "$1 -> $2" >expect
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 886b6953e40..997aa9dea28 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -390,7 +390,7 @@ test_expect_success TAR_HUGE,LONG_IS_64BIT 'system tar can read our huge size' '
test_cmp expect actual
'
-test_expect_success LONG_IS_64BIT 'set up repository with far-future commit' '
+test_expect_success TIME_IS_64BIT 'set up repository with far-future commit' '
rm -f .git/index &&
echo content >file &&
git add file &&
@@ -398,11 +398,11 @@ test_expect_success LONG_IS_64BIT 'set up repository with far-future commit' '
git commit -m "tempori parendum"
'
-test_expect_success LONG_IS_64BIT 'generate tar with future mtime' '
+test_expect_success TIME_IS_64BIT 'generate tar with future mtime' '
git archive HEAD >future.tar
'
-test_expect_success TAR_HUGE,LONG_IS_64BIT 'system tar can read our future mtime' '
+test_expect_success TAR_HUGE,TIME_IS_64BIT 'system tar can read our future mtime' '
echo 4147 >expect &&
tar_info future.tar | cut -d" " -f2 >actual &&
test_cmp expect actual
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 86d77c16dd3..6151a3d70f8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1163,3 +1163,5 @@ build_option () {
test_lazy_prereq LONG_IS_64BIT '
test 8 -le "$(build_option sizeof-long)"
'
+
+test_lazy_prereq TIME_IS_64BIT 'test-date is64bit'
--
2.11.1.windows.1.379.g44ae0bc
^ permalink raw reply related
* Re: [PATCH v2] convert: add "status=delayed" to filter process protocol
From: Jakub Narębski @ 2017-02-27 22:11 UTC (permalink / raw)
To: Lars Schneider, Jeff King
Cc: Git List, Junio C Hamano, Torsten Bögershausen, Eric Wong,
Taylor Blau
In-Reply-To: <E4A6D866-D046-4CF3-8050-666FD6C98F7B@gmail.com>
W dniu 27.02.2017 o 11:32, Lars Schneider pisze:
>
>> On 27 Feb 2017, at 10:58, Jeff King <peff@peff.net> wrote:
>>
>> On Sun, Feb 26, 2017 at 07:48:16PM +0100, Lars Schneider wrote:
>>
>>> +If the request cannot be fulfilled within a reasonable amount of time
>>> +then the filter can respond with a "delayed" status and a flush packet.
>>> +Git will perform the same request at a later point in time, again. The
>>> +filter can delay a response multiple times for a single request.
>>> +------------------------
>>> +packet: git< status=delayed
>>> +packet: git< 0000
>>> +------------------------
Is it something that happens instead of filter process sending the contents
of file, or is it something that happens after sending some part of the
contents (maybe empty) instead of empty list to keep "status=success"
unchanged or instead of "status=error" if there was a problem processing
file?
>>> +
>>
>> So Git just asks for the same content again? I see two issues with that:
>>
>> 1. Does git have to feed the blob content again? That can be expensive
>> to access or to keep around in memory.
>>
>> 2. What happens when the item isn't ready on the second request? I can
>> think of a few options:
>>
>> a. The filter immediately says "nope, still delayed". But then
>> Git ends up busy-looping with "is this one ready yet?"
>>
>> b. The filter blocks until the item is ready. But then if other
>> items _are_ ready, Git cannot work on processing them. We lose
>> parallelism.
>>
>> c. You could do a hybrid: block until _some_ item is ready, and
>> then issue "delayed" responses for everything that isn't
>> ready. Then if you assume that Git is looping over and over
>> through the set of objects, it will either block or pick up
>> _something_ on each loop.
>>
>> But it makes a quadratic number of requests in the worst case.
>> E.g., imagine you have N items and the last one is available
>> first, then the second-to-last, and so on. You'll ask N times,
>> then N-1, then N-2, and so on.
The current solution is a 'busy loop' one that I wrote about[1][2],
see below.
>
> I completely agree - I need to change that. However, the goal of the v2
> iteration was to get the "convert" interface in an acceptable state.
> That's what I intended to say in the patch comment section:
>
> "Please ignore all changes behind async_convert_to_working_tree() and
> async_filter_finish() for now as I plan to change the implementation
> as soon as the interface is in an acceptable state."
I think that it is more important to start with a good abstraction,
and the proposal for protocol, rather than getting bogged down in
implementation details that may change as the idea for protocol
extension changes.
>>
>> I think it would be much more efficient to do something like:
>>
>> [Git issues a request and gives it an opaque index id]
>> git> command=smudge
>> git> pathname=foo
>> git> index=0
>> git> 0000
>> git> CONTENT
>> git> 0000
>>
>> [The data isn't ready yet, so the filter tells us so...]
>> git< status=delayed
>> git< 0000
So is it only as replacement for "status=success" + contents or
"status=abort", that is upfront before sending any part of the file?
Or, as one can assume from the point of the paragraph with the
"status=delayed", it is about replacing null list for success or
"status=error" after sending some part (maybe empty) of a file,
that is:
[filter driver says that it can process contents]
git< status=success
git< 0000
git< PARTIAL_SMUDGED_CONTENT (maybe empty)
[there was some delay, for example one of shards is slow]
git< 0000
git< status=delayed
git< 0000
>>
>> [Git may make other requests, that are either served or delayed]
>> git> command=smudge
>> git> pathname=foo
>> git> index=1
>> git> 0000
>> git< status=success
>> git< 0000
>> git< CONTENT
>> git< 0000
>>
>> [Now Git has processed all of the items, and each one either has its
>> final status, or has been marked as delayed. So we ask for a delayed
>> item]
>> git> command=wait
>> git> 0000
In my proposal[2] I have called this "command=continue"... but at this
point it is bikeshedding. I think "command=wait" (or "await" ;-))
might be better.
>>
>> [Some time may pass if nothing is ready. But eventually we get...]
>> git< status=success
Or
git< status=resumed
If it would not be undue burden on the filter driver process, we might
require for it to say where to continue at (in bytes), e.g.
git< from=16426
That should, of course, go below index/pathname line.
>> git< index=0
Or a filter driver could have used pathname as an index, that is
git< pathname=path/testfile.dat
>> git< 0000
>> git< CONTENT
>> git< 0000
>>
>> From Git's side, the loop is something like:
>>
>> while (delayed_items > 0) {
>> /* issue a wait, and get back the status/index pair */
>> status = send_wait(&index);
>> delayed_items--;
This looks like my 'event loop' proposal[1][2], see below.
>>
>> /*
>> * use "index" to find the right item in our list of files;
>> * the format can be opaque to the filter, so we could index
>> * it however we like. But probably numeric indices in an array
>> * are the simplest.
>> */
>> assert(index > 0 && index < nr_items);
>> item[index].status = status;
>> if (status == SUCCESS)
>> read_content(&item[index]);
>> }
>>
>> and the filter side just attaches the "index" string to whatever its
>> internal queue structure is, and feeds it back verbatim when processing
>> that item finishes.
I have wrote about this for v1 version of this patch series:
[1]: https://public-inbox.org/git/ec8078ef-8ff2-d26f-ef73-5ef612737eee@gmail.com/
[2]: https://public-inbox.org/git/17fa31a5-8689-2766-952b-704f433a5b3a@gmail.com/
>
> That could work! I had something like that in mind:
>
> I teach Git a new command "list_completed" or similar. The filter
> blocks this call until at least one item is ready for Git.
> Then the filter responds with a list of paths that identify the
> "ready items". Then Git asks for these ready items just with the
> path and not with any content. Could that work? Wouldn't the path
> be "unique" to identify a blob per filter run?
Why in the "drain" phase it is still Git that needs to ask filter for
contents, one file after another? Wouldn't it be easier and simpler
for filter to finish sending contents, and send signal that it has
finished continue'ing?
To summarize my earlier emails, current proposal looks for me as if
it were a "busy loop" solution, that is[2]:
while there are delayed paths:
for each delayed path:
request path from filter [a]
fetch the thing (supporting "delayed") [b]
if path done
do the caller's thing to use buf
remove it from delayed paths list
Footnotes:
----------
a) We don't send the Git-side contents of blob again, isn't it?
So we need some protocol extension / new understanding anyway.
for example that we don't send contents if we request path again.
b) If path is not ready at all, filter protocol would send status=delayed
with empty contents. This means that we would immediately go to the
next path, if there is one.
There are some cases where busy loop is preferable, but I don't think
it is the case here.
The "event loop" solution, which is I think what Peff proposed, would
be something like this (from the protocol point of view, rather than
from the Git code point of view)[1]:
while there are delayed paths:
get path that is ready from filter
fetch the thing to buf (supporting "delayed")
if path done
do the caller's thing to use buf
(e.g. finish checkout path, eof convert, etc.)
We can either trust filter process to tell us when it finished sending
delayed paths, or keep list of paths that are being delayed in Git.
Also, one thing that we need to be solved, assuming that the proposed
extension allows to send partial data from filter to be delayed and
continued later, is that Git needs to keep this partial response in buf;
this is because of precedence of gitattributes applying:
Interaction between checkin/checkout attributes
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
In the check-in codepath, the worktree file is first converted with
`filter` driver (if specified and corresponding driver defined), then
the result is processed with `ident` (if specified), and then finally
with `text` (again, if specified and applicable).
In the check-out codepath, the blob content is first converted with
`text`, and then `ident` and fed to `filter`.
Or maybe I misunderstood something; I am a bit confused by check-in
vs check-out there...
Note that with support of "command=wait" / "command=continue" Git
could interrupt sending contents, and drain unfinished files, if
it needs it... then continue sending rest of files. Well, if the
protocol extension is interpreted to allow for this.
Best regards,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 1/2] apply: guard against renames of non-existant empty files
From: René Scharfe @ 2017-02-27 22:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Vegard Nossum, git, Christian Couder, Michal Zalewski
In-Reply-To: <xmqqinnvwg2d.fsf@gitster.mtv.corp.google.com>
Am 27.02.2017 um 21:10 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Would it make sense to mirror the previously existing condition and
>> check for is_new instead? I.e.:
>>
>> if ((!patch->is_delete && !patch->new_name) ||
>> (!patch->is_new && !patch->old_name)) {
>>
>
> Yes, probably.
>
>> or
>>
>> if (!(patch->is_delete || patch->new_name) ||
>> !(patch->is_new || patch->old_name)) {
>
> This happens after calling parse_git_header() so we should know the
> actual value of is_delete and is_new by now (instead of mistaking
> -1 aka "unknown" as true), so this rewrite would also be OK.
The two variants are logically equivalent -- (!a && !b) == !(a || b). I
wonder if the second one may be harder to read, though.
René
^ permalink raw reply
* Re: [PATCH 2/2] apply: handle assertion failure gracefully
From: René Scharfe @ 2017-02-27 22:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Vegard Nossum, git, Christian Couder, Michal Zalewski
In-Reply-To: <xmqqmvd7wgc7.fsf@gitster.mtv.corp.google.com>
Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>>> diff --git a/apply.c b/apply.c
>>> index cbf7cc7f2..9219d2737 100644
>>> --- a/apply.c
>>> +++ b/apply.c
>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>>> if (!old_name)
>>> return 0;
>>>
>>> - assert(patch->is_new <= 0);
>>
>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that
>> line. Its intent was to handle diffs that contain an old name even for
>> a file that's created. Citing from its commit message: "When we
>> cannot be sure by parsing the patch that it is not a creation patch,
>> we shouldn't complain when if there is no such a file." Why not stop
>> complaining also in case we happen to know for sure that it's a
>> creation patch? I.e., why not replace the assert() with:
>>
>> if (patch->is_new == 1)
>> goto is_new;
>>
>>> previous = previous_patch(state, patch, &status);
>
> When the caller does know is_new is true, old_name must be made/left
> NULL. That is the invariant this assert is checking to catch an
> error in the calling code.
There are some places in apply.c that set ->is_new to 1, but none of
them set ->old_name to NULL at the same time.
Having to keep these two members in sync sounds iffy anyway. Perhaps
accessors can help, e.g. a setter which frees old_name when is_new is
set to 1, or a getter which returns NULL for old_name if is_new is 1.
René
^ permalink raw reply
* Re: [PATCH] t6300: avoid creating refs/heads/HEAD
From: Junio C Hamano @ 2017-02-27 22:18 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170227214147.5ezxskhihi3cc77m@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The "other" stuff could sometimes be useful, I guess. It's not _always_
> wrong to do:
>
> git branch -f @{upstream} foo
>
> It depends on what your @{upstream} resolves to. Switching to just using
> interpret_nth_prior_checkout() would break the case when it resolves to
> a local branch. I'm not sure if we're OK with that or not. If we want to
> keep all the existing cases working, I think we need something like the
> "not_in_refs_heads" patch I posted elsewhere.
I haven't seen that patch, but yes, telling the caller if the
returned value is meant to be used inside refs/heads/ is the right
approach and makes it possible for "@{upstream}" (and just "@") to
be handled sensibly in "git branch -m @{that at-mark thing}".
^ permalink raw reply
* Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
From: Thomas Gummerer @ 2017-02-27 21:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Johannes Schindelin, sunny, Jakub Narębski,
Matthieu Moy
In-Reply-To: <xmqq60jvwewc.fsf@gitster.mtv.corp.google.com>
On 02/27, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > + test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null || exit 1
>
> This silent "exit 1" made me scratch my head, but --error-unmatch
> would have already given an error message, like
>
> error: pathspec 'no such' did not match any file(s) known to git.
> Did you forget to 'git add'?
>
> so that would be OK.
Yeah exactly, the plan was to let --error-unmatch print the error
message, as it's better at giving a good error message than we could
easily do here I think.
Maybe this deserves a comment so others won't have the same doubts
about this line?
^ permalink raw reply
* Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
From: Thomas Gummerer @ 2017-02-27 21:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Johannes Schindelin, sunny, Jakub Narębski,
Matthieu Moy
In-Reply-To: <xmqqwpcbuyrd.fsf@gitster.mtv.corp.google.com>
On 02/27, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > if test -z "$patch_mode"
> > then
> > - git reset --hard ${GIT_QUIET:+-q}
> > + if test $# != 0
> > + then
> > + git reset ${GIT_QUIET:+-q} -- "$@"
> > + git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z --modified "$@")
>
> "ls-files -z" on the command line?
>
> Apparently new tests do not cover the correctness of this codepath.
>
> I wonder if this
>
> git ls-files -z --modified "$@" |
> git checkout-index -z --force --stdin
>
> is what the above "checkout" wanted to do. The "reset" in the
> previous step presumably updated the index entries that match
> specified pathspec to those of the HEAD, so checking out the paths
> that match "$@" from the index would be the same as checking them
> out from the HEAD (while updating the index with them).
Yes, you're completely right, this is exactly what it should have
done. Sorry for being slow on getting this right.
> Perhaps squash the following into an appropriate patch in the
> series?
Thanks, I'll squash this in and re-roll.
> git-stash.sh | 3 ++-
> t/t3903-stash.sh | 16 ++++++++++++++++
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 28d0624c75..9c70662cc8 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -300,7 +300,8 @@ push_stash () {
> if test $# != 0
> then
> git reset ${GIT_QUIET:+-q} -- "$@"
> - git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z --modified "$@")
> + git ls-files -z --modified -- "$@" |
> + git checkout-index -z --force --stdin
> git clean --force ${GIT_QUIET:+-q} -d -- "$@"
> else
> git reset --hard ${GIT_QUIET:+-q}
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index f7733b4dd4..e868aafab2 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -891,4 +891,20 @@ test_expect_success 'stash without verb with pathspec' '
> test_path_is_file bar
> '
>
> +test_expect_success 'stash with pathspec matching multiple paths' '
> + echo original >file &&
> + echo original >other-file &&
> + git commit -m "two" file other-file &&
> + echo modified >file &&
> + echo modified >other-file &&
> + git stash -- "*file" &&
> + echo original >expect &&
> + test_cmp expect file &&
> + test_cmp expect other-file &&
> + git stash pop &&
> + echo modified >expect &&
> + test_cmp expect file &&
> + test_cmp expect other-file
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH] t6300: avoid creating refs/heads/HEAD
From: Junio C Hamano @ 2017-02-27 21:44 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <xmqqshmzuyam.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> ... I suspect that calling interpret_empty_at() from
> that function is fundamentally flawed. The "@" end user types never
> means refs/heads/HEAD, and HEAD@{either reflog or -1} would not mean
> anything that should be taken as a branch_name, either.
The latter should read "HEAD@{either reflog or -1 or 'upstream'}"
Or do we make HEAD@{upstream} to mean "deref HEAD to learn the
current branch name and then take its upstream"? If so @@{upstream}
might logically make sense, but I do not see why @{upstream} without
HEAD or @ is not sufficient to begin with, so...
^ permalink raw reply
* [PATCH 2/6] Specify explicitly where we parse timestamps
From: Johannes Schindelin @ 2017-02-27 21:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1488231002.git.johannes.schindelin@gmx.de>
Currently, Git's source code represents all timestamps as `unsigned
long`. In preparation for using `time_t` instead, let's introduce a
symbol `parse_timestamp` (currently being defined to `strtoul`) where
appropriate, so that we can later easily switch to use `strtoull()`
instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/am.c | 2 +-
bundle.c | 2 +-
commit.c | 4 ++--
date.c | 6 +++---
fsck.c | 2 +-
git-compat-util.h | 2 ++
pretty.c | 2 +-
ref-filter.c | 2 +-
t/helper/test-date.c | 2 +-
tag.c | 2 +-
upload-pack.c | 2 +-
11 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 31fb60578f6..75e2d939036 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -882,7 +882,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)
char *end;
errno = 0;
- timestamp = strtoul(str, &end, 10);
+ timestamp = parse_timestamp(str, &end, 10);
if (errno)
return error(_("invalid timestamp"));
diff --git a/bundle.c b/bundle.c
index bbf4efa0a0a..f43bfcf5ff3 100644
--- a/bundle.c
+++ b/bundle.c
@@ -227,7 +227,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
line = memchr(line, '>', lineend ? lineend - line : buf + size - line);
if (!line++)
goto out;
- date = strtoul(line, NULL, 10);
+ date = parse_timestamp(line, NULL, 10);
result = (revs->max_age == -1 || revs->max_age < date) &&
(revs->min_age == -1 || revs->min_age > date);
out:
diff --git a/commit.c b/commit.c
index 2cf85158b48..7f56b643704 100644
--- a/commit.c
+++ b/commit.c
@@ -90,7 +90,7 @@ static unsigned long parse_commit_date(const char *buf, const char *tail)
if (buf >= tail)
return 0;
/* dateptr < buf && buf[-1] == '\n', so strtoul will stop at buf-1 */
- return strtoul(dateptr, NULL, 10);
+ return parse_timestamp(dateptr, NULL, 10);
}
static struct commit_graft **commit_graft;
@@ -608,7 +608,7 @@ static void record_author_date(struct author_date_slab *author_date,
!ident.date_begin || !ident.date_end)
goto fail_exit; /* malformed "author" line */
- date = strtoul(ident.date_begin, &date_end, 10);
+ date = parse_timestamp(ident.date_begin, &date_end, 10);
if (date_end != ident.date_end)
goto fail_exit; /* malformed date */
*(author_date_slab_at(author_date, commit)) = date;
diff --git a/date.c b/date.c
index a996331f5b3..a8848f6e141 100644
--- a/date.c
+++ b/date.c
@@ -510,7 +510,7 @@ static int match_digit(const char *date, struct tm *tm, int *offset, int *tm_gmt
char *end;
unsigned long num;
- num = strtoul(date, &end, 10);
+ num = parse_timestamp(date, &end, 10);
/*
* Seconds since 1970? We trigger on that for any numbers with
@@ -658,7 +658,7 @@ static int match_object_header_date(const char *date, unsigned long *timestamp,
if (*date < '0' || '9' < *date)
return -1;
- stamp = strtoul(date, &end, 10);
+ stamp = parse_timestamp(date, &end, 10);
if (*end != ' ' || stamp == ULONG_MAX || (end[1] != '+' && end[1] != '-'))
return -1;
date = end + 2;
@@ -1066,7 +1066,7 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num,
time_t now)
{
char *end;
- unsigned long number = strtoul(date, &end, 10);
+ time_t number = parse_timestamp(date, &end, 10);
switch (*end) {
case ':':
diff --git a/fsck.c b/fsck.c
index 939792752bf..33a66e68a83 100644
--- a/fsck.c
+++ b/fsck.c
@@ -690,7 +690,7 @@ static int fsck_ident(const char **ident, struct object *obj, struct fsck_option
p++;
if (*p == '0' && p[1] != ' ')
return report(options, obj, FSCK_MSG_ZERO_PADDED_DATE, "invalid author/committer line - zero-padded date");
- if (date_overflows(strtoul(p, &end, 10)))
+ if (date_overflows(parse_timestamp(p, &end, 10)))
return report(options, obj, FSCK_MSG_BAD_DATE_OVERFLOW, "invalid author/committer line - date causes integer overflow");
if ((end == p || *end != ' '))
return report(options, obj, FSCK_MSG_BAD_DATE, "invalid author/committer line - bad date");
diff --git a/git-compat-util.h b/git-compat-util.h
index ef6d560e156..5eff97bea2e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -319,6 +319,8 @@ extern char *gitdirname(char *);
#define PRIo32 "o"
#endif
+#define parse_timestamp strtoul
+
#ifndef PATH_SEP
#define PATH_SEP ':'
#endif
diff --git a/pretty.c b/pretty.c
index 5e683830d9d..6d1e1e87e7d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -409,7 +409,7 @@ const char *show_ident_date(const struct ident_split *ident,
long tz = 0;
if (ident->date_begin && ident->date_end)
- date = strtoul(ident->date_begin, NULL, 10);
+ date = parse_timestamp(ident->date_begin, NULL, 10);
if (date_overflows(date))
date = 0;
else {
diff --git a/ref-filter.c b/ref-filter.c
index 3820b21cc75..6fab0db5e0d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -637,7 +637,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
if (!eoemail)
goto bad;
- timestamp = strtoul(eoemail + 2, &zone, 10);
+ timestamp = parse_timestamp(eoemail + 2, &zone, 10);
if (timestamp == ULONG_MAX)
goto bad;
tz = strtol(zone, NULL, 10);
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index 4727bea255c..98637053760 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -33,7 +33,7 @@ static void show_dates(const char **argv, const char *format)
* Do not use our normal timestamp parsing here, as the point
* is to test the formatting code in isolation.
*/
- t = strtol(*argv, &arg, 10);
+ t = parse_timestamp(*argv, &arg, 10);
while (*arg == ' ')
arg++;
tz = atoi(arg);
diff --git a/tag.c b/tag.c
index 243d1fdbbcb..55d07725777 100644
--- a/tag.c
+++ b/tag.c
@@ -111,7 +111,7 @@ static unsigned long parse_tag_date(const char *buf, const char *tail)
if (buf >= tail)
return 0;
/* dateptr < buf && buf[-1] == '\n', so strtoul will stop at buf-1 */
- return strtoul(dateptr, NULL, 10);
+ return parse_timestamp(dateptr, NULL, 10);
}
int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)
diff --git a/upload-pack.c b/upload-pack.c
index 7597ba3405e..8c47dc1707a 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -775,7 +775,7 @@ static void receive_needs(void)
}
if (skip_prefix(line, "deepen-since ", &arg)) {
char *end = NULL;
- deepen_since = strtoul(arg, &end, 0);
+ deepen_since = parse_timestamp(arg, &end, 0);
if (!end || *end || !deepen_since ||
/* revisions.c's max_age -1 is special */
deepen_since == -1)
--
2.11.1.windows.1.379.g44ae0bc
^ permalink raw reply related
* [PATCH 0/6] Use time_t
From: Johannes Schindelin @ 2017-02-27 21:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
Git v2.9.2 was released in a hurry to accomodate for platforms like
Windows, where the `unsigned long` data type is 32-bit even for 64-bit
setups.
The quick fix was to simply disable all the testing with "absurd" future
dates.
However, we can do much better than that, as `time_t` exists, and at
least on 64-bit Windows it is 64-bit. Meaning: we *can* support these
absurd future dates on those platforms.
So let's do this.
One notable fallout of this patch series is that on 64-bit Linux (and
other platforms where `unsigned long` is 64-bit), we now limit the range
of dates to LONG_MAX (i.e. the *signed* maximum value). This needs to be
done as `time_t` can be signed (and indeed is at least on my Ubuntu
setup).
Obviously, I think that we can live with that, and I hope that all
interested parties agree.
Johannes Schindelin (6):
t0006 & t5000: prepare for 64-bit time_t
Specify explicitly where we parse timestamps
Introduce a new "printf format" for timestamps
Prepare for timestamps to use 64-bit signed types
ref-filter: avoid using `unsigned long` for catch-all data type
Use time_t where appropriate
Documentation/technical/api-parse-options.txt | 8 +--
Makefile | 4 ++
archive-tar.c | 5 +-
builtin/am.c | 4 +-
builtin/blame.c | 14 ++---
builtin/fsck.c | 6 +-
builtin/gc.c | 2 +-
builtin/log.c | 4 +-
builtin/merge-base.c | 2 +-
builtin/name-rev.c | 6 +-
builtin/pack-objects.c | 4 +-
builtin/prune.c | 4 +-
builtin/receive-pack.c | 10 +--
builtin/reflog.c | 24 +++----
builtin/show-branch.c | 4 +-
builtin/worktree.c | 4 +-
bundle.c | 4 +-
cache.h | 14 ++---
commit.c | 16 ++---
commit.h | 2 +-
config.mak.uname | 2 +
credential-cache--daemon.c | 12 ++--
date.c | 90 +++++++++++++--------------
fetch-pack.c | 8 +--
fsck.c | 2 +-
git-compat-util.h | 10 +++
http-backend.c | 4 +-
parse-options-cb.c | 4 +-
pretty.c | 4 +-
reachable.c | 10 ++-
reachable.h | 4 +-
ref-filter.c | 22 +++----
reflog-walk.c | 8 +--
refs.c | 14 ++---
refs.h | 8 +--
refs/files-backend.c | 4 +-
revision.c | 6 +-
revision.h | 4 +-
sha1_name.c | 6 +-
t/helper/test-date.c | 11 ++--
t/helper/test-parse-options.c | 4 +-
t/t0006-date.sh | 4 +-
t/t5000-tar-tree.sh | 6 +-
t/test-lib.sh | 2 +
tag.c | 4 +-
tag.h | 2 +-
upload-pack.c | 8 +--
vcs-svn/fast_export.c | 8 +--
vcs-svn/fast_export.h | 4 +-
vcs-svn/svndump.c | 2 +-
wt-status.c | 2 +-
51 files changed, 221 insertions(+), 199 deletions(-)
base-commit: e7e07d5a4fcc2a203d9873968ad3e6bd4d7419d7
Published-As: https://github.com/dscho/git/releases/tag/time_t-may-be-int64-v1
Fetch-It-Via: git fetch https://github.com/dscho/git time_t-may-be-int64-v1
--
2.11.1.windows.1.379.g44ae0bc
^ permalink raw reply
* Re: [PATCH] t6300: avoid creating refs/heads/HEAD
From: Jeff King @ 2017-02-27 21:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqshmzuyam.fsf@gitster.mtv.corp.google.com>
On Mon, Feb 27, 2017 at 01:19:29PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I suspect there are a lot of other places that are less clear cut. E.g.,
> > I think just:
> >
> > git branch foo bar
> >
> > will put "foo" through the same interpretation. So you could do:
> >
> > git branch -f @{-1} bar
> >
> > Is that insane? Maybe. But it does work now.
>
> No, it _is_ very sensible, so is "git checkout -B @{-1} <someplace>"
>
> Perhaps interpret-branch-name that does not error out when given "@"
> is what is broken? I suspect that calling interpret_empty_at() from
> that function is fundamentally flawed. The "@" end user types never
> means refs/heads/HEAD, and HEAD@{either reflog or -1} would not mean
> anything that should be taken as a branch_name, either.
>
> So perhaps what interpret_empty_at() does is necessary for the "four
> capital letters is too many to type, so just type one key while
> holding a shift", but it should be called from somewhere else, and
> not from interpret_branch_name()?
I think _most_ of interpret_branch_name() is in the same boat. The
"@{upstream}" mark is not likely to give you a branch in refs/heads
either.
So in practice, I think strbuf_check_branch_ref() could probably get by
with just calling interpret_nth_prior_checkout(). Or if you prefer, to
rip everything out of interpret_branch_name() except that. :) But that
other stuff has to go somewhere, and there are some challenges with the
recursion from reinterpret().
The "other" stuff could sometimes be useful, I guess. It's not _always_
wrong to do:
git branch -f @{upstream} foo
It depends on what your @{upstream} resolves to. Switching to just using
interpret_nth_prior_checkout() would break the case when it resolves to
a local branch. I'm not sure if we're OK with that or not. If we want to
keep all the existing cases working, I think we need something like the
"not_in_refs_heads" patch I posted elsewhere.
-Peff
^ permalink raw reply
* Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
From: Junio C Hamano @ 2017-02-27 20:35 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Jeff King, Johannes Schindelin, sunny, Jakub Narębski,
Matthieu Moy
In-Reply-To: <20170225213306.2410-5-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> + test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null || exit 1
This silent "exit 1" made me scratch my head, but --error-unmatch
would have already given an error message, like
error: pathspec 'no such' did not match any file(s) known to git.
Did you forget to 'git add'?
so that would be OK.
^ permalink raw reply
* [PATCH 3/6] Introduce a new "printf format" for timestamps
From: Johannes Schindelin @ 2017-02-27 21:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1488231002.git.johannes.schindelin@gmx.de>
Currently, Git's source code treats all timestamps as if they were
unsigned longs. Therefore, it is okay to write "%lu" when printing them.
There is a substantial problem with that, though: at least on Windows,
time_t is *larger* than unsigned long, and hence we will want to switch
to using time_t instead.
So let's introduce the pseudo format "PRItime" (currently simply being
"lu") so that it is easy later on to change the data type to time_t.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/blame.c | 6 +++---
builtin/fsck.c | 2 +-
builtin/log.c | 2 +-
builtin/receive-pack.c | 4 ++--
date.c | 26 +++++++++++++-------------
fetch-pack.c | 2 +-
git-compat-util.h | 1 +
t/helper/test-date.c | 2 +-
t/helper/test-parse-options.c | 2 +-
upload-pack.c | 2 +-
vcs-svn/fast_export.c | 4 ++--
11 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index cffc6265408..c9486dd580b 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1736,11 +1736,11 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)
get_commit_info(suspect->commit, &ci, 1);
printf("author %s\n", ci.author.buf);
printf("author-mail %s\n", ci.author_mail.buf);
- printf("author-time %lu\n", ci.author_time);
+ printf("author-time %"PRItime"\n", ci.author_time);
printf("author-tz %s\n", ci.author_tz.buf);
printf("committer %s\n", ci.committer.buf);
printf("committer-mail %s\n", ci.committer_mail.buf);
- printf("committer-time %lu\n", ci.committer_time);
+ printf("committer-time %"PRItime"\n", ci.committer_time);
printf("committer-tz %s\n", ci.committer_tz.buf);
printf("summary %s\n", ci.summary.buf);
if (suspect->commit->object.flags & UNINTERESTING)
@@ -1853,7 +1853,7 @@ static const char *format_time(unsigned long time, const char *tz_str,
strbuf_reset(&time_buf);
if (show_raw_time) {
- strbuf_addf(&time_buf, "%lu %s", time, tz_str);
+ strbuf_addf(&time_buf, "%"PRItime" %s", time, tz_str);
}
else {
const char *time_str;
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 1a5caccd0f5..5413c76e7a6 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -407,7 +407,7 @@ static void fsck_handle_reflog_sha1(const char *refname, unsigned char *sha1,
if (timestamp && name_objects)
add_decoration(fsck_walk_options.object_names,
obj,
- xstrfmt("%s@{%ld}", refname, timestamp));
+ xstrfmt("%s@{%"PRItime"}", refname, timestamp));
obj->used = 1;
mark_object_reachable(obj);
} else {
diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc2d88..24612c2299a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -903,7 +903,7 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
static void gen_message_id(struct rev_info *info, char *base)
{
struct strbuf buf = STRBUF_INIT;
- strbuf_addf(&buf, "%s.%lu.git.%s", base,
+ strbuf_addf(&buf, "%s.%"PRItime".git.%s", base,
(unsigned long) time(NULL),
git_committer_info(IDENT_NO_NAME|IDENT_NO_DATE|IDENT_STRICT));
info->message_id = strbuf_detach(&buf, NULL);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 1dbb8a06922..4a878645847 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -456,12 +456,12 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)
struct strbuf buf = STRBUF_INIT;
unsigned char sha1[20];
- strbuf_addf(&buf, "%s:%lu", path, stamp);
+ strbuf_addf(&buf, "%s:%"PRItime, path, stamp);
hmac_sha1(sha1, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));;
strbuf_release(&buf);
/* RFC 2104 5. HMAC-SHA1-80 */
- strbuf_addf(&buf, "%lu-%.*s", stamp, 20, sha1_to_hex(sha1));
+ strbuf_addf(&buf, "%"PRItime"-%.*s", stamp, 20, sha1_to_hex(sha1));
return strbuf_detach(&buf, NULL);
}
diff --git a/date.c b/date.c
index a8848f6e141..97ab5fcc349 100644
--- a/date.c
+++ b/date.c
@@ -100,41 +100,41 @@ void show_date_relative(unsigned long time, int tz,
diff = now->tv_sec - time;
if (diff < 90) {
strbuf_addf(timebuf,
- Q_("%lu second ago", "%lu seconds ago", diff), diff);
+ Q_("%"PRItime" second ago", "%"PRItime" seconds ago", diff), diff);
return;
}
/* Turn it into minutes */
diff = (diff + 30) / 60;
if (diff < 90) {
strbuf_addf(timebuf,
- Q_("%lu minute ago", "%lu minutes ago", diff), diff);
+ Q_("%"PRItime" minute ago", "%"PRItime" minutes ago", diff), diff);
return;
}
/* Turn it into hours */
diff = (diff + 30) / 60;
if (diff < 36) {
strbuf_addf(timebuf,
- Q_("%lu hour ago", "%lu hours ago", diff), diff);
+ Q_("%"PRItime" hour ago", "%"PRItime" hours ago", diff), diff);
return;
}
/* We deal with number of days from here on */
diff = (diff + 12) / 24;
if (diff < 14) {
strbuf_addf(timebuf,
- Q_("%lu day ago", "%lu days ago", diff), diff);
+ Q_("%"PRItime" day ago", "%"PRItime" days ago", diff), diff);
return;
}
/* Say weeks for the past 10 weeks or so */
if (diff < 70) {
strbuf_addf(timebuf,
- Q_("%lu week ago", "%lu weeks ago", (diff + 3) / 7),
+ Q_("%"PRItime" week ago", "%"PRItime" weeks ago", (diff + 3) / 7),
(diff + 3) / 7);
return;
}
/* Say months for the past 12 months or so */
if (diff < 365) {
strbuf_addf(timebuf,
- Q_("%lu month ago", "%lu months ago", (diff + 15) / 30),
+ Q_("%"PRItime" month ago", "%"PRItime" months ago", (diff + 15) / 30),
(diff + 15) / 30);
return;
}
@@ -145,20 +145,20 @@ void show_date_relative(unsigned long time, int tz,
unsigned long months = totalmonths % 12;
if (months) {
struct strbuf sb = STRBUF_INIT;
- strbuf_addf(&sb, Q_("%lu year", "%lu years", years), years);
+ strbuf_addf(&sb, Q_("%"PRItime" year", "%"PRItime" years", years), years);
strbuf_addf(timebuf,
/* TRANSLATORS: "%s" is "<n> years" */
- Q_("%s, %lu month ago", "%s, %lu months ago", months),
+ Q_("%s, %"PRItime" month ago", "%s, %"PRItime" months ago", months),
sb.buf, months);
strbuf_release(&sb);
} else
strbuf_addf(timebuf,
- Q_("%lu year ago", "%lu years ago", years), years);
+ Q_("%"PRItime" year ago", "%"PRItime" years ago", years), years);
return;
}
/* Otherwise, just years. Centuries is probably overkill. */
strbuf_addf(timebuf,
- Q_("%lu year ago", "%lu years ago", (diff + 183) / 365),
+ Q_("%"PRItime" year ago", "%"PRItime" years ago", (diff + 183) / 365),
(diff + 183) / 365);
}
@@ -179,7 +179,7 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
if (mode->type == DATE_UNIX) {
strbuf_reset(&timebuf);
- strbuf_addf(&timebuf, "%lu", time);
+ strbuf_addf(&timebuf, "%"PRItime, time);
return timebuf.buf;
}
@@ -188,7 +188,7 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
if (mode->type == DATE_RAW) {
strbuf_reset(&timebuf);
- strbuf_addf(&timebuf, "%lu %+05d", time, tz);
+ strbuf_addf(&timebuf, "%"PRItime" %+05d", time, tz);
return timebuf.buf;
}
@@ -643,7 +643,7 @@ static void date_string(unsigned long date, int offset, struct strbuf *buf)
offset = -offset;
sign = '-';
}
- strbuf_addf(buf, "%lu %c%02d%02d", date, sign, offset / 60, offset % 60);
+ strbuf_addf(buf, "%"PRItime" %c%02d%02d", date, sign, offset / 60, offset % 60);
}
/*
diff --git a/fetch-pack.c b/fetch-pack.c
index 601f0779a19..54fb35e39c5 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -357,7 +357,7 @@ static int find_common(struct fetch_pack_args *args,
packet_buf_write(&req_buf, "deepen %d", args->depth);
if (args->deepen_since) {
unsigned long max_age = approxidate(args->deepen_since);
- packet_buf_write(&req_buf, "deepen-since %lu", max_age);
+ packet_buf_write(&req_buf, "deepen-since %"PRItime, max_age);
}
if (args->deepen_not) {
int i;
diff --git a/git-compat-util.h b/git-compat-util.h
index 5eff97bea2e..4365012c536 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -319,6 +319,7 @@ extern char *gitdirname(char *);
#define PRIo32 "o"
#endif
+#define PRItime "lu"
#define parse_timestamp strtoul
#ifndef PATH_SEP
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index 98637053760..ba309ec1760 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -52,7 +52,7 @@ static void parse_dates(const char **argv, struct timeval *now)
strbuf_reset(&result);
parse_date(*argv, &result);
- if (sscanf(result.buf, "%lu %d", &t, &tz) == 2)
+ if (sscanf(result.buf, "%"PRItime" %d", &t, &tz) == 2)
printf("%s -> %s\n",
*argv, show_date(t, tz, DATE_MODE(ISO8601)));
else
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index a01430c24bd..7d93627e454 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -161,7 +161,7 @@ int cmd_main(int argc, const char **argv)
show(&expect, &ret, "boolean: %d", boolean);
show(&expect, &ret, "integer: %d", integer);
show(&expect, &ret, "magnitude: %lu", magnitude);
- show(&expect, &ret, "timestamp: %lu", timestamp);
+ show(&expect, &ret, "timestamp: %"PRItime, timestamp);
show(&expect, &ret, "string: %s", string ? string : "(not set)");
show(&expect, &ret, "abbrev: %d", abbrev);
show(&expect, &ret, "verbose: %d", verbose);
diff --git a/upload-pack.c b/upload-pack.c
index 8c47dc1707a..c2be661f6d4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -859,7 +859,7 @@ static void receive_needs(void)
argv_array_push(&av, "rev-list");
if (deepen_since)
- argv_array_pushf(&av, "--max-age=%lu", deepen_since);
+ argv_array_pushf(&av, "--max-age=%"PRItime, deepen_since);
if (deepen_not.nr) {
argv_array_push(&av, "--not");
for (i = 0; i < deepen_not.nr; i++) {
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 97cba39cdf5..6c9f2866d8b 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -73,7 +73,7 @@ void fast_export_begin_note(uint32_t revision, const char *author,
static int firstnote = 1;
size_t loglen = strlen(log);
printf("commit %s\n", note_ref);
- printf("committer %s <%s@%s> %lu +0000\n", author, author, "local", timestamp);
+ printf("committer %s <%s@%s> %"PRItime" +0000\n", author, author, "local", timestamp);
printf("data %"PRIuMAX"\n", (uintmax_t)loglen);
fwrite(log, loglen, 1, stdout);
if (firstnote) {
@@ -107,7 +107,7 @@ void fast_export_begin_commit(uint32_t revision, const char *author,
}
printf("commit %s\n", local_ref);
printf("mark :%"PRIu32"\n", revision);
- printf("committer %s <%s@%s> %lu +0000\n",
+ printf("committer %s <%s@%s> %"PRItime" +0000\n",
*author ? author : "nobody",
*author ? author : "nobody",
*uuid ? uuid : "local", timestamp);
--
2.11.1.windows.1.379.g44ae0bc
^ permalink raw reply related
* [PATCH 4/6] Prepare for timestamps to use 64-bit signed types
From: Johannes Schindelin @ 2017-02-27 21:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1488231002.git.johannes.schindelin@gmx.de>
Currently, Git's source code uses the unsigned long type to represent
timestamps. However, this type is limited to 32-bit e.g. on 64-bit
Windows. Hence it is a suboptimal type for this use case.
In any case, we need to use the time_t type to represent timestamps
since we often send those values to system functions which are declared
to accept time_t parameters.
So let's prepare for the case where timestamps are represented as 64-bit
signed integers by introducing the Makefile option TIME_T_IS_INT64.
As we have to resort to using `strtoull()` (and casting the parsed,
unsigned value to an `int64_t`), the check in the `date_overflows()`
helper has to be relaxed: a value of ULLONG_MAX (cast to `int64_t`)
now *also* indicates an overflow.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Makefile | 4 ++++
archive-tar.c | 5 ++++-
builtin/name-rev.c | 2 +-
builtin/prune.c | 2 +-
builtin/worktree.c | 2 +-
credential-cache--daemon.c | 2 +-
date.c | 8 ++++----
git-compat-util.h | 7 +++++++
ref-filter.c | 2 +-
9 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/Makefile b/Makefile
index 8e4081e0619..0232cf62d33 100644
--- a/Makefile
+++ b/Makefile
@@ -1518,6 +1518,10 @@ ifdef HAVE_GETDELIM
BASIC_CFLAGS += -DHAVE_GETDELIM
endif
+ifdef TIME_T_IS_INT64
+ BASIC_CFLAGS += -DTIME_T_IS_INT64
+endif
+
ifeq ($(TCLTK_PATH),)
NO_TCLTK = NoThanks
endif
diff --git a/archive-tar.c b/archive-tar.c
index 380e3aedd23..695339a2369 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -27,9 +27,12 @@ static int write_tar_filter_archive(const struct archiver *ar,
*/
#if ULONG_MAX == 0xFFFFFFFF
#define USTAR_MAX_SIZE ULONG_MAX
-#define USTAR_MAX_MTIME ULONG_MAX
#else
#define USTAR_MAX_SIZE 077777777777UL
+#endif
+#if TIME_MAX == 0xFFFFFFFF
+#define USTAR_MAX_MTIME TIME_MAX
+#else
#define USTAR_MAX_MTIME 077777777777UL
#endif
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48b65e..a0f16407b93 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -145,7 +145,7 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
struct name_ref_data *data = cb_data;
int can_abbreviate_output = data->tags_only && data->name_only;
int deref = 0;
- unsigned long taggerdate = ULONG_MAX;
+ unsigned long taggerdate = TIME_MAX;
if (data->tags_only && !starts_with(path, "refs/tags/"))
return 0;
diff --git a/builtin/prune.c b/builtin/prune.c
index 8f4f0522856..1e5eb0292b1 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -111,7 +111,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
};
char *s;
- expire = ULONG_MAX;
+ expire = TIME_MAX;
save_commit_buffer = 0;
check_replace_refs = 0;
ref_paranoia = 1;
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 831fe058a53..3df95e112e5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -131,7 +131,7 @@ static int prune(int ac, const char **av, const char *prefix)
OPT_END()
};
- expire = ULONG_MAX;
+ expire = TIME_MAX;
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
if (ac)
usage_with_options(worktree_usage, options);
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 46c5937526a..b298ac01e4f 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -52,7 +52,7 @@ static int check_expirations(void)
static unsigned long wait_for_entry_until;
int i = 0;
unsigned long now = time(NULL);
- unsigned long next = (unsigned long)-1;
+ unsigned long next = TIME_MAX;
/*
* Initially give the client 30 seconds to actually contact us
diff --git a/date.c b/date.c
index 97ab5fcc349..23dee2964c1 100644
--- a/date.c
+++ b/date.c
@@ -659,7 +659,7 @@ static int match_object_header_date(const char *date, unsigned long *timestamp,
if (*date < '0' || '9' < *date)
return -1;
stamp = parse_timestamp(date, &end, 10);
- if (*end != ' ' || stamp == ULONG_MAX || (end[1] != '+' && end[1] != '-'))
+ if (*end != ' ' || stamp == TIME_MAX || (end[1] != '+' && end[1] != '-'))
return -1;
date = end + 2;
ofs = strtol(date, &end, 10);
@@ -762,7 +762,7 @@ int parse_expiry_date(const char *date, unsigned long *timestamp)
* of the past, and there is nothing from the future
* to be kept.
*/
- *timestamp = ULONG_MAX;
+ *timestamp = TIME_MAX;
else
*timestamp = approxidate_careful(date, &errors);
@@ -1184,8 +1184,8 @@ int date_overflows(unsigned long t)
{
time_t sys;
- /* If we overflowed our unsigned long, that's bad... */
- if (t == ULONG_MAX)
+ /* If we overflowed our timestamp data type, that's bad... */
+ if ((uintmax_t)t >= TIME_MAX)
return 1;
/*
diff --git a/git-compat-util.h b/git-compat-util.h
index 4365012c536..5cf1133532d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -319,8 +319,15 @@ extern char *gitdirname(char *);
#define PRIo32 "o"
#endif
+#ifdef TIME_T_IS_INT64
+#define PRItime PRId64
+#define parse_timestamp strtoull
+#define TIME_MAX INT64_MAX
+#else
#define PRItime "lu"
#define parse_timestamp strtoul
+#define TIME_MAX ULONG_MAX
+#endif
#ifndef PATH_SEP
#define PATH_SEP ':'
diff --git a/ref-filter.c b/ref-filter.c
index 6fab0db5e0d..07c1f372351 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -638,7 +638,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
if (!eoemail)
goto bad;
timestamp = parse_timestamp(eoemail + 2, &zone, 10);
- if (timestamp == ULONG_MAX)
+ if (timestamp == TIME_MAX)
goto bad;
tz = strtol(zone, NULL, 10);
if ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)
--
2.11.1.windows.1.379.g44ae0bc
^ permalink raw reply related
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