threads / patch / 45015

patchdoc: add note about ignoring --no-create-reflog

Subject: [PATCH] doc: add note about ignoring --no-create-reflog

## tl;dr

10 messages between Feb 1, 2017 and Feb 1, 2017. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

cornelius.weig@tngtech.com· Feb 1, 2017, 22:07 UTC · lore
From: Cornelius Weig <cornelius.weig@tngtech.com>

The commands git-branch and git-tag accept a `--create-reflog` argument. On the other hand, the negated form `--no-create-reflog` is accepted as a valid option but has no effect. This silent noop may puzzle users.

To communicate that this behavior is intentional, add a short note in the manuals for git-branch and git-tag.

Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
Notes:
    In a previous discussion (<xmqqbmunrwbf.fsf@gitster.mtv.corp.google.com>) it
    was found that git-branch and git-tag accept a "--no-create-reflog" argument,
    but it has no effect, does not produce a warning, and is undocumented.
 Documentation/git-branch.txt | 1 +
 Documentation/git-tag.txt    | 1 +
 2 files changed, 2 insertions(+)
Show changes to 2 files +2 −0

Documentation/git-branch.txt, Documentation/git-tag.txt

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 1fae4ee..fca3754 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -91,6 +91,7 @@ OPTIONS
 	based sha1 expressions such as "<branchname>@\{yesterday}".
 	Note that in non-bare repositories, reflogs are usually
 	enabled by default by the `core.logallrefupdates` config option.
+	The negated form `--no-create-reflog` is silently ignored.
 
 -f::
 --force::
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5b2288c..b0b933e 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -152,6 +152,7 @@ This option is only applicable when listing tags without annotation lines.
 --create-reflog::
 	Create a reflog for the tag. To globally enable reflogs for tags, see
 	`core.logAllRefUpdates` in linkgit:git-config[1].
+	The negated form `--no-create-reflog` is silently ignored.
 
 <tagname>::
 	The name of the tag to create, delete, or describe.
-- 
2.10.2
Junio C Hamano· Feb 1, 2017, 22:30 UTC · re: cornelius.weig@tngtech.com · lore

Re: [PATCH] doc: add note about ignoring --no-create-reflog

cornelius.weig@tngtech.com writes:
> From: Cornelius Weig <cornelius.weig@tngtech.com>
>
> The commands git-branch and git-tag accept a `--create-reflog` argument.

For the purpose of contrasting the above with "--no-create-reflog", I find it a bit too weak to just say "accept". How about

    The commands git-branch and git-tag accept a `--create-reflog`
    option, and creates reflog even in a repository where
    core.logallrefupdates configuration is set not to.

or something? After all "--no-create-reflog" is accepted. It just does not override the configured (or unconfigured) default.

> On the other hand, the negated form `--no-create-reflog` is accepted as
> a valid option but has no effect. This silent noop may puzzle users.
True, very true.
> To communicate that this behavior is intentional, add a short note in
> the manuals for git-branch and git-tag.

Hmph. The added "short note" merely states the fact; it does not hint that it is intentional or it explains what reasoning is behind that intention.

Show 7 quoted lines
> Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
> ---
>
> Notes:
>     In a previous discussion (<xmqqbmunrwbf.fsf@gitster.mtv.corp.google.com>) it
>     was found that git-branch and git-tag accept a "--no-create-reflog" argument,
>     but it has no effect, does not produce a warning, and is undocumented.

Reading what Peff said in the thread, I do not think we actively wanted this behaviour; we agreed that it is merely acceptable.

So perhaps s/this behaviour is intentional/this is known/ to weaken the log message? That way, when somebody else who really cares comes later and finds this commit that adds explicit notes to these manual pages via "git blame", s/he would not be dissuaded from making things better. Such an update may make it warn when core.logallrefupdates is not set to false (and continue to ignore the command line option), or it may make the command line option actually override the configured default.

With such an update to the log message, I think the patch looks good.

