Re: [RFC PATCH v5 0/8] rebase-interactive

[Jeff Hostetler]

On 3/27/2018 1:07 AM, Junio C Hamano wrote:

Jeff Hostetler  writes:

[...]

So I would think it is most sensible to have double, uintmax_t and
intmax_t variants.  If you do not care about the extra value range
that unsigned integral types afford, a single intmax_t variant would
also be fine.


I'll reroll with just the double and intmax_t variants.
Thanks for the feedback and sorry for all the noise.

Jeff

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

Jeff Hostetler  writes:

> I did the uint64_t for the unsigned ns times.
>
> I did the other one for the usual signed ints.
>
> I could convert them both to a single signed 64 bit typed function
> if we only want to have one function.

I still think having sized version is a horrible idea, and recommend
instrad to use "the widest possible on the platform" type, for the
same reason why you would only have variant for double but not for
float.

intmax and uintmax are by definition wide enough to hold any value
that would fit in any integral type the platform supports, so if a
caller that wants to handle 64-bit unsigned timestamp for example
uses uint64_t variable to pass such a timestamp around, and the
platform is capable of groking that code, you should be able to
safely pass that to json serializer you are writing that takes
uintmax_t just fine, and (1) your caller that passes around uint64_t
timestamps won't compile on a platform that is incapable of doing
64-bit and you have bigger problem than uintmax_t being narrower
than 64-bit on such a platform, and (2) your caller can just pass
uint64_t value to your JSON formatter that expects uintmax_t without
explicit casting, as normal integral type promotion rule would
apply.

So I would think it is most sensible to have double, uintmax_t and
intmax_t variants.  If you do not care about the extra value range
that unsigned integral types afford, a single intmax_t variant would
also be fine.

Re: [RFC PATCH v5 0/8] rebase-interactive

[Johannes Schindelin]

Hi all,

On Mon, 26 Mar 2018, Wink Saville wrote:

> > I was just going by what the reported compiler error message was.
> > It said that "unsigned long" didn't match the uint64_t variable.
> > And that made me nervous.
> >
> > If all of the platforms we build on define uintmax_t >= 64 bits,
> > then it doesn't matter.
> >
> > If we do have a platform where uintmax_t is u32, then we'll have a
> > lot more breakage than in just the new function I added.
> >
> > Thanks,
> > Jeff
> 
> Should we add a "_Static_assert" that sizeof(uintmax_t) >= sizeof(uint64_t) ?

To come back to the subject that all of the mails in this here mail thread
of you gentle people bear: Wink's patches look good to me, and I would
like to have them be bumped down the next/master waterfall.

Ciao,
Dscho

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

Wink Saville  writes:

> Should we add a "_Static_assert" that sizeof(uintmax_t) >= sizeof(uint64_t) ?

If that expression compiles, then both types are understood by the
platform.  Because

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html

tells us:

Greatest-width integer types

The following type designates a signed integer type capable of
representing any value of any signed integer type: intmax_t

The following type designates an unsigned integer type capable of
representing any value of any unsigned integer type: uintmax_t

These types are required.

we know what that expression evaluates to, no?

Re: [RFC PATCH v5 0/8] rebase-interactive

[Wink Saville]

> I was just going by what the reported compiler error message was.
> It said that "unsigned long" didn't match the uint64_t variable.
> And that made me nervous.
>
> If all of the platforms we build on define uintmax_t >= 64 bits,
> then it doesn't matter.
>
> If we do have a platform where uintmax_t is u32, then we'll have a
> lot more breakage than in just the new function I added.
>
> Thanks,
> Jeff

Should we add a "_Static_assert" that sizeof(uintmax_t) >= sizeof(uint64_t) ?

Re: [RFC PATCH v5 0/8] rebase-interactive

[Jeff Hostetler]

On 3/26/2018 2:00 PM, Junio C Hamano wrote:

Jeff Hostetler  writes:


