From: Peter Krefting Date: Mon, 01 Apr 2024 15:50:32 GMT Subject: Re: [PATCH v2] bisect: Honor log.date Message-ID: In-Reply-To: <20240401023225.GA2639800@coredump.intra.peff.net> Junio C Hamano: > Yup, that is semi-understandable, but especially given that it is > one of the options used by the original "diff-tree"'s invocation, > and that we are trying to replace it with "show" from the same > family of commands, it is a bit of disappointment. Indeed. I will make the necessary adjustments. > FYI, attached is a comparison between the diff-tree output and > output from show with my choice of options for "show" picked from > the top of my head. I am trying to run some comparisons, but I'm not entirely certain what the parameters are that were passed to "ls-tree", as it doesn't actually run it through a command line. I tried the v1.0.0^0 and are seeing discrepancies in the line count. I need to check if it is my configuration that causes it, or something else: $ git diff-tree --pretty --stat --summary --cc v1.0.0^0 | grep clone-pack.c clone-pack.c | 153 ++---------------- $ git show --stat --summary --no-abbrev-commit v1.0.0^0 | grep clone-pack.c clone-pack.c | 151 ++---------------- (these are the options I've currently landed on) > I do not think I personally like the --stat output applied to a > merge (--stat and --summary do not work N-way like --cc does for > patch text), but I think these options are the closest parallel to > what we have been giving to "diff-tree". I don't really have a preference here. I usually only look at when something changed (which is why I initially targetted the date format; in Sweden the YYYY-MM-DD date format is the most prevalent) and the commit message (for bug tracker and code-review references and so on), less so the actual diff details (those I can look into later). > $ git show -s --stat --summary --first-parent v1.0.0^0 Hmm, the git show manual page doesn't document supporting "--first-parent". Jeff King: > I guess that commit is what brought me into the cc. I have not been > following this topic too closely, but generally I'm in favor of > using "git show". I even suggested it back then, but I think > Christian preferred not using an external process if we could avoid > it. I saw the code that tried to avoid calling one. I don't know the internals well enough here to figure out if we can do without, even when using git show? That made me realize, if "git show" runs things through a pager, wouldn't it then lose the "%s is the first %s commit\n" message printed by bisect_next_all() before calling the function to show the contents? Is that fixable? > The thread from 2019 is here: > > http://lore.kernel.org/git/20190222061949.GA9875@sigill.intra.peff.net > > which links to the earlier discussion about "git show": > > https://lore.kernel.org/git/CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com/ These two seems to also get it to honor settings (one to not colorize output, for instance). So this would be a step further. > I do think keeping --summary is important; it's the only place we show > mode changes, for example. Yes, will fix that. I hadn't realized I lost that, since it wasn't something I have been using myself. > - "--no-patch" is doing nothing (passing --stat is enough to suppress > the default behavior of showing the patch). Indeed. And it also negates "--summary", so I have dropped that. > - "--pretty=medium" is redundant at best (it's the default), Dropped. > - I'm not sure what the intent is in adding --no-abbrev-commit. It is > already the default not to abbreviate it in the "commit " line, > and if the user has set log.abbrevcommit, shouldn't we respect that? I think I added it because the diff-tree command did something similar. I can drop that as well ("bisect" displays the full commit hash anyway). I guess it mostly is for merges where we show the parent hashes? -- \\// Peter - http://www.softwolves.pp.se/