[v3,1/2] revision: differentiate if --no-abbrev asked explicitly
diff mbox series

Message ID 9a26c5b6110081cd8d029f2ab0327c4a1d228ef7.1597364493.git.congdanhqx@gmail.com
State Superseded
Headers show
Series
  • diff: index-line: respect --abbrev in object's name
Related show

Commit Message

Đoàn Trần Công Danh Aug. 14, 2020, 12:23 a.m. UTC
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(-)

Comments

Junio C Hamano Aug. 14, 2020, 12:50 a.m. UTC | #1
Đ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-<.
Đoàn Trần Công Danh Aug. 14, 2020, 12:59 a.m. UTC | #2
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.
Junio C Hamano Aug. 14, 2020, 1:06 a.m. UTC | #3
Đ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
Đoàn Trần Công Danh Aug. 14, 2020, 2:50 p.m. UTC | #4
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.
Junio C Hamano Aug. 19, 2020, 10:50 p.m. UTC | #5
Đ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.

Patch
diff mbox series

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)) {