builtin/blame.c: bit field constants into bit shift format #382

harry-hov · 2019-10-09T19:12:39Z

we are looking at bitfield constants, and elsewhere in the Git source code, such cases are handled via bit shift operators rather than octal numbers, which also makes it easier to spot holes in the range (if, say, 1<<5 was missing, it is easier to spot it between 1<<4 and 1<<6 than it is to spot a missing 040 between a 020 and a 0100).
Also, bit shifts lead to low-level optimizations because they require fewer calculations for the CPU.

Special Thanks to @dscho for helping me out throughout the process.

gitgitgadget · 2019-10-09T19:13:27Z

Welcome to GitGitGadget

Hi @harry-hov, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that this Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

the lines should not exceed 76 columns,
the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
the commit messages' body should be describing the "why?" of the change.
Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form /allow <username>.

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions.

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via:

curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

harry-hov · 2019-10-09T19:18:41Z

Hey @dscho here is my first contribution to the Git.
Although, I have read the coding guidelines. Still there is a possibility that I may have missed some points.
I would love to have your review.

Regarding #357

dscho · 2019-10-10T11:23:30Z

/allow

gitgitgadget · 2019-10-10T11:24:34Z

User harry-hov is now allowed to use GitGitGadget.

dscho · 2019-10-10T11:25:05Z

Regarding #357

You could put this into the commit message, as "This addresses #357". That would make it easier for reviewers on the Git mailing list (who do not necessarily frequent GitHub let alone this here fork), and it would also close the ticket automatically once the Git maintainer merges the patch into master.

dscho · 2019-10-10T11:26:48Z

Turing preprocessor constants

This made me chuckle :-) I know you meant "Turning", not https://en.wikipedia.org/wiki/Alan_Turing.

The commit message could possibly repeat a little bit from the ticket that inspired you. If you look around in Git's commit history, we really like detailed commit messages.

builtin/blame.c

dscho

I think it would be fine for contributing as-is, but of course you could always improve the commit message (not only the typo, but also a more detailed explanation, e.g. explaining how this helps type safety, concretely) and you could use the 1<<0 style for the constants and align them.

Also keep in mind that the PR description will be sent as cover letter.

harry-hov · 2019-10-10T16:34:19Z

Thanks, @dscho for the suggestions and comments.
I will implement the changes as suggested by you.

dscho · 2019-10-11T14:26:45Z

dscho force-pushed the gitgitgadget:pu branch from 8d9027f to ef2d425 7 hours ago

Oh. I see that you made pu the base branch. That branch is rewritten multiple times per day, though, and force-pushed. Therefore, it is too moving a target.

How about rebasing your commit to master and then editing the base branch in this PR?

harry-hov · 2019-10-12T20:38:41Z

Hey @dscho!
Just made some changes
Please have a look 😀

builtin/blame.c

harry-hov · 2019-10-12T23:08:58Z

@dscho resolved those comments
am I doing it right ??

dscho · 2019-10-13T22:27:40Z

builtin/blame.c

-#define OUTPUT_LINE_PORCELAIN	01000
-#define OUTPUT_COLOR_LINE	02000
-#define OUTPUT_SHOW_AGE_WITH_COLOR	04000
+#define	OUTPUT_ANNOTATE_COMPAT	(1<<0)


Sorry, I should have been clearer: I meant to align the values (i.e. 1<<0, 1<<1, etc), with spaces, and not the keys. There should not be more than one single space between the #define and the OUTPUT_ANNOTATE_COMPAT and friends.

harry-hov · 2019-10-14T19:36:22Z

Hey @dscho
It's me again, looking forward to your review.

PS:- Sorry for bothering you again and again.

dscho · 2019-10-14T21:53:50Z

Hey @dscho
It's me again, looking forward to your review.

Sure.

The patch is in a good shape, now let's focus on the commit message:

As most of the preprocessor bit field constant are in bit shift format. e.g. a<<b

So, Converting these bit field constant into bit shift format makes them look more like bit fields constants.

These seem to be slightly incomplete sentences with little power to explain the motivation behind the patch.

