git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 1/2] revision: differentiate if --no-abbrev asked explicitly
Date: Tue, 11 Aug 2020 11:54:01 -0700	[thread overview]
Message-ID: <xmqqwo259fp2.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <9daef7445c09d355d4801f64e427c27dd6c44afb.1597146478.git.congdanhqx@gmail.com> (=?utf-8?B?IsSQb8OgbiBUcuG6p24gQ8O0bmc=?= Danh"'s message of "Tue, 11 Aug 2020 18:49:56 +0700")

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> When we see `--no-abbrev' in command's arguments, we reset `abbrev' of
> `diff_options` to 0, thus, on a later stage, the object id won't
> be shortened (by not set object_id[abbrev] to '\0').

s/by not set/&ting/ probably?  

Please do not write pseudo-code that does not exist anywhere in the
codebase that pretends to be a quote from some real code.  It wasts
reviewer's time looking for non-existing code to see what really is
going on.

I think you meant this line in diff_abbrev_oid():

	if (abbrev)
		hex[abbrev] = '\0';

so you can say that more explicitly, perhaps:

  ... we reset the 'abbrev' field in diff-options to 0 and this
  value is looked at diff_abbrev_oid() to decide not to truncate
  the object name.

or somesuch.

> While not doing anything is very effective way to show full object id,
> we couldn't differentiate if --no-abbrev or not.

You mean you want to tell if --no-abbrev and/or --full-index was
given from the command line and act differently?  You need to
justify why you need to be able to do so (which you attempted in the
remainder of the proposed log message), and also you need to justify
why changing revs->abbrev's meaning is the best way to do so (which
you did not).  In fill_metainfo(), which you are going to touch in
[2/2], you can peek o->flags.full_index to see if --full-index was
given, and the fact that revs->abbrev is not DEFAULT_ABBREV (which
is how the field is initialized in repo_init_revisions()) but is 0
tells us that "--no-abbrev" was given.  --abbrev=0 would have busted
minimum abbrev and set the field to MINIMUM_ABBREV, --abbrev=99 would
have busted hash size and set the field to hexsz.  So 0 is the sign
that --no-abbrev was given.  No?

> In a later change, we want to extend --abbrev support to diff-patch
> format.

And it is unclear why this planned change requires it.

> Let's ask for full object id if we see --no-abbrev instead.

Hence, this "let's ask" is not justified very well.

> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  revision.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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)) {

  reply	other threads:[~2020-08-11 18:54 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-09  2:19 [RFC PATCH 0/2] extend --abbrev support to diff-patch format Đoàn Trần Công Danh
2020-08-09  2:19 ` [RFC PATCH 1/2] revision: differentiate if --no-abbrev asked explicitly Đoàn Trần Công Danh
2020-08-09  2:19 ` [RFC PATCH 2/2] diff: extend --abbrev support to diff-patch format Đoàn Trần Công Danh
2020-08-09 19:01 ` [RFC PATCH 0/2] " Junio C Hamano
2020-08-10 10:00   ` Jeff King
2020-08-10 12:31     ` Đoàn Trần Công Danh
2020-08-10 15:15       ` Junio C Hamano
2020-08-10 15:27         ` Jeff King
2020-08-11  0:33           ` Đoàn Trần Công Danh
2020-08-11  5:22             ` Jeff King
2020-08-11 12:07               ` Đoàn Trần Công Danh
2020-08-10 15:06     ` Junio C Hamano
2020-08-11 11:49 ` [PATCH v2 0/2] diff: index-line: respect --abbrev in object's name Đoàn Trần Công Danh
2020-08-11 11:49   ` [PATCH v2 1/2] revision: differentiate if --no-abbrev asked explicitly Đoàn Trần Công Danh
2020-08-11 18:54     ` Junio C Hamano [this message]
2020-08-11 11:49   ` [PATCH v2 2/2] diff: index-line: respect --abbrev in object's name Đoàn Trần Công Danh
2020-08-14  0:23 ` [PATCH v3 0/2] " Đoàn Trần Công Danh
2020-08-14  0:23   ` [PATCH v3 1/2] revision: differentiate if --no-abbrev asked explicitly Đoàn Trần Công Danh
2020-08-14  0:50     ` Junio C Hamano
2020-08-14  0:59       ` Đoàn Trần Công Danh
2020-08-14  1:06         ` Junio C Hamano
2020-08-14 14:50           ` Đoàn Trần Công Danh
2020-08-19 22:50             ` Junio C Hamano
2020-08-14  0:23   ` [PATCH v3 2/2] diff: index-line: respect --abbrev in object's name Đoàn Trần Công Danh
2020-08-14 15:18     ` SZEDER Gábor
2020-08-14 17:00       ` Junio C Hamano
2020-08-14 18:59         ` Junio C Hamano
2020-08-15  0:21           ` brian m. carlson
2020-08-15  2:27             ` Đoàn Trần Công Danh
2020-08-17 16:17               ` Junio C Hamano
2020-08-20  4:56               ` Junio C Hamano
2020-08-20 12:35 ` [PATCH v4 0/2] " Đoàn Trần Công Danh
2020-08-20 12:35   ` [PATCH v4 1/2] t4013: improve diff-post-processor logic Đoàn Trần Công Danh
2020-08-20 19:49     ` Junio C Hamano
2020-08-21 12:05       ` Đoàn Trần Công Danh
2020-08-21 15:44         ` Junio C Hamano
2020-08-20 12:35   ` [PATCH v4 2/2] diff: index-line: respect --abbrev in object's name Đoàn Trần Công Danh
2020-08-20 19:58     ` Junio C Hamano
2020-08-21 11:51 ` [PATCH v5 0/2] " Đoàn Trần Công Danh
2020-08-21 11:51   ` [PATCH v5 1/2] t4013: improve diff-post-processor logic Đoàn Trần Công Danh
2020-08-21 11:51   ` [PATCH v5 2/2] diff: index-line: respect --abbrev in object's name Đoàn Trần Công Danh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqwo259fp2.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=congdanhqx@gmail.com \
    --cc=git@vger.kernel.org \
    --subject='Re: [PATCH v2 1/2] revision: differentiate if --no-abbrev asked explicitly' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).