* Re: [PATCH v3 1/5] config: split out config_parse_options
From: Taylor Blau @ 2023-10-23 18:46 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Josh Steadmon, git, calvinwan, glencbz, gitster
In-Reply-To: <20231023175217.984090-1-jonathantanmy@google.com>
On Mon, Oct 23, 2023 at 10:52:17AM -0700, Jonathan Tan wrote:
> Josh Steadmon <steadmon@google.com> writes:
> > From: Glen Choo <chooglen@google.com>
> >
> > "struct config_options" is a disjoint set of options used by the config
> > parser (e.g. event listeners) and options used by config_with_options()
> > (e.g. to handle includes, choose which config files to parse).
>
> Can this sentence be reworded? In particular, "disjoint" is a word
> normally applied to two or more sets (meaning that they have no elements
> in common), but here it is used for only one.
The pedant in me agrees with you. I do think that the sentence reads a
little awkwardly. Perhaps instead:
"struct config_options" has members which serve two distinct
purposes. There are a set of members used by the configuration parse
(e.g. event listeners). There is also a set used by
config_with_options() (e.g to handle includes, choose which config
files to parse).
> Everything else looks good, and the reasoning (some functions only use
> a subset of the fields, and this subset is easily explained conceptually
> as those related to parsing) makes sense.
Yup.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3 3/5] config: report config parse errors using cb
From: Jonathan Tan @ 2023-10-23 18:41 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jonathan Tan, git, calvinwan, glencbz, gitster
In-Reply-To: <a888045c04d27864edf5751ea8641fdba596779c.1695330852.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> From: Glen Choo <chooglen@google.com>
>
> In a subsequent commit, config parsing will become its own library, and
> it's likely that the caller will want flexibility in handling errors
> (instead of being limited to the error handling we have in-tree).
>
> Move the Git-specific error handling into a config_parser_event_fn_t
> that responds to config errors, and make git_parse_source() always
> return -1 (careful inspection shows that it was always returning -1
> already). This makes CONFIG_ERROR_SILENT obsolete since that is
> equivalent to not specifying an error event listener. Also, remove
> CONFIG_ERROR_UNSET and the config_source 'default', since all callers
> are now expected to specify the error handling they want.
I think this has to be better explained. So:
- There is already a config_parser_event_fn_t that can be configured
by a user to receive emitted config events. This callback can return
negative to halt further config parsing.
- Currently, it is git_parse_source() that detects when an error
occurs, and it emits a CONFIG_EVENT_ERROR and either dies, prints
an error, or swallows the error depending on error_action; no
matter what error_action is, it halts config parsing, as one would
expect. This commit moves the die/print/swallow handling to a
config_parser_event_fn_t that will see the CONFIG_EVENT_ERROR and die/
print/swallow.
- This new config_parser_event_fn_t does not need to swallow, since
that's the same as not passing in a callback. So it just needs to die/
print.
> @@ -1039,6 +1042,29 @@ static int do_event(struct config_source *cs, enum config_event_t type,
> return 0;
> }
>
> +static int do_event_and_flush(struct config_source *cs,
> + enum config_event_t type,
> + struct parse_event_data *data)
> +{
> + int maybe_ret;
> +
> + if ((maybe_ret = flush_event(cs, type, data)) < 1)
> + return maybe_ret;
> +
> + start_event(cs, type, data);
> +
> + if ((maybe_ret = flush_event(cs, type, data)) < 1)
> + return maybe_ret;
> +
> + /*
> + * Not actually EOF, but this indicates we don't have a valid event
> + * to flush next time around.
> + */
> + data->previous_type = CONFIG_EVENT_EOF;
> +
> + return 0;
> +}
A lot of this function only makes sense if the type is ERROR, so maybe
rename this as flush_and_emit_error() (and don't take in a type). As
it is, right now there is some confusion about how you can flush (I'm
referring to the second flush) with the same type as what you passed
to start_event().
Also, I don't think we should set data->previous_type here. Instead
there should be a comment saying that if you're emitting ERROR, you
should halt config parsing. The return value here is useless too (it
signals whether we should halt config parsing, but the caller should
always halt, so we don't need to return anything).
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Jeff King @ 2023-10-23 18:40 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <ZTJaVzt75r0iHPzR@ugly>
On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:
> On Fri, Oct 20, 2023 at 06:13:10AM -0400, Jeff King wrote:
> > But one thing that gives me pause is that the neither before or after
> > this patch do we handle continuation lines like:
> >
> > Subject: this is the beginning
> > and this is more subject
> >
> > And it would probably be a lot easier to add when storing the headers in
> > a hash (it's not impossible to do it the other way, but you basically
> > have to delay processing each line with a small state machine).
> >
> that seems like a rather significant point, doesn't it?
Maybe. It depends on whether anybody is interested in adding
continuation support. Nobody has in the previous 18 years, and nobody
has asked for it.
> > So another option is to just fix the individual bugs separately.
> >
> ... so that seems preferable to me, given that the necessary fixes seem
> rather trivial.
They're not too bad. Probably:
1. lc() the keys we put into the hash
2. match to/cc/bcc and dereference their arrays
3. maybe handle 'body' separately from headers to avoid confusion
But there may be other similar bugs lurking. One I didn't mention: the
hash-based version randomly reorders headers!
> > I guess "readable" is up for debate here, but I find the inline handling
> > a lot easier to follow
> >
> any particular reason for that?
For the reasons I gave in the commit message: namely that the matching
and logic is in one place and doesn't need to be duplicated (e.g., the
special handling of to/cc/bcc, which caused a bug here).
> > (and it's half as many lines; most of the diffstat is the new tests).
>
> > - if ($parsed_email{'From'}) {
> > - $sender = delete($parsed_email{'From'});
> > - }
>
> this verbosity could be cut down somewhat using just
>
> $sender = delete($parsed_email{'From'});
>
> and if the value can be pre-set and needs to be preserved,
>
> $sender = delete($parsed_email{'From'}) // $sender;
>
> but this seems kind of counter-productive legibility-wise.
We do need to avoid overwriting the pre-set value. The "//" one would
work, but we support perl versions old enough that they don't have it.
-Peff
^ permalink raw reply
* Re: [PATCH v2] upload-pack: add tracing for fetches
From: Robert Coup @ 2023-10-23 18:28 UTC (permalink / raw)
To: Taylor Blau, Jeff King; +Cc: git, Robert Coup via GitGitGadget
In-Reply-To: <pull.1598.v2.git.1697577168128.gitgitgadget@gmail.com>
Hi Jeff & Taylor,
Sorry to nag, but would it be possible to give this v2 patch a
look-over? Would be good to get it progressed.
Thanks,
Rob :)
On Tue, 17 Oct 2023 at 22:12, Robert Coup via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Changes since V1
> ================
>
> * Don't generate the JSON event unless Trace2 is active.
> * Code style fix.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Oswald Buddenhagen @ 2023-10-23 18:16 UTC (permalink / raw)
To: Jacob Stopak; +Cc: Dragan Simic, git
In-Reply-To: <ZTatzlzCkPOW3Rn7.jacob@initialcommit.io>
On Mon, Oct 23, 2023 at 10:30:54AM -0700, Jacob Stopak wrote:
>On Mon, Oct 23, 2023 at 04:34:15PM +0200, Dragan Simic wrote:
>> On 2023-10-23 12:52, Oswald Buddenhagen wrote:
>> > 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_
>>
>> I'd never support that, FWIW.
>
>FWIW, I'd _never suggest_ that.
>
why, though?
doing that would extend the feature's reach about two orders of
magnitude among newbies, which is where it matters most.
>I very much value Git's current usage and wouldn't dream to make this
>the default.
>
making the default output format somewhat more verbose wouldn't really
"change the usage", though. and being able to permanently get rid of it
with a single command should alleviate any _reasonable_ concerns about
habit disruption.
regards
^ permalink raw reply
* Re: [PATCH v3 2/5] config: split do_event() into start and flush operations
From: Jonathan Tan @ 2023-10-23 18:05 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jonathan Tan, git, calvinwan, glencbz, gitster
In-Reply-To: <8a1463c223497fca2fd3f11a54db5d7e52d1d08a.1695330852.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> +static void start_event(struct config_source *cs, enum config_event_t type,
> + struct parse_event_data *data)
> +{
> + data->previous_type = type;
> + data->previous_offset = get_corrected_offset(cs, type);
> +}
It's a pity that get_corrected_offset() has to be called twice (once
here and once below) but I think that's the best we can do given how the
code is laid out (and I can't think of a better code layout either).
> +static int flush_event(struct config_source *cs, enum config_event_t type,
> + struct parse_event_data *data)
One thing confusing here is that the "type" is not what's being flushed,
but used to change details about how we flush. Technically all we need
is is_whitespace_type and is_eof_type, but that's clumsier to code. I
think the best we can do is add some documentation to this function,
maybe 'Flush the event started by a prior start_event(), if one exists.
The type of the event being flushed is not "type" but the type that was
passed to the prior start_event(); "type" here may merely change how the
flush is performed' or something like that.
> +{
> + if (!data->opts || !data->opts->event_fn)
> + return 0;
> +
> + if (type == CONFIG_EVENT_WHITESPACE &&
> + data->previous_type == type)
> + return 0;
>
> if (data->previous_type != CONFIG_EVENT_EOF &&
> data->opts->event_fn(data->previous_type, data->previous_offset,
> - offset, cs, data->opts->event_fn_data) < 0)
> + get_corrected_offset(cs, type), cs,
> + data->opts->event_fn_data) < 0)
> return -1;
Another confusing point here is how EOF is used both to mean
"start_event() was never called" and a true EOF. I think for now it's
best to just document this where we define CONFIG_EVENT_EOF.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Dragan Simic @ 2023-10-23 17:59 UTC (permalink / raw)
To: Jacob Stopak; +Cc: Oswald Buddenhagen, git
In-Reply-To: <ZTatzlzCkPOW3Rn7.jacob@initialcommit.io>
On 2023-10-23 19:30, Jacob Stopak wrote:
> On Mon, Oct 23, 2023 at 04:34:15PM +0200, Dragan Simic wrote:
>> On 2023-10-23 12:52, Oswald Buddenhagen wrote:
>>> 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.
Perhaps my broad classification of git users wasn't that great, and I'm
actually happy for being wrong there.
Just to clarify, in general I'd support the inclusion of the table
output formatting, but I'd need to test it in detail before saying a
definite yes. It would also need to be more feature-complete, and the
original author of the patches should commit to maintaining it as the
new git feature. Having different options available can't hurt, IMHO.
However, having that as the default is something I'll never support.
All this is just my opinion.
^ permalink raw reply
* Re: [PATCH v3 1/5] config: split out config_parse_options
From: Jonathan Tan @ 2023-10-23 17:52 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jonathan Tan, git, calvinwan, glencbz, gitster
In-Reply-To: <fa55b7836f112cb7c7ab9b80e745b9969421c768.1695330852.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> From: Glen Choo <chooglen@google.com>
>
> "struct config_options" is a disjoint set of options used by the config
> parser (e.g. event listeners) and options used by config_with_options()
> (e.g. to handle includes, choose which config files to parse).
Can this sentence be reworded? In particular, "disjoint" is a word
normally applied to two or more sets (meaning that they have no elements
in common), but here it is used for only one.
Everything else looks good, and the reasoning (some functions only use
a subset of the fields, and this subset is easily explained conceptually
as those related to parsing) makes sense.
^ permalink raw reply
* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-23 17:52 UTC (permalink / raw)
To: phillip.wood
Cc: git, Junio C Hamano, Christian Couder, Charvi Mendiratta,
Marc Branchaud, Johannes Sixt
In-Reply-To: <a85c80eb-65ab-4b8c-ba94-de71516da5ef@gmail.com>
On Mon, Oct 23, 2023 at 05:01:02PM +0100, Phillip Wood wrote:
>On 23/10/2023 14:00, Oswald Buddenhagen wrote:
>> +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.
>
i know that this is the case, see the previous thread (which i failed to
link by header, cf.
https://lore.kernel.org/all/20231020092707.917514-1-oswald.buddenhagen@gmx.de/T/#u
).
>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"
>
a cleaner solution would be recognizing the situation and not generating
these contradicting commands in the first place. of course that would be
more complexity, but it would also allow catching accidental use.
of course i can go back to documenting the status quo, but it seems kind
of wrong.
>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?
>
uhm. we clearly do. that means i was given incorrect advice in
https://lore.kernel.org/all/YjXRM5HiRizZ035p@ugly/T/#u (and so the
thread is still looking for a resolution) ...
regards
^ permalink raw reply
* Re: [PATCH] doc/git-bisect: clarify `git bisect run` syntax
From: Junio C Hamano @ 2023-10-23 17:46 UTC (permalink / raw)
To: Javier Mora
Cc: Patrick Steinhardt, Eric Sunshine, cousteau via GitGitGadget, git
In-Reply-To: <CAH1-q0hrfROfQROXGoCfde4MFkEjxjSMneDcqLO1pqYpe+bN9g@mail.gmail.com>
Javier Mora <cousteaulecommandant@gmail.com> writes:
>> 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?
In this case, a single patch is the way to go; otherwise we will
(tentatively) be in an inconsistent state after applying one until
the other gets applied.
> There are still multiple .po files containing the old string, I guess
> I don't need to touch those?
Correct.
> 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.
bisect--helper has been retired but most of the messages used by it
should have been in use by bisect proper, so only the "this message
appears here" comments may be wrong.
In any case, touching po/ is not in the scope of this isolated fix.
The i18n group has their own workflows to update the files there,
and those touching the code and docs should not have to touch them
in general.
>> I wonder if we should eventually move these into the
>> proper SYNOPSIS section.
>
> Seems reasonable. I was actually wondering about that.
But not as a part of this isolated fix.
^ permalink raw reply
* 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
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