Maybe a better way would be to say that we are looking at bitfield constants, and elsewhere in the Git source code, such cases are handled via bit shift operators rather than octal numbers, which also makes it easier to spot holes in the range (if, say, 1<<5 was missing, it is easier to spot it between 1<<4 and 1<<6 than it is to spot a missing 040 between a 020 and a 0100).

harry-hov · 2019-10-15T19:42:35Z

Maybe a better way would be to say that we are looking at bitfield constants, and elsewhere in the Git source code, such cases are handled via bit shift operators rather than octal numbers, which also makes it easier to spot holes in the range (if, say, 1<<5 was missing, it is easier to spot it between 1<<4 and 1<<6 than it is to spot a missing 040 between a 020 and a 0100).

Thanks @dscho
Done changes to the commit message.

dscho · 2019-10-16T12:39:03Z

Looks good!

dscho · 2019-10-16T12:44:19Z

BTW if you are as puzzled as I was why the "shipit" emoji is a squirrel: https://www.quora.com/On-GitHub-what-is-the-significance-of-the-Ship-It-squirrel

harry-hov · 2019-10-16T18:29:28Z

BTW if you are as puzzled as I was why the "shipit" emoji is a squirrel: https://www.quora.com/On-GitHub-what-is-the-significance-of-the-Ship-It-squirrel

No doubt why you are popular amongst Git Contributors 😄

harry-hov · 2019-10-17T17:45:45Z

Hi,

Thanks for your comments. I've modified the patch.
(i.e replaced "int" with "unsigned int").

Regards
Hariom Verma

harry-hov · 2019-10-17T17:46:06Z

/submit

gitgitgadget · 2019-10-17T17:47:05Z

Submitted as pull.382.v2.git.1571334411.gitgitgadget@gmail.com

harry-hov · 2019-10-17T17:53:44Z

Hi @dscho
I'm having some confusions regarding replying to comments in PATCH using this tool.

dscho · 2019-10-18T11:14:47Z

I'm having some confusions regarding replying to comments in PATCH using this tool.

I tried to describe it a little bit in https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis (you will see this in every mirrored response linked as "(reply to this)"). There are also instructions for command-line nerds at the bottom of every public-inbox page, see e.g. https://public-inbox.org/git/xmqqr23b7ref.fsf@gitster-ct.c.googlers.com

gitgitgadget · 2019-10-21T08:44:02Z

This branch is now known as hv/bitshift-constants-in-blame.

gitgitgadget · 2019-10-21T08:44:04Z

This patch series was integrated into pu via git@0926ab8.

gitgitgadget · 2019-10-22T11:34:59Z

This patch series was integrated into pu via git@2a11dc1.

gitgitgadget · 2019-10-23T06:00:01Z

This patch series was integrated into pu via git@ce72ff0.

gitgitgadget · 2019-10-23T12:00:34Z

This patch series was integrated into pu via git@76c5e9d.

gitgitgadget · 2019-10-24T05:53:29Z

This patch series was integrated into pu via git@136476c.

gitgitgadget · 2019-10-24T05:53:30Z

This patch series was integrated into next via git@bc84aae.

gitgitgadget · 2019-10-24T07:25:06Z

This patch series was integrated into pu via git@70a25b5.

gitgitgadget · 2019-10-28T05:37:06Z

This patch series was integrated into pu via git@aba629c.

gitgitgadget · 2019-10-30T07:47:30Z

This patch series was integrated into pu via git@ed8ceb6.

gitgitgadget · 2019-11-02T10:37:34Z

This patch series was integrated into pu via git@bc27da4.

gitgitgadget · 2019-11-04T05:33:36Z

This patch series was integrated into pu via git@4feb4a6.

gitgitgadget · 2019-11-06T13:37:52Z

This patch series was integrated into pu via git@04454b5.

gitgitgadget · 2019-11-07T11:14:50Z

This patch series was integrated into pu via git@c93af32.

gitgitgadget · 2019-11-08T08:48:12Z

This patch series was integrated into pu via git@d6a9d32.

gitgitgadget · 2019-11-10T08:58:59Z

This patch series was integrated into pu via git@d40095a.

gitgitgadget · 2019-11-11T04:50:45Z

This patch series was integrated into pu via git@8f40d89.

gitgitgadget · 2019-11-11T04:50:47Z

This patch series was integrated into master via git@8f40d89.

