git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Philippe Blain <levraiphilippeblain@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Elijah Newren <newren@gmail.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH 06/11] reset_head(): make default_reflog_action optional
Date: Fri, 01 Oct 2021 14:08:29 -0700	[thread overview]
Message-ID: <xmqqczoo5u7m.fsf@gitster.g> (raw)
In-Reply-To: <4503defe5912aecc4e6a50ac82a31aa7b230456b.1633082702.git.gitgitgadget@gmail.com> (Phillip Wood via GitGitGadget's message of "Fri, 01 Oct 2021 10:04:57 +0000")

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This parameter is only needed when a ref is going to be updated and
> the caller does not pass an explicit reflog message. Callers that are
> just discarding changes in the working tree like create_autostash() do
> not update any refs so should not have to worry about passing this
> parameter.

This only talks about internal implementation details of passing a
parameter that may not be used, but ...

> -	ret = update_refs(oid, switch_to_branch, reflog_head, reflog_orig_head,
> -			  default_reflog_action, flags);
> +	if (oid != &head_oid || update_orig_head || switch_to_branch)
> +		ret = update_refs(oid, switch_to_branch, reflog_head,
> +				  reflog_orig_head, default_reflog_action,
> +				  flags);

... doesn't this have a more significant behaviour change?

I am not sure if comparison between oid and head_oid can safely
cheat like the patch does, or if it is necessary to do a proper oid
comparison, but either way, this would stop calling update_refs(),
which in turn means it would have an externally visible effect, like
a hook no longer getting called, doesn't it?

It would be a change for the good---calling post-checkout hook when
you did "git checkout (no other arguments)" feels wasteful, but it
deserves to be told to end users, I would think.

Again, a new test to protect the change would go well in the same
patch.

Thanks.



  parent reply	other threads:[~2021-10-01 21:08 UTC|newest]

Thread overview: 85+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-01 10:04 [PATCH 00/11] rebase: reset_head() related fixes and improvements Phillip Wood via GitGitGadget
2021-10-01 10:04 ` [PATCH 01/11] rebase: factor out checkout for up to date branch Phillip Wood via GitGitGadget
2021-10-01 10:04 ` [PATCH 02/11] reset_head(): fix checkout Phillip Wood via GitGitGadget
2021-10-01 20:26   ` Junio C Hamano
2021-10-04  9:58     ` Phillip Wood
2021-10-04 16:13       ` Junio C Hamano
2021-10-01 22:47   ` Eric Sunshine
2021-10-01 10:04 ` [PATCH 03/11] reset_head(): don't run checkout hook if there is an error Phillip Wood via GitGitGadget
2021-10-01 20:52   ` Junio C Hamano
2021-10-04 10:00     ` Phillip Wood
2021-10-12  8:48       ` Ævar Arnfjörð Bjarmason
2021-10-01 10:04 ` [PATCH 04/11] reset_head(): remove action parameter Phillip Wood via GitGitGadget
2021-10-01 20:58   ` Junio C Hamano
2021-10-04 10:00     ` Phillip Wood
2021-10-01 10:04 ` [PATCH 05/11] reset_head(): factor out ref updates Phillip Wood via GitGitGadget
2021-10-01 21:00   ` Junio C Hamano
2021-10-04 10:03     ` Phillip Wood
2021-10-01 10:04 ` [PATCH 06/11] reset_head(): make default_reflog_action optional Phillip Wood via GitGitGadget
2021-10-01 21:03   ` Junio C Hamano
2021-10-01 21:08   ` Junio C Hamano [this message]
2021-10-04 10:03     ` Phillip Wood
2021-10-01 10:04 ` [PATCH 07/11] rebase: cleanup reset_head() calls Phillip Wood via GitGitGadget
2021-10-01 10:04 ` [PATCH 08/11] reset_head(): take struct rebase_head_opts Phillip Wood via GitGitGadget
2021-10-01 21:11   ` Junio C Hamano
2021-10-04 10:09     ` Phillip Wood
2021-10-01 10:05 ` [PATCH 09/11] rebase --apply: fix reflog Phillip Wood via GitGitGadget
2021-10-01 21:12   ` Junio C Hamano
2021-10-01 10:05 ` [PATCH 10/11] rebase --apply: set ORIG_HEAD correctly Phillip Wood via GitGitGadget
2021-10-01 21:18   ` Junio C Hamano
2021-10-01 10:05 ` [PATCH 11/11] rebase -m: don't fork git checkout Phillip Wood via GitGitGadget
2021-10-02  0:38 ` [PATCH 00/11] rebase: reset_head() related fixes and improvements Junio C Hamano
2021-10-02  4:58   ` Junio C Hamano
2021-10-02 12:27     ` Phillip Wood
2021-10-02 13:12       ` Phillip Wood
2021-10-02 13:38       ` René Scharfe
2021-10-06 14:03         ` Phillip Wood
2021-12-08 14:57 ` [PATCH v2 00/14] " Phillip Wood via GitGitGadget
2021-12-08 14:57   ` [PATCH v2 01/14] rebase: factor out checkout for up to date branch Phillip Wood via GitGitGadget
2021-12-09 21:04     ` Junio C Hamano
2021-12-08 14:57   ` [PATCH v2 02/14] t5403: refactor rebase post-checkout hook tests Phillip Wood via GitGitGadget
2021-12-09 18:24     ` Junio C Hamano
2021-12-08 14:57   ` [PATCH v2 03/14] rebase: pass correct arguments to post-checkout hook Phillip Wood via GitGitGadget
2021-12-09 18:53     ` Junio C Hamano
2021-12-08 14:57   ` [PATCH v2 04/14] rebase: do not remove untracked files on checkout Phillip Wood via GitGitGadget
2021-12-09 19:09     ` Junio C Hamano
2021-12-08 14:57   ` [PATCH v2 05/14] rebase --apply: don't run post-checkout hook if there is an error Phillip Wood via GitGitGadget
2021-12-08 14:57   ` [PATCH v2 06/14] reset_head(): remove action parameter Phillip Wood via GitGitGadget
2021-12-08 14:57   ` [PATCH v2 07/14] create_autostash(): remove unneeded parameter Phillip Wood via GitGitGadget
2021-12-09 19:17     ` Junio C Hamano
2022-01-25 11:06       ` Phillip Wood
2021-12-08 14:57   ` [PATCH v2 08/14] reset_head(): factor out ref updates Phillip Wood via GitGitGadget
2021-12-08 14:57   ` [PATCH v2 09/14] reset_head(): make default_reflog_action optional Phillip Wood via GitGitGadget
2021-12-09 19:23     ` Junio C Hamano
2021-12-08 14:57   ` [PATCH v2 10/14] rebase: cleanup reset_head() calls Phillip Wood via GitGitGadget
2021-12-09 19:26     ` Junio C Hamano
2022-01-25 11:07       ` Phillip Wood
2021-12-08 14:57   ` [PATCH v2 11/14] reset_head(): take struct rebase_head_opts Phillip Wood via GitGitGadget
2021-12-09 19:31     ` Junio C Hamano
2021-12-08 14:57   ` [PATCH v2 12/14] rebase --apply: fix reflog Phillip Wood via GitGitGadget
2021-12-09 20:49     ` Junio C Hamano
2021-12-08 14:58   ` [PATCH v2 13/14] rebase --apply: set ORIG_HEAD correctly Phillip Wood via GitGitGadget
2021-12-11 10:59     ` Elijah Newren
2021-12-08 14:58   ` [PATCH v2 14/14] rebase -m: don't fork git checkout Phillip Wood via GitGitGadget
2021-12-09 21:04   ` [PATCH v2 00/14] rebase: reset_head() related fixes and improvements Junio C Hamano
2022-01-26 10:53     ` Phillip Wood
2022-01-27 17:37       ` Junio C Hamano
2021-12-11 11:05   ` Elijah Newren
2022-01-26 13:05   ` [PATCH v3 " Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 01/14] rebase: factor out checkout for up to date branch Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 02/14] t5403: refactor rebase post-checkout hook tests Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 03/14] rebase: pass correct arguments to post-checkout hook Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 04/14] rebase: do not remove untracked files on checkout Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 05/14] rebase --apply: don't run post-checkout hook if there is an error Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 06/14] reset_head(): remove action parameter Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 07/14] reset_head(): factor out ref updates Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 08/14] reset_head(): make default_reflog_action optional Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 09/14] create_autostash(): remove unneeded parameter Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 10/14] rebase: cleanup reset_head() calls Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 11/14] reset_head(): take struct rebase_head_opts Phillip Wood via GitGitGadget
2022-01-26 13:35       ` Ævar Arnfjörð Bjarmason
2022-01-26 14:52         ` Phillip Wood
2022-01-26 13:05     ` [PATCH v3 12/14] rebase --apply: fix reflog Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 13/14] rebase --apply: set ORIG_HEAD correctly Phillip Wood via GitGitGadget
2022-01-26 13:05     ` [PATCH v3 14/14] rebase -m: don't fork git checkout Phillip Wood via GitGitGadget
2022-02-01 17:03     ` [PATCH v3 00/14] rebase: reset_head() related fixes and improvements Elijah Newren

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=xmqqczoo5u7m.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=levraiphilippeblain@gmail.com \
    --cc=newren@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).