* Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
From: Taylor Blau @ 2023-10-23 17:40 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Kristoffer Haugsbakk, git, stolee
In-Reply-To: <ZTZDqToqcsDiS5AP@tanuki>
On Mon, Oct 23, 2023 at 11:58:01AM +0200, Patrick Steinhardt wrote:
> On Wed, Oct 18, 2023 at 10:28:40PM +0200, Kristoffer Haugsbakk wrote:
> >
> > Factor out code that retrieves the global config file so that we can use
> > it in `gc.c` as well.
> >
> > Use the old name from the previous commit since this function acts
> > functionally the same as `git_system_config` but for “global”.
> >
> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > ---
> > builtin/config.c | 25 ++-----------------------
> > config.c | 24 ++++++++++++++++++++++++
> > config.h | 1 +
> > 3 files changed, 27 insertions(+), 23 deletions(-)
> >
> > diff --git a/builtin/config.c b/builtin/config.c
> > index 6fff2655816..df06b766fad 100644
> > --- a/builtin/config.c
> > +++ b/builtin/config.c
> > @@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> > }
> >
> > if (use_global_config) {
> > - char *user_config, *xdg_config;
> > -
> > - git_global_config_paths(&user_config, &xdg_config);
> > - if (!user_config)
> > - /*
> > - * It is unknown if HOME/.gitconfig exists, so
> > - * we do not know if we should write to XDG
> > - * location; error out even if XDG_CONFIG_HOME
> > - * is set and points at a sane location.
> > - */
> > - die(_("$HOME not set"));
> > -
> > + given_config_source.file = git_global_config();
> > given_config_source.scope = CONFIG_SCOPE_GLOBAL;
> > -
> > - if (access_or_warn(user_config, R_OK, 0) &&
> > - xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
> > - given_config_source.file = xdg_config;
> > - free(user_config);
> > - } else {
> > - given_config_source.file = user_config;
> > - free(xdg_config);
> > - }
> > - }
> > - else if (use_system_config) {
> > + } else if (use_system_config) {
> > given_config_source.file = git_system_config();
> > given_config_source.scope = CONFIG_SCOPE_SYSTEM;
> > } else if (use_local_config) {
> > diff --git a/config.c b/config.c
> > index d2cdda96edd..2ff766c56ff 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -2111,6 +2111,30 @@ char *git_system_config(void)
> > return system_config;
> > }
> >
> > +char *git_global_config(void)
> > +{
> > + char *user_config, *xdg_config;
> > +
> > + git_global_config_paths(&user_config, &xdg_config);
> > + if (!user_config)
> > + /*
> > + * It is unknown if HOME/.gitconfig exists, so
> > + * we do not know if we should write to XDG
>
> Nit: we don't know about the intent of the caller, so they may not want
> to write to the file but only read it.
I was going to suggest that we allow the caller to pass in the flags
that they wish for git_global_config() to pass down to access(2), but
was surprised to see that we always use R_OK.
But thinking on it for a moment longer, I realized that we don't care
about write-level permissions for the config, since we want to instead
open $GIT_DIR/config.lock for writing, and then rename() it into place,
meaning we only care about whether or not we have write permissions on
$GIT_DIR itself.
I think in the existing location of this code, the "if we should write"
portion of the comment is premature, since we don't know for sure
whether or not we are writing. So I'd be fine with leaving it as-is, but
changing the comment seems easy enough to do...
> > + * location; error out even if XDG_CONFIG_HOME
> > + * is set and points at a sane location.
> > + */
> > + die(_("$HOME not set"));
>
> Is it sensible to `die()` here in this new function that behaves more
> like a library function? I imagine it would be more sensible to indicate
> the error to the user and let them handle it accordingly.
Agreed.
Thanks,
Taylor
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Jacob Stopak @ 2023-10-23 17:30 UTC (permalink / raw)
To: Dragan Simic; +Cc: Oswald Buddenhagen, git
In-Reply-To: <bc55e29274da0d8059a8cd4383aa1b22@manjaro.org>
On Mon, Oct 23, 2023 at 04:34:15PM +0200, Dragan Simic wrote:
> On 2023-10-23 12:52, Oswald Buddenhagen wrote:
> > On Sun, Oct 22, 2023 at 02:55:05PM +0200, Dragan Simic wrote:
> > > Oh, that's awesome and I'm really happy to be wrong with my broad
> > > classification of VCS users. However, I still need to be convinced
> > > further, and I'd assign your example as an exception to the rules,
> >
> > i don't see myself as exceptional at all in that regard.
> > in fact, your second user group seems like unicorns, and the first
> > like a disparaging attitude from an elitist. in reality, users lie on
> > a spectrum of willingness to engage with the details of the tools they
> > use, and that willingness is circumstantial. a tool that is
> > forthcoming with information has a higher chance of being actively
> > engaged.
Just a note here, in my initial reply I was thinking of writing something
similar about how in reality users of a tool as ubiquitous as Git would
form a continuous spectrum in terms of their usage habits and trying to
neatly plop them into 2 categories by speculating on their motives is
an oversimplification to the point where it might not be so helpful
evaluating whether an option like this would make sense to implement.
To me the bigger question is much simpler:
"Would this feature improve the Git experience for a signficant number
of users?"
I have some evidence to support the claim that it would.
My Git-Sim tool does essentially what this proposal suggests and it has
about 31,000 installs since I released it early this year. Granted this
is a drop in the bucket in the grand scheme of things, but it still shows
that there is demand for such a thing.
Git-Sim is a visual dry-run tool for Git that creates images simulating
what the corresponding Git command will do, without actually making any
change to the underlying repo state. Another important aspect is that
command syntax mimics Git's exactly - so to simulate any Git command, like:
$ git add asdf.txt qwer.txt
You would just replace the executable name and run:
$ git-sim add asdf.txt qwer.txt
and it will show you in an image exactly what will happen.
This is important because even simulating the command requires the user
to know and use the Git CLI syntax for the command. It keeps them on the
command line to do all of their actual work, unlike other true "pointy
clicky GUI's" I've seen which expect the user to do all their work in the
GUI. In fact this tool and feature expect no pointing or clicking at all.
The purpose of this is that users actually do all their work in the CLI
and learn to use Git as intuitively as possible, the way the "spartan"
CLI folks use it, the way it is meant to be used.
The reason to include a format like this in Git instead of just in my
tool is simply to reach a wider audience and benefit more people. Of
course it also appears much more trustworthy when a feature is part of
the native tool itself instead of some external thing.
> > i for one think that it would be a perfectly valid experiment to go
> > all-in and beyond with jacob's proposal - _and make it the default_
> > (when the output is a tty). more advanced users who feel annoyed would
> > be expected to opt out of it via configuration, as they are for the
> > advice messages. because it's really the same idea, only thought
> > bigger.
>
> I'd never support that, FWIW.
FWIW, I'd _never suggest_ that. I very much value Git's current usage
and wouldn't dream to make this the default. This proposal is for an
optional flag to help users who would benefit from it, nothing more,
nothing less. Speculating on user motives to classify them into 2 broad
categories in order to prove the feature isn't helpful misses the point
that there is a (relatively large IMO) subset of users who would benefit
from it.
As an optional flag, experienced users wouldn't bat an eyelash, and the
type of users who installed my tool could use the flag on and off until
they feel confident enough to drop it. But it is always there in case
they need a refresher.
^ permalink raw reply
* Re: [PATCH] doc/git-bisect: clarify `git bisect run` syntax
From: Junio C Hamano @ 2023-10-23 17:23 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Eric Sunshine, cousteau via GitGitGadget, git, Javier Mora
In-Reply-To: <ZTYi55w_70ZlP8Ew@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> I also thought at least some commands we know the "-h" output and
>> SYNOPSIS match, we had tests to ensure they do not drift apart. We
>> would probably want to cover more subcommands with t0450.
>>
>> Thanks.
>
> If we don't want them to drift apart I wonder whether we could instead
> generate the synopsis from the output of `-h`? This reduces duplication
> at the cost of a more complex build process for our manpages.
There also is the cost of making it unusable to peek the source
git-foo.txt files as a quick way to get the usage guide, which I
think is a downside only felt by developers of Git (not developers
of other projects that happen to use Git), but still a downside.
But aside from that, it is an obviously possible direction to go [*]
and in fact I suspect we may have talked about it when Ævar made a
gigantic effort to clean these up in Sep-Oct 2022 timeframe, which
resulted in the series leading to a0343f30 (tests: assert consistent
whitespace in -h output, 2022-10-13) [*].
[Footnote]
* And another possibility is to go from the doc to the message
string, which may be even more involved, but at least the code
needs to go through the build process anyway, so the downside
might be lessor)
* https://lore.kernel.org/git/patch-v5-34.34-4de83d3d89a-20221013T153626Z-avarab@gmail.com/
^ permalink raw reply
* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Taylor Blau @ 2023-10-23 16:59 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: git, Junio C Hamano, Phillip Wood, Christian Couder,
Charvi Mendiratta, Marc Branchaud
In-Reply-To: <20231023130016.1093356-1-oswald.buddenhagen@gmx.de>
On Mon, Oct 23, 2023 at 03:00:16PM +0200, Oswald Buddenhagen wrote:
> ---
> Documentation/git-rebase.txt | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
The new documentation below looks fine, and I don't have strong
feelings beyond the proposed modifications.
The line wrapping is a little odd: it looks like each sentence begins on
a its own line. Did you mean for there to be a visual separation between
those sentences in the rendered doc? If so, replace the single line feed
with a pair of them.
If not, this looks good to me as-is.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 08/11] t4207: delete replace references via git-update-ref(1)
From: Taylor Blau @ 2023-10-23 16:42 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <c4d09e3e5dbd11221cc4d229b815434d441cdb4d.1697607222.git.ps@pks.im>
On Wed, Oct 18, 2023 at 07:35:37AM +0200, Patrick Steinhardt wrote:
> In t4207 we set up a set of replace objects via git-replace(1). Because
> these references should not be impacting subsequent tests we also set up
> some cleanup logic that deletes the replacement references via a call to
> `rm -rf`. This reaches into the internal implementation details of the
> reference backend and will thus break when we grow an alternative refdb
> implementation.
>
> Refactor the tests to delete the replacement refs via Git commands so
> that we become independent of the actual refdb that's in use. As we
> don't have a nice way to delete all replacements or all references in a
> certain namespace, we opt for a combination of git-for-each-ref(1) and
> git-update-ref(1)'s `--stdin` mode.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/t4207-log-decoration-colors.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
> index 21986a866df..d138e513a04 100755
> --- a/t/t4207-log-decoration-colors.sh
> +++ b/t/t4207-log-decoration-colors.sh
> @@ -71,7 +71,7 @@ ${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
> '
>
> test_expect_success 'test coloring with replace-objects' '
> - test_when_finished rm -rf .git/refs/replace* &&
> + test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} | git update-ref --stdin" &&
Here and below, should we avoid the for-each-ref showing up on the
left-hand side of the pipe? I'd think we want something closer to:
test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} >in && git update-ref --stdin <in" &&
But having to quote the --format argument with "${SQ}"s makes the whole
thing a little awkward to read and parse.
Do you think that something like the below would be a readability
improvement?
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index d138e513a0..de8f6638cb 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -70,8 +70,13 @@ ${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
cmp_filtered_decorations
'
--- >8 ---
+remove_replace_refs () {
+ git for-each-ref 'refs/replace*/**' --format='delete %(refname)' >in &&
+ git update-ref --stdin <in
+}
+
test_expect_success 'test coloring with replace-objects' '
- test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} | git update-ref --stdin" &&
+ test_when_finished remove_replace_refs &&
test_commit C &&
test_commit D &&
@@ -99,7 +104,7 @@ EOF
'
test_expect_success 'test coloring with grafted commit' '
- test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} | git update-ref --stdin" &&
+ test_when_finished remove_replace_refs &&
git replace --graft HEAD HEAD~2 &&
--- 8< ---
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH] doc/git-bisect: clarify `git bisect run` syntax
From: Javier Mora @ 2023-10-23 16:27 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Junio C Hamano, Eric Sunshine, cousteau via GitGitGadget, git
In-Reply-To: <ZTYi55w_70ZlP8Ew@tanuki>
> The output of `git bisect -h` suffers the same problem. Perhaps this
> patch can fix that, as well?
Certainly possible. Probably best if I put that on a second patch
though (i.e. a separate commit). Or should I just squash everything
together?
There are still multiple .po files containing the old string, I guess
I don't need to touch those?
Speaking of which, looking at the .po files I've found that there's
also a `git bisect--helper` command; I don't know if that's relevant
nor how to modify that.
> I wonder if we should eventually move these into the
> proper SYNOPSIS section.
Seems reasonable. I was actually wondering about that.
I can make an extra patch for that if you want, while I'm at it.
> If we don't want them to drift apart I wonder whether we could instead
> generate the synopsis from the output of `-h`? This reduces duplication
That's not a bad idea. Or maybe the other way around -- generate the
output of `-h` from the synopsis. Or generate both (manpage and help
message) from a "synopsis stub" file; I wonder if that could be easily
done.
El lun, 23 oct 2023 a las 8:38, Patrick Steinhardt (<ps@pks.im>) escribió:
>
> On Sun, Oct 22, 2023 at 05:35:41PM -0700, Junio C Hamano wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> >
> > > On Sun, Oct 22, 2023 at 4:03 PM cousteau via GitGitGadget
> > > <gitgitgadget@gmail.com> wrote:
> > >> The description of the `git bisect run` command syntax at the beginning
> > >> of the manpage is `git bisect run <cmd>...`, which isn't quite clear
> > >> about what `<cmd>` is or what the `...` mean; one could think that it is
> > >> the whole (quoted) command line with all arguments in a single string,
> > >> or that it supports multiple commands, or that it doesn't accept
> > >> commands with arguments at all.
> > >>
> > >> Change to `git bisect run <cmd> [<arg>...]` to clarify the syntax.
> > >
> > > Okay, makes sense.
> > >
> > >> Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
> > >> ---
> > >> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> > >> @@ -26,7 +26,7 @@ on the subcommand:
> > >> - git bisect run <cmd>...
> > >> + git bisect run <cmd> [<arg>...]
> > >
> > > The output of `git bisect -h` suffers the same problem. Perhaps this
> > > patch can fix that, as well?
> >
> > Good eyes.
> >
> > Not a new problem and obviously can be left outside of this simple
> > update, but I wonder if we should eventually move these into the
> > proper SYNOPSIS section. Other multi-modal commands like "git
> > checkout", "git rebase", etc. do list different forms all in the
> > SYNOPSIS section.
> >
> > I also thought at least some commands we know the "-h" output and
> > SYNOPSIS match, we had tests to ensure they do not drift apart. We
> > would probably want to cover more subcommands with t0450.
> >
> > Thanks.
>
> If we don't want them to drift apart I wonder whether we could instead
> generate the synopsis from the output of `-h`? This reduces duplication
> at the cost of a more complex build process for our manpages.
>
> Not saying that this is necessarily a good idea, just throwing it out
> there.
>
> Patrick
^ permalink raw reply
* [PATCH v3] builtin/branch.c: adjust error messages to coding guidelines
From: Isoken June Ibizugbe @ 2023-10-23 16:06 UTC (permalink / raw)
To: git; +Cc: Isoken June Ibizugbe, rjusto
In-Reply-To: <39fd1327b2ae4f73689d70561a2f738d@manjaro.org>
As per the CodingGuidelines document, it is recommended that error messages
such as die(), error() and warning(), should start with a lowercase letter
and should not end with a period.
This patch adjusts tests to match updated messages.
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
builtin/branch.c | 66 +++++++++++++++++++--------------------
t/t2407-worktree-heads.sh | 2 +-
t/t3200-branch.sh | 16 +++++-----
t/t3202-show-branch.sh | 10 +++---
4 files changed, 47 insertions(+), 47 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a..e7ee9bd0f1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
(head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != merged) {
if (merged)
warning(_("deleting branch '%s' that has been merged to\n"
- " '%s', but not yet merged to HEAD."),
+ " '%s', but not yet merged to HEAD"),
name, reference_name);
else
warning(_("not deleting branch '%s' that is not yet merged to\n"
- " '%s', even though it is merged to HEAD."),
+ " '%s', even though it is merged to HEAD"),
name, reference_name);
}
free(reference_name_to_free);
@@ -190,13 +190,13 @@ static int check_branch_commit(const char *branchname, const char *refname,
{
struct commit *rev = lookup_commit_reference(the_repository, oid);
if (!force && !rev) {
- error(_("Couldn't look up commit object for '%s'"), refname);
+ error(_("couldn't look up commit object for '%s'"), refname);
return -1;
}
if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
- error(_("The branch '%s' is not fully merged.\n"
+ error(_("the branch '%s' is not fully merged.\n"
"If you are sure you want to delete it, "
- "run 'git branch -D %s'."), branchname, branchname);
+ "run 'git branch -D %s'"), branchname, branchname);
return -1;
}
return 0;
@@ -207,7 +207,7 @@ static void delete_branch_config(const char *branchname)
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "branch.%s", branchname);
if (git_config_rename_section(buf.buf, NULL) < 0)
- warning(_("Update of config-file failed"));
+ warning(_("update of config-file failed"));
strbuf_release(&buf);
}
@@ -260,7 +260,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (kinds == FILTER_REFS_BRANCHES) {
const char *path;
if ((path = branch_checked_out(name))) {
- error(_("Cannot delete branch '%s' "
+ error(_("cannot delete branch '%s' "
"used by worktree at '%s'"),
bname.buf, path);
ret = 1;
@@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
&oid, &flags);
if (!target) {
if (remote_branch) {
- error(_("remote-tracking branch '%s' not found."), bname.buf);
+ error(_("remote-tracking branch '%s' not found"), bname.buf);
} else {
char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
char *virtual_target = resolve_refdup(virtual_name,
@@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
"Did you forget --remote?"),
bname.buf);
else
- error(_("branch '%s' not found."), bname.buf);
+ error(_("branch '%s' not found"), bname.buf);
FREE_AND_NULL(virtual_target);
}
ret = 1;
@@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
continue;
if (is_worktree_being_rebased(wt, target))
- die(_("Branch %s is being rebased at %s"),
+ die(_("branch %s is being rebased at %s"),
target, wt->path);
if (is_worktree_being_bisected(wt, target))
- die(_("Branch %s is being bisected at %s"),
+ die(_("branch %s is being bisected at %s"),
target, wt->path);
}
}
@@ -578,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (ref_exists(oldref.buf))
recovery = 1;
else
- die(_("Invalid branch name: '%s'"), oldname);
+ die(_("invalid branch name: '%s'"), oldname);
}
for (int i = 0; worktrees[i]; i++) {
@@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
if (oldref_usage & IS_HEAD)
- die(_("No commit on branch '%s' yet."), oldname);
+ die(_("no commit on branch '%s' yet"), oldname);
else
- die(_("No branch named '%s'."), oldname);
+ die(_("no branch named '%s'"), oldname);
}
/*
@@ -624,32 +624,32 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && !(oldref_usage & IS_ORPHAN) &&
rename_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch rename failed"));
+ die(_("branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch copy failed"));
+ die(_("branch copy failed"));
if (recovery) {
if (copy)
- warning(_("Created a copy of a misnamed branch '%s'"),
+ warning(_("created a copy of a misnamed branch '%s'"),
interpreted_oldname);
else
- warning(_("Renamed a misnamed branch '%s' away"),
+ warning(_("renamed a misnamed branch '%s' away"),
interpreted_oldname);
}
if (!copy && (oldref_usage & IS_HEAD) &&
replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
logmsg.buf))
- die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to %s, but HEAD is not updated"), newname);
strbuf_release(&logmsg);
strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
strbuf_addf(&newsection, "branch.%s", interpreted_newname);
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is renamed, but update of config-file failed"));
+ die(_("branch is renamed, but update of config-file failed"));
if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is copied, but update of config-file failed"));
+ die(_("branch is copied, but update of config-file failed"));
strbuf_release(&oldref);
strbuf_release(&newref);
strbuf_release(&oldsection);
@@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
head = resolve_refdup("HEAD", 0, &head_oid, NULL);
if (!head)
- die(_("Failed to resolve HEAD as a valid ref."));
+ die(_("failed to resolve HEAD as a valid ref"));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
@@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc) {
if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
+ die(_("cannot give description to detached HEAD"));
branch_name = head;
} else if (argc == 1) {
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
@@ -878,8 +878,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf))
error((!argc || branch_checked_out(branch_ref.buf))
- ? _("No commit on branch '%s' yet.")
- : _("No branch named '%s'."),
+ ? _("no commit on branch '%s' yet")
+ : _("no branch named '%s'"),
branch_name);
else if (!edit_branch_description(branch_name))
ret = 0; /* happy */
@@ -892,8 +892,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc)
die(_("branch name required"));
else if ((argc == 1) && filter.detached)
- die(copy? _("cannot copy the current branch while not on any.")
- : _("cannot rename the current branch while not on any."));
+ die(copy? _("cannot copy the current branch while not on any")
+ : _("cannot rename the current branch while not on any"));
else if (argc == 1)
copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
else if (argc == 2)
@@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not set upstream of HEAD to %s when "
- "it does not point to any branch."),
+ "it does not point to any branch"),
new_upstream);
die(_("no such branch '%s'"), argv[0]);
}
if (!ref_exists(branch->refname)) {
if (!argc || branch_checked_out(branch->refname))
- die(_("No commit on branch '%s' yet."), branch->name);
+ die(_("no commit on branch '%s' yet"), branch->name);
die(_("branch '%s' does not exist"), branch->name);
}
@@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not unset upstream of HEAD when "
- "it does not point to any branch."));
+ "it does not point to any branch"));
die(_("no such branch '%s'"), argv[0]);
}
if (!branch_has_merge_config(branch))
- die(_("Branch '%s' has no upstream information"), branch->name);
+ die(_("branch '%s' has no upstream information"), branch->name);
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.remote", branch->name);
@@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
+ die(_("the -a, and -r, options to 'git branch' do not take a branch name.\n"
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
- die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
+ die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead"));
if (recurse_submodules) {
create_branches_recursively(the_repository, branch_name,
diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
index 469443d8ae..f6835c91dc 100755
--- a/t/t2407-worktree-heads.sh
+++ b/t/t2407-worktree-heads.sh
@@ -45,7 +45,7 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '
grep "cannot force update the branch" err &&
test_must_fail git branch -D wt-$i 2>err &&
- grep "Cannot delete branch" err || return 1
+ grep "cannot delete branch" err || return 1
done
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 080e4f24a6..3182abde27 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -291,10 +291,10 @@ test_expect_success 'git branch -M topic topic should work when main is checked
test_expect_success 'git branch -M and -C fail on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: cannot rename the current branch while not on any." >expect &&
+ echo "fatal: cannot rename the current branch while not on any" >expect &&
test_must_fail git branch -M must-fail 2>err &&
test_cmp expect err &&
- echo "fatal: cannot copy the current branch while not on any." >expect &&
+ echo "fatal: cannot copy the current branch while not on any" >expect &&
test_must_fail git branch -C must-fail 2>err &&
test_cmp expect err
'
@@ -943,7 +943,7 @@ test_expect_success 'deleting currently checked out branch fails' '
git worktree add -b my7 my7 &&
test_must_fail git -C my7 branch -d my7 &&
test_must_fail git branch -d my7 2>actual &&
- grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
+ grep "^error: cannot delete branch .my7. used by worktree at " actual &&
rm -r my7 &&
git worktree prune
'
@@ -954,7 +954,7 @@ test_expect_success 'deleting in-use branch fails' '
git -C my7 bisect start HEAD HEAD~2 &&
test_must_fail git -C my7 branch -d my7 &&
test_must_fail git branch -d my7 2>actual &&
- grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
+ grep "^error: cannot delete branch .my7. used by worktree at " actual &&
rm -r my7 &&
git worktree prune
'
@@ -1024,7 +1024,7 @@ test_expect_success '--set-upstream-to fails on multiple branches' '
test_expect_success '--set-upstream-to fails on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: could not set upstream of HEAD to main when it does not point to any branch." >expect &&
+ echo "fatal: could not set upstream of HEAD to main when it does not point to any branch" >expect &&
test_must_fail git branch --set-upstream-to main 2>err &&
test_cmp expect err
'
@@ -1072,7 +1072,7 @@ test_expect_success 'use --set-upstream-to modify a particular branch' '
'
test_expect_success '--unset-upstream should fail if given a non-existent branch' '
- echo "fatal: Branch '"'"'i-dont-exist'"'"' has no upstream information" >expect &&
+ echo "fatal: branch '"'"'i-dont-exist'"'"' has no upstream information" >expect &&
test_must_fail git branch --unset-upstream i-dont-exist 2>err &&
test_cmp expect err
'
@@ -1094,7 +1094,7 @@ test_expect_success 'test --unset-upstream on HEAD' '
test_must_fail git config branch.main.remote &&
test_must_fail git config branch.main.merge &&
# fail for a branch without upstream set
- echo "fatal: Branch '"'"'main'"'"' has no upstream information" >expect &&
+ echo "fatal: branch '"'"'main'"'"' has no upstream information" >expect &&
test_must_fail git branch --unset-upstream 2>err &&
test_cmp expect err
'
@@ -1108,7 +1108,7 @@ test_expect_success '--unset-upstream should fail on multiple branches' '
test_expect_success '--unset-upstream should fail on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: could not unset upstream of HEAD when it does not point to any branch." >expect &&
+ echo "fatal: could not unset upstream of HEAD when it does not point to any branch" >expect &&
test_must_fail git branch --unset-upstream 2>err &&
test_cmp expect err
'
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index b17f388f56..2cdb834b37 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -10,7 +10,7 @@ GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
test_expect_success 'error descriptions on empty repository' '
current=$(git branch --show-current) &&
cat >expect <<-EOF &&
- error: No commit on branch '\''$current'\'' yet.
+ error: no commit on branch '\''$current'\'' yet
EOF
test_must_fail git branch --edit-description 2>actual &&
test_cmp expect actual &&
@@ -21,7 +21,7 @@ test_expect_success 'error descriptions on empty repository' '
test_expect_success 'fatal descriptions on empty repository' '
current=$(git branch --show-current) &&
cat >expect <<-EOF &&
- fatal: No commit on branch '\''$current'\'' yet.
+ fatal: no commit on branch '\''$current'\'' yet
EOF
test_must_fail git branch --set-upstream-to=non-existent 2>actual &&
test_cmp expect actual &&
@@ -224,7 +224,7 @@ done
test_expect_success 'error descriptions on non-existent branch' '
cat >expect <<-EOF &&
- error: No branch named '\''non-existent'\'.'
+ error: no branch named '\''non-existent'\''
EOF
test_must_fail git branch --edit-description non-existent 2>actual &&
test_cmp expect actual
@@ -238,7 +238,7 @@ test_expect_success 'fatal descriptions on non-existent branch' '
test_cmp expect actual &&
cat >expect <<-EOF &&
- fatal: No branch named '\''non-existent'\''.
+ fatal: no branch named '\''non-existent'\''
EOF
test_must_fail git branch -c non-existent new-branch 2>actual &&
test_cmp expect actual &&
@@ -253,7 +253,7 @@ test_expect_success 'error descriptions on orphan branch' '
test_branch_op_in_wt() {
test_orphan_error() {
test_must_fail git $* 2>actual &&
- test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
+ test_i18ngrep "no commit on branch .orphan-branch. yet$" actual
} &&
test_orphan_error -C wt branch $1 $2 && # implicit branch
test_orphan_error -C wt branch $1 orphan-branch $2 && # explicit branch
--
2.42.0.346.g24618a8a3e.dirty
^ permalink raw reply related
* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Phillip Wood @ 2023-10-23 16:01 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Cc: Junio C Hamano, Christian Couder, Charvi Mendiratta,
Marc Branchaud
In-Reply-To: <20231023130016.1093356-1-oswald.buddenhagen@gmx.de>
Hi Oswald
On 23/10/2023 14:00, Oswald Buddenhagen wrote:
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index e7b39ad244..337df9ef2f 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -890,20 +890,21 @@ command "pick" with the command "reword".
> To drop a commit, replace the command "pick" with "drop", or just
> delete the matching line.
>
> -If you want to fold two or more commits into one, replace the command
> -"pick" for the second and subsequent commits with "squash" or "fixup".
> -If the commits had different authors, the folded commit will be
> -attributed to the author of the first commit. The suggested commit
> -message for the folded commit is the concatenation of the first
> -commit's message with those identified by "squash" commands, omitting the
> -messages of commits identified by "fixup" commands, unless "fixup -c"
> -is used. In that case the suggested commit message is only the message
> -of the "fixup -c" commit, and an editor is opened allowing you to edit
> -the message. The contents (patch) of the "fixup -c" commit are still
> -incorporated into the folded commit. If there is more than one "fixup -c"
> -commit, the message from the final one is used. You can also use
> -"fixup -C" to get the same behavior as "fixup -c" except without opening
> -an editor.
> +If you want to fold two or more commits into one (that is, to combine
> +their contents/patches), replace the command "pick" for the second and
> +subsequent commits with "squash" or "fixup".
> +The commit message for the folded commit is the concatenation of the
> +message of the first commit with those of commits identified by "squash"
> +commands, omitting those of commits identified by "fixup" commands,
> +unless "fixup -c" is used. In the latter case, the message is obtained
> +only from the "fixup -c" commit (having more than one of these is
> +incorrect).
This change is incorrect - it is perfectly fine to have more than one
"fixup -c" command. In that case we use the message of the commit of the
final "fixup -c" command. One case where there can be multiple "fixup
-c" commands is when a commit has been reworded several times via "git
commit --fixup=reword:<commit>" and the user runs "git rebase --autosquash"
> +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 first commit which contributes to the suggested commit message also
> +determines the author, along with the date/timestamp.
In the case of
pick A
fixup -C B
don't we keep the authorship from A and just use the commit message from B?
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v3 0/3] rebase refactoring
From: Phillip Wood @ 2023-10-23 15:43 UTC (permalink / raw)
To: Junio C Hamano, Oswald Buddenhagen; +Cc: git
In-Reply-To: <xmqq5y31osmg.fsf@gitster.g>
On 20/10/2023 23:07, Junio C Hamano wrote:
> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> broken out of the bigger series, as the aggregation just unnecessarily holds it
>> up.
>>
>> v3: removed "stray" footer. so more of a RESEND than an actual new version.
>>
>> Oswald Buddenhagen (3):
>> rebase: simplify code related to imply_merge()
>> rebase: handle --strategy via imply_merge() as well
>> rebase: move parse_opt_keep_empty() down
>>
>> builtin/rebase.c | 44 ++++++++++++++------------------------------
>> 1 file changed, 14 insertions(+), 30 deletions(-)
>
> Looking quite straight-forward and I didn't see anythihng
> potentially controversial.
Yes they look good, thanks Oswald
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v3] builtin/branch.c: adjust error messages to coding guidelines
From: Dragan Simic @ 2023-10-23 15:05 UTC (permalink / raw)
To: Isoken June Ibizugbe; +Cc: git, rjusto
In-Reply-To: <20231023145708.4029-1-isokenjune@gmail.com>
On 2023-10-23 16:57, Isoken June Ibizugbe wrote:
> As per the CodingGuidelines document, it is recommended that error
> messages such as die(), error() and warning(),
> should start with a lowercase letter and should not end with a period.
>
> This patch adjusts tests to match updated messages.
>
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
Please, wrap the commit comment at the column 78. That's how it works
nearly everywhere.
> ---
> builtin/branch.c | 66 +++++++++++++++++++--------------------
> t/t2407-worktree-heads.sh | 2 +-
> t/t3200-branch.sh | 16 +++++-----
> t/t3202-show-branch.sh | 10 +++---
> 4 files changed, 47 insertions(+), 47 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 2ec190b14a..e7ee9bd0f1 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -173,11 +173,11 @@ static int branch_merged(int kind, const char
> *name,
> (head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) :
> 0) != merged) {
> if (merged)
> warning(_("deleting branch '%s' that has been merged to\n"
> - " '%s', but not yet merged to HEAD."),
> + " '%s', but not yet merged to HEAD"),
> name, reference_name);
> else
> warning(_("not deleting branch '%s' that is not yet merged to\n"
> - " '%s', even though it is merged to HEAD."),
> + " '%s', even though it is merged to HEAD"),
> name, reference_name);
> }
> free(reference_name_to_free);
> @@ -190,13 +190,13 @@ static int check_branch_commit(const char
> *branchname, const char *refname,
> {
> struct commit *rev = lookup_commit_reference(the_repository, oid);
> if (!force && !rev) {
> - error(_("Couldn't look up commit object for '%s'"), refname);
> + error(_("couldn't look up commit object for '%s'"), refname);
> return -1;
> }
> if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
> - error(_("The branch '%s' is not fully merged.\n"
> + error(_("the branch '%s' is not fully merged.\n"
> "If you are sure you want to delete it, "
> - "run 'git branch -D %s'."), branchname, branchname);
> + "run 'git branch -D %s'"), branchname, branchname);
> return -1;
> }
> return 0;
> @@ -207,7 +207,7 @@ static void delete_branch_config(const char
> *branchname)
> struct strbuf buf = STRBUF_INIT;
> strbuf_addf(&buf, "branch.%s", branchname);
> if (git_config_rename_section(buf.buf, NULL) < 0)
> - warning(_("Update of config-file failed"));
> + warning(_("update of config-file failed"));
> strbuf_release(&buf);
> }
>
> @@ -260,7 +260,7 @@ static int delete_branches(int argc, const char
> **argv, int force, int kinds,
> if (kinds == FILTER_REFS_BRANCHES) {
> const char *path;
> if ((path = branch_checked_out(name))) {
> - error(_("Cannot delete branch '%s' "
> + error(_("cannot delete branch '%s' "
> "used by worktree at '%s'"),
> bname.buf, path);
> ret = 1;
> @@ -275,7 +275,7 @@ static int delete_branches(int argc, const char
> **argv, int force, int kinds,
> &oid, &flags);
> if (!target) {
> if (remote_branch) {
> - error(_("remote-tracking branch '%s' not found."), bname.buf);
> + error(_("remote-tracking branch '%s' not found"), bname.buf);
> } else {
> char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
> char *virtual_target = resolve_refdup(virtual_name,
> @@ -290,7 +290,7 @@ static int delete_branches(int argc, const char
> **argv, int force, int kinds,
> "Did you forget --remote?"),
> bname.buf);
> else
> - error(_("branch '%s' not found."), bname.buf);
> + error(_("branch '%s' not found"), bname.buf);
> FREE_AND_NULL(virtual_target);
> }
> ret = 1;
> @@ -518,11 +518,11 @@ static void
> reject_rebase_or_bisect_branch(struct worktree **worktrees,
> continue;
>
> if (is_worktree_being_rebased(wt, target))
> - die(_("Branch %s is being rebased at %s"),
> + die(_("branch %s is being rebased at %s"),
> target, wt->path);
>
> if (is_worktree_being_bisected(wt, target))
> - die(_("Branch %s is being bisected at %s"),
> + die(_("branch %s is being bisected at %s"),
> target, wt->path);
> }
> }
> @@ -578,7 +578,7 @@ static void copy_or_rename_branch(const char
> *oldname, const char *newname, int
> if (ref_exists(oldref.buf))
> recovery = 1;
> else
> - die(_("Invalid branch name: '%s'"), oldname);
> + die(_("invalid branch name: '%s'"), oldname);
> }
>
> for (int i = 0; worktrees[i]; i++) {
> @@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char
> *oldname, const char *newname, int
>
> if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
> if (oldref_usage & IS_HEAD)
> - die(_("No commit on branch '%s' yet."), oldname);
> + die(_("no commit on branch '%s' yet"), oldname);
> else
> - die(_("No branch named '%s'."), oldname);
> + die(_("no branch named '%s'"), oldname);
> }
>
> /*
> @@ -624,32 +624,32 @@ static void copy_or_rename_branch(const char
> *oldname, const char *newname, int
>
> if (!copy && !(oldref_usage & IS_ORPHAN) &&
> rename_ref(oldref.buf, newref.buf, logmsg.buf))
> - die(_("Branch rename failed"));
> + die(_("branch rename failed"));
> if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
> - die(_("Branch copy failed"));
> + die(_("branch copy failed"));
>
> if (recovery) {
> if (copy)
> - warning(_("Created a copy of a misnamed branch '%s'"),
> + warning(_("created a copy of a misnamed branch '%s'"),
> interpreted_oldname);
> else
> - warning(_("Renamed a misnamed branch '%s' away"),
> + warning(_("renamed a misnamed branch '%s' away"),
> interpreted_oldname);
> }
>
> if (!copy && (oldref_usage & IS_HEAD) &&
> replace_each_worktree_head_symref(worktrees, oldref.buf,
> newref.buf,
> logmsg.buf))
> - die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
> + die(_("branch renamed to %s, but HEAD is not updated"), newname);
>
> strbuf_release(&logmsg);
>
> strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> if (!copy && git_config_rename_section(oldsection.buf,
> newsection.buf) < 0)
> - die(_("Branch is renamed, but update of config-file failed"));
> + die(_("branch is renamed, but update of config-file failed"));
> if (copy && strcmp(interpreted_oldname, interpreted_newname) &&
> git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> - die(_("Branch is copied, but update of config-file failed"));
> + die(_("branch is copied, but update of config-file failed"));
> strbuf_release(&oldref);
> strbuf_release(&newref);
> strbuf_release(&oldsection);
> @@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const
> char *prefix)
>
> head = resolve_refdup("HEAD", 0, &head_oid, NULL);
> if (!head)
> - die(_("Failed to resolve HEAD as a valid ref."));
> + die(_("failed to resolve HEAD as a valid ref"));
> if (!strcmp(head, "HEAD"))
> filter.detached = 1;
> else if (!skip_prefix(head, "refs/heads/", &head))
> @@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const
> char *prefix)
>
> if (!argc) {
> if (filter.detached)
> - die(_("Cannot give description to detached HEAD"));
> + die(_("cannot give description to detached HEAD"));
> branch_name = head;
> } else if (argc == 1) {
> strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
> @@ -878,8 +878,8 @@ int cmd_branch(int argc, const char **argv, const
> char *prefix)
> strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
> if (!ref_exists(branch_ref.buf))
> error((!argc || branch_checked_out(branch_ref.buf))
> - ? _("No commit on branch '%s' yet.")
> - : _("No branch named '%s'."),
> + ? _("no commit on branch '%s' yet")
> + : _("no branch named '%s'"),
> branch_name);
> else if (!edit_branch_description(branch_name))
> ret = 0; /* happy */
> @@ -892,8 +892,8 @@ int cmd_branch(int argc, const char **argv, const
> char *prefix)
> if (!argc)
> die(_("branch name required"));
> else if ((argc == 1) && filter.detached)
> - die(copy? _("cannot copy the current branch while not on any.")
> - : _("cannot rename the current branch while not on any."));
> + die(copy? _("cannot copy the current branch while not on any")
> + : _("cannot rename the current branch while not on any"));
> else if (argc == 1)
> copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
> else if (argc == 2)
> @@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv,
> const char *prefix)
> if (!branch) {
> if (!argc || !strcmp(argv[0], "HEAD"))
> die(_("could not set upstream of HEAD to %s when "
> - "it does not point to any branch."),
> + "it does not point to any branch"),
> new_upstream);
> die(_("no such branch '%s'"), argv[0]);
> }
>
> if (!ref_exists(branch->refname)) {
> if (!argc || branch_checked_out(branch->refname))
> - die(_("No commit on branch '%s' yet."), branch->name);
> + die(_("no commit on branch '%s' yet"), branch->name);
> die(_("branch '%s' does not exist"), branch->name);
> }
>
> @@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv,
> const char *prefix)
> if (!branch) {
> if (!argc || !strcmp(argv[0], "HEAD"))
> die(_("could not unset upstream of HEAD when "
> - "it does not point to any branch."));
> + "it does not point to any branch"));
> die(_("no such branch '%s'"), argv[0]);
> }
>
> if (!branch_has_merge_config(branch))
> - die(_("Branch '%s' has no upstream information"), branch->name);
> + die(_("branch '%s' has no upstream information"), branch->name);
>
> strbuf_reset(&buf);
> strbuf_addf(&buf, "branch.%s.remote", branch->name);
> @@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv,
> const char *prefix)
> const char *start_name = argc == 2 ? argv[1] : head;
>
> if (filter.kind != FILTER_REFS_BRANCHES)
> - die(_("The -a, and -r, options to 'git branch' do not take a branch
> name.\n"
> + die(_("the -a, and -r, options to 'git branch' do not take a branch
> name.\n"
> "Did you mean to use: -a|-r --list <pattern>?"));
>
> if (track == BRANCH_TRACK_OVERRIDE)
> - die(_("the '--set-upstream' option is no longer supported. Please
> use '--track' or '--set-upstream-to' instead."));
> + die(_("the '--set-upstream' option is no longer supported. Please
> use '--track' or '--set-upstream-to' instead"));
>
> if (recurse_submodules) {
> create_branches_recursively(the_repository, branch_name,
> diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
> index 469443d8ae..f6835c91dc 100755
> --- a/t/t2407-worktree-heads.sh
> +++ b/t/t2407-worktree-heads.sh
> @@ -45,7 +45,7 @@ test_expect_success 'refuse to overwrite: checked
> out in worktree' '
> grep "cannot force update the branch" err &&
>
> test_must_fail git branch -D wt-$i 2>err &&
> - grep "Cannot delete branch" err || return 1
> + grep "cannot delete branch" err || return 1
> done
> '
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 080e4f24a6..3182abde27 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -291,10 +291,10 @@ test_expect_success 'git branch -M topic topic
> should work when main is checked
> test_expect_success 'git branch -M and -C fail on detached HEAD' '
> git checkout HEAD^{} &&
> test_when_finished git checkout - &&
> - echo "fatal: cannot rename the current branch while not on any."
> >expect &&
> + echo "fatal: cannot rename the current branch while not on any"
> >expect &&
> test_must_fail git branch -M must-fail 2>err &&
> test_cmp expect err &&
> - echo "fatal: cannot copy the current branch while not on any."
> >expect &&
> + echo "fatal: cannot copy the current branch while not on any" >expect
> &&
> test_must_fail git branch -C must-fail 2>err &&
> test_cmp expect err
> '
> @@ -943,7 +943,7 @@ test_expect_success 'deleting currently checked
> out branch fails' '
> git worktree add -b my7 my7 &&
> test_must_fail git -C my7 branch -d my7 &&
> test_must_fail git branch -d my7 2>actual &&
> - grep "^error: Cannot delete branch .my7. used by worktree at " actual
> &&
> + grep "^error: cannot delete branch .my7. used by worktree at " actual
> &&
> rm -r my7 &&
> git worktree prune
> '
> @@ -954,7 +954,7 @@ test_expect_success 'deleting in-use branch fails'
> '
> git -C my7 bisect start HEAD HEAD~2 &&
> test_must_fail git -C my7 branch -d my7 &&
> test_must_fail git branch -d my7 2>actual &&
> - grep "^error: Cannot delete branch .my7. used by worktree at " actual
> &&
> + grep "^error: cannot delete branch .my7. used by worktree at " actual
> &&
> rm -r my7 &&
> git worktree prune
> '
> @@ -1024,7 +1024,7 @@ test_expect_success '--set-upstream-to fails on
> multiple branches' '
> test_expect_success '--set-upstream-to fails on detached HEAD' '
> git checkout HEAD^{} &&
> test_when_finished git checkout - &&
> - echo "fatal: could not set upstream of HEAD to main when it does not
> point to any branch." >expect &&
> + echo "fatal: could not set upstream of HEAD to main when it does not
> point to any branch" >expect &&
> test_must_fail git branch --set-upstream-to main 2>err &&
> test_cmp expect err
> '
> @@ -1072,7 +1072,7 @@ test_expect_success 'use --set-upstream-to
> modify a particular branch' '
> '
>
> test_expect_success '--unset-upstream should fail if given a
> non-existent branch' '
> - echo "fatal: Branch '"'"'i-dont-exist'"'"' has no upstream
> information" >expect &&
> + echo "fatal: branch '"'"'i-dont-exist'"'"' has no upstream
> information" >expect &&
> test_must_fail git branch --unset-upstream i-dont-exist 2>err &&
> test_cmp expect err
> '
> @@ -1094,7 +1094,7 @@ test_expect_success 'test --unset-upstream on
> HEAD' '
> test_must_fail git config branch.main.remote &&
> test_must_fail git config branch.main.merge &&
> # fail for a branch without upstream set
> - echo "fatal: Branch '"'"'main'"'"' has no upstream information"
> >expect &&
> + echo "fatal: branch '"'"'main'"'"' has no upstream information"
> >expect &&
> test_must_fail git branch --unset-upstream 2>err &&
> test_cmp expect err
> '
> @@ -1108,7 +1108,7 @@ test_expect_success '--unset-upstream should
> fail on multiple branches' '
> test_expect_success '--unset-upstream should fail on detached HEAD' '
> git checkout HEAD^{} &&
> test_when_finished git checkout - &&
> - echo "fatal: could not unset upstream of HEAD when it does not point
> to any branch." >expect &&
> + echo "fatal: could not unset upstream of HEAD when it does not point
> to any branch" >expect &&
> test_must_fail git branch --unset-upstream 2>err &&
> test_cmp expect err
> '
> diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
> index b17f388f56..2cdb834b37 100755
> --- a/t/t3202-show-branch.sh
> +++ b/t/t3202-show-branch.sh
> @@ -10,7 +10,7 @@ GIT_TEST_DATE_NOW=1251660000; export
> GIT_TEST_DATE_NOW
> test_expect_success 'error descriptions on empty repository' '
> current=$(git branch --show-current) &&
> cat >expect <<-EOF &&
> - error: No commit on branch '\''$current'\'' yet.
> + error: no commit on branch '\''$current'\'' yet
> EOF
> test_must_fail git branch --edit-description 2>actual &&
> test_cmp expect actual &&
> @@ -21,7 +21,7 @@ test_expect_success 'error descriptions on empty
> repository' '
> test_expect_success 'fatal descriptions on empty repository' '
> current=$(git branch --show-current) &&
> cat >expect <<-EOF &&
> - fatal: No commit on branch '\''$current'\'' yet.
> + fatal: no commit on branch '\''$current'\'' yet
> EOF
> test_must_fail git branch --set-upstream-to=non-existent 2>actual &&
> test_cmp expect actual &&
> @@ -224,7 +224,7 @@ done
>
> test_expect_success 'error descriptions on non-existent branch' '
> cat >expect <<-EOF &&
> - error: No branch named '\''non-existent'\'.'
> + error: no branch named '\''non-existent'\''
> EOF
> test_must_fail git branch --edit-description non-existent 2>actual &&
> test_cmp expect actual
> @@ -238,7 +238,7 @@ test_expect_success 'fatal descriptions on
> non-existent branch' '
> test_cmp expect actual &&
>
> cat >expect <<-EOF &&
> - fatal: No branch named '\''non-existent'\''.
> + fatal: no branch named '\''non-existent'\''
> EOF
> test_must_fail git branch -c non-existent new-branch 2>actual &&
> test_cmp expect actual &&
> @@ -253,7 +253,7 @@ test_expect_success 'error descriptions on orphan
> branch' '
> test_branch_op_in_wt() {
> test_orphan_error() {
> test_must_fail git $* 2>actual &&
> - test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
> + test_i18ngrep "no commit on branch .orphan-branch. yet$" actual
> } &&
> test_orphan_error -C wt branch $1 $2 && # implicit
> branch
> test_orphan_error -C wt branch $1 orphan-branch $2 && # explicit
> branch
^ permalink raw reply
* [PATCH v3] builtin/branch.c: adjust error messages to coding guidelines
From: Isoken June Ibizugbe @ 2023-10-23 14:57 UTC (permalink / raw)
To: git; +Cc: Isoken June Ibizugbe, rjusto
As per the CodingGuidelines document, it is recommended that error messages such as die(), error() and warning(),
should start with a lowercase letter and should not end with a period.
This patch adjusts tests to match updated messages.
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
builtin/branch.c | 66 +++++++++++++++++++--------------------
t/t2407-worktree-heads.sh | 2 +-
t/t3200-branch.sh | 16 +++++-----
t/t3202-show-branch.sh | 10 +++---
4 files changed, 47 insertions(+), 47 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a..e7ee9bd0f1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
(head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != merged) {
if (merged)
warning(_("deleting branch '%s' that has been merged to\n"
- " '%s', but not yet merged to HEAD."),
+ " '%s', but not yet merged to HEAD"),
name, reference_name);
else
warning(_("not deleting branch '%s' that is not yet merged to\n"
- " '%s', even though it is merged to HEAD."),
+ " '%s', even though it is merged to HEAD"),
name, reference_name);
}
free(reference_name_to_free);
@@ -190,13 +190,13 @@ static int check_branch_commit(const char *branchname, const char *refname,
{
struct commit *rev = lookup_commit_reference(the_repository, oid);
if (!force && !rev) {
- error(_("Couldn't look up commit object for '%s'"), refname);
+ error(_("couldn't look up commit object for '%s'"), refname);
return -1;
}
if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
- error(_("The branch '%s' is not fully merged.\n"
+ error(_("the branch '%s' is not fully merged.\n"
"If you are sure you want to delete it, "
- "run 'git branch -D %s'."), branchname, branchname);
+ "run 'git branch -D %s'"), branchname, branchname);
return -1;
}
return 0;
@@ -207,7 +207,7 @@ static void delete_branch_config(const char *branchname)
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "branch.%s", branchname);
if (git_config_rename_section(buf.buf, NULL) < 0)
- warning(_("Update of config-file failed"));
+ warning(_("update of config-file failed"));
strbuf_release(&buf);
}
@@ -260,7 +260,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (kinds == FILTER_REFS_BRANCHES) {
const char *path;
if ((path = branch_checked_out(name))) {
- error(_("Cannot delete branch '%s' "
+ error(_("cannot delete branch '%s' "
"used by worktree at '%s'"),
bname.buf, path);
ret = 1;
@@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
&oid, &flags);
if (!target) {
if (remote_branch) {
- error(_("remote-tracking branch '%s' not found."), bname.buf);
+ error(_("remote-tracking branch '%s' not found"), bname.buf);
} else {
char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
char *virtual_target = resolve_refdup(virtual_name,
@@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
"Did you forget --remote?"),
bname.buf);
else
- error(_("branch '%s' not found."), bname.buf);
+ error(_("branch '%s' not found"), bname.buf);
FREE_AND_NULL(virtual_target);
}
ret = 1;
@@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
continue;
if (is_worktree_being_rebased(wt, target))
- die(_("Branch %s is being rebased at %s"),
+ die(_("branch %s is being rebased at %s"),
target, wt->path);
if (is_worktree_being_bisected(wt, target))
- die(_("Branch %s is being bisected at %s"),
+ die(_("branch %s is being bisected at %s"),
target, wt->path);
}
}
@@ -578,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (ref_exists(oldref.buf))
recovery = 1;
else
- die(_("Invalid branch name: '%s'"), oldname);
+ die(_("invalid branch name: '%s'"), oldname);
}
for (int i = 0; worktrees[i]; i++) {
@@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
if (oldref_usage & IS_HEAD)
- die(_("No commit on branch '%s' yet."), oldname);
+ die(_("no commit on branch '%s' yet"), oldname);
else
- die(_("No branch named '%s'."), oldname);
+ die(_("no branch named '%s'"), oldname);
}
/*
@@ -624,32 +624,32 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && !(oldref_usage & IS_ORPHAN) &&
rename_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch rename failed"));
+ die(_("branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch copy failed"));
+ die(_("branch copy failed"));
if (recovery) {
if (copy)
- warning(_("Created a copy of a misnamed branch '%s'"),
+ warning(_("created a copy of a misnamed branch '%s'"),
interpreted_oldname);
else
- warning(_("Renamed a misnamed branch '%s' away"),
+ warning(_("renamed a misnamed branch '%s' away"),
interpreted_oldname);
}
if (!copy && (oldref_usage & IS_HEAD) &&
replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
logmsg.buf))
- die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to %s, but HEAD is not updated"), newname);
strbuf_release(&logmsg);
strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
strbuf_addf(&newsection, "branch.%s", interpreted_newname);
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is renamed, but update of config-file failed"));
+ die(_("branch is renamed, but update of config-file failed"));
if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is copied, but update of config-file failed"));
+ die(_("branch is copied, but update of config-file failed"));
strbuf_release(&oldref);
strbuf_release(&newref);
strbuf_release(&oldsection);
@@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
head = resolve_refdup("HEAD", 0, &head_oid, NULL);
if (!head)
- die(_("Failed to resolve HEAD as a valid ref."));
+ die(_("failed to resolve HEAD as a valid ref"));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
@@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc) {
if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
+ die(_("cannot give description to detached HEAD"));
branch_name = head;
} else if (argc == 1) {
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
@@ -878,8 +878,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf))
error((!argc || branch_checked_out(branch_ref.buf))
- ? _("No commit on branch '%s' yet.")
- : _("No branch named '%s'."),
+ ? _("no commit on branch '%s' yet")
+ : _("no branch named '%s'"),
branch_name);
else if (!edit_branch_description(branch_name))
ret = 0; /* happy */
@@ -892,8 +892,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc)
die(_("branch name required"));
else if ((argc == 1) && filter.detached)
- die(copy? _("cannot copy the current branch while not on any.")
- : _("cannot rename the current branch while not on any."));
+ die(copy? _("cannot copy the current branch while not on any")
+ : _("cannot rename the current branch while not on any"));
else if (argc == 1)
copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
else if (argc == 2)
@@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not set upstream of HEAD to %s when "
- "it does not point to any branch."),
+ "it does not point to any branch"),
new_upstream);
die(_("no such branch '%s'"), argv[0]);
}
if (!ref_exists(branch->refname)) {
if (!argc || branch_checked_out(branch->refname))
- die(_("No commit on branch '%s' yet."), branch->name);
+ die(_("no commit on branch '%s' yet"), branch->name);
die(_("branch '%s' does not exist"), branch->name);
}
@@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not unset upstream of HEAD when "
- "it does not point to any branch."));
+ "it does not point to any branch"));
die(_("no such branch '%s'"), argv[0]);
}
if (!branch_has_merge_config(branch))
- die(_("Branch '%s' has no upstream information"), branch->name);
+ die(_("branch '%s' has no upstream information"), branch->name);
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.remote", branch->name);
@@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
+ die(_("the -a, and -r, options to 'git branch' do not take a branch name.\n"
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
- die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
+ die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead"));
if (recurse_submodules) {
create_branches_recursively(the_repository, branch_name,
diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
index 469443d8ae..f6835c91dc 100755
--- a/t/t2407-worktree-heads.sh
+++ b/t/t2407-worktree-heads.sh
@@ -45,7 +45,7 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '
grep "cannot force update the branch" err &&
test_must_fail git branch -D wt-$i 2>err &&
- grep "Cannot delete branch" err || return 1
+ grep "cannot delete branch" err || return 1
done
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 080e4f24a6..3182abde27 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -291,10 +291,10 @@ test_expect_success 'git branch -M topic topic should work when main is checked
test_expect_success 'git branch -M and -C fail on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: cannot rename the current branch while not on any." >expect &&
+ echo "fatal: cannot rename the current branch while not on any" >expect &&
test_must_fail git branch -M must-fail 2>err &&
test_cmp expect err &&
- echo "fatal: cannot copy the current branch while not on any." >expect &&
+ echo "fatal: cannot copy the current branch while not on any" >expect &&
test_must_fail git branch -C must-fail 2>err &&
test_cmp expect err
'
@@ -943,7 +943,7 @@ test_expect_success 'deleting currently checked out branch fails' '
git worktree add -b my7 my7 &&
test_must_fail git -C my7 branch -d my7 &&
test_must_fail git branch -d my7 2>actual &&
- grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
+ grep "^error: cannot delete branch .my7. used by worktree at " actual &&
rm -r my7 &&
git worktree prune
'
@@ -954,7 +954,7 @@ test_expect_success 'deleting in-use branch fails' '
git -C my7 bisect start HEAD HEAD~2 &&
test_must_fail git -C my7 branch -d my7 &&
test_must_fail git branch -d my7 2>actual &&
- grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
+ grep "^error: cannot delete branch .my7. used by worktree at " actual &&
rm -r my7 &&
git worktree prune
'
@@ -1024,7 +1024,7 @@ test_expect_success '--set-upstream-to fails on multiple branches' '
test_expect_success '--set-upstream-to fails on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: could not set upstream of HEAD to main when it does not point to any branch." >expect &&
+ echo "fatal: could not set upstream of HEAD to main when it does not point to any branch" >expect &&
test_must_fail git branch --set-upstream-to main 2>err &&
test_cmp expect err
'
@@ -1072,7 +1072,7 @@ test_expect_success 'use --set-upstream-to modify a particular branch' '
'
test_expect_success '--unset-upstream should fail if given a non-existent branch' '
- echo "fatal: Branch '"'"'i-dont-exist'"'"' has no upstream information" >expect &&
+ echo "fatal: branch '"'"'i-dont-exist'"'"' has no upstream information" >expect &&
test_must_fail git branch --unset-upstream i-dont-exist 2>err &&
test_cmp expect err
'
@@ -1094,7 +1094,7 @@ test_expect_success 'test --unset-upstream on HEAD' '
test_must_fail git config branch.main.remote &&
test_must_fail git config branch.main.merge &&
# fail for a branch without upstream set
- echo "fatal: Branch '"'"'main'"'"' has no upstream information" >expect &&
+ echo "fatal: branch '"'"'main'"'"' has no upstream information" >expect &&
test_must_fail git branch --unset-upstream 2>err &&
test_cmp expect err
'
@@ -1108,7 +1108,7 @@ test_expect_success '--unset-upstream should fail on multiple branches' '
test_expect_success '--unset-upstream should fail on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: could not unset upstream of HEAD when it does not point to any branch." >expect &&
+ echo "fatal: could not unset upstream of HEAD when it does not point to any branch" >expect &&
test_must_fail git branch --unset-upstream 2>err &&
test_cmp expect err
'
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index b17f388f56..2cdb834b37 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -10,7 +10,7 @@ GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
test_expect_success 'error descriptions on empty repository' '
current=$(git branch --show-current) &&
cat >expect <<-EOF &&
- error: No commit on branch '\''$current'\'' yet.
+ error: no commit on branch '\''$current'\'' yet
EOF
test_must_fail git branch --edit-description 2>actual &&
test_cmp expect actual &&
@@ -21,7 +21,7 @@ test_expect_success 'error descriptions on empty repository' '
test_expect_success 'fatal descriptions on empty repository' '
current=$(git branch --show-current) &&
cat >expect <<-EOF &&
- fatal: No commit on branch '\''$current'\'' yet.
+ fatal: no commit on branch '\''$current'\'' yet
EOF
test_must_fail git branch --set-upstream-to=non-existent 2>actual &&
test_cmp expect actual &&
@@ -224,7 +224,7 @@ done
test_expect_success 'error descriptions on non-existent branch' '
cat >expect <<-EOF &&
- error: No branch named '\''non-existent'\'.'
+ error: no branch named '\''non-existent'\''
EOF
test_must_fail git branch --edit-description non-existent 2>actual &&
test_cmp expect actual
@@ -238,7 +238,7 @@ test_expect_success 'fatal descriptions on non-existent branch' '
test_cmp expect actual &&
cat >expect <<-EOF &&
- fatal: No branch named '\''non-existent'\''.
+ fatal: no branch named '\''non-existent'\''
EOF
test_must_fail git branch -c non-existent new-branch 2>actual &&
test_cmp expect actual &&
@@ -253,7 +253,7 @@ test_expect_success 'error descriptions on orphan branch' '
test_branch_op_in_wt() {
test_orphan_error() {
test_must_fail git $* 2>actual &&
- test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
+ test_i18ngrep "no commit on branch .orphan-branch. yet$" actual
} &&
test_orphan_error -C wt branch $1 $2 && # implicit branch
test_orphan_error -C wt branch $1 orphan-branch $2 && # explicit branch
--
2.42.0.346.g24618a8a3e.dirty
^ permalink raw reply related
* Re: Outreachy Internship Project Timeline
From: Christian Couder @ 2023-10-23 14:47 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <CACS=G2zPH4+R6L8bQ8w=b8u1i6Zsj6xwMibghPKzPX7oGko_tw@mail.gmail.com>
On Sun, Oct 22, 2023 at 7:17 PM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> On the Outreachy website under the "Final application section", this
> statement appears:
> "Please work with your mentor to provide a timeline of the work you
> plan to accomplish on the project and what tasks you will finish at
> each step. Make sure take into account any time commitments you have
> during the Outreachy internship round. If you are still working on
> your contributions and need more time, you can leave this blank and
> edit your application later."
> Is there a link to where I can access the project timeline and
> subsequently edit it?
We don't provide a timeline. Providing one is part of writing an
application for the project and I am Ok to work with you on your
application by reviewing it.
I recently reviewed Isoken's application, see the thread starting with:
https://lore.kernel.org/git/CAJHH8bEfM8KmwhHX_Fmcb0A2zpr8L75vgNhfvZy-uitpSXNUvQ@mail.gmail.com/
and Luma's application, see the thread starting with:
https://lore.kernel.org/git/CAFR+8Dz717pcc2Lm_J29xxiBt-kUrMP4JAUbm=3XaJuJPYseHg@mail.gmail.com/
Hopefully my comments on these applications can help you get a better
idea of what we would like to see in your application.
See also https://git.github.io/General-Application-Information/,
especially the "Application (required)" section, about what should or
could be in your application.
Best,
Christian.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Dragan Simic @ 2023-10-23 14:34 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Jacob Stopak, git
In-Reply-To: <ZTZQZhtTxvGT/s81@ugly>
On 2023-10-23 12:52, Oswald Buddenhagen wrote:
> On Sun, Oct 22, 2023 at 02:55:05PM +0200, Dragan Simic wrote:
>> Oh, that's awesome and I'm really happy to be wrong with my broad
>> classification of VCS users. However, I still need to be convinced
>> further, and I'd assign your example as an exception to the rules,
>
> i don't see myself as exceptional at all in that regard.
> in fact, your second user group seems like unicorns, and the first
> like a disparaging attitude from an elitist. in reality, users lie on
> a spectrum of willingness to engage with the details of the tools they
> use, and that willingness is circumstantial. a tool that is
> forthcoming with information has a higher chance of being actively
> engaged.
Actually, I see myself as some kind of a slave worker who just keeps
typing on his keyboard and helps the elite (i.e. the "normal people") to
enjoy their lives. In other words, my viewpoint is totally opposite of
how you perceived it.
>> and because you use the command line a lot.
>>
> in my experience, this isn't uncommon for users of "discrete" vcs'es
> at all, even if they aren't too interested in the details. they just
> copy "magic incantations" from stackoverflow, etc. - disgusting,
> right? ;-)
Good point. Various commands are often simply copied and pasted with
little understanding. I guess that makes people content, as one of
their life choices. I can respect that.
>> I also ask myself why would I use git-gui or any other GUI utility?
>> To me, clicking on something that represents a file is often simply
>> wrong.
>
> that makes you an outlier. most people find point-and-click
> interaction rather intuitive and significantly more efficient than
> encoding their intent into character sequences.
Well, I guess I'm different. As I already wrote above, I see myself as
some kind of a "slave worker" who helps in enabling others (i.e. the
"elite") to do whatever they want.
> you should however consider whether your preferences are a good
> default for the wider audience, even within the context of the command
> line.
Actually, some of my own preferences for my environment, when it comes
to the git configuration, are not the defaults.
> i for one think that it would be a perfectly valid experiment to go
> all-in and beyond with jacob's proposal - _and make it the default_
> (when the output is a tty). more advanced users who feel annoyed would
> be expected to opt out of it via configuration, as they are for the
> advice messages. because it's really the same idea, only thought
> bigger.
I'd never support that, FWIW.
^ permalink raw reply
* Re: [RFC][Outreachy] Seeking Git Community Feedback on My Application
From: Christian Couder @ 2023-10-23 14:24 UTC (permalink / raw)
To: Isoken Ibizugbe; +Cc: git
In-Reply-To: <CAJHH8bEfM8KmwhHX_Fmcb0A2zpr8L75vgNhfvZy-uitpSXNUvQ@mail.gmail.com>
On Thu, Oct 19, 2023 at 11:26 AM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
>
> Dear Git Community and Mentors,
>
> I hope you're doing well. I'm excited to share my application draft
> for the Outreachy program with the Git project. Your feedback is
> invaluable, and I'm eager to align the project with the community's
> needs. Please review the attached draft and share your insights.
Thanks for your project application!
[...]
> Why am I interested in working with the Git chosen project?
>
> Git has been a cornerstone for software development, enabling
> developers worldwide to collaborate, innovate, and create exceptional
> software. I would say without Git, my journey to pursuing my software
> engineering career would be impossible, as I use it almost every day.
> Yet, in this constantly evolving landscape, there is always room for
> improvement, even in a well-established project. The Git project
> currently relies on end-to-end tests, and this is where I see an
> opportunity to make a profound impact. Being able to test libraries in
> isolation via unit tests or mocks speeds up determining the root cause
> of bugs. I am deeply passionate about contributing to this project and
> firmly believe in the power of open-source software and the collective
> intelligence of the community. A successful completion of this project
> will significantly improve Git's testing capabilities and bring the
> benefits of fewer errors, faster work and better testing for all
> parts.
Ok.
[...]
> Contributions to Git
>
> I have actively participated in Git's mailing list discussions and
> contributed to a micro-project;
>
> - builtin/branch.c: Adjust error messages such as die(), error(), and
> warning() messages used in branch, to conform to coding guidelines
> (https://lore.kernel.org/git/20231019084052.567922-1-isokenjune@gmail.com/)
> - Implemented changes to fix broken tests based on reviews from the
> community (https://lore.kernel.org/git/20231019084052.567922-1-isokenjune@gmail.com/)
> - In review.
Nice!
> Project Goals:
>
> - Improve Testing Efficiency: Transitioning from end-to-end tests to
> unit tests will enable more efficient testing of error conditions.
> - Codebase Stability: Unit tests enhance code stability and facilitate
> easier debugging through isolation.
> - Simplify Testing: Writing unit tests in pure C simplifies test
> setup, data passing, and reduces testing runtime by eliminating
> separate processes for each test.
Ok.
> Project Milestones:
>
> - Add useful tests of library-like code
> - Integrate with stdlib work
Not sure what you call "stdlib" here.
> - Run alongside regular make test target
>
> Project Timeline:
>
> 1. Oct 2 - Nov 20: Community Bonding
>
> - Understanding the structure of Git
> - Getting familiar with the code
I think some of this time is also spent on working on a microproject,
writing an application and perhaps doing other things that regular Git
developers do.
> 2. Dec 4 - Jan 15: Add useful tests of library-like code
>
> - Identify and document the current state of the tests in the Git
> t/helper directory.
It would be nice if you could already take a look at that and tell us
about it in your application. There are different things in t/helper.
Some are worth porting and others are not. You might not want (or have
time to) to classify everything right now, but if you can identify a
few of each kind, and use those, or just one of them, as an example,
that would be great.
> - Confirm the licensing and compatibility requirements for the chosen
> unit testing framework.
I think those who have been working on the unit test framework have
already done this.
> - Develop unit tests for these library-like components.
Not sure what are "these library-like components". An example would
perhaps help.
> - Execute the tests and ensure they cover various scenarios, including
> error conditions.
> - Run the tests and address any initial issues or bugs to ensure they
> work as intended.
Ok.
> - Document the new tests and their coverage.
What kind of documentation would that be?
> - Seek feedback and support from mentors and the Git community
>
> 3. Jan 15 - Feb 15: Integrate with Stdlib Work
>
> - Collaborate with the team working on standard library integration.
Not sure what "standard library". Actually, maybe you are talking
about the goal of having a "standard library" implementation for Git
which is described in this report from the Virtual Contributor's
Summit:
https://lore.kernel.org/git/ZRrfN2lbg14IOLiK@nand.local/
It's true that the unit test framework would help with that goal. So
yeah maybe you will have to collaborate with the team working on that
goal. I am not sure at what step the work on this library will be when
the internship will start though.
> - Ensure that the tests for library-like code align with stdlib work.
> - Verify that the tests effectively check the compatibility and
> interaction of the code with standard libraries.
> - Gather feedback and insights from the Git community on the
> integrated tests, addressing any concerns or suggestions.
Ok, but I think it would be more interesting to follow the steps with
an example test.
> 4. Feb 15 - March 1: Run Alongside Regular 'make test' Target and finalize
>
> - Configure the testing framework to run alongside the regular 'make
> test' target.
I think others will likely take care of that sooner.
> - Ensure that the new tests are included in the standard testing suite.
> - Execute 'make test' with the new tests and verify that they pass successfully.
> - Document the integration process and how the new tests are included
> in the standard testing procedure.
> - Perform comprehensive testing of the entire unit testing framework.
> - Ensure all migrated tests are working correctly within the new framework.
> - Document the entire process of migrating Git's tests
> - Prepare a final project report
Ok, but here also following an example test would be more interesting.
> Technical Requirements
>
> According to the documentation on the unit test project
> (https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc),
> the suggested best framework for the Git project is the "Custom TAP
> framework" (Phillip Wood's TAP implementation), as it aligns with
> Git's licensing requirements, is vendorable, and can be customized by
> Git's developers as needed, but it may require some additional
> development work for features like parallel execution and mock
> support, but it offers a strong foundation for unit testing within the
> Git project.
Yeah, right. Thanks for summarizing that document!
> Relevant Projects
>
> Simple shell - A project based on emulating a shell. It was a
> collaborative project which we managed using Git.
> (https://github.com/Junie06/simple_shell).
> This project was written in C, which allowed me to apply my C language
> knowledge, essential for Git projects.
> I'm proficient in using Shell for scripting, redirections, and
> permissions, as shown in my work
> (https://github.com/Junie06/alx-system_engineering-devops).
> Creating the simple shell project deepened my understanding of how
> shells work, and I even attempted to replicate a shell environment.
> Collaborating on the Simple Shell project reinforced my Git skills.
Ok, nice!
Best,
Christian.
^ permalink raw reply
* Re: [PATCH 01/11] t: add helpers to test for reference existence
From: Patrick Steinhardt @ 2023-10-23 13:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <xmqqmswfud7p.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 9775 bytes --]
On Wed, Oct 18, 2023 at 09:06:50AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > There are two major ways to check for the existence of a reference in
> > our tests:
> >
> > - `git rev-parse --verify` can be used to check for existence of a
> > reference. This only works in the case where the reference is well
> > formed though and resolves to an actual object ID. This does not
> > work with malformed reference names or invalid contents.
> >
> > - `test_path_is_file` can be used to check for existence of a loose
> > reference if it is known to not resolve to an actual object ID. It
> > by necessity reaches into implementation details of the reference
> > backend though.
>
> True. It would be ideal if we can limit the use of latter when we
> _care_ how the ref is stored (e.g., "we expect it to be stored as a
> loose ref, not packed"). "The ref R at must be pointing at the
> commit X" is better asserted by using the former (or "git show-ref")
> as we not just only want to see the .git/refs/R file holding the
> object name X, but also want to see the production git tools observe
> the same---if what rev-parse or show-ref observes is different from
> the expected state and they say ref R does not point at commit X, we
> should complain (rev-parse or show-ref may be broken in the version
> of Git being tested, but we can assume that their breakage will be
> caught elsewhere in the test suite as well, so as long as we trust
> them, using them as the validator is better than going into the
> implementation detail and assuming things like "new refs always
> appear as a loose ref" that we might want to change in the future).
>
> > Similarly, there are two equivalent ways to check for the absence of a
> > reference:
> >
> > - `test_must_fail git rev-parse` can be used to check for the
> > absence of a reference. It could fail due to a number of reasons
> > though, and all of these reasons will be thrown into the same bag
> > as an absent reference.
> >
> > - `test_path_is_missing` can be used to check explicitly for the
> > absence of a loose reference, but again reaches into internal
> > implementation details of the reference backend.
> >
> > So both our tooling to check for the presence and for the absence of
> > references in tests is lacking as either failure cases are thrown into
> > the same bag or we need to reach into internal implementation details of
> > the respective reference backend.
>
> > Introduce a new subcommand for our ref-store test helper that explicitly
> > checks only for the presence or absence of a reference. This addresses
> > these limitations:
> >
> > - We can check for the presence of references with malformed names.
>
> But for the purpose of tests, we can control the input. When we
> perform an operation that we expect a ref R to be created, we would
> know R is well formed and we can validate using a tool that we know
> would be broken when fed a malformed name. So I do not see this as
> a huge "limitation".
This is explicitly about the case where such a ref R is not well-formed
though. This limitation was mostly a problem in t1430-bad-ref-name.sh,
which verifies many such scenarios.
> > - We can check for the presence of references that don't resolve.
>
> Do you mean a dangling symbolic ref? We are using a wrong tool if
> you are using rev-parse for that, aren't we? Isn't symbolic-ref
> there for us for this exact use case? That is
Again, t1430-bad-ref-name.sh has been the inspiration for this:
```
$ git symbolic-ref refs/heads/bad...name refs/heads/master
$ git symbolic-ref refs/heads/bad...name
fatal: No such ref: refs/heads/bad...name
```
The mismatch that you can write but not read the reference is kind of
astonishing though. We could fix this limitation, but I think there were
more usecases than only bad reference names. I honestly can't quite
remember right now.
> > - We can explicitly handle the case where a reference is missing by
> > special-casing ENOENT errors.
>
> You probably know the error conditions refs_read_raw_ref() can be
> broken better than I do, but this feels a bit too intimate with how
> the method for the files backend happens to be implemented, which at
> the same time, can risk that [a] other backends can implement their
> "ref does not resolve to an object name---is it because it is
> missing?" report incorrectly and [b] we would eventually want to
> know error conditions other than "the ref requested is missing" and
> at that point we would need more "special casing", which does not
> smell easy to scale.
We actually rely on some of these error codes to be consistent across
backends. E.g. "refs.c" itself has higher-level logic that verifies
specific error codes when resolving symrefs. And as we explicitly made
these error codes part of the API design with `refs_read_raw_ref()` my
assumption is that any other backend needs to match the behaviour here.
I also think that this is a somewhat sane assumption to make. While it
may not be a good idea to tie this to standard error codes, the backend
should indeed be able to signal specific error cases to the caller. We
could refactor this to be more explicit about the expected failure cases
in the form of specialized error codes. But I'm not sure whether that
would be worth it for now, but it's sure something to keep in mind for
future patch series.
> > - We don't need to reach into implementation details of the backend,
> > which would allow us to use this helper for the future reftable
> > backend.
>
> This is exactly what we want to aim for.
>
> > Next to this subcommand we also provide two wrappers `test_ref_exists`
> > and `test_ref_missing` that make the helper easier to use.
>
> Hmmmm. This may introduce "who watches the watchers" problem, no?
> I briefly wondered if a better approach is to teach the production
> code, e.g., rev-parse, to optionally give more detailed diag. It
> essentially may be the same (making the code in test-ref-store.c
> added by this patch available from rev-parse, we would easily get
> there), so I do not think the distinction matters.
>
> > diff --git a/t/README b/t/README
> > index 61080859899..779f7e7dd86 100644
> > --- a/t/README
> > +++ b/t/README
> > @@ -928,6 +928,15 @@ see test-lib-functions.sh for the full list and their options.
> > committer times to defined state. Subsequent calls will
> > advance the times by a fixed amount.
> >
> > + - test_ref_exists <ref>, test_ref_missing <ref>
> > +
> > + Check whether a reference exists or is missing. In contrast to
> > + git-rev-parse(1), these helpers also work with invalid reference
> > + names and references whose contents are unresolvable. The latter
> > + function also distinguishes generic errors from the case where a
> > + reference explicitly doesn't exist and is thus safer to use than
> > + `test_must_fail git rev-parse`.
> > +
> > - test_commit <message> [<filename> [<contents>]]
> >
> > Creates a commit with the given message, committing the given
> > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> > index 48552e6a9e0..7400f560ab6 100644
> > --- a/t/helper/test-ref-store.c
> > +++ b/t/helper/test-ref-store.c
> > @@ -1,6 +1,6 @@
> > #include "test-tool.h"
> > #include "hex.h"
> > -#include "refs.h"
> > +#include "refs/refs-internal.h"
> > #include "setup.h"
> > #include "worktree.h"
> > #include "object-store-ll.h"
> > @@ -221,6 +221,30 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)
> > return ret;
> > }
> >
> > +static int cmd_ref_exists(struct ref_store *refs, const char **argv)
> > +{
> > + const char *refname = notnull(*argv++, "refname");
> > + struct strbuf unused_referent = STRBUF_INIT;
> > + struct object_id unused_oid;
> > + unsigned int unused_type;
> > + int failure_errno;
> > +
> > + if (refs_read_raw_ref(refs, refname, &unused_oid, &unused_referent,
> > + &unused_type, &failure_errno)) {
> > + /*
> > + * We handle ENOENT separately here such that it is possible to
> > + * distinguish actually-missing references from any kind of
> > + * generic error.
> > + */
> > + if (failure_errno == ENOENT)
> > + return 17;
>
> Can we tell between the cases where the ref itself is missing, and
> the requested ref is symbolic and points at a missing ref? This
> particular case might be OK, but there may other cases where this
> "special case" may not be narrow enough.
Yes, because `refs_read_raw_ref()` doesn't concern itself with recursive
resolving of the reference, which is done at a higher level. It only
reads and parses the reference without caring whether their target
actually exists.
> As long we are going to spend cycles to refine the classification of
> error conditions, which is a very good thing to aim for the reason
> described in the proposed log message, namely "rev-parse can fail
> for reasons other than the ref being absent", I have to wonder again
> that the fruit of such an effort should become available in the
> production code, instead of being kept only in test-tool.
Fair enough, I'm happy to lift this up into production code. I just
didn't think this would be all that useful in general, but I can see
that somebody might want to use such functionality as part of our
plumbing interfaces.
I wonder what the best spot would be for it. Should we add a new
`--exists` switch to git-rev-parse(1)?
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 01/11] t: add helpers to test for reference existence
From: Patrick Steinhardt @ 2023-10-23 13:58 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <CAPig+cTueWuiWiw9_vou0Ud71utVMRkk5tR==tfkkMXBj9qWxA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]
On Wed, Oct 18, 2023 at 01:08:45PM -0400, Eric Sunshine wrote:
> On Wed, Oct 18, 2023 at 1:35 AM Patrick Steinhardt <ps@pks.im> wrote:
> > Introduce a new subcommand for our ref-store test helper that explicitly
> > checks only for the presence or absence of a reference. This addresses
> > these limitations:
> > [...]
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> > @@ -221,6 +221,30 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)
> > +static int cmd_ref_exists(struct ref_store *refs, const char **argv)
> > +{
> > + const char *refname = notnull(*argv++, "refname");
> > + struct strbuf unused_referent = STRBUF_INIT;
> > + struct object_id unused_oid;
> > + unsigned int unused_type;
> > + int failure_errno;
> > +
> > + if (refs_read_raw_ref(refs, refname, &unused_oid, &unused_referent,
> > + &unused_type, &failure_errno)) {
> > + /*
> > + * We handle ENOENT separately here such that it is possible to
> > + * distinguish actually-missing references from any kind of
> > + * generic error.
> > + */
> > + if (failure_errno == ENOENT)
> > + return 17;
> > + return -1;
> > + }
> > +
> > + strbuf_release(&unused_referent);
> > + return 0;
> > +}
>
> Unless refs_read_raw_ref() guarantees that `unused_referent` remains
> unallocated upon failure[*], then the early returns inside the
> conditional leak the strbuf. True, the program is exiting immediately
> anyhow, so this (potential) leak isn't significant, but it seems odd
> to clean up in one case (return 0) but not in the others (return -1 &
> 17).
>
> [*] In my (admittedly brief) scan of the code and documentation, I
> didn't see any such promise.
Agreed, let's just be thorough and plug any potential memor leak here.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 08/11] t4207: delete replace references via git-update-ref(1)
From: Patrick Steinhardt @ 2023-10-23 13:58 UTC (permalink / raw)
To: Han-Wen Nienhuys; +Cc: git
In-Reply-To: <CAFQ2z_MiOazk8qLvEn5xfHyMwY=5pa6rEJD3vC3+rnHz4AeQ2Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]
On Wed, Oct 18, 2023 at 03:27:11PM +0200, Han-Wen Nienhuys wrote:
> On Wed, Oct 18, 2023 at 7:35 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > In t4207 we set up a set of replace objects via git-replace(1). Because
> > these references should not be impacting subsequent tests we also set up
> > some cleanup logic that deletes the replacement references via a call to
> > `rm -rf`. This reaches into the internal implementation details of the
> > reference backend and will thus break when we grow an alternative refdb
> > implementation.
> >
> > Refactor the tests to delete the replacement refs via Git commands so
> > that we become independent of the actual refdb that's in use. As we
> > don't have a nice way to delete all replacements or all references in a
> > certain namespace, we opt for a combination of git-for-each-ref(1) and
> > git-update-ref(1)'s `--stdin` mode.
>
> There is a test helper that can directly access the ref database,
> t/helper/test-ref-store.c.
>
> If you use that manipulate refs for testing purposes, you make the
> test independent of behavior git-for-each-ref/git-update-ref, which is
> what you want for testing replace-objects?
Is there any specific reason why we shouldn't be using
git-for-each-ref(1) or git-update-ref(1) here? Neither of those commands
are part of the system under test, as we rather care about git-log(1)
here. So as those commands are already being verified in other tests I
think it should be fine to assume that they work as intended here.
Happy to hear differing viewpoints though.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 04/11] t: convert tests to not write references via the filesystem
From: Patrick Steinhardt @ 2023-10-23 13:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <xmqqfs27r5ng.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 5886 bytes --]
On Wed, Oct 18, 2023 at 02:18:27PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > @@ -434,7 +432,7 @@ test_expect_success 'Query "main@{2005-05-28}" (past end of history)' '
> > test_i18ngrep -F "warning: log for ref $m unexpectedly ended on $ld" e
> > '
> >
> > -rm -f .git/$m .git/logs/$m expect
> > +git update-ref -d $m
>
> We are not clearing "expect" file. I do not know if it matters
> here, but I am only recording what I noticed.
Oops, will fix.
> > diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> > index 10a539158c4..5cce24f1006 100755
> > --- a/t/t1450-fsck.sh
> > +++ b/t/t1450-fsck.sh
> > @@ -115,15 +115,16 @@ test_expect_success 'zlib corrupt loose object output ' '
> > '
> >
> > test_expect_success 'branch pointing to non-commit' '
> > - git rev-parse HEAD^{tree} >.git/refs/heads/invalid &&
> > + tree_oid=$(git rev-parse --verify HEAD^{tree}) &&
> > + test-tool ref-store main update-ref msg refs/heads/invalid $tree_oid $ZERO_OID REF_SKIP_OID_VERIFICATION &&
>
> I have mixed feelings on this.
>
> In olden days, plumbing commands tended to allow to pass anything
> the user told them to use, but in more recent versions of Git, we,
> probably by mistake, managed to butcher some of the plumbing
> commands to make them unable to deliberately "break" repositories,
> one victim being "update-ref", i.e.
>
> $ git update-ref refs/heads/invalid HEAD^{tree}
>
> is rejected these days (I just checked with v1.3.0 and it allows me
> to do this), and that is one of the reasons why we manually broke
> the repository in these tests.
My first try was indeed to use git-update-ref(1) to update the ref to
the tree object, and I was surprised to learn that it would not let me
do so.
> We need to have a warning message in
> comments near the implementation of "ref-store update-ref" that says
> never ever attempt to share code with the production version of
> update-ref---otherwise this (or the "safety" given to the plumbing
> command, possibly by mistake) will be broken, depending on which
> direction such a sharing goes. On the other hand, forcing us to
> keep two separate implementations, one deliberately loose to allow
> us corrupt repositories, the other for production and actively used,
> would mean the former one that is only used for validation would risk
> bitrotting.
Wouldn't any eventual bitrot be contained by the tests though? As our
use of the test helper grows via this patch series its behaviour will
also be verified more thoroughly.
> > test_when_finished "git update-ref -d refs/heads/invalid" &&
>
> Not a problem this patch introduces, but I think it is a better
> discipline to have when_finished clean-up routine prepared before we
> do actual damage, i.e. if I were writing this test today from scratch,
> I would expect it to be before "git rev-parse >.git/refs/heads/invalid"
> is done.
I'll fix this while at it.
> > test_must_fail git fsck 2>out &&
> > test_i18ngrep "not a commit" out
> > '
>
> A #leftoverbit that is not relevant to the topic; we should clean
> these test_i18ngrep and replace them with a plain "grep".
>
> > test_expect_success 'HEAD link pointing at a funny object' '
> > - test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
> > - mv .git/HEAD .git/SAVED_HEAD &&
> > + saved_head=$(git rev-parse --verify HEAD) &&
> > + test_when_finished "git update-ref HEAD ${saved_head}" &&
> > echo $ZERO_OID >.git/HEAD &&
>
> Are you sure .git/HEAD when this test is entered is a detached HEAD?
> The title of the test says "HEAD link", and I take it to mean HEAD
> is a symlink, and we save it away, while we create a loose ref that
> points at 0{40} in a detached HEAD state. Actually, the original
> would also work if HEAD is detached on entry. In either case,
> moving SAVED_HEAD back to HEAD would restore the original state.
>
> But the updated one only works if HEAD upon entry is already
> detached. Is this intended?
Yes and no -- it's a reflection of the state when this test runs. One
problem in this test suite here is that many of the tests' states are
heavily interwoven with each other, which only makes this harder to
refactor without making any assumptions.
Well. Instead of restoring to whatever the state was previous to the
test we could also restore it to something sane-ish like "master". That
of course breaks other tests though... I'll investigate.
> > @@ -131,8 +132,8 @@ test_expect_success 'HEAD link pointing at a funny object' '
> > '
> >
> > test_expect_success 'HEAD link pointing at a funny place' '
> > - test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
> > - mv .git/HEAD .git/SAVED_HEAD &&
> > + saved_head=$(git rev-parse --verify HEAD) &&
> > + test_when_finished "git update-ref --no-deref HEAD ${saved_head}" &&
>
> Likewise. Use of "update-ref" in the previous one vs "update-ref
> --no-deref" in this one to recover from the damage the tests make
> makes me feel that we may be assuming too much.
>
> > echo "ref: refs/funny/place" >.git/HEAD &&
>
> Even though "git symbolic-ref" refuses to point HEAD outside refs/,
> as plumbing command should, it allows it to point it outside refs/heads/.
> so this line should probably become
>
> git symbolic-ref HEAD refs/funny/place
>
> in the same spirit as the rest of the series.
Yup, this will be adressed in a later patch.
Patrick
> > @@ -391,7 +393,7 @@ test_expect_success 'tag pointing to nonexistent' '
> >
> > tag=$(git hash-object -t tag -w --stdin <invalid-tag) &&
> > test_when_finished "remove_object $tag" &&
> > - echo $tag >.git/refs/tags/invalid &&
> > + git update-ref refs/tags/invalid $tag &&
>
> Good (not just this one, but similar ones throughout this patch).
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 04/11] t: convert tests to not write references via the filesystem
From: Patrick Steinhardt @ 2023-10-23 13:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <xmqq1qdru6ds.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 4699 bytes --]
On Wed, Oct 18, 2023 at 11:34:23AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > -test_expect_success "fail to create $n" '
> > - test_when_finished "rm -f .git/$n_dir" &&
> > - touch .git/$n_dir &&
> > - test_must_fail git update-ref $n $A
> > +test_expect_success "fail to create $n due to file/directory conflict" '
> > + test_when_finished "git update-ref -d refs/heads/gu" &&
> > + git update-ref refs/heads/gu $A &&
> > + test_must_fail git update-ref refs/heads/gu/fixes $A
> > '
>
> OK, the original checks "if a random garbage file, which may not
> necessarily be a ref, exists at $n_dir, we cannot create a ref at
> $n_dir/fixes, due to D/F conflict" more directly, but as long as our
> intention is to enforce the D/F restriction across different ref
> backends [*], creating a ref at $n_dir and making sure $n_dir/fixes
> cannot be created is an equivalent check that is better (because it
> can be applied for other backends).
>
> Side note: there is no fundamental need to, though, and there
> are cases where being able to have the 'seen' branch and
> 'seen/ps/ref-test-tools' branches at the same time is
> beneficial---packed-refs and ref-table backends would not
> have such an inherent limitation, but they can of course be
> castrated to match what files-backend can(not) do.
I think initially it is beneficial to keep any such restriction and cut
back new backends to match them, even though it's more work. The main
reason why I think this makes sense is that hosting providers are the
most likely to update to the newer backend fast. It would be detrimental
to the users though if hosting providers that converted to the newer
backend were able to store such references that would be conflicting
with the files backend because they wouldn't be able to clone such
repositories anymore. And this isn't only true for hosting providers,
but essentially whenever two repositories that use different backends
try to interact with each other.
So even though I wish for a future where we don't need to care for D/F
conflicts anymore, I think the time isn't ripe for it and we should for
now aim for matching behaviour.
> > @@ -222,7 +220,7 @@ test_expect_success 'delete symref without dereference when the referred ref is
> >
> > test_expect_success 'update-ref -d is not confused by self-reference' '
> > git symbolic-ref refs/heads/self refs/heads/self &&
> > - test_when_finished "rm -f .git/refs/heads/self" &&
> > + test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF refs/heads/self" &&
> > test_path_is_file .git/refs/heads/self &&
>
> I trust that this will be corrected to use some wrapper around "git
> symbolic-ref" (or an equivalent for it as a test-tool subcommand) in
> some future patch, if not in this series?
Yup, this is getting fixed in a subsequent patch. I had two different
options to structure this series:
- Either by test file so that questions like this don't come up when
a reviewer sees in the context that there are still tests that
exercise the filesystem directly. This was my first approach.
- The alternative is to write it such that we address common
patterns globally, which I then rewrote my first version to.
There were two reasons why I didn't like the first iteration:
- Commit messages would basically have to reexplain the exact same
thing for every single testcase as the motivation is the same
everywhere.
- I think it's easier to review when you only see the same kind of
transformation per patch.
But yes, it does have the downside that the reader is now left wondering
why the other call to `test_path_is_file` still exists.
Patrick
> > test_must_fail git update-ref -d refs/heads/self &&
> > test_path_is_file .git/refs/heads/self
>
> Likewise.
>
> > @@ -230,7 +228,7 @@ test_expect_success 'update-ref -d is not confused by self-reference' '
> >
> > test_expect_success 'update-ref --no-deref -d can delete self-reference' '
> > git symbolic-ref refs/heads/self refs/heads/self &&
> > - test_when_finished "rm -f .git/refs/heads/self" &&
> > + test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF refs/heads/self" &&
> > test_path_is_file .git/refs/heads/self &&
> > git update-ref --no-deref -d refs/heads/self &&
> > test_must_fail git show-ref --verify -q refs/heads/self
>
> We already have the "ref is missing" test here.
>
> I'll stop at this point for now; will hopefully continue in a
> separate message later. Thanks.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 02/11] t: allow skipping expected object ID in `ref-store update-ref`
From: Patrick Steinhardt @ 2023-10-23 13:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <xmqqil73ud5j.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2507 bytes --]
On Wed, Oct 18, 2023 at 09:08:08AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > We require the caller to pass both the old and new expected object ID to
> > our `test-tool ref-store update-ref` helper. When trying to update a
> > symbolic reference though it's impossible to specify the expected object
> > ID, which means that the test would instead have to force-update the
> > reference. This is currently impossible though.
> >
> > Update the helper to optionally skip verification of the old object ID
> > in case the test passes in an empty old object ID as input.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > t/helper/test-ref-store.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
>
> Good.
>
> Even better would be to make the old one optional, though.
I was also a bit torn when writing this. We could of course make the
behaviour conditional on whether `argc` is 4 or 5. But I wasn't quite
sure how important it is to provide a nice UI for this test helper, and
we don't have `argc` readily available. It's not hard to count them
manually, but until now I was under the impression that the test helpers
only need to be "good enough".
Patrick
> > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> > index 7400f560ab6..7dc83137584 100644
> > --- a/t/helper/test-ref-store.c
> > +++ b/t/helper/test-ref-store.c
> > @@ -322,16 +322,19 @@ static int cmd_update_ref(struct ref_store *refs, const char **argv)
> > const char *new_sha1_buf = notnull(*argv++, "new-sha1");
> > const char *old_sha1_buf = notnull(*argv++, "old-sha1");
> > unsigned int flags = arg_flags(*argv++, "flags", transaction_flags);
> > - struct object_id old_oid;
> > + struct object_id old_oid, *old_oid_ptr = NULL;
> > struct object_id new_oid;
> >
> > - if (get_oid_hex(old_sha1_buf, &old_oid))
> > - die("cannot parse %s as %s", old_sha1_buf, the_hash_algo->name);
> > + if (*old_sha1_buf) {
> > + if (get_oid_hex(old_sha1_buf, &old_oid))
> > + die("cannot parse %s as %s", old_sha1_buf, the_hash_algo->name);
> > + old_oid_ptr = &old_oid;
> > + }
> > if (get_oid_hex(new_sha1_buf, &new_oid))
> > die("cannot parse %s as %s", new_sha1_buf, the_hash_algo->name);
> >
> > return refs_update_ref(refs, msg, refname,
> > - &new_oid, &old_oid,
> > + &new_oid, old_oid_ptr,
> > flags, UPDATE_REFS_DIE_ON_ERR);
> > }
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 00/11] t: reduce direct disk access to data structures
From: Patrick Steinhardt @ 2023-10-23 13:58 UTC (permalink / raw)
To: Han-Wen Nienhuys; +Cc: Junio C Hamano, git
In-Reply-To: <CAFQ2z_Om724+o+EG1FAhC9VrvJECnQ5UA+Z04Rzycpi_mXvMHg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3406 bytes --]
On Thu, Oct 19, 2023 at 12:13:12PM +0200, Han-Wen Nienhuys wrote:
> On Wed, Oct 18, 2023 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:
> > > this patch series refactors a bunch of our tests to perform less direct
> > > disk access to on-disk data structures. Instead, the tests are converted
> > > to use Git tools or our test-tool to access data to the best extent
> > > possible. This serves two benefits:
> >
> > Laudable goal.
> >
> > > - We increase test coverage of our own code base.
> >
> > Meaning the new code added to test-tool for this series will also
> > get tested and bugs spotted?
For now all the helpers of the test-tool only have implicit test
coverage, but I get your point. If we decide to instead transform this
test tool into production-level code as you suggested (e.g. `git
rev-parse --exists`) then this becomes less of a discussion point as we
would of course have proper test coverage for it.
> > > - We become less dependent on the actual on-disk format.
> >
> > Yes, this is very desirable. Without looking at the implementation,
> > I see some issues aiming for this goal may involve. [a] using the
> > production code for validation would mean our expectation to be
> > compared to the reality to be validated can be affected by the same
> > bug, making two wrongs to appear right; [b] using a separate
> > implementation used only for validation would at least mean we will
> > have to make the same mistake in unique part of both implementations
> > that is less likely to miss bugs compared to [a], but bugs in shared
> > part of the production code and validation code will be hidden the
> > same way as [a].
>
> I think it would be really great if there were separate unittests for
> the ref backend API. Some of the reftable work was needlessly
> difficult because the contract of the API was underspecified. The API
> is well compartmentalized in refs-internal.h, and a lot of the API
> behavior can be tested as a black box, eg.
>
> * setup symref HEAD pointing to R1
> * setup transaction updating ref R1 from C1 to C2
> * commit transaction, check that it succeeds
> * read ref R1, check if it is C2
> * read reflog for R1, see that it has a C1 => C2 update
> * read reflog for HEAD, see that it has a C1 => C2 update
>
> Tests for the loose/packed backend could directly mess with the
> on-disk files to test failure scenarios.
>
> With unittests like that, the tests can zoom in on the functionality
> of the ref backend, and provide more convenient coverage for
> dynamic/static analysis.
Agreed. Ideally, I think our tests should be split up into two layers:
1. Low-level tests which are backend specific. These allow us to
assert the on-disk state of the respective backend thoroughly and
also allow us to explicitly test for edge cases that are only of
relevance to this specific backend.
The reftable backend in its current (non-upstream) already has
such tests, but we don't really have explicit tests for the files
backend. This is a gap that should ideally be filled at some
point in time.
2. Higher-level tests should then be allowed to assume that the
underlying logic works as expected. These tests are thus free to
use plumbing tools that tie into the reference backends.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [Outreachy] Move existing tests to a unit testing framework
From: Christian Couder @ 2023-10-23 13:41 UTC (permalink / raw)
To: Achu Luma; +Cc: Junio C Hamano, git
In-Reply-To: <CAFR+8Dwxr3iV+R7een0t2sYXUWu1XHhQcLVuqMhOsSg9Bt4wrg@mail.gmail.com>
On Fri, Oct 20, 2023 at 3:16 PM Achu Luma <ach.lumap@gmail.com> wrote:
>
> Dear Git Community and Mentors,
>
> I hope this email finds you well. As a follow-up to my previous application, I'd like to provide additional details on the process of migrating existing unit tests from the t/helper/ directory to the new Git unit test framework.
Thanks for these details!
> -- Identify Target Unit Tests: Start by identifying the specific unit tests in the t/helper/ directory that we want to port to the new Git unit test framework. Ensure that the tests are suitable for migration and that the benefits of doing so outweigh the effort(By avoiding integration tests). The following points have been developed with on going work on the unit-tests framework visible here:
>
> 1- https://lore.kernel.org/git/0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com/
> 2- https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc
Maybe if you have time you could add some descriptions or comments
related to the above emails and documents. For example you could tell
what the new unit test framework will be like, how the unit tests will
look like, etc. Maybe a short overview would be nice.
You could also try to apply the patches in the series that adds the
test framework, or alternatively use the 'seen' branch where the
series has been merged, and start playing with it by writing, or
porting, a small example test.
> -- Create a New C Test File: For each unit test I plan to migrate, create a new C source file (.c) in the Git project's test suite directory(t/unit-tests). Name it appropriately to reflect the purpose of the test.
Could you provide an example of what the new name would be for an
existing test that is worth porting?
> -- Include Necessary Headers:In the new C test file, include the necessary Git unit test framework headers. Typically, this includes headers like "test-lib.h" and others relevant to the specific test.
> #include "test-lib.h"
Maybe you could continue the above example and tell which headers
would be needed for it?
> -- Convert Test Logic: Refactor the test logic from the original Shell script into the new C-based test format. Use the testing macros provided by the Git unit test framework, such as test_expect_success, test_expect_failure, etc., to define the tests.
> test_expect_success("simple progress display", "{
> // Test logic here...
> }");
Ok, a simple example would be nice too.
> -- Add Test Descriptions: Provide clear and informative descriptions for each test using the testing macros. These descriptions will help in identifying the purpose of each test when the test suite is run.
This would seem to be part of the previous step, as you would have to
provide a description when using the testing macro. But Ok.
> -- Define a Test Entry Point: Create a cmd_main function as the entry point for the C-based tests. Inside this function, include the test functions using the testing macros.
> int cmd_main(int argc, const char **argv) {
> // Test functions...
> return test_done();
> }
Yeah, continuing an example would be nice.
> -- Ensure TAP Format Output: Ensure that the C-based tests produce output in the Test Anything Protocol (TAP) format. This format includes the test name, status (ok or not ok), and any diagnostic information.
That means using TEST* macros in the cmd_main() function, as they
should do the right thing or is there more to be done here?
> -- Test Interaction: Ensure that the migrated tests interact correctly with the new Git unit test framework and any other tests that may be relevant. Consider dependencies and interactions with other parts of the Git project.
I am not sure what work would be needed here. Is there more to do than
compiling the test files? Having an example would be nice.
> -- Test Execution: Run the migrated tests to verify that they produce the expected results when executed as part of the Git project's test suite. Use the Git testing framework's test runners to execute the tests.
Ok.
> -- Documentation Update: Update the Git project's documentation to reflect the changes made during the migration. Include a reference to the original unit tests in the t/helper/ directory and indicate that these tests have been ported to the new Git unit test framework.
I am not sure that we would want that. I think we might instead want
to document things in t/helper/ that we don't want to port to the new
unit test framework and why.
> By following these points, I think I can successfully port existing unit tests from the t/helper/ directory to use the new Git unit test framework. This migration helps standardize and streamline the testing process within the Git project, improving code quality and maintainability.
Yeah!
> Next Steps:
>
> I am eager to discuss these suggestions and collaborate with the Git community to ensure the success of this project. I will continue to engage with the community, seek guidance, and refine my proposal as per your suggestions.
> I look forward to the opportunity to contribute to the Git project and help make it even more robust and reliable.
Thanks for this application and sorry for the late answer!
Best,
Christian.
^ permalink raw reply
* [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-23 13:00 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Phillip Wood, Christian Couder, Charvi Mendiratta,
Marc Branchaud
Create a clear top-down structure which makes it hopefully unambiguous
what happens when.
Also mention the timestamp along with the author - this is primarily
meant to include the keywords somebody might be searching for, like I
did a year ago.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v2:
- slight adjustments inspired by marc. however, i left most things
unchanged or even went in the opposite direction, because i assume the
readers to be sufficiently context-sensitive, and the objective is
merely to be not actively confusing. adding redundancy in the name of
clarity would just make the text stylistically inferior and arguably
harder to read.
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
Cc: Christian Couder <christian.couder@gmail.com>
Cc: Charvi Mendiratta <charvi077@gmail.com>
Cc: Marc Branchaud <marcnarc@xiplink.com>
---
Documentation/git-rebase.txt | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e7b39ad244..337df9ef2f 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -890,20 +890,21 @@ command "pick" with the command "reword".
To drop a commit, replace the command "pick" with "drop", or just
delete the matching line.
-If you want to fold two or more commits into one, replace the command
-"pick" for the second and subsequent commits with "squash" or "fixup".
-If the commits had different authors, the folded commit will be
-attributed to the author of the first commit. The suggested commit
-message for the folded commit is the concatenation of the first
-commit's message with those identified by "squash" commands, omitting the
-messages of commits identified by "fixup" commands, unless "fixup -c"
-is used. In that case the suggested commit message is only the message
-of the "fixup -c" commit, and an editor is opened allowing you to edit
-the message. The contents (patch) of the "fixup -c" commit are still
-incorporated into the folded commit. If there is more than one "fixup -c"
-commit, the message from the final one is used. You can also use
-"fixup -C" to get the same behavior as "fixup -c" except without opening
-an editor.
+If you want to fold two or more commits into one (that is, to combine
+their contents/patches), replace the command "pick" for the second and
+subsequent commits with "squash" or "fixup".
+The commit message for the folded commit is the concatenation of the
+message of the first commit with those of commits identified by "squash"
+commands, omitting those of commits identified by "fixup" commands,
+unless "fixup -c" is used. In the latter case, the message is obtained
+only from the "fixup -c" commit (having more than one of these is
+incorrect).
+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 first commit which contributes to the suggested commit message also
+determines the author, along with the date/timestamp.
`git rebase` will stop when "pick" has been replaced with "edit" or
when a command fails due to merge errors. When you are done editing
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related
* Re: [PATCH 00/11] t: reduce direct disk access to data structures
From: Patrick Steinhardt @ 2023-10-23 11:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Han-Wen Nienhuys, Jacob Keller
In-Reply-To: <xmqqlebzzeh8.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]
On Wed, Oct 18, 2023 at 04:40:35PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > As usual, I forgot to mention that this applies on top of current
> > master, which is at a9ecda2788 (The eighteenth batch, 2023-10-13) at the
> > time of writing. I look forward to the day where I remember to include
> > this information in the cover letter...
>
> I heard rumors that we have a --base option that records such a
> piece of information in the format-patch output (I haven't used it
> myself). Would it have helped, or perhaps it still needs some
> usability tweak?
I was aware of the option, but somehow never thought to use it. I guess
others are in the same boat as this is a semi-frequent topic to come up
on the mailing list.
What will fix this for me from now on is that I've now discovered the
`format.useAutoBase` config. To fix this for everybody else without
having to go through the same process we could change the default to
`format.useAutoBase=auto`. This can lead to hard-to-understand errors as
pointed out by 7efba5fa39f (format-patch: teach format.useAutoBase
"whenAble" option, 2020-10-01) though:
```
$ git format-patch -1 <an old commit>
fatal: base commit shouldn't be in revision list
```
But this was fixed via the new "whenAble" value for this configuration,
which will cause Git to ignore the case where it's unable to figure out
the base commit. So maybe we should consider to make it the new default?
I cannot think of any real downside. In the best case we simplify the
life of many maintainers of mailing list based projects. In the worst
case where we are unable to figure out the base commit nothing changes.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v1 4/4] maintenance: use XDG config if it exists
From: Eric Sunshine @ 2023-10-23 11:39 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Kristoffer Haugsbakk, git, stolee
In-Reply-To: <ZTZDsIcrB0zwHlFR@tanuki>
On Mon, Oct 23, 2023 at 5:58 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Wed, Oct 18, 2023 at 10:28:41PM +0200, Kristoffer Haugsbakk wrote:
> > `git maintenance register` registers the repository in the user's global
> > config. `$XDG_CONFIG_HOME/git/config` is supposed to be used if
> > `~/.gitconfig` does not exist. However, this command creates a
> > `~/.gitconfig` file and writes to that one even though the XDG variant
> > exists.
> >
> > This used to work correctly until 50a044f1e4 (gc: replace config
> > subprocesses with API calls, 2022-09-27), when the command started calling
> > the config API instead of git-config(1).
> >
> > Also change `unregister` accordingly.
> >
> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > ---
> > +test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '
> > + XDG_CONFIG_HOME=.config &&
> > + test_when_finished rm -r "$XDG_CONFIG_HOME"/git/config &&
> > + export "XDG_CONFIG_HOME" &&
>
> Also, I think we need to unset this variable at the end of this test as
> tests don't run in a subshell. [...]
Yup, well spotted. Almost the entire body of this test should be in a
subshell to ensure that the environment variable does not live beyond
the end of this test. But test_when_finished() can't be used in a
subshell, so a little care is needed:
test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '
test_when_finished rm -r .config/git/config &&
(
XDG_CONFIG_HOME=.config &&
...
)
'
> > + mkdir -p "$XDG_CONFIG_HOME"/git &&
> > + touch "$XDG_CONFIG_HOME"/git/config &&
If the timestamp of the file is not significant, then we use `>` to
create it rather than `touch`:
>"$XDG_CONFIG_HOME"/git/config &&
> > + git maintenance register &&
> > + git config --file="$XDG_CONFIG_HOME"/git/config --get maintenance.repo >actual &&
> > + pwd >expect &&
> > + test_cmp expect actual
> > +'
^ 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