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

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

[cornelius.weig]

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(+)

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

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

[Junio C Hamano]

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.

> 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.

>  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.

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

[Jeff King]

On Wed, Feb 01, 2017 at 02:30:38PM -0800, Junio C Hamano wrote:

> > 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.

> 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

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

[Junio C Hamano]

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(+)

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

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

[Cornelius Weig]

On 02/02/2017 12:11 AM, Junio C Hamano wrote:
> 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!

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

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

[Jeff King]

On Wed, Feb 01, 2017 at 03:11:57PM -0800, Junio C Hamano wrote:

> 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

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

[Cornelius Weig]

>   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.

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

[Junio C Hamano]

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.

> 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?

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

[Jeff King]

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.

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.