Thanks.
Show 28 quoted lines
>  Documentation/git-branch.txt | 1 +
>  Documentation/git-tag.txt    | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 1fae4ee..fca3754 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -91,6 +91,7 @@ OPTIONS
>  	based sha1 expressions such as "<branchname>@\{yesterday}".
>  	Note that in non-bare repositories, reflogs are usually
>  	enabled by default by the `core.logallrefupdates` config option.
> +	The negated form `--no-create-reflog` is silently ignored.
>  
>  -f::
>  --force::
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 5b2288c..b0b933e 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -152,6 +152,7 @@ This option is only applicable when listing tags without annotation lines.
>  --create-reflog::
>  	Create a reflog for the tag. To globally enable reflogs for tags, see
>  	`core.logAllRefUpdates` in linkgit:git-config[1].
> +	The negated form `--no-create-reflog` is silently ignored.
>  
>  <tagname>::
>  	The name of the tag to create, delete, or describe.
Jeff King· Feb 1, 2017, 22:35 UTC · re: Junio C Hamano · lore

Re: [PATCH] doc: add note about ignoring --no-create-reflog

On Wed, Feb 01, 2017 at 02:30:38PM -0800, Junio C Hamano wrote:
Show 16 quoted lines
> > Notes:
> >     In a previous discussion (<xmqqbmunrwbf.fsf@gitster.mtv.corp.google.com>) it
> >     was found that git-branch and git-tag accept a "--no-create-reflog" argument,
> >     but it has no effect, does not produce a warning, and is undocumented.
> 
> Reading what Peff said in the thread, I do not think we actively
> wanted this behaviour; we agreed that it is merely acceptable.
> 
> So perhaps s/this behaviour is intentional/this is known/ to weaken
> the log message?  That way, when somebody else who really cares
> comes later and finds this commit that adds explicit notes to these
> manual pages via "git blame", s/he would not be dissuaded from
> making things better.  Such an update may make it warn when
> core.logallrefupdates is not set to false (and continue to ignore
> the command line option), or it may make the command line option
> actually override the configured default.

Yeah, I'd consider it more of a "known bug" or "known limitation" than anything.

Those can go in a separate section, but they're probably more likely to be read when supplied next to the actual option.

Show 8 quoted lines
> With such an update to the log message, I think the patch looks
> good.
> [...]
> > @@ -91,6 +91,7 @@ OPTIONS
> >  	based sha1 expressions such as "<branchname>@\{yesterday}".
> >  	Note that in non-bare repositories, reflogs are usually
> >  	enabled by default by the `core.logallrefupdates` config option.
> > +	The negated form `--no-create-reflog` is silently ignored.

This might be nitpicking, but it's _not_ ignored. It still negates an earlier "--create-reflog". It is only that it does not override the decision to create a reflog caused by the setting of core.logallrefupdates.

-Peff
Junio C Hamano· Feb 1, 2017, 23:11 UTC · re: Jeff King · lore

Re: [PATCH] doc: add note about ignoring --no-create-reflog

Jeff King <peff@peff.net> writes:
> This might be nitpicking, but it's _not_ ignored. It still negates an
> earlier "--create-reflog". It is only that it does not override the
> decision to create a reflog caused by the setting of
> core.logallrefupdates.
OK, rolling them all into one, how about this as an amend?
-- >8 --
From: Cornelius Weig <cornelius.weig@tngtech.com>
Date: Wed, 1 Feb 2017 23:07:27 +0100
Subject: [PATCH] doc: add note about ignoring '--no-create-reflog'

The commands git-branch and git-tag accept the '--create-reflog' option, and create reflog even when core.logallrefupdates configuration is explicitly set not to.

On the other hand, the negated form '--no-create-reflog' is accepted as a valid option but has no effect (other than overriding an earlier '--create-reflog' on the command line). This silent noop may puzzle users. To communicate that this is a known limitation, add a short note in the manuals for git-branch and git-tag.

Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-branch.txt | 3 +++
 Documentation/git-tag.txt    | 3 +++
 2 files changed, 6 insertions(+)
Show changes to 2 files +6 −0

Documentation/git-branch.txt, Documentation/git-tag.txt

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 5516a47b54..102e426fd8 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -91,6 +91,9 @@ OPTIONS
 	based sha1 expressions such as "<branchname>@\{yesterday}".
 	Note that in non-bare repositories, reflogs are usually
 	enabled by default by the `core.logallrefupdates` config option.
