* Re: [PATCH v2 5/5] ci: add support for GitLab CI
From: Phillip Wood @ 2023-10-27 13:17 UTC (permalink / raw)
To: Oswald Buddenhagen, Patrick Steinhardt; +Cc: git
In-Reply-To: <ZTuYjzOfG0y27Sho@ugly>
On 27/10/2023 12:01, Oswald Buddenhagen wrote:
> On Fri, Oct 27, 2023 at 11:25:41AM +0200, Patrick Steinhardt wrote:
>> + export GIT_PROVE_OPTS="--timer --jobs $(nproc)"
>> + export GIT_TEST_OPTS="--verbose-log -x"
>>
> fwiw (as this is again only copied), export with assignment is a
> bash-ism
Not according to
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#export
SYNOPSIS
export name[=word]...
DESCRIPTION
The shell shall give the export attribute to the variables
corresponding to the specified names, which shall cause them
to be in the environment of subsequently executed commands. If
the name of a variable is followed by = word, then the value
of that variable shall be set to word.
It is true that in our test suite we separate a variable assignment when
exporting. Presumably that is because someone reported that their shell
did not support the "export name=WORD" syntax in the past. As we're
already using this syntax with the same docker images in Github Actions
I think we can assume it is safe here.
Best Wishes
Phillip
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Marc Branchaud @ 2023-10-27 13:20 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Johannes Sixt, Junio C Hamano, Phillip Wood, phillip.wood, git
In-Reply-To: <ZTuw7ziOnTunMmML@ugly>
On 2023-10-27 08:45, Oswald Buddenhagen wrote:
> On Fri, Oct 27, 2023 at 08:34:40AM -0400, Marc Branchaud wrote:
>> I never use "fixup -C" (or -c), but I do use squash/fixup a lot. I
>> find that I would prefer it if Git used the most recent Author date
>> from the set of commits being combined, rather than preserving the
>> picked commit's Author date.
>>
> that would be unreliable, as plain amends wouldn't be reflected. that
> may be rare in your workflow, but still.
I'm not talking about amends, plain or otherwise. I'm talking about
fixup/squash.
(Why do you focus so much an making rebase and commit behave
identically? There is no reason to do so just because they happen to
share a couple of parameter names.)
>> Sometimes it takes quite a while for me to get a piece of work sorted
>> out, and I would rather have the Author date in the end-result commit
>> reflect the work's completion time than its initiation time.
>>
> afaict, you need to get used to `--amend --reset-author` all commits
> before you push to achieve this reliably. that can be easily automated
> by using -x with rebase -i (filter-repo (ex filter-branch) would also
> work).
Yes, I know how to force my desired author date on commits, thanks.
M.
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Oswald Buddenhagen @ 2023-10-27 13:26 UTC (permalink / raw)
To: Marc Branchaud
Cc: Johannes Sixt, Junio C Hamano, Phillip Wood, phillip.wood, git
In-Reply-To: <6d100655-ffd4-4282-87b5-cfdd101dba63@xiplink.com>
On Fri, Oct 27, 2023 at 09:20:04AM -0400, Marc Branchaud wrote:
>
>On 2023-10-27 08:45, Oswald Buddenhagen wrote:
>> On Fri, Oct 27, 2023 at 08:34:40AM -0400, Marc Branchaud wrote:
>>> I never use "fixup -C" (or -c), but I do use squash/fixup a lot. I
>>> find that I would prefer it if Git used the most recent Author date
>>> from the set of commits being combined, rather than preserving the
>>> picked commit's Author date.
>>>
>> that would be unreliable, as plain amends wouldn't be reflected. that
>> may be rare in your workflow, but still.
>
>I'm not talking about amends, plain or otherwise.
>
but why wouldn't you? your use case of marking the date of completion
naturally covers all ways of amending commits, whether directly or via
squashing.
regards
^ permalink raw reply
* Re: [RFC PATCH v2 0/6] Noobify format for status, add, restore
From: Dragan Simic @ 2023-10-27 13:32 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <20231026224615.675172-1-jacob@initialcommit.io>
On 2023-10-27 00:46, Jacob Stopak wrote:
> Take into account reviewer feedback by doing several things
> differently:
>
> * Rename this feature (for now) as "noob format mode" (or just "noob
> mode") instead of the original "--table" verbiage. As pointed out,
> this no longer ties the name of the setting to it's proposed
> implementation detail as a table. Noob mode is not necessarily the
> right name, just a placeholder for now. Unless people like it :D
>
> * Instead of manually having to invoke the -t, --table every time
> this
> format is to be used, set the config option "status.noob" to true.
> Although this is logically tied to the status command, there are
> many
> commands that produce status output, (and this series adds more),
> so
> assume that if the user wants to see the status this way, that it
> should be enabled whenever the status info is displayed.
How would "status.noob" relate to and coexist with possible future
configuration options named "<command>.verbose", which would be somewhat
similar to the currently existing "commit.verbose" option? IOW, perhaps
it would be better to have per-command options "<command>.verbose =
noob" or, even better, "<command>.verbose = extended", to make it all
more future-proof and more granular.
> * When running "git add" and "git restore" while noob mode is
> enabled,
> perform the add/restore function as usual, but display the table
> formatted output with arrows showing how file changes moved around.
> Displaying the output in this understandable format after each
> command execution allows the noob to immediately see what they did.
> Although this series only implements for status, add, and restore,
> this output format would make sense in other commands like rm, mv,
> commit, clean, and stash.
>
> * Works consistently with commands that already have a --dry-run
> (-n) option. The dry run shows the exact same output, but
> doesn't actually do the thing.
>
> * If `advice.statusHints` is true, add a table footer with status
> hints.
> Shorten these hints so that they are still clear but better fit
> into a
> table. Make the hint text yellow to distinguish them. The hints
> only
> appear when explicitly running "git status", which helps the user
> answer the question "what can I do next?". Hints are omitted in
> "impact" commands like add and restore. Having hints here distracts
> from the file change moves being showed in the table by arrows.
>
> TODO:
>
> * "git status" outputs myriad other information depending on the
> state
> of the repo, like branch info, merge conflicts, rebase info,
> bisect,
> etc. Need to think about how to convey that info with the new
> setting.
>
> * Some commands (like stash) might need more than 3 table columns to
> display everything clearly.
>
> * For destructive commands, think about adding a prompt describing
> the
> effect, so the user can confirm before the action is taken.
>
> * Fix horrible things in the patch series code.
>
> * Probably other things.
>
> Play around with it! It's fun!
>
> Jacob Stopak (6):
> status: add noob format from status.noob config
> status: handle long paths in noob format
> add: implement noob mode
> add: set unique color for noob mode arrows
> restore: implement noob mode
> status: add advice status hints as table footer
>
> Makefile | 2 +
> builtin/add.c | 47 +++++--
> builtin/checkout.c | 46 +++++--
> builtin/commit.c | 157 +----------------------
> commit.c | 2 +
> noob.c | 198 +++++++++++++++++++++++++++++
> noob.h | 21 ++++
> read-cache-ll.h | 10 +-
> read-cache.c | 41 +++++-
> table.c | 301 +++++++++++++++++++++++++++++++++++++++++++++
> table.h | 6 +
> wt-status.c | 75 +++++++----
> wt-status.h | 6 +
> 13 files changed, 708 insertions(+), 204 deletions(-)
> create mode 100644 noob.c
> create mode 100644 noob.h
> create mode 100644 table.c
> create mode 100644 table.h
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Marc Branchaud @ 2023-10-27 13:46 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Johannes Sixt, Junio C Hamano, Phillip Wood, phillip.wood, git
In-Reply-To: <ZTu6cqUec3L2PpUC@ugly>
On 2023-10-27 09:26, Oswald Buddenhagen wrote:
> On Fri, Oct 27, 2023 at 09:20:04AM -0400, Marc Branchaud wrote:
>>
>> On 2023-10-27 08:45, Oswald Buddenhagen wrote:
>>> On Fri, Oct 27, 2023 at 08:34:40AM -0400, Marc Branchaud wrote:
>>>> I never use "fixup -C" (or -c), but I do use squash/fixup a lot. I
>>>> find that I would prefer it if Git used the most recent Author date
>>>> from the set of commits being combined, rather than preserving the
>>>> picked commit's Author date.
>>>>
>>> that would be unreliable, as plain amends wouldn't be reflected. that
>>> may be rare in your workflow, but still.
>>
>> I'm not talking about amends, plain or otherwise.
>>
> but why wouldn't you? your use case of marking the date of completion
> naturally covers all ways of amending commits, whether directly or via
> squashing.
Please do not presume what my use cases might be. I'm quite happy with
commit's behaviour, but not happy with rebase's fixup/squash behaviour
because it's too much work to achieve the desired results. (Results
which, as I said, I don't care about enough to bother changing anyway).
M.
^ permalink raw reply
* Re: [PATCH 5/5] ci: add support for GitLab CI
From: Phillip Wood @ 2023-10-27 14:32 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Patrick Steinhardt, git
In-Reply-To: <ZTuUa5bA8dh29cyv@ugly>
On 27/10/2023 11:43, Oswald Buddenhagen wrote:
> On Fri, Oct 27, 2023 at 11:22:35AM +0100, Phillip Wood wrote:
>> On 27/10/2023 09:17, Patrick Steinhardt wrote:
>>>>> + CI_BRANCH="$CI_COMMIT_REF_NAME"
>>>>> + CI_COMMIT="$CI_COMMIT_SHA"
>>>>>
>>>> assignments need no quoting to prevent word splitting.
>>>> repeats below.
>>>>
>>>>> + case "$CI_JOB_IMAGE" in
>>>>>
>>>> ... as does the selector in case statements.
>>>
>>> True, but I'm simply matching the coding style in this script.
>>
>> I think it is quite common for us to quote variables when it isn't
>> strictly necessary as it makes it clear to anyone reading the script
>> that there is no word splitting going on
>
>> and ensures that we don't start splitting the variable if the contents
>> changes in the future.
>>
> the point was that it *isn't* content-dependent; it's simply the shell
> rules.
Oh, I'd misunderstood what you were saying which was that assignment and
case statements are not subject to field splitting.
> of course, many people (apparently you included) don't know these
> subtleties
I find this comment to be condescending, needlessly antagonistic and
completely at odds with the norms of constructive discussion on this list.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v3] bugreport: reject positional arguments
From: Phillip Wood @ 2023-10-27 14:41 UTC (permalink / raw)
To: Eric Sunshine, emilyshaffer; +Cc: git, Emily Shaffer, Sheik, Dragan Simic
In-Reply-To: <CAPig+cTmYtWR=QN3LeN9yw3HmsKEmD2fUiRjKf=eJHhAZyT-yA@mail.gmail.com>
On 26/10/2023 18:18, Eric Sunshine wrote:
> On Thu, Oct 26, 2023 at 11:55 AM <emilyshaffer@google.com> wrote:
>> git-bugreport already rejected unrecognized flag arguments, like
>> `--diaggnose`, but this doesn't help if the user's mistake was to forget
>> the `--` in front of the argument. This can result in a user's intended
>> argument not being parsed with no indication to the user that something
>> went wrong. Since git-bugreport presently doesn't take any positionals
>> at all, let's reject all positionals and give the user a usage hint.
>>
>> Signed-off-by: Emily Shaffer <nasamuffin@google.com>
>> ---
>> Per Eric's and Dragan's comments, dropped the null checking for argv[0].
>> No point in being too paranoid, I suppose :)
>>
>> Note that after this morning it's not likely that I'll be able to find
>> time to update this again so quickly, so if there are other nits,
>> reviewers can feel free to send their own rerolls rather than waiting
>> for me to see it and turn the patch around.
>
> Thanks. This version looks good enough to me. Just one minor comment below...
>
>> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
>> @@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' '
>> +test_expect_success 'incorrect positional arguments abort with usage and hint' '
>> + test_must_fail git bugreport false 2>output &&
>> + test_i18ngrep usage output &&
>> + test_i18ngrep false output &&
>> + test_path_is_missing git-bugreport-*
>> +'
>
> I didn't really pay attention to the test in earlier rounds so didn't
> notice this, but these days we just use 'grep' rather than
> 'test_i18ngrep'. (Indeed, the existing tests in this script use
> 'grep'.)
It is rather unfortunate that test_i18ngrep was deprecated without
providing an alternative that offers the same debugging experience. When
test_i18ngrep fails it prints a message with the pattern and text that
failed to match so it is easy to see where the test failed. If grep
fails there is no output and so unless the test is run with "-x" it can
be hard to see which command caused the test to fail.
Best Wishes
Phillip
^ permalink raw reply
* Re: Constant recompilation when GENERATE_COMPILATION_DATABASE is set
From: Philippe Blain @ 2023-10-27 15:41 UTC (permalink / raw)
To: brian m. carlson, git
In-Reply-To: <ZTui2NcJ3Ax_PIGO@tapette.crustytoothpaste.net>
Hi brian,
Le 2023-10-27 à 07:45, brian m. carlson a écrit :
> I typically use clangd with Git for the nice language server protocol
> (LSP) support. I noticed that when GENERATE_COMPILATION_DATABASE is set
> (which is used so clangd can find the proper flags), `make` rebuilds all
> of the files every time, even if I do it back to back. This persists
> even after a `git clean -dxf`.
>
> Obviously, this is not great, since it means the turnaround time to
> compile changes is slower. Unfortunately, I'm not a huge expert in make
> and I'm unsure what the right solution is here. I'd appreciate anyone's
> thoughts on how to improve this.
>
> This is on a Debian sid (unstable) amd64 system with the following
> configuration. The version of clang in use is Debian's 16.0.6.
>
> ----
> NO_OPENSSL=1
> DC_SHA1=1
> NETTLE_SHA256=1
> DEVELOPER=1
> NO_SVN_TESTS=1
> NO_PYTHON=1
> USE_ASCIIDOCTOR=1
> USE_ASCIIDOCTOR_MANPAGE=1
> CC=clang
> GENERATE_COMPILATION_DATABASE=yes
> CSPRNG_METHOD=getrandom getentropy
> ----
>
Indeed, with GENERATE_COMPILATION_DATABASE=yes and CC=clang I can reproduce with:
make clean
make -j
make -j # this rebuilds everything
make -j # this does not rebuild anything
so the second 'make' invocation is buggy, but not subsequent ones.
I took a look at the 'make -d' output and we can see it is the dependency on
the 'compile_commands' directory that is the culprit, on the second invocation
it is newer than the the first object file that makes checks.
This makes sense since each time we compile an object we put the JSON fragment in
compile_commands, so the directory is updated each time an object is built, so
clearly its modification date will be newer than the one of the first object built.
One way to fix this is to use the same trick that is used for the '.depend' directories
created to hold the Makefile dependencies:
diff --git a/Makefile b/Makefile
index 5776309365..f6f7255dd1 100644
--- a/Makefile
+++ b/Makefile
@@ -2701,7 +2701,7 @@ endif
compdb_dir = compile_commands
ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
-missing_compdb_dir = $(compdb_dir)
+missing_compdb_dir = $(filter-out $(wildcard $(compdb_dir)), $(compdb_dir))
$(missing_compdb_dir):
@mkdir -p $@
That is, we define missing_compdb_dir to 'compile_commands' only if the directory
is not yet created.
I'll try to send a proper patch for this.
Thanks for the report,
Philippe.
^ permalink raw reply related
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Junio C Hamano @ 2023-10-27 15:44 UTC (permalink / raw)
To: Todd Zullinger
Cc: Michael Strawbridge, Jeff King, Uwe Kleine-König,
Luben Tuikov, git, entwicklung
In-Reply-To: <ZTp7xvXDw1GF-NUB@pobox.com>
Todd Zullinger <tmz@pobox.com> writes:
> Alternately, perhaps having Email::Valid as an optional
> dependency is worth reconsidering. If it's truly important
> to validation, make it a requirement. If it's not, then
> drop it to simplify the code and avoid these sort of issues.
Reducing the possible "valid" configurations we support is a very
tempting proposition.
> If I make the git package require it to ensure consistent
> behavior then some folks will -quite rightly- complain that
> it should not be a requirement. If I keep it an optional
> dependency, then debugging becomes more difficult for the
> reasons we've seen in these recent (and not-so-recent)
> threads.
Very true.
> I'd lean toward dropping the dependency entirely and leave
> the more basic validation of git-send-email in place. That
> may not catch every type of address error, but I would argue
> that what we do without Email::Valid is perfectly reasonable
> for checking basic email address syntax sanity.
Yes, it is very tempting, and given that we have to keep our
fallback codepath working for those without Email::Valid ANYWAY,
as long as the dependency is merely optional, I very much agree
with your argument here.
> On a related note, one issue¹ we had reported in Fedora
> after making Email::Valid a requirement was that it rejected
> messages where the local part was too long, per the relevant
> RFC's. But these were generated addresses from GitLab. The
> addresses worked in practice. While Email::Valid was
> technically correct in rejecting such addresses, it didn't
> improve the experience of git send-email users.
I am of two minds here. I can sympathize with both positions.
- Trying to be strict to what we send out to the world by using
Email::Valid that tries to be more RFC kosher matches "be strict
in what you send out, be lenient in what you receive" mantra
- Rejecting what works in practice and in real world tend to help
users.
If we require Email::Valid, then sriking the balance between the
above two will entirely become the responsibility of them; any
end-user who complains "the validation is overly strict" will get
"talk to authors of Email::Valid".
If we ditch Email::Valid, it will become _our_ responsibility, which
means a bit of extra maintenance burden to this project. But perhaps
it is worth it? I dunno.
Having Email::Valid as an optional dependency does not place us in a
position better than either of these two, so from that point of view,
too, I like your "we should either make it required or unused, not
an optional dependency" very much.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 5/5] ci: add support for GitLab CI
From: Oswald Buddenhagen @ 2023-10-27 15:53 UTC (permalink / raw)
To: phillip.wood; +Cc: Patrick Steinhardt, git
In-Reply-To: <d00b02e9-fb05-44bc-90ee-1851ef98dd26@gmail.com>
On Fri, Oct 27, 2023 at 02:17:02PM +0100, Phillip Wood wrote:
>On 27/10/2023 12:01, Oswald Buddenhagen wrote:
>> On Fri, Oct 27, 2023 at 11:25:41AM +0200, Patrick Steinhardt wrote:
>>> + export GIT_PROVE_OPTS="--timer --jobs $(nproc)"
>>> + export GIT_TEST_OPTS="--verbose-log -x"
>>>
>> fwiw (as this is again only copied), export with assignment is a
>> bash-ism
>
>Not according to
>https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#export
>
hmm, it's there since at least SUSv1, aka XPG4v2, in 1994. i didn't
bother digging deeper.
>It is true that in our test suite we separate a variable assignment
>when exporting. Presumably that is because someone reported that their
>shell did not support the "export name=WORD" syntax in the past.
>
most likely it's just a historical default, not the result of a specific
bug report. the assumption that it's bash-specific is wide-spread.
>As we're already using this syntax with the same docker images in
>Github Actions I think we can assume it is safe here.
>
i guess so. docker seems a tad unlikely to run some ancient bourne
shells ...
regards
^ permalink raw reply
* Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
From: Junio C Hamano @ 2023-10-27 15:54 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Kristoffer Haugsbakk, Taylor Blau, git, stolee
In-Reply-To: <ZTjMMC1GiPJUXnQm@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> This parameter would only exist for the purpose of the error message,
> right? If so, I think that'd be overkill. If we want to have differing
> errors depending on how the function is called the best way to handle
> that would likely be to generate the error message at the callsite
> instead of in the library itself.
We would need to make sure the lower-level helpers need to be able
to tell what kind of failure they saw (in other words, why they are
failing) to the callers, which may require a bit of designing the
error return convention and plumbing through necessary pieces of
information, but the longer term payoff would be great.
I do not think this is such a case, but if the lower-level needs to
fail differently (e.g., the thing not existing is acceptable when
writing as we will create a new one, but is a fail-worthy error when
reading), then the caller needs to give that down the callchain,
though.
^ permalink raw reply
* Re: [PATCH v3] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-27 16:12 UTC (permalink / raw)
To: Marc Branchaud
Cc: git, Junio C Hamano, Phillip Wood, Taylor Blau, Christian Couder,
Charvi Mendiratta
In-Reply-To: <56e3e974-a027-439f-871d-c7fbae65a04e@xiplink.com>
On Fri, Oct 27, 2023 at 09:14:42AM -0400, Marc Branchaud wrote:
>On 2023-10-25 06:29, Oswald Buddenhagen wrote:
>> The behavior in the presence of multiple "fixup -c" is somewhat
>> questionable, as arguably it would be better to complain about it rather
>> than letting the last instance win. But for the time being we document
>> the status quo, with a note that it is not guaranteed. Note that
>> actually changing it would require --autosquash eliding the superseded
>> uses.
>
>I do not think this kind of editorializing belongs in the commit's
>message, but this likely isn't the first commit message that expresses
>an opinion.
>
commmit messages should elaborate alternatives considered, which
includes ones which depend on changes that can be reasonably expected to
possibly happen at some point.
>But I think you should remove the "but this should not be relied upon"
>phrase. This reads as if Git's current behaviour is undefined, which
>most definitely is not true.
>
>Even changing this to something like "but this might change in the
>future" is unhelpful. Everything in Git is subject to change over a
>long-enough time span, so the same could be said about every aspect of Git.
>
>Until the behaviour actually changes, it's perfectly fine for people to
>use multiple "fixup -c" commands. There's no reason to scare them off
>of it.
>
things can't change overnight; the resistance even the most trivial
behavior changes meet is enormous. so explicitly documenting long in
advance that something is subject to change is basically the only way to
get it changed at all.
specifically for this feature, there is no reason at all to rely on this
behavior when hand-editing the todo list, and occurrences most likely
indicate a mistake, which is why i would prefer it to be rejected.
regards
^ permalink raw reply
* Re: [RFC PATCH v2 0/6] Noobify format for status, add, restore
From: Jacob Stopak @ 2023-10-27 17:13 UTC (permalink / raw)
To: Dragan Simic; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <ca47d328c280e4b4c13bfa6dd9958a57@manjaro.org>
On Fri, Oct 27, 2023 at 03:32:40PM +0200, Dragan Simic wrote:
> On 2023-10-27 00:46, Jacob Stopak wrote:
> > Take into account reviewer feedback by doing several things differently:
> >
> > * Rename this feature (for now) as "noob format mode" (or just "noob
> > mode") instead of the original "--table" verbiage. As pointed out,
> > this no longer ties the name of the setting to it's proposed
> > implementation detail as a table. Noob mode is not necessarily the
> > right name, just a placeholder for now. Unless people like it :D
> >
> > * Instead of manually having to invoke the -t, --table every time this
> > format is to be used, set the config option "status.noob" to true.
> > Although this is logically tied to the status command, there are
> > many
> > commands that produce status output, (and this series adds more), so
> > assume that if the user wants to see the status this way, that it
> > should be enabled whenever the status info is displayed.
>
> How would "status.noob" relate to and coexist with possible future
> configuration options named "<command>.verbose", which would be somewhat
> similar to the currently existing "commit.verbose" option? IOW, perhaps it
> would be better to have per-command options "<command>.verbose = noob" or,
> even better, "<command>.verbose = extended", to make it all more
> future-proof and more granular.
Hmm, do there currently exist other <command>.verbose config settings
besides for commit? From what I can tell from "git help config", the
commit.verbose setting is the only one I see, and it just adds the diff
info into the editor if the user runs git commit without the -m flag, but
otherwise there seems to be no extra verbosity outputted.
I noticed that git add and git mv have "verbose" (-v, --verbose) cli flags
which just output the name of the file being added or renamed, and that
certain other commands like git branch has a verbose output which includes
the branch head commit hash and message in the output, so I guess this one
is actually kindof verbose in that it outputs more than the non-empty
default output.
So it seems like currently "verbose" is used for various things among the
command set, sometimes meaning "add something into the template if one is
used" or "add some tiny output to a command that has no default output"
(which still seems more "--shy" than "--verbose" :P) or "add some
additional output to a command that already has some sparse output".
Another thing is that commands like status have multiple flags that can be
used to specify the output format, such as --short, --long, --porcelain,
etc, but only --short seems to be configurable as a git config setting.
Is there a reason (besides backward compatibility I guess) that these
aren't rolled into a single thing like --format=<type>? This seems like
it would be the easiest way to future proof for new formats like
--format=verbose, --format=noob, --format=extended, etc.
From a noob's perspective though, does adding a config setting for each
command really make sense? I'm kindof envisioning this setting now as a
"mode" that is either enabled for all commands it affects or for none.
And it's highly unlikely a newish user would individually discover which
commands this "extended" format is available for, and run "git config
<command>.verbose = extended" for every one. I mean we could do that
in case there are folks who only want it for specific commands, but to
fulfill it's purpose I think there should definetely be some general way
to enable the setting for all commands that have it.
^ permalink raw reply
* Re: [PATCH 5/5] ci: add support for GitLab CI
From: Oswald Buddenhagen @ 2023-10-27 17:47 UTC (permalink / raw)
To: Phillip Wood; +Cc: Patrick Steinhardt, git
In-Reply-To: <b6ef74b7-2c77-4621-9969-d911c34561d5@crinan.ddns.net>
On Fri, Oct 27, 2023 at 03:32:48PM +0100, Phillip Wood wrote:
>On 27/10/2023 11:43, Oswald Buddenhagen wrote:
>> On Fri, Oct 27, 2023 at 11:22:35AM +0100, Phillip Wood wrote:
>>>>>> + CI_BRANCH="$CI_COMMIT_REF_NAME"
>>>>>> + CI_COMMIT="$CI_COMMIT_SHA"
>>>>>>
>>>>> assignments need no quoting to prevent word splitting.
>>>>> repeats below.
>>>>>
>>> I think it is quite common for us to quote variables when it isn't
>>> strictly necessary as it makes it clear to anyone reading the script
>>> that there is no word splitting going on
>>
>>> and ensures that we don't start splitting the variable if the contents
>>> changes in the future.
>>>
>> the point was that it *isn't* content-dependent; it's simply the shell
>> rules.
>
>Oh, I'd misunderstood what you were saying which was that assignment and
>case statements are not subject to field splitting.
>
>> of course, many people (apparently you included) don't know these
>> subtleties
>
>I find this comment to be condescending, needlessly antagonistic and
>completely at odds with the norms of constructive discussion on this list.
>
the observation was necessary for the point i subsequently made (which
was basically agreeing with the first part of your response).
i think it's a rather uncontroversial statement that many people don't
know the ins and outs of the shell's word splitting rules. in fact, this
is to be expected given how arbitrary the rules seem.
that you seem to be among these people was a reasonable inference from
what you actually wrote. mentioning it was meant to provide a segue and
add emphasis.
regards
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Kousik Sanagavarapu @ 2023-10-27 18:40 UTC (permalink / raw)
To: Liam Beguin; +Cc: git
In-Reply-To: <20231026-pretty-email-domain-v1-2-5d6bfa6615c0@gmail.com>
Hi Liam,
Liam Beguin <liambeguin@gmail.com> wrote:
> Subject: Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
Since we are adding both '%aa' and '%aA', it would be better to
to include both in the commit subject, but since it is already long
enough, in my opinion
pretty: add formats for domain-part of email address
would convey the gist of the commit to the reader better.
> Many reports use the email domain to keep track of organizations
> contributing to projects.
> Add support for formatting the domain-part of a contributor's address so
> that this can be done using git itself, with something like:
>
> git shortlog -sn --group=format:%aA v2.41.0..v2.42.0
>
> Signed-off-by: Liam Beguin <liambeguin@gmail.com>
A very very very minor nit but the commit message would read better as
... contributing to projects, so add support for ...
Feel free to ignore it.
> ---
> Documentation/pretty-formats.txt | 6 ++++++
> pretty.c | 13 ++++++++++++-
> t/t4203-mailmap.sh | 28 ++++++++++++++++++++++++++++
> t/t6006-rev-list-format.sh | 6 ++++--
> 4 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index a22f6fceecdd..72102a681c3a 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -195,6 +195,9 @@ The placeholders are:
> '%al':: author email local-part (the part before the '@' sign)
> '%aL':: author email local-part (see '%al') respecting .mailmap, see
> linkgit:git-shortlog[1] or linkgit:git-blame[1])
> +'%aa':: author email domain-part (the part after the '@' sign)
> +'%aA':: author email domain-part (see '%al') respecting .mailmap, see
> + linkgit:git-shortlog[1] or linkgit:git-blame[1])
> '%ad':: author date (format respects --date= option)
> '%aD':: author date, RFC2822 style
> '%ar':: author date, relative
> @@ -213,6 +216,9 @@ The placeholders are:
> '%cl':: committer email local-part (the part before the '@' sign)
> '%cL':: committer email local-part (see '%cl') respecting .mailmap, see
> linkgit:git-shortlog[1] or linkgit:git-blame[1])
> +'%ca':: committer email domain-part (the part before the '@' sign)
> +'%cA':: committer email domain-part (see '%cl') respecting .mailmap, see
> + linkgit:git-shortlog[1] or linkgit:git-blame[1])
> '%cd':: committer date (format respects --date= option)
> '%cD':: committer date, RFC2822 style
> '%cr':: committer date, relative
> diff --git a/pretty.c b/pretty.c
> index cf964b060cd1..4f5d081589ea 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -791,7 +791,7 @@ static size_t format_person_part(struct strbuf *sb, char part,
> mail = s.mail_begin;
> maillen = s.mail_end - s.mail_begin;
>
> - if (part == 'N' || part == 'E' || part == 'L') /* mailmap lookup */
> + if (part == 'N' || part == 'E' || part == 'L' || part == 'A') /* mailmap lookup */
> mailmap_name(&mail, &maillen, &name, &namelen);
> if (part == 'n' || part == 'N') { /* name */
> strbuf_add(sb, name, namelen);
> @@ -808,6 +808,17 @@ static size_t format_person_part(struct strbuf *sb, char part,
> strbuf_add(sb, mail, maillen);
> return placeholder_len;
> }
> + if (part == 'a' || part == 'A') { /* domain-part */
> + const char *at = memchr(mail, '@', maillen);
> + if (at) {
> + at += 1;
> + maillen -= at - mail;
> + strbuf_add(sb, at, maillen);
> + } else {
> + strbuf_add(sb, mail, maillen);
> + }
> + return placeholder_len;
> + }
>
> if (!s.date_begin)
> goto skip;
So, if we have a domain-name, we grab it, else (the case where we don't
have '@') we grab it as-is. Looks good.
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 2016132f5161..35bf7bb05bea 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -624,6 +624,34 @@ test_expect_success 'Log output (local-part email address)' '
> test_cmp expect actual
> '
>
> +test_expect_success 'Log output (domain-part email address)' '
> + cat >expect <<-EOF &&
> + Author email cto@coompany.xx has domain-part coompany.xx
> + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> +
> + Author email me@company.xx has domain-part company.xx
> + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> +
> + Author email me@company.xx has domain-part company.xx
> + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> +
> + Author email nick2@company.xx has domain-part company.xx
> + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> +
> + Author email bugs@company.xx has domain-part company.xx
> + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> +
> + Author email bugs@company.xx has domain-part company.xx
> + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> +
> + Author email author@example.com has domain-part example.com
> + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> + EOF
> +
> + git log --pretty=format:"Author email %ae has domain-part %aa%nCommitter email %ce has domain-part %ca%n" >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'Log output with --use-mailmap' '
> test_config mailmap.file complex.map &&
>
> diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
> index 573eb97a0f7f..34c686becf2d 100755
> --- a/t/t6006-rev-list-format.sh
> +++ b/t/t6006-rev-list-format.sh
> @@ -163,11 +163,12 @@ commit $head1
> EOF
>
> # we don't test relative here
> -test_format author %an%n%ae%n%al%n%ad%n%aD%n%at <<EOF
> +test_format author %an%n%ae%n%al%aa%n%ad%n%aD%n%at <<EOF
> commit $head2
> $GIT_AUTHOR_NAME
> $GIT_AUTHOR_EMAIL
> $TEST_AUTHOR_LOCALNAME
> +$TEST_AUTHOR_DOMAIN
> Thu Apr 7 15:13:13 2005 -0700
> Thu, 7 Apr 2005 15:13:13 -0700
> 1112911993
> @@ -180,11 +181,12 @@ Thu, 7 Apr 2005 15:13:13 -0700
> 1112911993
> EOF
>
> -test_format committer %cn%n%ce%n%cl%n%cd%n%cD%n%ct <<EOF
> +test_format committer %cn%n%ce%n%cl%ca%n%cd%n%cD%n%ct <<EOF
> commit $head2
> $GIT_COMMITTER_NAME
> $GIT_COMMITTER_EMAIL
> $TEST_COMMITTER_LOCALNAME
> +$TEST_COMMITTER_DOMAIN
> Thu Apr 7 15:13:13 2005 -0700
> Thu, 7 Apr 2005 15:13:13 -0700
> 1112911993
>
> --
> 2.39.0
The tests look good too.
I should say I'm skeptical of the new format's name though. I know '%ad' is
taken... but maybe it's just me.
Thanks
^ permalink raw reply
* Re: Constant recompilation when GENERATE_COMPILATION_DATABASE is set
From: brian m. carlson @ 2023-10-27 19:36 UTC (permalink / raw)
To: Philippe Blain; +Cc: git
In-Reply-To: <76c5f401-0b29-b19a-8424-5bca5df2252e@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 858 bytes --]
On 2023-10-27 at 15:41:23, Philippe Blain wrote:
> One way to fix this is to use the same trick that is used for the '.depend' directories
> created to hold the Makefile dependencies:
>
> diff --git a/Makefile b/Makefile
> index 5776309365..f6f7255dd1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2701,7 +2701,7 @@ endif
> compdb_dir = compile_commands
>
> ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
> -missing_compdb_dir = $(compdb_dir)
> +missing_compdb_dir = $(filter-out $(wildcard $(compdb_dir)), $(compdb_dir))
> $(missing_compdb_dir):
> @mkdir -p $@
>
> That is, we define missing_compdb_dir to 'compile_commands' only if the directory
> is not yet created.
I can confirm that this solves my problem very nicely. Thanks for such
a fast response.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH v8 1/3] unit tests: Add a project plan document
From: Christian Couder @ 2023-10-27 20:12 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, phillip.wood123, linusa, calvinwan, gitster, rsbecker
In-Reply-To: <81c5148a1267b8f9ce432a950340f0fa16b4d773.1696889530.git.steadmon@google.com>
On Tue, Oct 10, 2023 at 12:22 AM Josh Steadmon <steadmon@google.com> wrote:
>
> In our current testing environment, we spend a significant amount of
> effort crafting end-to-end tests for error conditions that could easily
> be captured by unit tests (or we simply forgo some hard-to-setup and
> rare error conditions). Describe what we hope to accomplish by
> implementing unit tests, and explain some open questions and milestones.
> Discuss desired features for test frameworks/harnesses, and provide a
> preliminary comparison of several different frameworks.
Nit: Not sure why the test framework comparison is "preliminary" as we
have actually selected a unit test framework and are adding it in the
next patch of the series. I understand that this was perhaps written
before the choice was made, but maybe we might want to update that
now.
> diff --git a/Documentation/technical/unit-tests.txt b/Documentation/technical/unit-tests.txt
> new file mode 100644
> index 0000000000..b7a89cc838
> --- /dev/null
> +++ b/Documentation/technical/unit-tests.txt
> @@ -0,0 +1,220 @@
> += Unit Testing
> +
> +In our current testing environment, we spend a significant amount of effort
> +crafting end-to-end tests for error conditions that could easily be captured by
> +unit tests (or we simply forgo some hard-to-setup and rare error conditions).
> +Unit tests additionally provide stability to the codebase and can simplify
> +debugging through isolation. Writing unit tests in pure C, rather than with our
> +current shell/test-tool helper setup, simplifies test setup, simplifies passing
> +data around (no shell-isms required), and reduces testing runtime by not
> +spawning a separate process for every test invocation.
> +
> +We believe that a large body of unit tests, living alongside the existing test
> +suite, will improve code quality for the Git project.
I agree with that.
> +== Choosing a framework
> +
> +We believe the best option is to implement a custom TAP framework for the Git
> +project. We use a version of the framework originally proposed in
> +https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[1].
Nit: Logically I would think that our opinion should come after the
comparison and be backed by it.
> +== Choosing a test harness
> +
> +During upstream discussion, it was occasionally noted that `prove` provides many
> +convenient features, such as scheduling slower tests first, or re-running
> +previously failed tests.
> +
> +While we already support the use of `prove` as a test harness for the shell
> +tests, it is not strictly required. The t/Makefile allows running shell tests
> +directly (though with interleaved output if parallelism is enabled). Git
> +developers who wish to use `prove` as a more advanced harness can do so by
> +setting DEFAULT_TEST_TARGET=prove in their config.mak.
> +
> +We will follow a similar approach for unit tests: by default the test
> +executables will be run directly from the t/Makefile, but `prove` can be
> +configured with DEFAULT_UNIT_TEST_TARGET=prove.
Nice that it can be used.
The rest of the file looks good.
^ permalink raw reply
* Re: [PATCH v8 2/3] unit tests: add TAP unit test framework
From: Christian Couder @ 2023-10-27 20:15 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, phillip.wood123, linusa, calvinwan, gitster, rsbecker
In-Reply-To: <00d3c95a81449bf49c4ce992d862d7a858691840.1696889530.git.steadmon@google.com>
On Tue, Oct 10, 2023 at 12:22 AM Josh Steadmon <steadmon@google.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This patch contains an implementation for writing unit tests with TAP
> output. Each test is a function that contains one or more checks. The
> test is run with the TEST() macro and if any of the checks fail then the
> test will fail. A complete program that tests STRBUF_INIT would look
> like
>
> #include "test-lib.h"
> #include "strbuf.h"
>
> static void t_static_init(void)
> {
> struct strbuf buf = STRBUF_INIT;
>
> check_uint(buf.len, ==, 0);
> check_uint(buf.alloc, ==, 0);
> check_char(buf.buf[0], ==, '\0');
> }
>
> int main(void)
> {
> TEST(t_static_init(), "static initialization works);
>
> return test_done();
> }
>
> The output of this program would be
>
> ok 1 - static initialization works
> 1..1
>
> If any of the checks in a test fail then they print a diagnostic message
> to aid debugging and the test will be reported as failing. For example a
> failing integer check would look like
>
> # check "x >= 3" failed at my-test.c:102
I wonder if it would be a bit better to say that the test was an
integer test for example with "check_int(x >= 3) failed ..."
> # left: 2
> # right: 3
I like "expected" and "actual" better than "left" and "right", not
sure how it's possible to have that in a way consistent with the shell
tests though.
> not ok 1 - x is greater than or equal to three
>
> There are a number of check functions implemented so far. check() checks
> a boolean condition, check_int(), check_uint() and check_char() take two
> values to compare and a comparison operator. check_str() will check if
> two strings are equal. Custom checks are simple to implement as shown in
> the comments above test_assert() in test-lib.h.
Yeah, nice.
> Tests can be skipped with test_skip() which can be supplied with a
> reason for skipping which it will print. Tests can print diagnostic
> messages with test_msg(). Checks that are known to fail can be wrapped
> in TEST_TODO().
Maybe TEST_TOFIX() would be a bit more clear, but "TODO" is something
that is more likely to be searched for than "TOFIX", so Ok.
> There are a couple of example test programs included in this
> patch. t-basic.c implements some self-tests and demonstrates the
> diagnostic output for failing test. The output of this program is
> checked by t0080-unit-test-output.sh. t-strbuf.c shows some example
> unit tests for strbuf.c
>
> The unit tests will be built as part of the default "make all" target,
> to avoid bitrot. If you wish to build just the unit tests, you can run
> "make build-unit-tests". To run the tests, you can use "make unit-tests"
> or run the test binaries directly, as in "./t/unit-tests/bin/t-strbuf".
Nice!
> +unit-tests-prove:
> + @echo "*** prove - unit tests ***"; $(PROVE) $(GIT_PROVE_OPTS) $(UNIT_TESTS)
Nice, but DEFAULT_TEST_TARGET=prove isn't used. So not sure how
important or relevant the 'prove' related sections are in the
Documentation/technical/unit-tests.txt file introduced by the previous
patch.
> +int test_assert(const char *location, const char *check, int ok)
> +{
> + assert(ctx.running);
> +
> + if (ctx.result == RESULT_SKIP) {
> + test_msg("skipping check '%s' at %s", check, location);
> + return 1;
> + } else if (!ctx.todo) {
I think it would be a bit clearer without the "else" above and with
the "if (!ctx.todo) {" starting on a new line.
> + if (ok) {
> + test_pass();
> + } else {
> + test_msg("check \"%s\" failed at %s", check, location);
> + test_fail();
> + }
> + }
> +
> + return !!ok;
> +}
Otherwise it looks good to me.
^ permalink raw reply
* Re: [PATCH v8 0/3] Add unit test framework and project plan
From: Christian Couder @ 2023-10-27 20:26 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, phillip.wood123, linusa, calvinwan, gitster, rsbecker
In-Reply-To: <cover.1696889529.git.steadmon@google.com>
On Tue, Oct 10, 2023 at 12:22 AM Josh Steadmon <steadmon@google.com> wrote:
>
> In our current testing environment, we spend a significant amount of
> effort crafting end-to-end tests for error conditions that could easily
> be captured by unit tests (or we simply forgo some hard-to-setup and
> rare error conditions). Unit tests additionally provide stability to the
> codebase and can simplify debugging through isolation. Turning parts of
> Git into libraries[1] gives us the ability to run unit tests on the
> libraries and to write unit tests in C. Writing unit tests in pure C,
> rather than with our current shell/test-tool helper setup, simplifies
> test setup, simplifies passing data around (no shell-isms required), and
> reduces testing runtime by not spawning a separate process for every
> test invocation.
>
> This series begins with a project document covering our goals for adding
> unit tests and a discussion of alternative frameworks considered, as
> well as the features used to evaluate them. A rendered preview of this
> doc can be found at [2]. It also adds Phillip Wood's TAP implemenation
> (with some slightly re-worked Makefile rules) and a sample strbuf unit
> test. Finally, we modify the configs for GitHub and Cirrus CI to run the
> unit tests. Sample runs showing successful CI runs can be found at [3],
> [4], and [5].
I took a look at this version and I like it very much. I left a few
comments. My only real wish would be to use something like "actual"
and "expected" instead of "left" and "right" in case of test failure
and perhaps in the argument names of functions and macros. Not sure it
is easy to do in the same way as in the shell framework though.
Thanks!
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Junio C Hamano @ 2023-10-27 22:31 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Jeff King, Michael Strawbridge, Bagas Sanjaya, Git Mailing List
In-Reply-To: <ZTjedSluwyrVY+L9@ugly>
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
>>The "//" operator was added in perl 5.10. I'm not sure what you found
>>that makes you think the ship has sailed. The only hits for "//" I see
>>look like the end of substitution regexes ("s/foo//" and similar).
>>
> grep with spaces around the operator, then you can see the instance in
> git-credential-netrc.perl easily.
Good find, but given the relative prevalence in use between netrc
helper and send-email, my conclusion is rather opposite. It seems
to indicate that avoiding "//" would still be prudent if the only
tool we can find find broken on 5.008 is the netrc helper.
^ permalink raw reply
* Re: [PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`
From: Junio C Hamano @ 2023-10-27 23:12 UTC (permalink / raw)
To: Jeff King
Cc: Taylor Blau, git, Elijah Newren, Eric W. Biederman,
Patrick Steinhardt
In-Reply-To: <20231025073736.GB2145145@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Alternatively, I think some of our other OO code just leaves room for
> a type-specific void pointer, like:
>
> struct bulk_checkin_source {
> ...the usual stuff...
>
> void *magic_type_data;
> };
>
> and then the init_bulk_checkin_source_from_fd() function allocates its
> own heap struct for the magic_type_data field and sticks the int in
> there.
Yup. All the pros-and-cons makes sense. I earlier said I found
this a good place to stop, exactly because the full OO with
struct specific_subclass {
struct vtbl *vtbl;
struct base_class common_data_specific_to_instance;
... other instance specific data members here ...;
}
would require us to add too many supporting infrastructure struct
types; with only a few subclasses in use, it is a bit too much to
justify.
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Junio C Hamano @ 2023-10-27 23:24 UTC (permalink / raw)
To: Marc Branchaud
Cc: Johannes Sixt, Phillip Wood, Oswald Buddenhagen, phillip.wood,
git
In-Reply-To: <70b8d4d8-f4b5-4cd7-b73a-1d7393d84266@xiplink.com>
Marc Branchaud <marcnarc@xiplink.com> writes:
> I never use "fixup -C" (or -c), but I do use squash/fixup a lot. I
> find that I would prefer it if Git used the most recent Author date
> from the set of commits being combined, rather than preserving the
> picked commit's Author date. Sometimes it takes quite a while for me
> to get a piece of work sorted out, and I would rather have the Author
> date in the end-result commit reflect the work's completion time than
> its initiation time.
Yeah, I can sympathize but with both positions, as I can see why
most people would want "minor fixups and typofixes" to retain the
original authorship date, and when concluding a "combining the whole
steps together to reach this final single patch" development, they
would want to record the completion date. The "take the one's
authorship and apply only the effects and not metadata from the
fixups" is a good match for the former. To support the latter, we
can just ignore the timestamp of any commits that were involved in
the end result, and record the time "rebase -i" was concluded
instead, but the tool is not set up for doing so.
> The current behaviour means that when scanning through commits with
> tools like gitk (which shows just the Author date in its list of
> commits) I'll often see what I feel are inaccurate or confusing dates
> there,...
Yup, exactly. Two opposing worldviews, which is not even per-user,
but depends on why the "fixup/squash" was used, exists, but the tool
was designed to support the "small fixup for work that was mostly
done already" use case, so the other usecase is left for people to
say "Yes, I know how to force my desired author date on commits,
thanks." ;-)
> Anyway, this is a minor itch for me that I've never felt the need to
> scratch. I just thought I'd mention it since the topic is being
> discussed.
Yup, it is a very good observation. Giving it a good UI to support
both worldviews would be a good exercise, as we all need both
behaviour from time to time.
Thanks.
^ permalink raw reply
* Re: [PATCH v3] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Junio C Hamano @ 2023-10-27 23:34 UTC (permalink / raw)
To: Marc Branchaud
Cc: Oswald Buddenhagen, git, Phillip Wood, Taylor Blau,
Christian Couder, Charvi Mendiratta
In-Reply-To: <56e3e974-a027-439f-871d-c7fbae65a04e@xiplink.com>
Marc Branchaud <marcnarc@xiplink.com> writes:
> I do not think this kind of editorializing belongs in the commit's
> message, but this likely isn't the first commit message that expresses
> an opinion.
Thanks for saying this.
> I like the overall phrasing here.
>
> But I think you should remove the "but this should not be relied upon"
> phrase. This reads as if Git's current behaviour is undefined, which
> most definitely is not true.
>
> Even changing this to something like "but this might change in the
> future" is unhelpful. Everything in Git is subject to change over a
> long-enough time span, so the same could be said about every aspect of
> Git.
>
> Until the behaviour actually changes, it's perfectly fine for people
> to use multiple "fixup -c" commands. There's no reason to scare them
> off of it.
And that would simplify the description to make it easier to follow
by readers who are *not* involved in the development process.
>
>> +If the resulting commit message is a concatenation of multiple messages,
>> +an editor is opened allowing you to edit it. This is also the case for a
>> +message obtained via "fixup -c", while using "fixup -C" instead skips
>> +the editor; this is analogous to the behavior of `git commit`.
>> +The author information (including date/timestamp) always comes from
>> +the first commit; this is the case even if "fixup -c/-C" is used,
>> +contrary to what `git commit` does.
>
> This phrasing is much better.
>
> Thanks for putting up with my pedantry!
Thanks for a good review. I guess the patch is very near the finish
line?
^ permalink raw reply
* Re: [RFC PATCH v2 0/6] Noobify format for status, add, restore
From: Dragan Simic @ 2023-10-28 0:06 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <ZTvvz6/GFdwagVa+.jacob@initialcommit.io>
On 2023-10-27 19:13, Jacob Stopak wrote:
> On Fri, Oct 27, 2023 at 03:32:40PM +0200, Dragan Simic wrote:
>> On 2023-10-27 00:46, Jacob Stopak wrote:
>> > Take into account reviewer feedback by doing several things differently:
>> >
>> > * Rename this feature (for now) as "noob format mode" (or just "noob
>> > mode") instead of the original "--table" verbiage. As pointed out,
>> > this no longer ties the name of the setting to it's proposed
>> > implementation detail as a table. Noob mode is not necessarily the
>> > right name, just a placeholder for now. Unless people like it :D
>> >
>> > * Instead of manually having to invoke the -t, --table every time this
>> > format is to be used, set the config option "status.noob" to true.
>> > Although this is logically tied to the status command, there are
>> > many
>> > commands that produce status output, (and this series adds more), so
>> > assume that if the user wants to see the status this way, that it
>> > should be enabled whenever the status info is displayed.
>>
>> How would "status.noob" relate to and coexist with possible future
>> configuration options named "<command>.verbose", which would be
>> somewhat
>> similar to the currently existing "commit.verbose" option? IOW,
>> perhaps it
>> would be better to have per-command options "<command>.verbose = noob"
>> or,
>> even better, "<command>.verbose = extended", to make it all more
>> future-proof and more granular.
>
> Hmm, do there currently exist other <command>.verbose config settings
> besides for commit? From what I can tell from "git help config", the
> commit.verbose setting is the only one I see, and it just adds the diff
> info into the editor if the user runs git commit without the -m flag,
> but
> otherwise there seems to be no extra verbosity outputted.
They currently don't exist, but that's something I've planned to
implement, e.g. to "add.verbose" as a new configuration option. It
should be usable, while not being messy or intrusive as a new feature.
> I noticed that git add and git mv have "verbose" (-v, --verbose) cli
> flags
> which just output the name of the file being added or renamed, and that
> certain other commands like git branch has a verbose output which
> includes
> the branch head commit hash and message in the output, so I guess this
> one
> is actually kindof verbose in that it outputs more than the non-empty
> default output.
Yes, those are the basic per-command verbosity modes or levels, as I
call them. The way I see it, your patches would add new, extended
per-command verbosity levels.
> So it seems like currently "verbose" is used for various things among
> the
> command set, sometimes meaning "add something into the template if one
> is
> used" or "add some tiny output to a command that has no default output"
> (which still seems more "--shy" than "--verbose" :P) or "add some
> additional output to a command that already has some sparse output".
Yes, that's the basic verbosity, as I named it above.
> Another thing is that commands like status have multiple flags that can
> be
> used to specify the output format, such as --short, --long,
> --porcelain,
> etc, but only --short seems to be configurable as a git config setting.
> Is there a reason (besides backward compatibility I guess) that these
> aren't rolled into a single thing like --format=<type>? This seems like
> it would be the easiest way to future proof for new formats like
> --format=verbose, --format=noob, --format=extended, etc.
That's a good question, but I'd need to go through the commit history to
be able to provide some kind of an explanation. It could also be all
packed into "status.verbose" as a new configuration option.
> From a noob's perspective though, does adding a config setting for each
> command really make sense? I'm kindof envisioning this setting now as a
> "mode" that is either enabled for all commands it affects or for none.
> And it's highly unlikely a newish user would individually discover
> which
> commands this "extended" format is available for, and run "git config
> <command>.verbose = extended" for every one. I mean we could do that
> in case there are folks who only want it for specific commands, but to
> fulfill it's purpose I think there should definetely be some general
> way
> to enable the setting for all commands that have it.
Quite frankly, we shouldn't expect that all users are noobs, and as a
result dumb everything down just to make them as comfortable as
possible. On the other hand, perhaps not everyone would like to have
extended verbosity enabled for all commands, just as not everyone uses
"-v" for all commands.
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Junio C Hamano @ 2023-10-28 0:12 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: Liam Beguin, git
In-Reply-To: <20231027184357.21049-1-five231003@gmail.com>
Kousik Sanagavarapu <five231003@gmail.com> writes:
> Liam Beguin <liambeguin@gmail.com> wrote:
>> Subject: Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
>
> Since we are adding both '%aa' and '%aA', it would be better to
> to include both in the commit subject, but since it is already long
> enough, in my opinion
>
> pretty: add formats for domain-part of email address
>
> would convey the gist of the commit to the reader better.
;-). Very good.
>> Many reports use the email domain to keep track of organizations
>> contributing to projects.
Grouping @gmail.com addresses do not smell all that useful, though.
More importantly, it is not clear what "Many reports" refers to. If
they are *not* verbatim output from "git log" family of commands,
iow, they are produced by post-processing output from "git log"
family of commands, then I do not quite see why %aa is useful at
all.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox