Re: [PATCH v6 2/6] reset: introduce show-new-head-line option

[Thomas Gummerer]

On 04/02, Junio C Hamano wrote:
> This is completely offtopic tangent, but I wonder how hidden-bool or
> hidden options[] element in general interacts with the recent
> addition of helping command line completion.  Are we already doing
> the right thing?

I had a quick look at this, and it looks like we're doing the right
thing here.  The following snipped from the 'show_gitcomp' function in
parse-options.c:


>   for (; opts->type != OPTION_END; opts++) {
>   const char *suffix = "";
>
>   if (!opts->long_name)
>   continue;
>   if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE))
>   continue;

So if the PARSE_OPT_HIDDEN flag is given, we skip printing the option,
which seems like the right thing to do.

Re: [PATCH v6 2/6] reset: introduce show-new-head-line option

[Thomas Gummerer]

On 04/02, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > +test_expect_success 'reset --no-show-new-head-line suppresses "HEAD is now 
> > at" output' '
> > +   git reset --hard --no-show-new-head-line HEAD >actual &&
> > +   ! grep "HEAD is now at"  > +'
> 
> As builtin/reset.c::print_new_head_line() does this:
> 
>   printf(_("HEAD is now at %s"), hex);
> 
> this needs to use "test_i18ngrep !" instead, no?

Ah yes you're right.  Thanks for catching this.

Re: [PATCH v6 2/6] reset: introduce show-new-head-line option

[Thomas Gummerer]

On 04/02, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > Introduce a new --show-new-head-line command line option, that
> > determines whether the "HEAD is now at ..." message is printed or not.
> > It is enabled by default to preserve the current behaviour.
> >
> > It will be used in a subsequent commit to disable printing the "HEAD is
> > now at ..." line in 'git worktree add', so it can be replaced with a
> > custom one, that explains the behaviour better.
> >
> > We cannot just pass the --quite flag from 'git worktree add', as that
> > would also hide progress output when checking out large working trees,
> > which is undesirable.
> >
> > As this option is only for internal use, which we probably don't want
> > to advertise to our users, at least until there's a need for it, make
> > it a hidden flag.
> 
> As a temporary hack, adding something like this may be OK, but in
> the longer run, I think we should (at least) consider refactoring
> "reset --hard" codepath to a freestanding and silent helper function
> so that both cmd_reset() and add_worktree() can call it and report
> the outcome in their own phrasing.

I agree that refactoring this would have been nicer.  The biggest
obstacle there is that the 'git reset' needs to run with a different
"GIT_DIR" and "GIT_WORK_TREE", which is not easy to accomplish in the
current code.

It does seem like something that would be much easier once we have
'struct repository' being passed through everywhere.  I'd rather wait
a bit more for the 'struct repository' series to land, which will
hopefully make the refactoring easier (although I didn't have time to
follow the series closely).

> And I support the decision not to advertise this as a new feature or
> an option by implementing it as hidden-bool.
> 
> This is completely offtopic tangent, but I wonder how hidden-bool or
> hidden options[] element in general interacts with the recent
> addition of helping command line completion.  Are we already doing
> the right thing?

Re: [PATCH v6 2/6] reset: introduce show-new-head-line option

[Junio C Hamano]

Thomas Gummerer  writes:

> +test_expect_success 'reset --no-show-new-head-line suppresses "HEAD is now 
> at" output' '
> + git reset --hard --no-show-new-head-line HEAD >actual &&
> + ! grep "HEAD is now at"  +'

As builtin/reset.c::print_new_head_line() does this:

printf(_("HEAD is now at %s"), hex);

this needs to use "test_i18ngrep !" instead, no?

Re: [PATCH v6 2/6] reset: introduce show-new-head-line option

[Junio C Hamano]

Thomas Gummerer  writes:

> Introduce a new --show-new-head-line command line option, that
> determines whether the "HEAD is now at ..." message is printed or not.
> It is enabled by default to preserve the current behaviour.
>
> It will be used in a subsequent commit to disable printing the "HEAD is
> now at ..." line in 'git worktree add', so it can be replaced with a
> custom one, that explains the behaviour better.
>
> We cannot just pass the --quite flag from 'git worktree add', as that
> would also hide progress output when checking out large working trees,
> which is undesirable.
>
> As this option is only for internal use, which we probably don't want
> to advertise to our users, at least until there's a need for it, make
> it a hidden flag.

As a temporary hack, adding something like this may be OK, but in
the longer run, I think we should (at least) consider refactoring
"reset --hard" codepath to a freestanding and silent helper function
so that both cmd_reset() and add_worktree() can call it and report
the outcome in their own phrasing.

And I support the decision not to advertise this as a new feature or
an option by implementing it as hidden-bool.

This is completely offtopic tangent, but I wonder how hidden-bool or
hidden options[] element in general interacts with the recent
addition of helping command line completion.  Are we already doing
the right thing?

[PATCH v6 2/6] reset: introduce show-new-head-line option

[Thomas Gummerer]

Introduce a new --show-new-head-line command line option, that
determines whether the "HEAD is now at ..." message is printed or not.
It is enabled by default to preserve the current behaviour.

It will be used in a subsequent commit to disable printing the "HEAD is
now at ..." line in 'git worktree add', so it can be replaced with a
custom one, that explains the behaviour better.

We cannot just pass the --quite flag from 'git worktree add', as that
would also hide progress output when checking out large working trees,
which is undesirable.

As this option is only for internal use, which we probably don't want
to advertise to our users, at least until there's a need for it, make
it a hidden flag.

Signed-off-by: Thomas Gummerer 
---
 builtin/reset.c  | 5 -
 t/t7102-reset.sh | 5 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index e15f595799..54b2576449 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -288,6 +288,7 @@ static int git_reset_config(const char *var, const char 
*value, void *cb)
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
int reset_type = NONE, update_ref_status = 0, quiet = 0;
+   int show_new_head_line = 1;
int patch_mode = 0, unborn;
const char *rev;
struct object_id oid;
@@ -310,6 +311,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
OPT_BOOL('p', "patch", _mode, N_("select hunks 
interactively")),
OPT_BOOL('N', "intent-to-add", _to_add,
N_("record only the fact that removed paths 
will be added later")),
+   OPT_HIDDEN_BOOL(0, "show-new-head-line", _new_head_line, 
N_("show information on the new HEAD")),
OPT_END()
};
 
@@ -403,7 +405,8 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
 * switched to, saving the previous head in ORIG_HEAD before. */
update_ref_status = reset_refs(rev, );
 
-   if (reset_type == HARD && !update_ref_status && !quiet)
+   if (reset_type == HARD && !update_ref_status && !quiet &&
+   show_new_head_line)
print_new_head_line(lookup_commit_reference());
}
if (!pathspec.nr)
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 95653a08ca..a160f78aba 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -568,4 +568,9 @@ test_expect_success 'reset --mixed sets up work tree' '
test_cmp expect actual
 '
 
+test_expect_success 'reset --no-show-new-head-line suppresses "HEAD is now at" 
output' '
+   git reset --hard --no-show-new-head-line HEAD >actual &&
+   ! grep "HEAD is now at"