+	The negated form `--no-create-reflog` does not override the
+	default, even though it overrides `--create-reflog` that appears
+	earlier on the command line.
 
 -f::
 --force::
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 2ac25a9bb3..fd7eeae075 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -152,6 +152,9 @@ This option is only applicable when listing tags without annotation lines.
 --create-reflog::
 	Create a reflog for the tag. To globally enable reflogs for tags, see
 	`core.logAllRefUpdates` in linkgit:git-config[1].
+	The negated form `--no-create-reflog` does not override the
+	default, even though it overrides `--create-reflog` that appears
+	earlier on the command line.
 
 <tagname>::
 	The name of the tag to create, delete, or describe.
-- 
2.11.0-800-g4bf73cb6b2
Cornelius Weig· Feb 1, 2017, 23:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] doc: add note about ignoring --no-create-reflog

On 02/02/2017 12:11 AM, Junio C Hamano wrote:
Show 6 quoted lines
> Jeff King <peff@peff.net> writes:
> 
>> This might be nitpicking, but it's _not_ ignored. It still negates an
>> earlier "--create-reflog". It is only that it does not override the
>> decision to create a reflog caused by the setting of
>> core.logallrefupdates.
This corner case is quite important. Glad you thought about it!
Show 51 quoted lines
> -- >8 --
> From: Cornelius Weig <cornelius.weig@tngtech.com>
> Date: Wed, 1 Feb 2017 23:07:27 +0100
> Subject: [PATCH] doc: add note about ignoring '--no-create-reflog'
> 
> The commands git-branch and git-tag accept the '--create-reflog'
> option, and create reflog even when core.logallrefupdates
> configuration is explicitly set not to.
> 
> On the other hand, the negated form '--no-create-reflog' is accepted
> as a valid option but has no effect (other than overriding an
> earlier '--create-reflog' on the command line). This silent noop may
> puzzle users.  To communicate that this is a known limitation, add a
> short note in the manuals for git-branch and git-tag.
> 
> Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-branch.txt | 3 +++
>  Documentation/git-tag.txt    | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 5516a47b54..102e426fd8 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -91,6 +91,9 @@ OPTIONS
>  	based sha1 expressions such as "<branchname>@\{yesterday}".
>  	Note that in non-bare repositories, reflogs are usually
>  	enabled by default by the `core.logallrefupdates` config option.
> +	The negated form `--no-create-reflog` does not override the
> +	default, even though it overrides `--create-reflog` that appears
> +	earlier on the command line.
>  
>  -f::
>  --force::
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 2ac25a9bb3..fd7eeae075 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -152,6 +152,9 @@ This option is only applicable when listing tags without annotation lines.
>  --create-reflog::
>  	Create a reflog for the tag. To globally enable reflogs for tags, see
>  	`core.logAllRefUpdates` in linkgit:git-config[1].
> +	The negated form `--no-create-reflog` does not override the
> +	default, even though it overrides `--create-reflog` that appears
> +	earlier on the command line.
>  
>  <tagname>::
>  	The name of the tag to create, delete, or describe.
> 

Your amended version is quite concise and says everything there is to say. Thanks

Jeff King· Feb 1, 2017, 23:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] doc: add note about ignoring --no-create-reflog

On Wed, Feb 01, 2017 at 03:11:57PM -0800, Junio C Hamano wrote:
Show 11 quoted lines
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 5516a47b54..102e426fd8 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -91,6 +91,9 @@ OPTIONS
>  	based sha1 expressions such as "<branchname>@\{yesterday}".
>  	Note that in non-bare repositories, reflogs are usually
>  	enabled by default by the `core.logallrefupdates` config option.
> +	The negated form `--no-create-reflog` does not override the
> +	default, even though it overrides `--create-reflog` that appears
> +	earlier on the command line.

Should this perhaps say "currently" or "this may change in the future", so that people (including those who might want to fix it later) know that it's a limitation and not intentional?

I'd also probably say it a little shorter, like:
  The negated form `--no-create-reflog` only overrides an earlier
  `--create-reflog`, but currently does not negate the setting of
  `core.logallrefupdates`.

