Formatting options are ignored when tracking functions

Formatting options are ignored when tracking functions

[Aidan]

Git log with formatting options do not work when using -L:<funcname>:<file>

Works as intended for path
---------------------------
aidan@AidanLaptop:~/Repos/git$ git log --oneline -- advice.c
a20f70478f add: warn when asked to update SKIP_WORKTREE entries
3b990aa645 push: parse and set flag for "--force-if-includes"
c4a09cc9cc Merge branch 'hw/advise-ng'
---------------------------

Works as intended for revision range
---------------------------
aidan@AidanLaptop:~/Repos/git$ git log 91e70e00ac..HEAD --oneline
107691cb07 (HEAD -> master, origin/master, origin/HEAD) Merge branch 
'ds/sparse-index-protections'
2b8b1aa6ad Merge branch 'tz/c-locale-output-is-no-more'
c69f2f8c86 Merge branch 'cs/http-use-basic-after-failed-negotiate'
---------------------------

Does not work tracking a function
---------------------------
aidan@AidanLaptop:~/Repos/git$ git log -L:vadvise:advice.c --oneline
b3b18d1621 advice: revamp advise API
diff --git a/advice.c b/advice.c
--- a/advice.c
+++ b/advice.c
@@ -99,19 +141,23 @@
-static void vadvise(const char *advice, va_list params)
+static void vadvise(const char *advice, int display_instructions,
+                   const char *key, va_list params)
  {
         struct strbuf buf = STRBUF_INIT;
         const char *cp, *np;

         strbuf_vaddf(&buf, advice, params);

---------------------------

Is this a bug or am I invoking the command incorrectly?

Kind regards,
Aidan

Re: Formatting options are ignored when tracking functions

[Philippe Blain]

Hi Aidan,

I believe this is working as advertised: only the first line of the commit message is shown. 
However as mentioned in the doc, the -L option also triggers patch output (-p), 
which you can omit if you explicitely add --no-patch (or shorter, -s). 

Cheers,
Philippe. 

Re: Formatting options are ignored when tracking functions

[Bagas Sanjaya]

Hi Philippe,

On 22/05/21 08.58, Philippe Blain wrote:
> Hi Aidan,
> 
> I believe this is working as advertised: only the first line of the commit message is shown.
> However as mentioned in the doc, the -L option also triggers patch output (-p),
> which you can omit if you explicitely add --no-patch (or shorter, -s).
> 
> Cheers,
> Philippe.
> 

So the complete command for --oneline with -L in order to produce similar
output as other cases is `git log -L:<function>:<file> --oneline --no-patch`.

I wonder if we can make --oneline implies --no-patch when -L is given,
right?

-- 
An old man doll... just what I always wanted! - Clara

RE: Formatting options are ignored when tracking functions

[Felipe Contreras]

Aidan wrote:
> Does not work tracking a function
> ---------------------------
> aidan@AidanLaptop:~/Repos/git$ git log -L:vadvise:advice.c --oneline
> b3b18d1621 advice: revamp advise API
> diff --git a/advice.c b/advice.c
> --- a/advice.c
> +++ b/advice.c
> @@ -99,19 +141,23 @@
> -static void vadvise(const char *advice, va_list params)
> +static void vadvise(const char *advice, int display_instructions,
> +                   const char *key, va_list params)
>   {
>          struct strbuf buf = STRBUF_INIT;
>          const char *cp, *np;
> 
>          strbuf_vaddf(&buf, advice, params);
> 
> ---------------------------
> 
> Is this a bug or am I invoking the command incorrectly?

According to the documentation -L implies --patch, so you need to do:

  git log -L:vadvise:advice.c --oneline --no-patch

I would not expect such behavior so to me that's a bug.

-- 
Felipe Contreras

Re: Formatting options are ignored when tracking functions

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> I believe this is working as advertised: only the first line of
> the commit message is shown.
> However as mentioned in the doc, the -L option also triggers patch
> output (-p), which you can omit if you explicitely add --no-patch
> (or shorter, -s).

Heh, I think "working as advertised" is not wrong per-se, but this
feels like a clear design mistake to me, at least at the UI level.
Admittedly, I've never used "log -L" in scripts and I've always used
it interactively, in a context that I want to see the patch output,
so this did not bother me so far.

But what commits -L decides have relevant changes and how these
commits are shown ought to be orthogonal.  It surely may need to run
the content-level diff machinery to see if each commit affects the
area of the code specified via the -L option, but just like "git log
-S<text>" can be used to find commits that change the number of
occurrences of <text>, and allows the users to choose to view them
with "-p" (but not force the --patch mode on), it should behave the
same way, I would think.

With a clear migration plan, we should be able to fix this over
time, I would think.

Re: Formatting options are ignored when tracking functions

[Philippe Blain]

Hi Junio,

Le 2021-05-23 à 14:11, Junio C Hamano a écrit :
> Philippe Blain <levraiphilippeblain@gmail.com> writes:
> 
>> I believe this is working as advertised: only the first line of
>> the commit message is shown.
>> However as mentioned in the doc, the -L option also triggers patch
>> output (-p), which you can omit if you explicitely add --no-patch
>> (or shorter, -s).
> 
> Heh, I think "working as advertised" is not wrong per-se, but this
> feels like a clear design mistake to me, at least at the UI level.
> Admittedly, I've never used "log -L" in scripts and I've always used
> it interactively, in a context that I want to see the patch output,
> so this did not bother me so far.
> 
> But what commits -L decides have relevant changes and how these
> commits are shown ought to be orthogonal.  It surely may need to run
> the content-level diff machinery to see if each commit affects the
> area of the code specified via the -L option, but just like "git log
> -S<text>" can be used to find commits that change the number of
> occurrences of <text>, and allows the users to choose to view them
> with "-p" (but not force the --patch mode on), it should behave the
> same way, I would think.
> 
> With a clear migration plan, we should be able to fix this over
> time, I would think.
> 

I agree with that. I was just pointing out that it does work as the doc
says it should work, without implying anything about what should be the
behaviour in an ideal world.

In fact in an ideal world, '-L' would support all "kinds" of diff output,
i.e. --stat, --summary, etc.

I do not have a clear opinion on a migration path; if consensus can be reached
that not implying '--patch' is a better behaviour, then changing the behaviour
would be OK. If some people use scripts that parse 'git log -L' ouptut expecting
that '-p' is implied, I would expect it's pretty easy to notice the breakage and
add the now-required switch... but I'll let others be the judge of that.

Cheers,

Philippe.

Re: Formatting options are ignored when tracking functions

[Felipe Contreras]

Philippe Blain wrote:
> Le 2021-05-23 à 14:11, Junio C Hamano a écrit :
> I do not have a clear opinion on a migration path; if consensus can be reached
> that not implying '--patch' is a better behaviour, then changing the behaviour
> would be OK. If some people use scripts that parse 'git log -L' ouptut expecting
> that '-p' is implied, I would expect it's pretty easy to notice the breakage and
> add the now-required switch... but I'll let others be the judge of that.

I agree; it should be easy to notice the breakage. I don't think any
migration path would help, we should just switch. But the git project
has a tendency to worry about hypothetical users.

This is the only thing I can imagine doing for those users:

--- a/builtin/log.c
+++ b/builtin/log.c
@@ -262,8 +262,13 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
                load_ref_decorations(&decoration_filter, decoration_style);
        }
 
-       if (rev->line_level_traverse)
+       if (rev->line_level_traverse) {
+               if (!(rev->diffopt.output_format & DIFF_FORMAT_NO_OUTPUT) && !rev->diff) {
+                       advise("In the future you will need to specify --patch. Hit Enter to proceed.");
+                       getc(stdin);
+               }
                line_log_init(rev, line_cb.prefix, &line_cb.args);
+       }
 
        setup_pager();
 }

-- 
Felipe Contreras