| Message ID | 9a26c5b6110081cd8d029f2ab0327c4a1d228ef7.1597364493.git.congdanhqx@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | diff: index-line: respect --abbrev in object's name | expand |
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > When we see --no-abbrev in command's arguments, we reset the 'abbrev' > field in diff-options to 0 and this value will be looked at > diff_abbrev_oid() to decide not to truncate the object name. > > In a later change, we want to extend --abbrev support to diff-patch > format. When --abbrev supporting diff-patch, we need to differentiate > those below scenarios: > > * None of those options --abbrev, --no-abbrev, and --full-index are > asked. diff-patch should keep old behavior of using DEFAULT_ABBREV > for the index length. > * --no-abbrev is asked, diff-patch should treat this option as same as > --full-index and show full object name in index line. Sorry, but are you saying that the above two cases cannot be differentiated in the current code? * If none of --abbrev, --no-abbrev, --full-index are given, then diff.c::prep_parse_options() will leave options->flags.full_index and options->abbrev untouched. They are initialized to false and DEFAULT_ABBREV (typically -1 when unconfigured). * If --no-abbrev is given, options->abbrev is set to 0. options->flags.full_index is not touched. So you should be able to tell these two apart by only looking at options->flags.full_index bit. Perhaps, even though you said "we need to differentiate", you meant something else? > While not doing anything is very effective way to show full object id, > we couldn't differentiate if --no-abbrev or not. Hmph. --no-abbrev without --full-index would not set flags.full_index bit; using --full-index would set the bit. Are you planning to do something special when BOTH --no-abbrev and --full-index is given? I am confused X-<.
On 2020-08-13 17:50:31-0700, Junio C Hamano <gitster@pobox.com> wrote: > Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > > > When we see --no-abbrev in command's arguments, we reset the 'abbrev' > > field in diff-options to 0 and this value will be looked at > > diff_abbrev_oid() to decide not to truncate the object name. > > > > In a later change, we want to extend --abbrev support to diff-patch > > format. When --abbrev supporting diff-patch, we need to differentiate > > those below scenarios: > > > > * None of those options --abbrev, --no-abbrev, and --full-index are > > asked. diff-patch should keep old behavior of using DEFAULT_ABBREV > > for the index length. > > * --no-abbrev is asked, diff-patch should treat this option as same as > > --full-index and show full object name in index line. > > Sorry, but are you saying that the above two cases cannot be > differentiated in the current code? > > * If none of --abbrev, --no-abbrev, --full-index are given, then > diff.c::prep_parse_options() will leave options->flags.full_index > and options->abbrev untouched. They are initialized to false and > DEFAULT_ABBREV (typically -1 when unconfigured). > > * If --no-abbrev is given, options->abbrev is set to 0. > options->flags.full_index is not touched. > > So you should be able to tell these two apart by only looking at > options->flags.full_index bit. Perhaps, even though you said "we > need to differentiate", you meant something else? Oops, I shouldn't say anything about --full-index in the second point to reduce confusion. Let me list some combination here: * none of --abbrev --no-abbrev --full-index -> default short index * --abbrev --full-index -> full-index * --full-index --abbrev -> full-index * --abbrev --no-abbrev -> full-index * --no-abbrev --abbrev=[n] -> shortened index to n char So, we can't use full_index bit, because --no-abbrev can be defeated by --abbrev, but --full-index will always win --abbrev. > > > While not doing anything is very effective way to show full object id, > > we couldn't differentiate if --no-abbrev or not. > > Hmph. --no-abbrev without --full-index would not set > flags.full_index bit; using --full-index would set the bit. Are you > planning to do something special when BOTH --no-abbrev and --full-index > is given? I am confused X-<. I'm not planning for anything special when both --no-abbrev and --full-index is given. I'm planning for: * BOTH --abbrev and --no-abbrev but NOT --full-index; * BOTH --abbrev AND --full-index Sorry for the confusion, I hope it's clear now, and you could help me rephase a bit to reduce confusion.
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > Let me list some combination here: > > * none of --abbrev --no-abbrev --full-index -> default short index > * --abbrev --full-index -> full-index > * --full-index --abbrev -> full-index > * --abbrev --no-abbrev -> full-index > * --no-abbrev --abbrev=[n] -> shortened index to n char > > So, we can't use full_index bit, because --no-abbrev can be defeated > by --abbrev, but --full-index will always win --abbrev. Sure, I wasn't suggesting to flip the flags.full_index bit upon seeing "--no-abbrev". When --no-abbrev is in effect (i.e. the last one among --no-abbrev, --abbrev, or --abbrev=n), .abbrev field is set to 0. So wouldn't it be sufficient to say - If flags.full_index bit is set, show the full object name - If abbrev is 0, show the full object name - All other cases, after clamping the value of abbrev to reasonable value, truncat the object name to that length What am I missing? > I'm planning for: > > * BOTH --abbrev and --no-abbrev but NOT --full-index; > * BOTH --abbrev AND --full-index
On 2020-08-13 18:06:22-0700, Junio C Hamano <gitster@pobox.com> wrote: > Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > > > Let me list some combination here: > > > > * none of --abbrev --no-abbrev --full-index -> default short index > > * --abbrev --full-index -> full-index > > * --full-index --abbrev -> full-index > > * --abbrev --no-abbrev -> full-index > > * --no-abbrev --abbrev=[n] -> shortened index to n char > > > > So, we can't use full_index bit, because --no-abbrev can be defeated > > by --abbrev, but --full-index will always win --abbrev. > > Sure, I wasn't suggesting to flip the flags.full_index bit upon > seeing "--no-abbrev". When --no-abbrev is in effect (i.e. the last > one among --no-abbrev, --abbrev, or --abbrev=n), .abbrev field is > set to 0. So wouldn't it be sufficient to say > > - If flags.full_index bit is set, show the full object name > > - If abbrev is 0, show the full object name > > - All other cases, after clamping the value of abbrev to reasonable > value, truncat the object name to that length > > What am I missing? No, you didn't miss anything. It's obviously me, who screwed up the logical thinking. Originally, I come up with something along the line in 2/2: int abbrev = o->flags.full_index ? hexsz : DEFAULT_ABBREV; if (!o->abbrev) abbrev = o->abbrev; I couldn't recalled what I wrote, but that logic requires 1/2, after I come up with 1/2, I re-analysed 2/2 and come up with current logic. I failed to re-visit 1/2 to check if it's necessary. It's all MY fault. Sorry for wasting everyone's time in 1/2. Please eject 1/2 from this series.
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: >> What am I missing? > > No, you didn't miss anything. > > It's obviously me, who screwed up the logical thinking. > > Originally, I come up with something along the line in 2/2: > > int abbrev = o->flags.full_index ? hexsz : DEFAULT_ABBREV; > if (!o->abbrev) > abbrev = o->abbrev; > > I couldn't recalled what I wrote, but that logic requires 1/2, > after I come up with 1/2, I re-analysed 2/2 and come up with current > logic. > > I failed to re-visit 1/2 to check if it's necessary. > It's all MY fault. > > Sorry for wasting everyone's time in 1/2. > > Please eject 1/2 from this series. OK. 2/2 would work without this step.
diff --git a/revision.c b/revision.c index 3dcf689341..24027b1af3 100644 --- a/revision.c +++ b/revision.c @@ -2430,7 +2430,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg } else if (!strcmp(arg, "--always")) { revs->always_show_header = 1; } else if (!strcmp(arg, "--no-abbrev")) { - revs->abbrev = 0; + revs->abbrev = hexsz; } else if (!strcmp(arg, "--abbrev")) { revs->abbrev = DEFAULT_ABBREV; } else if (skip_prefix(arg, "--abbrev=", &optarg)) {
When we see --no-abbrev in command's arguments, we reset the 'abbrev' field in diff-options to 0 and this value will be looked at diff_abbrev_oid() to decide not to truncate the object name. In a later change, we want to extend --abbrev support to diff-patch format. When --abbrev supporting diff-patch, we need to differentiate those below scenarios: * None of those options --abbrev, --no-abbrev, and --full-index are asked. diff-patch should keep old behavior of using DEFAULT_ABBREV for the index length. * --no-abbrev is asked, diff-patch should treat this option as same as --full-index and show full object name in index line. While not doing anything is very effective way to show full object id, we couldn't differentiate if --no-abbrev or not. We can differentiate those above scenarios by either: * Add a new field in diff-options to mark if --no-abbrev was asked. With this option, we have a new field for a single purpose, and one more thing to worry about. * Treat --no-abbrev as same as --full-index. This option is problematic, since we want to allow --abbrev overwrite --no-abbrev again. On the other hand, we also need to keep our promises with scripter who uses --full-index to ask for full object names in index line, so, we need to respsect --full-index regardless of --abbrev. * Set 'abbrev' field in diff-options to the length of the hash we are using. With this option, we can differentiate if --no-abbrev was asked ('abbrev' is hash's length) or none of --[no-]abbrev was asked ('abbrev' is '0'), script with --full-index still works and our headache is kept as is. Let's ask for full object id if we see --no-abbrev. Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> --- revision.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)