I guess that really isn't much shorter (I wondered if you could cut out the "overrides --create-reflog" part, since that is the normal and expected behavior, but I had trouble wording it to do so).

-Peff
Cornelius Weig· Feb 1, 2017, 23:23 UTC · re: Jeff King · lore

Re: [PATCH] doc: add note about ignoring --no-create-reflog

>   The negated form `--no-create-reflog` only overrides an earlier
>   `--create-reflog`, but currently does not negate the setting of
>   `core.logallrefupdates`.

Even better than Junio's version. I especially like that it mentions where the default setting comes from.

Junio C Hamano· Feb 1, 2017, 23:27 UTC · re: Jeff King · lore

Re: [PATCH] doc: add note about ignoring --no-create-reflog

Jeff King <peff@peff.net> writes:
> Should this perhaps say "currently" or "this may change in the future",
> so that people (including those who might want to fix it later) know
> that it's a limitation and not intentional?
Good point.
Show 9 quoted lines
> I'd also probably say it a little shorter, like:
>
>   The negated form `--no-create-reflog` only overrides an earlier
>   `--create-reflog`, but currently does not negate the setting of
>   `core.logallrefupdates`.
>
> I guess that really isn't much shorter (I wondered if you could cut out
> the "overrides --create-reflog" part, since that is the normal and
> expected behavior, but I had trouble wording it to do so).

I had the same trouble wording. Another thing I noticed was that I deliberately left it vague what "default" this does not override, because it appears to me that those who do not set logallrefupdates will get the compiled-in default and that is also not overriden.

IOW, "does not negate the setting of core.logallrefupdates" will open us to reports "I do not have the configuration set, but I still get reflog even when --no-create-reflog is given".

   The negated form `--no-create-reflog` currently does not negate
   the default; it overrides an earlier `--create-reflog`, though.
perhaps?
Jeff King· Feb 1, 2017, 23:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] doc: add note about ignoring --no-create-reflog

On Wed, Feb 01, 2017 at 03:27:09PM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> I had the same trouble wording.  Another thing I noticed was that I
> deliberately left it vague what "default" this does not override,
> because it appears to me that those who do not set logallrefupdates
> will get the compiled-in default and that is also not overriden.
> 
> IOW, "does not negate the setting of core.logallrefupdates" will
> open us to reports "I do not have the configuration set, but I still
> get reflog even when --no-create-reflog is given".
> 
>    The negated form `--no-create-reflog` currently does not negate
>    the default; it overrides an earlier `--create-reflog`, though.
> 
> perhaps?

True. I thought the default was "off", and that we merely set the config when initializing a repo. But looking again, it really is checking is_bare_repository() at runtime.

I still think it is OK to mention, as the description of core.logallrefupdates is where we document the behavior and the defaults. So even with "I do not have it set", that is still the key to find more information.

I do not care that strongly either way, though. This is a minor issue, and I suspect just about any note would be helpful.

-Peff
Junio C Hamano· Feb 1, 2017, 23:54 UTC · re: Jeff King · lore

Re: [PATCH] doc: add note about ignoring --no-create-reflog

Jeff King <peff@peff.net> writes:
Show 24 quoted lines
> On Wed, Feb 01, 2017 at 03:27:09PM -0800, Junio C Hamano wrote:
>
>> I had the same trouble wording.  Another thing I noticed was that I
>> deliberately left it vague what "default" this does not override,
>> because it appears to me that those who do not set logallrefupdates
>> will get the compiled-in default and that is also not overriden.
>> 
>> IOW, "does not negate the setting of core.logallrefupdates" will
>> open us to reports "I do not have the configuration set, but I still
>> get reflog even when --no-create-reflog is given".
>> 
>>    The negated form `--no-create-reflog` currently does not negate
>>    the default; it overrides an earlier `--create-reflog`, though.
>> 
>> perhaps?
>
> True. I thought the default was "off", and that we merely set the config
> when initializing a repo. But looking again, it really is checking
> is_bare_repository() at runtime.
>
> I still think it is OK to mention, as the description of
> core.logallrefupdates is where we document the behavior and the
> defaults. So even with "I do not have it set", that is still the key to
> find more information.

OK, let's take yours as the final and merge it down to 'next' soonish.

← back to recent threads