-
Notifications
You must be signed in to change notification settings - Fork 610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/show commit in main #77
Conversation
Signed-off-by: Valentin Haenel <valentin.haenel@gmx.de>
Signed-off-by: Valentin Haenel <valentin.haenel@gmx.de>
Signed-off-by: Valentin Haenel <valentin.haenel@gmx.de>
return parse_bool(&opt_show_id, argv[2]); | ||
|
||
if (!strcmp(argv[0], "id-len")) | ||
return parse_int(&opt_id_len, argv[2], 1, 40); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Git, the minimum abbrev size is 4 (see environment.c:17).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it would be smart to parse the core.abbrev
git config option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, using core.abbrev would be nice.
BTW, I would have squashed those 3 commits, and used "abbrev" instead of "id" to use Git terminology, but that's just me. Otherwise I tested it and it works fine! |
One thing i just noticed, is that searching for a partial commit ID using |
@esc They are not searchable because they are not made searchable. See main_grep: https://github.com/jonas/tig/blob/master/tig.c#L7301 |
Yeah, I just discovered that. Preparing a patch to make them search-able. |
@esc It is because the argument to draw_field is the number of (character) columns to stretch this field including a separator/padding character at the end. |
Suggested-by: Vivien Didelot <vivien@didelot.org>
Suggested-by: Jonas Fonseca <jonas.fonseca@gmail.com>
@jonas Thanks! In that case I'll remove the |
The option value specifies the length including the padding character. I initially added one to mean: "if I specify an id-len of 7, I get 7 hex chars". However it seems more sensible to make this option consistent with the others to mean: "If i specify an id-len of 7, I get 6 hex chars plus a single whitespace for padding."
There are now two settings that can affect how long the commit ID in the main view is: 'core.abbrev' and 'id-width'. The first is a core option, the second is a tig specific one. In case tig finds the first setting it respects this. In case the user wishes tig to use a different value, the second option is provided as an oevrride.
'show-id' will make the commit IDs visible, and 'id-width' configures how many characters to show. The latter is also applied to the blame view. Reviewed-by: Vivien Didelot <vivien@didelot.org> Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
Thanks, I squash-merged this and made a couple of cosmetic changes, but the end result should be the same. Also, I ended up renaming the _cols/_COLS variables to _width/WIDTH and changing draw_field() to automatically add an extra character for the padding to simplify the handling of core.abbrev. |
Awesome! |
I was missing the feature to show the (abbreviated) commit ID in the main view. The three commits in this branch implement this feature.
I have taken care to implement the feature, the settings including color for this feature, the possibility to toggle this feature and documentation for all of this.
By default the display is disabled, to not confuse regular users of Tig with a new default main view.
One question remains. I don't understand why need to use
+1
when drawing the commit ID to get7
shown characters if the value ofopt_id_len
is7
?