gitgitgadget · 2019-11-11T04:50:49Z

Closed via 8f40d89.

dscho reviewed Oct 10, 2019

View reviewed changes

builtin/blame.c Outdated Show resolved Hide resolved

dscho reviewed Oct 10, 2019

View reviewed changes

dscho force-pushed the pu branch from 8d9027f to ef2d425 Compare October 11, 2019 07:38

harry-hov changed the base branch from pu to master October 11, 2019 16:15

harry-hov force-pushed the enum branch 2 times, most recently from 52f7a4a to 988033e Compare October 12, 2019 20:26

harry-hov changed the title ~~Preprocessor constants into enums~~ builtin/blame.c: bit field constants into bit shift format Oct 12, 2019

dscho reviewed Oct 12, 2019

View reviewed changes

builtin/blame.c Outdated Show resolved Hide resolved

harry-hov force-pushed the enum branch from 988033e to 7d5152a Compare October 12, 2019 23:04

weekly-digest bot mentioned this pull request Oct 13, 2019

Weekly Digest (6 October, 2019 - 13 October, 2019) #393

Closed

dscho reviewed Oct 13, 2019

View reviewed changes

harry-hov force-pushed the enum branch 2 times, most recently from 1e4e65d to 5dc8543 Compare October 14, 2019 19:27

harry-hov force-pushed the enum branch from 5dc8543 to 3b4b8e0 Compare October 15, 2019 19:32

weekly-digest bot mentioned this pull request Oct 20, 2019

Weekly Digest (13 October, 2019 - 20 October, 2019) #409

Closed

gitgitgadget bot added the pu label Oct 21, 2019

gitgitgadget bot added the next label Oct 24, 2019

gitgitgadget bot added the master label Nov 11, 2019

gitgitgadget bot closed this Nov 11, 2019

harry-hov deleted the enum branch November 23, 2019 08:29

builtin/blame.c: bit field constants into bit shift format #382

builtin/blame.c: bit field constants into bit shift format #382

Uh oh!

Conversation

harry-hov commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitgitgadget bot commented Oct 9, 2019

Welcome to GitGitGadget

Contributing the patches

Uh oh!

harry-hov commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Oct 10, 2019

Uh oh!

gitgitgadget bot commented Oct 10, 2019

Uh oh!

dscho commented Oct 10, 2019

Uh oh!

dscho commented Oct 10, 2019

Uh oh!

Uh oh!

dscho left a comment

Choose a reason for hiding this comment

Uh oh!

harry-hov commented Oct 10, 2019

Uh oh!

dscho commented Oct 11, 2019

Uh oh!

harry-hov commented Oct 12, 2019

Uh oh!

Uh oh!

harry-hov commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

harry-hov commented Oct 14, 2019

Uh oh!

dscho commented Oct 14, 2019

Uh oh!

harry-hov commented Oct 15, 2019

Uh oh!

dscho commented Oct 16, 2019

Uh oh!

dscho commented Oct 16, 2019

Uh oh!

harry-hov commented Oct 16, 2019

Uh oh!

harry-hov commented Oct 17, 2019

Uh oh!

harry-hov commented Oct 17, 2019

Uh oh!

gitgitgadget bot commented Oct 17, 2019

Uh oh!

harry-hov commented Oct 17, 2019

Uh oh!

dscho commented Oct 18, 2019

Uh oh!

gitgitgadget bot commented Oct 21, 2019

Uh oh!

gitgitgadget bot commented Oct 21, 2019

Uh oh!

gitgitgadget bot commented Oct 22, 2019

Uh oh!

gitgitgadget bot commented Oct 23, 2019

Uh oh!

gitgitgadget bot commented Oct 23, 2019

Uh oh!

gitgitgadget bot commented Oct 24, 2019

Uh oh!

gitgitgadget bot commented Oct 24, 2019

Uh oh!

gitgitgadget bot commented Oct 24, 2019

Uh oh!

gitgitgadget bot commented Oct 28, 2019

Uh oh!

gitgitgadget bot commented Oct 30, 2019

harry-hov commented Oct 9, 2019 •

edited

Loading

harry-hov commented Oct 9, 2019 •

edited

Loading

harry-hov commented Oct 12, 2019 •

edited

Loading