I am concerned that the above compiler error message says that uintmax_t
is defined as an "unsigned long" (which is defined as *at least* 32 bits,
but not necessarily 64.  But a uint64_t is defined as a "unsigned long long"
and guaranteed as a 64 bit value.


On a platform whose uintmax_t is u32, is it realistic to expect that
we would be able to use u64, even if we explicitly ask for it, in
the first place?

In other words, on a platform that handles uint64_t, I would expect
uintmax_t to be wide enough to hold an uint64_t value without
truncation.



I was just going by what the reported compiler error message was.
It said that "unsigned long" didn't match the uint64_t variable.
And that made me nervous.

If all of the platforms we build on define uintmax_t >= 64 bits,
then it doesn't matter.

If we do have a platform where uintmax_t is u32, then we'll have a
lot more breakage than in just the new function I added.

Thanks,
Jeff

Re: [RFC PATCH v5 0/8] rebase-interactive

[Jeff Hostetler]

On 3/26/2018 1:57 PM, Junio C Hamano wrote:

Jeff Hostetler  writes:


I defined that routine to take a uint64_t because I wanted to
pass a nanosecond value received from getnanotime() and that's
what it returns.


Hmph, but the target format does not have different representation
of inttypes in different sizes, no?

I personally doubt that we would benefit from having a group of
functions (i.e. format_int{8,16,32,64}_to_json()) that callers have
to choose from, depending on the exact size of the integer they want
to serialize.  The de-serializing side would be the same story.

Even if the variable a potential caller of the formetter is a sized
type that is different from uintmax_t, the caller shouldn't have to
add an extra cast.

Am I missing some obvious merit for having these separate functions
for explicit sizes?



I did the uint64_t for the unsigned ns times.

I did the other one for the usual signed ints.

I could convert them both to a single signed 64 bit typed function
if we only want to have one function.

Jeff

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

Jeff Hostetler  writes:

> I am concerned that the above compiler error message says that uintmax_t
> is defined as an "unsigned long" (which is defined as *at least* 32 bits,
> but not necessarily 64.  But a uint64_t is defined as a "unsigned long long"
> and guaranteed as a 64 bit value.

On a platform whose uintmax_t is u32, is it realistic to expect that
we would be able to use u64, even if we explicitly ask for it, in
the first place?

In other words, on a platform that handles uint64_t, I would expect
uintmax_t to be wide enough to hold an uint64_t value without
truncation.

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

Jeff Hostetler  writes:

> I defined that routine to take a uint64_t because I wanted to
> pass a nanosecond value received from getnanotime() and that's
> what it returns.

Hmph, but the target format does not have different representation
of inttypes in different sizes, no?  

I personally doubt that we would benefit from having a group of
functions (i.e. format_int{8,16,32,64}_to_json()) that callers have
to choose from, depending on the exact size of the integer they want
to serialize.  The de-serializing side would be the same story.

Even if the variable a potential caller of the formetter is a sized
type that is different from uintmax_t, the caller shouldn't have to
add an extra cast.

Am I missing some obvious merit for having these separate functions
for explicit sizes?

Re: [RFC PATCH v5 0/8] rebase-interactive

[Jeff Hostetler]

On 3/26/2018 11:56 AM, Junio C Hamano wrote:

Wink Saville  writes:


json-writer.c:123:38:  error:  format specifies type 'uintmax_t' (aka
'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned
long long') [-Werror,-Wformat]

 strbuf_addf(>json, ":%"PRIuMAX, value);
  ~~ ^
json-writer.c:228:37:  error:  format specifies type 'uintmax_t' (aka
'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned
long long') [-Werror,-Wformat] [0m

 strbuf_addf(>json, "%"PRIuMAX, value);
  ~~ ^
2 errors generated.
make: *** [json-writer.o] Error 1
make: *** Waiting for unfinished jobs


For whatever reason, our codebase seems to shy away from PRIu64,
even though there are liberal uses of PRIu32.  Showing the value
casted to uintmax_t with PRIuMAX seems to be our preferred way to
say "We cannot say how wide this type is on different platforms, and
are playing safe by using widest-possible int type" (e.g. showing a
pid_t value from daemon.c).

In this codepath, the actual values are specified to be uint64_t, so
the use of PRIu64 may be OK, but I have to wonder why the codepath
is not dealing with uintmax_t in the first place.  When even larger
than present archs are prevalent in N years and 64-bit starts to
feel a tad small (like we feel for 16-bit ints these days), it will
feel a bit silly to have a subsystem that is limited to such a
"fixed and a tad small these days" types and pretend it to be be a
generic seriealizer, I suspect.



I defined that routine to take a uint64_t because I wanted to
pass a nanosecond value received from getnanotime() and that's
what it returns.

My preference would be to change the PRIuMAX to PRIu64, but there
aren't any other references in the code to that symbol and I didn't
want to start a new trend here.

I am concerned that the above compiler error message says that uintmax_t
is defined as an "unsigned long" (which is defined as *at least* 32 bits,
but not necessarily 64.  But a uint64_t is defined as a "unsigned long long"
and guaranteed as a 64 bit value.

So while I'm not really worried about 128 bit integers right now, I'm
more concerned about 32 bit compilers truncating that value without any
warnings.

Jeff

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

Wink Saville  writes:

> json-writer.c:123:38:  error:  format specifies type 'uintmax_t' (aka
> 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned
> long long') [-Werror,-Wformat]
>
> strbuf_addf(>json, ":%"PRIuMAX, value);
>  ~~ ^
> json-writer.c:228:37:  error:  format specifies type 'uintmax_t' (aka
> 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned
> long long') [-Werror,-Wformat] [0m
>
> strbuf_addf(>json, "%"PRIuMAX, value);
>  ~~ ^
> 2 errors generated.
> make: *** [json-writer.o] Error 1
> make: *** Waiting for unfinished jobs

For whatever reason, our codebase seems to shy away from PRIu64,
even though there are liberal uses of PRIu32.  Showing the value
casted to uintmax_t with PRIuMAX seems to be our preferred way to
say "We cannot say how wide this type is on different platforms, and
are playing safe by using widest-possible int type" (e.g. showing a
pid_t value from daemon.c).

In this codepath, the actual values are specified to be uint64_t, so
the use of PRIu64 may be OK, but I have to wonder why the codepath
is not dealing with uintmax_t in the first place.  When even larger
than present archs are prevalent in N years and 64-bit starts to
feel a tad small (like we feel for 16-bit ints these days), it will
feel a bit silly to have a subsystem that is limited to such a
"fixed and a tad small these days" types and pretend it to be be a
generic seriealizer, I suspect.

Re: [RFC PATCH v5 0/8] rebase-interactive

[Wink Saville]

>> I queued everything (with all patch 3-8/8 retitled to share a
>> common prefix, so that "git shortlog" output would stay sane)
>> and I think I resolved the conflicts with Dscho's recreate-merges
>> topic correctly.  Please double check what will appear on 'pu' later
>> today.
>>
>> Thanks.
>>
>
> OK, thank you!

I looked at 'pu' and it LGTM, so I pushed 'pu' as a branch to my
github account to test with Travis-CI. All the linux builds are
green, but the 2 OSX builds are red[1] and the logs show compile
errors:

CC ident.o
CC json-writer.o

json-writer.c:123:38:  error:  format specifies type 'uintmax_t' (aka
'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned
long long') [-Werror,-Wformat]

strbuf_addf(>json, ":%"PRIuMAX, value);
 ~~ ^
json-writer.c:228:37:  error:  format specifies type 'uintmax_t' (aka
'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned
long long') [-Werror,-Wformat] [0m

strbuf_addf(>json, "%"PRIuMAX, value);
 ~~ ^
2 errors generated.
make: *** [json-writer.o] Error 1
make: *** Waiting for unfinished jobs

[RFC Patch 1/1] changes the PRIuMax to PRIu64 to correct the compile error
and now Travis-CI is green [2].

[1]: https://travis-ci.org/winksaville/git/builds/357660624
[2]: https://travis-ci.org/winksaville/git/builds/357681929

-- Wink

Re: [RFC PATCH v5 0/8] rebase-interactive

[Wink Saville]

On Fri, Mar 23, 2018 at 3:39 PM, Junio C Hamano  wrote:
> Wink Saville  writes:
>
>> On Fri, Mar 23, 2018 at 2:25 PM, Wink Saville  wrote:
>>> Reworked patch 1 so that all of the backend scriptlets
>>> used by git-rebase use a normal function style invocation.
>>>
>>> Merged the previous patch 2 and 3 have been squashed which
>>> provides reviewers a little easier time to detect any changes
>>> during extraction of the functions.
>>>
>>> Wink Saville (8):
>>>   rebase-interactive: simplify pick_on_preserving_merges
>>>   rebase: update invocation of rebase dot-sourced scripts
>>>   Indent function git_rebase__interactive
>>>   Extract functions out of git_rebase__interactive
>>>   Add and use git_rebase__interactive__preserve_merges
>>>   Remove unused code paths from git_rebase__interactive
>>>   Remove unused code paths from git_rebase__interactive__preserve_merges
>>>   Remove merges_option and a blank line
>>>
>>>  git-rebase--am.sh  |  11 --
>>>  git-rebase--interactive.sh | 407 
>>> -
>>>  git-rebase--merge.sh   |  11 --
>>>  git-rebase.sh  |   1 +
>>>  4 files changed, 216 insertions(+), 214 deletions(-)
>>>
>>> --
>>> 2.16.2
>>>
>>
>> Argh, I misspelled Junio's email address, so when you reply-all try
>> to remember to remove "gis...@pobox.com" from the cc: list.
>
> Heh, too late ;-)
>
> I queued everything (with all patch 3-8/8 retitled to share a
> common prefix, so that "git shortlog" output would stay sane)
> and I think I resolved the conflicts with Dscho's recreate-merges
> topic correctly.  Please double check what will appear on 'pu' later
> today.
>
> Thanks.
>

OK, thank you!

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

Wink Saville  writes:

> On Fri, Mar 23, 2018 at 2:25 PM, Wink Saville  wrote:
>> Reworked patch 1 so that all of the backend scriptlets
>> used by git-rebase use a normal function style invocation.
>>
>> Merged the previous patch 2 and 3 have been squashed which
>> provides reviewers a little easier time to detect any changes
>> during extraction of the functions.
>>
>> Wink Saville (8):
>>   rebase-interactive: simplify pick_on_preserving_merges
>>   rebase: update invocation of rebase dot-sourced scripts
>>   Indent function git_rebase__interactive
>>   Extract functions out of git_rebase__interactive
>>   Add and use git_rebase__interactive__preserve_merges
>>   Remove unused code paths from git_rebase__interactive
>>   Remove unused code paths from git_rebase__interactive__preserve_merges
>>   Remove merges_option and a blank line
>>
>>  git-rebase--am.sh  |  11 --
>>  git-rebase--interactive.sh | 407 
>> -
>>  git-rebase--merge.sh   |  11 --
>>  git-rebase.sh  |   1 +
>>  4 files changed, 216 insertions(+), 214 deletions(-)
>>
>> --
>> 2.16.2
>>
>
> Argh, I misspelled Junio's email address, so when you reply-all try
> to remember to remove "gis...@pobox.com" from the cc: list.

Heh, too late ;-)

I queued everything (with all patch 3-8/8 retitled to share a
common prefix, so that "git shortlog" output would stay sane)
and I think I resolved the conflicts with Dscho's recreate-merges
topic correctly.  Please double check what will appear on 'pu' later
today.

Thanks.

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

Wink Saville  writes:

> Wink Saville (8):
>   rebase-interactive: simplify pick_on_preserving_merges
>   rebase: update invocation of rebase dot-sourced scripts
>   Indent function git_rebase__interactive
>   Extract functions out of git_rebase__interactive
>   Add and use git_rebase__interactive__preserve_merges
>   Remove unused code paths from git_rebase__interactive
>   Remove unused code paths from git_rebase__interactive__preserve_merges
>   Remove merges_option and a blank line

I felt that the structure of steps 5-7 that adds an identical copy
first and then removes irrelevant parts from both copies was a bit
unusual, but I do not think of a better structure I would use if I
were doing this series myself, and more importantly, the entire
series was a pleasant and straight-forward read.

Will queue and wait for input from others.

Thanks.

Re: [RFC PATCH v5 0/8] rebase-interactive

[Wink Saville]

On Fri, Mar 23, 2018 at 2:25 PM, Wink Saville  wrote:
> Reworked patch 1 so that all of the backend scriptlets
> used by git-rebase use a normal function style invocation.
>
> Merged the previous patch 2 and 3 have been squashed which
> provides reviewers a little easier time to detect any changes
> during extraction of the functions.
>
> Wink Saville (8):
>   rebase-interactive: simplify pick_on_preserving_merges
>   rebase: update invocation of rebase dot-sourced scripts
>   Indent function git_rebase__interactive
>   Extract functions out of git_rebase__interactive
>   Add and use git_rebase__interactive__preserve_merges
>   Remove unused code paths from git_rebase__interactive
>   Remove unused code paths from git_rebase__interactive__preserve_merges
>   Remove merges_option and a blank line
>
>  git-rebase--am.sh  |  11 --
>  git-rebase--interactive.sh | 407 
> -
>  git-rebase--merge.sh   |  11 --
>  git-rebase.sh  |   1 +
>  4 files changed, 216 insertions(+), 214 deletions(-)
>
> --
> 2.16.2
>

Argh, I misspelled Junio's email address, so when you reply-all try
to remember to remove "gis...@pobox.com" from the cc: list.

Sorry,
Wink