* Re: [PATCH v2 5/5] completion: add an use __git_compute_second_level_config_vars_for_section
From: Patrick Steinhardt @ 2024-02-08 7:42 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, Philippe Blain
In-Reply-To: <a2e792c911e1b9fa77d27ec327f6a9dfe06d4de4.1706534882.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]
On Mon, Jan 29, 2024 at 01:28:01PM +0000, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
[snip]
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 2934ceb7637..0e8fd63bfdb 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2605,6 +2612,15 @@ __git_compute_first_level_config_vars_for_section ()
> printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
> }
>
> +__git_compute_second_level_config_vars_for_section ()
> +{
> + section="$1"
This should be `local section`, as well.
> + __git_compute_config_vars_all
> + local this_section="__git_second_level_config_vars_for_section_${section}"
> + test -n "${!this_section}" ||
> + printf -v "__git_second_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars_all" | grep -E "^${section}\.<" | awk -F. '{print $3}')"
> +}
> +
> __git_config_sections=
> __git_compute_config_sections ()
> {
> @@ -2749,10 +2765,13 @@ __git_complete_config_variable_name ()
> done
>
> case "$cur_" in
> - branch.*.*)
> + branch.*.*|guitool.*.*|difftool.*.*|man.*.*|mergetool.*.*|remote.*.*|submodule.*.*|url.*.*)
> local pfx="${cur_%.*}."
> cur_="${cur_##*.}"
> - __gitcomp "remote pushRemote merge mergeOptions rebase" "$pfx" "$cur_" "$sfx"
> + local section="${pfx%.*.}"
> + __git_compute_second_level_config_vars_for_section "${section}"
> + local this_section="__git_second_level_config_vars_for_section_${section}"
> + __gitcomp "${!this_section}" "$pfx" "$cur_" "$sfx"
> return
> ;;
Nice.
[snip]
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index f28d8f531b7..24ff786b273 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2593,6 +2593,16 @@ test_expect_success 'git config - variable name - __git_compute_first_level_conf
> submodule.recurse Z
> EOF
> '
Missing a newline.
> +test_expect_success 'git config - variable name - __git_compute_second_level_config_vars_for_section' '
> + test_completion "git config branch.main." <<-\EOF
> + branch.main.description Z
> + branch.main.remote Z
> + branch.main.pushRemote Z
> + branch.main.merge Z
> + branch.main.mergeOptions Z
> + branch.main.rebase Z
> + EOF
> +'
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/1] completion: don't complete revs when --no-format-patch
From: Patrick Steinhardt @ 2024-02-08 7:57 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git
In-Reply-To: <a718b5ee-afb0-44bd-a299-3208fac43506@smtp-relay.sendinblue.com>
[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]
On Mon, Jan 08, 2024 at 04:08:30PM -0900, Britton Leo Kerin wrote:
> In this case the user has specifically said they don't want send-email
> to run format-patch so revs aren't valid argument completions (and it's
> likely revs and dirs do have some same names or prefixes as in
> Documentation/MyFirstContribution.txt 'psuh').
>
> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> ---
> contrib/completion/git-completion.bash | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 185b47d802..c983f3b2ab 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1242,10 +1242,12 @@ __git_find_last_on_cmdline ()
> while test $# -gt 1; do
> case "$1" in
> --show-idx) show_idx=y ;;
> + --) shift && break ;;
> *) return 1 ;;
> esac
> shift
> done
> + [ $# -eq 1 ] || return 1 # return 1 if we got wrong # of non-opts
> local wordlist="$1"
>
> while [ $c -gt "$__git_cmd_idx" ]; do
> @@ -2429,7 +2431,9 @@ _git_send_email ()
> return
> ;;
> esac
> - __git_complete_revlist
> + if [ "$(__git_find_last_on_cmdline -- "--format-patch --no-format-patch")" != "--no-format-patch" ]; then
> + __git_complete_revlist
> + fi
> }
While this second hunk here makes perfect sense to me, there is no
explanation why we need to change `__git_find_last_on_cmdline ()`. It's
already used with "--guess --no-guess" in another place, so I would
think that it ought to work alright for this usecase, too. Or is it that
the existing callsite of this function is buggy, too? If so, we should
likely fix that in a separate patch together with a test.
Also, adding a test for git-send-email that exercises this new behaviour
would be very much welcome, too.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 01/30] object-file-convert: stubs for converting from one object format to another
From: Linus Arver @ 2024-02-08 8:23 UTC (permalink / raw)
To: Eric W. Biederman, Junio C Hamano
Cc: git, brian m. carlson, Eric Sunshine, Eric W. Biederman
In-Reply-To: <20231002024034.2611-1-ebiederm@gmail.com>
"Eric W. Biederman" <ebiederm@gmail.com> writes:
> From: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Two basic functions are provided:
> - convert_object_file Takes an object file it's type and hash algorithm
> and converts it into the equivalent object file that would
> have been generated with hash algorithm "to".
Should probably be
- convert_object_file takes an object file, its type, and hash algorithm
and converts it into the equivalent object file using hash
algorithm "to".
It would be nice if you gave the function name some clue of what sort of
conversion is being done though. Something like
"convert_object_file_hash" or "switch_object_file_hash". I like "switch"
because the word "hash" itself means to convert some input bytes into
another set of (aka "hashed") bytes, and the indirect shadowing of
"convert" in this way can be avoided here by using a different word.The
whole point of this function is to switch the hashing scheme from one to
another, while still keeping everything else the same, so "switch" seems
more appropriate.
Unless, of course, we already pervasively use "convert" this way
elsewhere in the codebase (I have not checked).
> For blob objects there is no conversation to be done and it is an
s/conversation/conversion
> error to use this function on them.
Could you explain why no conversion is needed for blob objects, and also
why it should be an error (and not just a NOP)?
In the code we can also call BUG() if the from/to algos are the same.
It's probably worth mentioning in here as well?
Also for such detailed explanations, I think it's much better
to place them as comments directly above the function (and only mention
the important bits about these helper functions, other than the fact
that they will come in handy in later patches, in the log message).
> For commit, tree, and tag objects embedded oids are replaced by the
> oids of the objects they refer to with those objects and their
> object ids reencoded in with the hash algorithm "to".
That's a little wordy. I assume embedded oids just mean oids that these
objects refer to (e.g., commit objects have oids, but for example the
tree referred by a commit would be an example of an embedded oid). If
so, then how about just
For commit, tree, and tag objects both their oids and embedded
(dependent) oids are converted using hash algorithm "to".
I dropped "reencoded" in favor of "converted" btecause that's the verb
you use in your function name "convert_object_file()".
> Signatures
> are rearranged so that they remain valid after the object has
> been reencoded.
Maybe s/reencoded/converted here as well? But also, it sounds odd to me
that signatures are simply "rearranged" and not "regenerated" or
"recreated" becaues "rearranged" means keeping most of the old stuff
around but just repositioning them, which doesn't sound like it's doing
justice to the meaning of a hash algo transition.
> - repo_oid_to_algop which takes an oid that refers to an object file
> and returns the oid of the equivalent object file generated
> with the target hash algorithm.
The name is odd to me because "repo_oid" doesn't make sense (a repo
is not an object so it doesn't have an oid), but also the "to_algop"
name (AFAICS "algop" just means "pointer to algorithm" in the codebase,
for examle in <hash-ll.h>).
Here's a possible rewording:
- switch_oid_hash takes an object file's oid and returns a new one
using hash algorithm "to".
> The pair of files object-file-convert.c and object-file-convert.h are
> introduced to hold as much of this logic as possible to keep this
> conversion logic cleanly separated from everything else and in the
> hopes that someday the code will be clean enough git can support
Did you mean "clean enough so that Git ..."?
> compiling out support for sha1 and the various conversion functions.
FYI you can cut down this ambitious sentence into two for readability,
like this:
The new files object-file-convert.{c,h} hold as much of this logic
as possible to keep this conversion logic cleanly separated from
everything else. This separation will help us easily phase out SHA1
support (perhaps as a compile-time flag) in the future.
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> Makefile | 1 +
> object-file-convert.c | 57 +++++++++++++++++++++++++++++++++++++++++++
> object-file-convert.h | 24 ++++++++++++++++++
> 3 files changed, 82 insertions(+)
> create mode 100644 object-file-convert.c
> create mode 100644 object-file-convert.h
>
> diff --git a/Makefile b/Makefile
> index 577630936535..f7e824f25cda 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1073,6 +1073,7 @@ LIB_OBJS += notes-cache.o
> LIB_OBJS += notes-merge.o
> LIB_OBJS += notes-utils.o
> LIB_OBJS += notes.o
> +LIB_OBJS += object-file-convert.o
> LIB_OBJS += object-file.o
> LIB_OBJS += object-name.o
> LIB_OBJS += object.o
> diff --git a/object-file-convert.c b/object-file-convert.c
> new file mode 100644
> index 000000000000..4777aba83636
> --- /dev/null
> +++ b/object-file-convert.c
> @@ -0,0 +1,57 @@
> +#include "git-compat-util.h"
> +#include "gettext.h"
> +#include "strbuf.h"
> +#include "repository.h"
> +#include "hash-ll.h"
> +#include "object.h"
> +#include "object-file-convert.h"
> +
> +int repo_oid_to_algop(struct repository *repo, const struct object_id *src,
> + const struct git_hash_algo *to, struct object_id *dest)
> +{
> + /*
> + * If the source algorithm is not set, then we're using the
> + * default hash algorithm for that object.
> + */
> + const struct git_hash_algo *from =
> + src->algo ? &hash_algos[src->algo] : repo->hash_algo;
Hmm, if we're only using "repo" to grab its hash_algo member as a
fallback, should this function be broken down into 2, one to get the
fallback algo and another that has the meat of this function without the
"repo" parameter?
I haven't read the rest of the series yet, let me keep reading.
> +
> + if (from == to) {
> + if (src != dest)
> + oidcpy(dest, src);
> + return 0;
> + }
> + return -1;
> +}
It's curious to me that you treat same-hash-algo as a NOP here, but as a
BUG() in the other helper. Why the difference (perhaps add a comment)?
> +int convert_object_file(struct strbuf *outbuf,
> + const struct git_hash_algo *from,
> + const struct git_hash_algo *to,
> + const void *buf, size_t len,
> + enum object_type type,
> + int gentle)
Is there a particular reason you chose to put the out parameter (outbuf)
at the beginning, rather than at the end? In the other helper function
the "dest" out param comes at the end. Style conflict...?
Also, you don't use buf or len, so you could have kept them out from
this patch (so that you only add them later as needed).
> +{
> + int ret;
> +
> + /* Don't call this function when no conversion is necessary */
Please avoid double-negation. How about simply
/* Refuse nonsensical conversion */
or simply drop the comment (as the BUG() description already serves the
same purpose?)
> + if ((from == to) || (type == OBJ_BLOB))
> + BUG("Refusing noop object file conversion");
I think it would be better if you separated these out and gave them
different BUG() messages. If we barf with a BUG() it would be so much
more helpful if the message we get is as specific as possible, rather
than leaving us guessing whether (as in this case) we passed in
identical hash algos or whether we tried to handle a blob object. Since
you already have the switch statement below, the OBJ_BLOB case could go
there easily enough.
Nit: I think BUG() messages are not supposed to be capitalized.
> + switch (type) {
> + case OBJ_COMMIT:
> + case OBJ_TREE:
> + case OBJ_TAG:
> + default:
> + /* Not implemented yet, so fail. */
> + ret = -1;
> + break;
> + }
> + if (!ret)
> + return 0;
> + if (gentle) {
> + strbuf_release(outbuf);
What does "gentle" mean here? But also if you are just freeing the
outbuf, then why not just name this param "free_outbuf"? But also it
seems a bit odd that the caller (who presumably owns the outbuf) is
asking a conversion function to possibly free it before returning.
> + return ret;
> + }
> + die(_("Failed to convert object from %s to %s"),
> + from->name, to->name);
> +}
> diff --git a/object-file-convert.h b/object-file-convert.h
> new file mode 100644
> index 000000000000..a4f802aa8eea
> --- /dev/null
> +++ b/object-file-convert.h
> @@ -0,0 +1,24 @@
> +#ifndef OBJECT_CONVERT_H
> +#define OBJECT_CONVERT_H
> +
> +struct repository;
> +struct object_id;
> +struct git_hash_algo;
> +struct strbuf;
> +#include "object.h"
It looks a bit odd that the forward declarations of these structs come
before the #include line. I don't think that's the pattern in our
codebase (maybe I'm wrong?).
In hindsight, the log message could have added that switching back
and forth between different hash algos is a fundamental (but currently
missing) operation, and that this is the reason why these conversion
functions (currently unfinished) are necessary as the first step for the
patch series. I realize that you've stated as much in your cover letter,
but it is always nice to have the intent embedded in the log message(s)
where applicable (such as the case in this patch that introduces a brand
new header file) to save future developers the hassle of looking up the
relevant cover letter.
Thanks.
> +int repo_oid_to_algop(struct repository *repo, const struct object_id *src,
> + const struct git_hash_algo *to, struct object_id *dest);
> +
> +/*
> + * Convert an object file from one hash algorithm to another algorithm.
> + * Return -1 on failure, 0 on success.
> + */
> +int convert_object_file(struct strbuf *outbuf,
> + const struct git_hash_algo *from,
> + const struct git_hash_algo *to,
> + const void *buf, size_t len,
> + enum object_type type,
> + int gentle);
> +
> +#endif /* OBJECT_CONVERT_H */
> --
> 2.41.0
^ permalink raw reply
* Re: [PATCH 1/2] show-ref --verify: accept pseudorefs
From: Patrick Steinhardt @ 2024-02-08 8:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood via GitGitGadget, git, Phillip Wood
In-Reply-To: <xmqq5xz0b3lu.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]
On Wed, Feb 07, 2024 at 09:12:29AM -0800, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > ... when CHERRY_PICK_HEAD exists. By calling refname_is_safe() instead
> > of comparing the refname to "HEAD" we can accept all one-level refs that
> > contain only uppercase ascii letters and underscores.
>
> Geez. We have at least three implementations to determine if a ref
> is a valid name?
`check_refname_format()` and `refname_is_safe()` are often used in
tandem. `check_refname_format()` performs the first set of checks to
verify whether the pathname components are valid, whereas
`refname_is_safe()` checks for refs which are unsafe e.g. because they
try to escape out of "refs/".
I think that we really ought to merge `refname_is_safe()` into
`check_refname_format()`. It would require us to introduce a new flag
`REFNAME_ALLOW_BAD_NAME` to the latter function so that it would accept
refs with a bad name that are otherwise safe. But I think we shouldn't
ever allow unsafe names, so merging these two functions would overall
reduce the potential for security-relevant issues.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
From: Vegard Nossum @ 2024-02-08 8:48 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood; +Cc: Kristoffer Haugsbakk, git, Jonathan Nieder
In-Reply-To: <xmqq8r3wcjq2.fsf@gitster.g>
On 07/02/2024 17:39, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> On 06/02/2024 03:54, Junio C Hamano wrote:
>>> Vegard Nossum <vegard.nossum@oracle.com> writes:
>>>
>>>> On 06/02/2024 00:09, Junio C Hamano wrote:
>>> Perhaps it is a good idea to squash them together as a single bugfix
>>> patch?
>>
>> I think so, I'm not sure we want to add a new test file just for this
>> either. Having the test in a separate file was handy for debugging but
>> I think something like the diff below would suffice though I wouldn't
>> object to checking the author of the cherry-picked commit
>
> Very true (I didn't even notice that the original "bug report
> disguised as a test addition" was inventing a totally new file).
I'm sorry, but I'm confused about what I'm supposed to do now.
There is now another test case and it sounds like you would prefer that
one over mine, but I didn't write it and there is no SOB, so I cannot
submit that with the fix if I were to "squash them together".
I am not a regular contributor so I don't have a good grasp on things
like why you don't want a new test file for this, or why you (as the
maintainer) can't just squash the patches yourself if that's what you
prefer.
Thanks,
Vegard
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Patrick Steinhardt @ 2024-02-08 8:50 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Junio C Hamano, Phillip Wood, phillip.wood, git
In-Reply-To: <CAOLa=ZQaXxwrXmbmFvGR59EDo3Eqa-Xfc3OG9+6ES-veDU8Bhg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3301 bytes --]
On Wed, Feb 07, 2024 at 06:02:34PM +0100, Karthik Nayak wrote:
> On Wed, Feb 7, 2024 at 5:46 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Karthik Nayak <karthik.188@gmail.com> writes:
> >
> > > This is a bit of a grey area, what I mean is that we do allow users to create
> > > non "refs/" prefixed refs:
> > >
> > > $ git update-ref foo @~1
> > >
> > > $ cat .git/foo
> > > 2b52187cd2930931c6d34436371f470bb26eef4f
> > >
> > > What I mean to say is that, by saying "--include-root-refs" it seems to imply
> > > that any such refs should be included too, but this simply is not the case.
> >
> > But isn't that a quality of implementation issue? I'd consider it a
> > bug once we have and enforce the definition of what pseudorefs are.
>
> Yeah, that makes sense. I'll use "--include-root-refs" then.
I'd still argue that this is too specific to the files backend. The fact
that we end up only adding root refs to the files backend to me is an
implementation-specific limitation and not a feature. What I really want
is to ask the backend to give me all refs that it knows of for the
current worktree.
The "reftable" backend has this snippet in its iterator:
```
/*
* The files backend only lists references contained in
* "refs/". We emulate the same behaviour here and thus skip
* all references that don't start with this prefix.
*/
if (!starts_with(iter->ref.refname, "refs/"))
continue;
```
If we add the new `--include-root-refs` flag then we would have to
extend it to query whether the ref either starts with "refs/" or whether
it might look like a pseudo-ref:
```
if (!starts_with(iter->ref.refname, "refs/") &&
!(flags & INCLUDE_ROOT_REFS || is_pseudoref(iter->ref.refname)))
continue;
```
The problem I have is that it still wouldn't end up surfacing all refs
which exist in the ref backend while being computationally more
expensive. So the original usecase I had in mind when pitching this
topic isn't actually addressed.
I know that in theory, the reftable backend shouldn't contain refs other
than "refs/" or pseudo-refs anyway. But regardless of that, I think that
formulating this in the form of "root refs" is too restrictive and too
much focussed on the "files" backend. I wouldn't be surprised if there
are ways to have git-update-ref(1) or other tools write refs with
unexpected names, and not being able to learn about those is a
limitation.
Putting this in the context of "Give me all refs which you know of for
the current worktree" is a lot simpler. It would still be equivalent to
"--include-root-refs" for the "files" backend, because the files backend
doesn't have a way to properly track all refs it has ever created. And
for the "reftable" backend it's as simple as this:
```
if (!(iter->flags & DO_FOR_EACH_INCLUDE_ALL_REFS) &&
!starts_with(iter->ref.refname, "refs/"))
```
I'm not sure about better terminology though. Something like
`--include-all-worktree-refs` could easily lead to confusion because it
might make the user think that they also want to list refs from other
worktrees. The best I can come up with is `--include-non-refs` -- pseudo
refs aren't real refs, and neither are refs which don't start with
"refs/".
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Phillip Wood @ 2024-02-08 10:28 UTC (permalink / raw)
To: Junio C Hamano, Karthik Nayak; +Cc: phillip.wood, git, ps
In-Reply-To: <xmqqcyt9fdc7.fsf@gitster.g>
Hi Juino
On 06/02/2024 22:16, Junio C Hamano wrote:
> - we do not want to say "pseudo refs", as I expect we would want to
> show HEAD that is (unfortunately) classified outside "pseudoref"
> class.
This is a bit of a tangent but I've been wondering what the practical
benefit of distinguishing between "HEAD" and "pseudoref" is. I don't
know the history so there may be a good reason but not classifying
"HEAD" as a "pseudoref" seems to make discussions like this more
complicated than they would be if "HEAD" were a "pseudoref". Would it be
beneficial to loosen the definition of "psuedoref" to remove the
restriction that they may not be symbolic refs or have a reflog?
Best Wishes
Phillip
^ permalink raw reply
* [PATCH v2 0/4] rev-list: allow missing tips with --missing
From: Christian Couder @ 2024-02-08 13:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, John Cai, Linus Arver,
Christian Couder
# Intro
A recent patch series, kn/rev-list-missing-fix [1] extended the
`--missing` option in git-rev-list(1) to support commit objects.
Unfortunately, git-rev-list(1) with `--missing` set to something other
than 'error' still fails, usually with a "fatal: bad object <oid>"
error message, when a missing object is passed as an argument.
This patch series removes this limitation and when using
`--missing=print` allows all missing objects to be printed including
those that are passed as arguments.
[1] https://lore.kernel.org/git/20231026101109.43110-1-karthik.188@gmail.com/
# Patch overview
Patches 1/4 (revision: clarify a 'return NULL' in get_reference()),
2/4 (oidset: refactor oidset_insert_from_set()) and
3/4 (t6022: fix 'test' style and 'even though' typo) are very small
preparatory cleanups.
Patch 4/4 (rev-list: allow missing tips with --missing=[print|allow*])
allows git-rev-list(1) with `--missing=<arg>` when <arg> is not
'error' to not fail if some tips it is passed are missing.
# Changes since V1
Thanks to Linus Arver, Eric Sunshine and Junio who commented on V1!
The changes since V1 are the following:
- In patch 1/4 (revision: clarify a 'return NULL' in
get_reference()), some 's/explicitely/explicitly/' typos were fixed
in the commit message. Thanks to a suggestion from Eric.
- Patch 2/4 (oidset: refactor oidset_insert_from_set()) is new. It
refactors some code into "oidset.{c,h}" to avoid duplicating code
to copy elements from one oidset into another one. Thanks to a
suggestion from Linus.
- Patch 3/4 (t6022: fix 'test' style and 'even though' typo) used to
fix only an 'even though' typo, but while at it I made it use
`if test ...` instead of `if [ ... ]` too. Thanks to a suggestion
from Junio.
- Patch 4/4 (rev-list: allow missing tips with
--missing=[print|allow*]) was changed so that missing tips are
always allowed when `--missing=[print|allow*]` is used, as
suggested by Junio. So:
- no new `--allow-missing-tips` option is implemented,
- no ugly early parsing loop is added,
- no new 'do_not_die_on_missing_tips' flag is added into
'struct rev_info',
- the 'do_not_die_on_missing_objects' is used more instead,
- the commit message as been changed accordingly.
- In patch 4/4 (rev-list: allow missing tips with
--missing=[print|allow*]), a code comment has been added before
`if (get_oid_with_context(...))` in "revision.c::get_reference()"
as suggested by Linus.
- In patch 4/4 (rev-list: allow missing tips with
--missing=[print|allow*]), a big NEEDSWORK comment has been added
before the ugly early parsing loops for the
`--exclude-promisor-objects` and `--missing=...` options in
"builtin/rev-list.c".
- In patch 4/4 (rev-list: allow missing tips with
--missing=[print|allow*]), a code comment now uses "missing tips"
instead of "missing commits" in "builtin/rev-list.c", as
suggested by Linus.
- In patch 4/4 (rev-list: allow missing tips with
--missing=[print|allow*]), the added tests in t6022 have the
following changes:
- variables 'obj' and 'tip' have been renamed to 'missing_tip'
and 'existing_tip' respectively as suggested by Linus,
- a comment explaining how the 'existing_tip' variable is useful
has been added as suggested by Linus,
- `if test ...` is used instead of `if [ ... ]`, as suggested by
Junio.
- Also in patch 4/4 (rev-list: allow missing tips with
--missing=[print|allow*]), the documentation of the `--missing=...`
option has been improved to talk about missing tips.
# Range-diff since V1
Unfortunately there has been many changes in patch 4/4, so the
range-diff shows different patches:
1: b8abbc1d42 ! 1: 5233a83181 revision: clarify a 'return NULL' in get_reference()
@@ Commit message
revision: clarify a 'return NULL' in get_reference()
In general when we know a pointer variable is NULL, it's clearer to
- explicitely return NULL than to return that variable.
+ explicitly return NULL than to return that variable.
In get_reference() when 'object' is NULL, we already return NULL
when 'revs->exclude_promisor_objects && is_promisor_object(oid)' is
true, but we return 'object' when 'revs->ignore_missing' is true.
- Let's make the code clearer and more uniform by also explicitely
+ Let's make the code clearer and more uniform by also explicitly
returning NULL when 'revs->ignore_missing' is true.
+ Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
## revision.c ##
-: ---------- > 2: cfa72c8cf1 oidset: refactor oidset_insert_from_set()
2: 208d43eb81 ! 3: 5668340516 t6022: fix 'even though' typo in comment
@@ Metadata
Author: Christian Couder <chriscool@tuxfamily.org>
## Commit message ##
- t6022: fix 'even though' typo in comment
+ t6022: fix 'test' style and 'even though' typo
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@@ t/t6022-rev-list-missing.sh: do
- # Blobs are shared by all commits, so evethough a commit/tree
+ # Blobs are shared by all commits, so even though a commit/tree
# might be skipped, its blob must be accounted for.
- if [ $obj != "HEAD:1.t" ]; then
+- if [ $obj != "HEAD:1.t" ]; then
++ if test $obj != "HEAD:1.t"
++ then
echo $(git rev-parse HEAD:1.t) >>expect.raw &&
+ echo $(git rev-parse HEAD:2.t) >>expect.raw
+ fi &&
3: 8be34ce359 < -: ---------- rev-list: add --allow-missing-tips to be used with --missing=...
-: ---------- > 4: 55792110ca rev-list: allow missing tips with --missing=[print|allow*]
Christian Couder (4):
revision: clarify a 'return NULL' in get_reference()
oidset: refactor oidset_insert_from_set()
t6022: fix 'test' style and 'even though' typo
rev-list: allow missing tips with --missing=[print|allow*]
Documentation/rev-list-options.txt | 4 ++
builtin/rev-list.c | 15 +++++++-
list-objects-filter.c | 11 +-----
oidset.c | 10 +++++
oidset.h | 6 +++
revision.c | 16 ++++++--
t/t6022-rev-list-missing.sh | 61 +++++++++++++++++++++++++++++-
7 files changed, 106 insertions(+), 17 deletions(-)
--
2.43.0.565.g97b5fd12a3.dirty
^ permalink raw reply
* [PATCH v2 1/4] revision: clarify a 'return NULL' in get_reference()
From: Christian Couder @ 2024-02-08 13:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, John Cai, Linus Arver,
Christian Couder, Eric Sunshine, Christian Couder
In-Reply-To: <20240208135055.2705260-1-christian.couder@gmail.com>
In general when we know a pointer variable is NULL, it's clearer to
explicitly return NULL than to return that variable.
In get_reference() when 'object' is NULL, we already return NULL
when 'revs->exclude_promisor_objects && is_promisor_object(oid)' is
true, but we return 'object' when 'revs->ignore_missing' is true.
Let's make the code clearer and more uniform by also explicitly
returning NULL when 'revs->ignore_missing' is true.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
revision.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/revision.c b/revision.c
index 2424c9bd67..4c5cd7c3ce 100644
--- a/revision.c
+++ b/revision.c
@@ -385,7 +385,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
if (!object) {
if (revs->ignore_missing)
- return object;
+ return NULL;
if (revs->exclude_promisor_objects && is_promisor_object(oid))
return NULL;
die("bad object %s", name);
--
2.43.0.565.g97b5fd12a3.dirty
^ permalink raw reply related
* [PATCH v2 2/4] oidset: refactor oidset_insert_from_set()
From: Christian Couder @ 2024-02-08 13:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, John Cai, Linus Arver,
Christian Couder, Christian Couder
In-Reply-To: <20240208135055.2705260-1-christian.couder@gmail.com>
In a following commit, we will need to add all the oids from a set into
another set. In "list-objects-filter.c", there is already a static
function called add_all() to do that.
Let's rename this function oidset_insert_from_set() and move it into
oidset.{c,h} to make it generally available.
While at it, let's remove a useless `!= NULL`.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
list-objects-filter.c | 11 +----------
oidset.c | 10 ++++++++++
oidset.h | 6 ++++++
3 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/list-objects-filter.c b/list-objects-filter.c
index da287cc8e0..4346f8da45 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -711,15 +711,6 @@ static void filter_combine__free(void *filter_data)
free(d);
}
-static void add_all(struct oidset *dest, struct oidset *src) {
- struct oidset_iter iter;
- struct object_id *src_oid;
-
- oidset_iter_init(src, &iter);
- while ((src_oid = oidset_iter_next(&iter)) != NULL)
- oidset_insert(dest, src_oid);
-}
-
static void filter_combine__finalize_omits(
struct oidset *omits,
void *filter_data)
@@ -728,7 +719,7 @@ static void filter_combine__finalize_omits(
size_t sub;
for (sub = 0; sub < d->nr; sub++) {
- add_all(omits, &d->sub[sub].omits);
+ oidset_insert_from_set(omits, &d->sub[sub].omits);
oidset_clear(&d->sub[sub].omits);
}
}
diff --git a/oidset.c b/oidset.c
index d1e5376316..91d1385910 100644
--- a/oidset.c
+++ b/oidset.c
@@ -23,6 +23,16 @@ int oidset_insert(struct oidset *set, const struct object_id *oid)
return !added;
}
+void oidset_insert_from_set(struct oidset *dest, struct oidset *src)
+{
+ struct oidset_iter iter;
+ struct object_id *src_oid;
+
+ oidset_iter_init(src, &iter);
+ while ((src_oid = oidset_iter_next(&iter)))
+ oidset_insert(dest, src_oid);
+}
+
int oidset_remove(struct oidset *set, const struct object_id *oid)
{
khiter_t pos = kh_get_oid_set(&set->set, *oid);
diff --git a/oidset.h b/oidset.h
index ba4a5a2cd3..262f4256d6 100644
--- a/oidset.h
+++ b/oidset.h
@@ -47,6 +47,12 @@ int oidset_contains(const struct oidset *set, const struct object_id *oid);
*/
int oidset_insert(struct oidset *set, const struct object_id *oid);
+/**
+ * Insert all the oids that are in set 'src' into set 'dest'; a copy
+ * is made of each oid inserted into set 'dest'.
+ */
+void oidset_insert_from_set(struct oidset *dest, struct oidset *src);
+
/**
* Remove the oid from the set.
*
--
2.43.0.565.g97b5fd12a3.dirty
^ permalink raw reply related
* [PATCH v2 3/4] t6022: fix 'test' style and 'even though' typo
From: Christian Couder @ 2024-02-08 13:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, John Cai, Linus Arver,
Christian Couder, Christian Couder
In-Reply-To: <20240208135055.2705260-1-christian.couder@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/t6022-rev-list-missing.sh | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index 211672759a..5f1be7abb5 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -46,9 +46,10 @@ do
git rev-list --objects --no-object-names \
HEAD ^$obj >expect.raw &&
- # Blobs are shared by all commits, so evethough a commit/tree
+ # Blobs are shared by all commits, so even though a commit/tree
# might be skipped, its blob must be accounted for.
- if [ $obj != "HEAD:1.t" ]; then
+ if test $obj != "HEAD:1.t"
+ then
echo $(git rev-parse HEAD:1.t) >>expect.raw &&
echo $(git rev-parse HEAD:2.t) >>expect.raw
fi &&
--
2.43.0.565.g97b5fd12a3.dirty
^ permalink raw reply related
* [PATCH v2 4/4] rev-list: allow missing tips with --missing=[print|allow*]
From: Christian Couder @ 2024-02-08 13:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, John Cai, Linus Arver,
Christian Couder, Christian Couder
In-Reply-To: <20240208135055.2705260-1-christian.couder@gmail.com>
In 9830926c7d (rev-list: add commit object support in `--missing`
option, 2023-10-27) we fixed the `--missing` option in `git rev-list`
so that it works with with missing commits, not just blobs/trees.
Unfortunately, such a command would still fail with a "fatal: bad
object <oid>" if it is passed a missing commit, blob or tree as an
argument (before the rev walking even begins).
When such a command is used to find the dependencies of some objects,
for example the dependencies of quarantined objects (see the
"QUARANTINE ENVIRONMENT" section in the git-receive-pack(1)
documentation), it would be better if the command would instead
consider such missing objects, especially commits, in the same way as
other missing objects.
If, for example `--missing=print` is used, it would be nice for some
use cases if the missing tips passed as arguments were reported in
the same way as other missing objects instead of the command just
failing.
We could introduce a new option to make it work like this, but most
users are likely to prefer the command to have this behavior as the
default one. Introducing a new option would require another dumb loop
to look for that options early, which isn't nice.
Also we made `git rev-list` work with missing commits very recently
and the command is most often passed commits as arguments. So let's
consider this as a bug fix related to these previous change.
While at it let's add a NEEDSWORK comment to say that we should get
rid of the existing ugly dumb loops that parse the
`--exclude-promisor-objects` and `--missing=...` options early.
Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/rev-list-options.txt | 4 +++
builtin/rev-list.c | 15 +++++++-
revision.c | 14 ++++++--
t/t6022-rev-list-missing.sh | 56 ++++++++++++++++++++++++++++++
4 files changed, 85 insertions(+), 4 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a583b52c61..bb753b6292 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -1019,6 +1019,10 @@ Unexpected missing objects will raise an error.
+
The form '--missing=print' is like 'allow-any', but will also print a
list of the missing objects. Object IDs are prefixed with a ``?'' character.
++
+If some tips passed to the traversal are missing, they will be
+considered as missing too, and the traversal will ignore them. In case
+we cannot get their Object ID though, an error will be raised.
--exclude-promisor-objects::
(For internal use only.) Prefilter object traversal at
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index b3f4783858..ec9556f135 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -545,6 +545,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
*
* Let "--missing" to conditionally set fetch_if_missing.
*/
+ /*
+ * NEEDSWORK: These dump loops to look for some options early
+ * are ugly. We really need setup_revisions() to have a
+ * mechanism to allow and disallow some sets of options for
+ * different commands (like rev-list, replay, etc). Such
+ * mechanism should do an early parsing of option and be able
+ * to manage the `--exclude-promisor-objects` and `--missing=...`
+ * options below.
+ */
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
if (!strcmp(arg, "--exclude-promisor-objects")) {
@@ -753,8 +762,12 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
if (arg_print_omitted)
oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);
- if (arg_missing_action == MA_PRINT)
+ if (arg_missing_action == MA_PRINT) {
oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
+ /* Already add missing tips */
+ oidset_insert_from_set(&missing_objects, &revs.missing_commits);
+ oidset_clear(&revs.missing_commits);
+ }
traverse_commit_list_filtered(
&revs, show_commit, show_object, &info,
diff --git a/revision.c b/revision.c
index 4c5cd7c3ce..80c349d347 100644
--- a/revision.c
+++ b/revision.c
@@ -388,6 +388,10 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
return NULL;
if (revs->exclude_promisor_objects && is_promisor_object(oid))
return NULL;
+ if (revs->do_not_die_on_missing_objects) {
+ oidset_insert(&revs->missing_commits, oid);
+ return NULL;
+ }
die("bad object %s", name);
}
object->flags |= flags;
@@ -1947,6 +1951,7 @@ void repo_init_revisions(struct repository *r,
init_display_notes(&revs->notes_opt);
list_objects_filter_init(&revs->filter);
init_ref_exclusions(&revs->ref_excludes);
+ oidset_init(&revs->missing_commits, 0);
}
static void add_pending_commit_list(struct rev_info *revs,
@@ -2178,13 +2183,18 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
if (revarg_opt & REVARG_COMMITTISH)
get_sha1_flags |= GET_OID_COMMITTISH;
+ /*
+ * Even if revs->do_not_die_on_missing_objects is set, we
+ * should error out if we can't even get an oid, as
+ * `--missing=print` should be able to report missing oids.
+ */
if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))
return revs->ignore_missing ? 0 : -1;
if (!cant_be_filename)
verify_non_filename(revs->prefix, arg);
object = get_reference(revs, arg, &oid, flags ^ local_flags);
if (!object)
- return revs->ignore_missing ? 0 : -1;
+ return (revs->ignore_missing || revs->do_not_die_on_missing_objects) ? 0 : -1;
add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);
free(oc.path);
@@ -3830,8 +3840,6 @@ int prepare_revision_walk(struct rev_info *revs)
FOR_EACH_OBJECT_PROMISOR_ONLY);
}
- oidset_init(&revs->missing_commits, 0);
-
if (!revs->reflog_info)
prepare_to_use_bloom_filter(revs);
if (!revs->unsorted_input)
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index 5f1be7abb5..78387eebb3 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -78,4 +78,60 @@ do
done
done
+for missing_tip in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
+do
+ # We want to check that things work when both
+ # - all the tips passed are missing (case existing_tip = ""), and
+ # - there is one missing tip and one existing tip (case existing_tip = "HEAD")
+ for existing_tip in "" "HEAD"
+ do
+ for action in "allow-any" "print"
+ do
+ test_expect_success "--missing=$action with tip '$missing_tip' missing and tip '$existing_tip'" '
+ oid="$(git rev-parse $missing_tip)" &&
+ path=".git/objects/$(test_oid_to_path $oid)" &&
+
+ # Before the object is made missing, we use rev-list to
+ # get the expected oids.
+ if test "$existing_tip" = "HEAD"
+ then
+ git rev-list --objects --no-object-names \
+ HEAD ^$missing_tip >expect.raw
+ else
+ >expect.raw
+ fi &&
+
+ # Blobs are shared by all commits, so even though a commit/tree
+ # might be skipped, its blob must be accounted for.
+ if test "$existing_tip" = "HEAD" && test $missing_tip != "HEAD:1.t"
+ then
+ echo $(git rev-parse HEAD:1.t) >>expect.raw &&
+ echo $(git rev-parse HEAD:2.t) >>expect.raw
+ fi &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ git rev-list --missing=$action --objects --no-object-names \
+ $oid $existing_tip >actual.raw &&
+
+ # When the action is to print, we should also add the missing
+ # oid to the expect list.
+ case $action in
+ allow-any)
+ ;;
+ print)
+ grep ?$oid actual.raw &&
+ echo ?$oid >>expect.raw
+ ;;
+ esac &&
+
+ sort actual.raw >actual &&
+ sort expect.raw >expect &&
+ test_cmp expect actual
+ '
+ done
+ done
+done
+
test_done
--
2.43.0.565.g97b5fd12a3.dirty
^ permalink raw reply related
* Re: [PATCH] Add ideas for GSoC 2024
From: Kaartic Sivaraam @ 2024-02-08 14:02 UTC (permalink / raw)
To: Christian Couder, Patrick Steinhardt
Cc: git, Taylor Blau, Junio C Hamano, Victoria Dye, Karthik Nayak
In-Reply-To: <CAP8UFD2yFr1uOjKOnMjznjR6BEzGqq=R7K85z2Jz4i=sG0CLJA@mail.gmail.com>
Hi Patrick amd Christian,
On 6 February 2024 1:43:02 pm IST, Christian Couder <christian.couder@gmail.com> wrote:
>On Tue, Feb 6, 2024 at 6:51 AM Patrick Steinhardt <ps@pks.im> wrote:
>> On Tue, Feb 06, 2024 at 12:25:31AM +0530, Kaartic Sivaraam wrote:
>
>> I don't quite mind either way. I think overall we have enough tests that
>> can be converted even if both projects got picked up separately. And the
>> reftable unit tests are a bit more involved than the other tests given
>> that their coding style doesn't fit at all into the Git project. So it's
>> not like they can just be copied over, they definitely need some special
>> care.
>>
>> Also, the technical complexity of the "reftable" backend is rather high,
>> which is another hurdle to take.
>>
>> Which overall makes me lean more towards keeping this as a separate
>> project now that I think about it.
Makes sense. I suppose we need to capture the distinction more clearly in the ideas page.
I've tweaked the doc for the same. Do check it out and feel free to suggest any corrections.
Ideas page: https://git.github.io/SoC-2024-Ideas/
>Ok, for me. If we have a contributor working on each of these 2
>projects, we just need to be clear that the contributors should not
>work together on the 2 projects as I think the GSoC forbids that.
>
Indeed. We must make sure to communicate this to selected contributors if we end up choosing two of them for the unit test migration projects.
On a related note, I think I could help as a co-mentor the non-reftable unit tests migration project if we don't find any other willing volunteer. :-)
I'm hoping I should be of some help on guiding the contributor as a co-mentor. Feel free to let me correct me if I might potentially lack required knowledge.
>> > That said, how helpful would it be to link the following doc in the unit
>> > testing related ideas?
>> >
>> > https://github.com/git/git/blob/master/Documentation/technical/unit-tests.txt
>>
>> Makes sense to me.
>
>To me too.
>
>> > Would it worth linking the reftable technical doc for the above ideas?
>> >
>> > https://git-scm.com/docs/reftable
>> >
>> > I could see it goes into a lot of detail. I'm just wondering if link to it
>> > would help someone who's looking to learn about reftable.
>>
>> Definitely doesn't hurt.
>
>I agree.
>
Thanks for the feedback. Included both of these links in relevant ideas too. Feel free to cross-check them!
--
Sivaraam
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
From: Phillip Wood @ 2024-02-08 14:26 UTC (permalink / raw)
To: Vegard Nossum, Junio C Hamano; +Cc: Kristoffer Haugsbakk, git, Jonathan Nieder
In-Reply-To: <ae8d96b7-93b0-4460-b7ed-ffebaddd6f97@oracle.com>
Hi Vegard
On 08/02/2024 08:48, Vegard Nossum wrote:
> I'm sorry, but I'm confused about what I'm supposed to do now.
>
> There is now another test case and it sounds like you would prefer that
> one over mine, but I didn't write it and there is no SOB, so I cannot
> submit that with the fix if I were to "squash them together".
Here's my SOB for the diff in
https://lore.kernel.org/git/4e6d503a-8564-4536-82a7-29c489f5fec3@gmail.com/
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
I think that typically for small suggestions like that we just add a
Helped-by: trailer but feel free to add my SOB if you want.
> I am not a regular contributor so I don't have a good grasp on things
> like why you don't want a new test file for this,
I think keeping related tests together helps contributors see which test
files to run when they're changing code (running the whole suite each
time is too slow). There is also a (small) setup overhead for each new
file. For tests like this it is a bit ambiguous whether it belongs with
the other "rebase --exec" tests or the other "cherry-pick" tests. I
opted to put it with the other "rebase --exec" tests as I think it is
really fixing a bug with rebase rather than cherry-pick.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 1/2] show-ref --verify: accept pseudorefs
From: phillip.wood123 @ 2024-02-08 14:34 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood via GitGitGadget
Cc: git, Patrick Steinhardt, Phillip Wood
In-Reply-To: <xmqq5xz0b3lu.fsf@gitster.g>
Hi Junio
On 07/02/2024 17:12, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> I think the helper you picked is the most sensible one, modulo a few
> nits.
>
> - We would want to teach refname_is_safe() to honor is_pseudoref()
> from Karthik's series to make rules more consistent.
Yes, I held off sending this series waiting for a while but then got
impatient. We may want to split out a helper from is_pseudoref() that
just checks the name without trying to read the ref for callers like
this which are going to read the ref anyway.
> - The refname_is_safe() helper is not just about the stuff at the
> root level. While starts_with("refs/") is overly lenient, it
> rejects nonsense like "refs/../trash". We would want to lose
> "starts_with() ||" part of the condition from here.
I left the "starts_with()" in as we check the refname when we look it up
with read_ref() so it seemed like wasted effort to do it here as well.
> These are minor non-blocking nits that we should keep in mind only
> for longer term maintenance, #leftoverbits after the dust settles.
>
> Will queue.
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...
From: Christian Couder @ 2024-02-08 15:03 UTC (permalink / raw)
To: Linus Arver
Cc: git, Junio C Hamano, Patrick Steinhardt, John Cai,
Christian Couder
In-Reply-To: <owlyr0hom257.fsf@fine.c.googlers.com>
On Wed, Feb 7, 2024 at 9:48 PM Linus Arver <linusa@google.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
> > It actually kind of "works" when you pass blobs or trees. It looks
> > like the command just doesn't use them (except for checking that they
> > actually exist) unless options like --objects, --missing=print and
> > perhaps others are used. So yeah, the doc might need an update, but I
> > think it could be a separate issue, as it's not new and doesn't depend
> > on this small series.
>
> SG. But also, if there's a way to put invisible (developer-facing, not
> user facing) comments inside the relevant doc file it might be easy
> enough to add a to this series.
It might seem easy to add/improve some doc, but there is sometimes
bikeshedding. So I don't think making this series dependent on such a
dco update is a good thing for both that doc update and this series. I
will try to work on such a doc update soon though.
> > "quarantined objects" refers to the following doc:
> >
> > https://www.git-scm.com/docs/git-receive-pack#_quarantine_environment
> >
> > So to clarify things, the above paragraph looks like the following now:
> >
> > "When such a command is used to find the dependencies of some objects,
> > for example the dependencies of quarantined objects (see the
> > "QUARANTINE ENVIRONMENT" section in the git-receive-pack(1)
> > documentation), it would be better if the command would instead
> > consider such missing objects, especially commits, in the same way as
> > other missing objects."
>
> This reads much better, and is a good motivation to keep in the log
> message.
Ok, I have kept it in the V2 I just sent:
https://lore.kernel.org/git/20240208135055.2705260-1-christian.couder@gmail.com/
By the way, sorry for forgetting to use the --in-reply-to=... option
when I sent it.
> > Yeah, I was surprised too. I only found the following similar code in
> > list-objects-filter.c:
> >
> > oidset_iter_init(src, &iter);
> > while ((src_oid = oidset_iter_next(&iter)) != NULL)
> > oidset_insert(dest, src_oid);
> >
> > So yeah perhaps we could create such a helper function.
>
> Perhaps a NEEDSWORK comment is appropriate?
I actually added another small patch in the V2 that refactors the
existing code into a function in oidset.{c,h}.
> >> With a few more context lines, the diff looks like
> >>
> >> --8<---------------cut here---------------start------------->8---
> >> if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))
> >> return revs->ignore_missing ? 0 : -1;
> >> if (!cant_be_filename)
> >> verify_non_filename(revs->prefix, arg);
> >> object = get_reference(revs, arg, &oid, flags ^ local_flags);
> >> if (!object)
> >> - return revs->ignore_missing ? 0 : -1;
> >> + return (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;
> >> add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
> >> add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);
> >> free(oc.path);
> >> return 0;
> >> --8<---------------cut here---------------end--------------->8---
> >>
> >> and it's not obvious to me why you only touched the "if (!object)" case
> >> but not the "if (get_oid_with_context(...))" case. Are there any
> >> subtleties here that would not be obvious to reviewers?
> >
> > I thought that if we can't get an oid, we cannot put anything in the
> > 'missing_commit' oidset anyway, so it might be better to error out.
>
> Ah, makes sense. This is a subtle detail, and perhaps worth keeping
> either as a comment (just above the "if (get_oid_with_context(...))"
> case) or in the log message.
I added a code comment just above the "if (get_oid_with_context(...))"
case in the V2.
> Ah, I see. I think you could add a comment like
>
> We want to check that things work as expected both:
>
> - when we pass only one missing tip (when $tip is ""), and:
> - when we pass one missing tip and a tip that is not missing (when
> $tip is "HEAD").
>
> at the top of the test case, and probably rename $obj to $missing_tip,
> and rename $tip to $existing_tip.
I have renamed the variables like you suggest and added a code comment.
> Aside: it's a bit unfortunate that the meaning of "missing" becomes
> overloaded slightly here because one could say '$tip == ""' is a
> "missing" tip becauuse the name is not provided. Subtle. Plus there's
> some interplay with the "if (get_oid_with_context(...))" case above
> because we will (?) hit that path if we do pass in "" (empty string arg)
> as a tip to rev-list. It might be worth saying in the comments in the
> implementation, something like
>
> The term "missing" here means the case where we could not find the object
> given the object_id. For example, given HEAD~1, its object id (oid)
> could be 82335b23aa7234320d6f8055243c852e4b6a5ca3. If no real object
> with this oid exists, it is considered missing. Providing an empty
> string to rev-list does not touch the "missing tip" codepath.
>
> I wrote the above hastily so it may need further edits to make it
> succinct. But the point is that this definition isn't spelled out in the
> test case, which makes the "" argument for $tip that much more subtle.
I think this is related to the --missing option in general (which has
been with us for around 6 years already), not specifically to this
series, so I think it can be done separately.
> >> OK so you are using this empty string to clear the expect.raw file. If
> >> that's true then what stops us from doing this at the start of every
> >> iteration?
> >
> > I am not sure what you mean. Both:
> >
> > git rev-list --objects --no-object-names HEAD ^$obj >expect.raw
> >
> > and:
> >
> > >expect.raw #2
> >
> > are clearing "expect.raw" as ">expect.raw" is used in both cases.
> >
> > If we did the latter at the start of every iteration, then when we do
> > the former afterwards, we would clear "expect.raw" raw again, while
> > there is no point in clearing it twice.
>
> Yes but doing it that way would separate "set up a clean environment for
> this test case" from "find the oid of the non-missing tip" from each
> other in the same if/else stanza, which I believe helps readability.
On one hand it can help readability, but on other hand some people
interested in test performance might see it as some waste. So I prefer
not to do it for now.
> >> Also, given how most of this is identical from the loop already in t6022
> >> just above it, it would help to add a comment at the top of this one
> >> explaining how it's different than the one above it.
> >
> > The one I added has an extra `for tip in "" "HEAD"` loop. Anyway I
> > think I will just modify the existing test in the next version I will
> > send, as I plan to implement Junio's suggestion and there will be no
> > new option.
Actually I changed my mind and didn't modify the existing test, but I
renamed the variables as you suggested.
> Now that I have your attention (was this my plan all along? ;) ), I will
> take this opportunity to ping you directly for a review of my own patch
> series for the trailers subsystem:
> https://lore.kernel.org/git/pull.1632.v4.git.1707196348.gitgitgadget@gmail.com/
> which is in its 4th iteration after many helpful comments from Junio.
> Please don't let the patch count (28) raise any alarms --- they used to
> be 10 but I broke them down into smaller steps to ease the review
> process.
I will try to take a look soon. Thanks for working on improving the
trailers subsystem!
^ permalink raw reply
* Bug: Commit fails when no global email address is set even though --author is used
From: Marcus Tillmanns @ 2024-02-08 15:26 UTC (permalink / raw)
To: git@vger.kernel.org
What did you do before the bug happened? (Steps to reproduce your issue)
* Set your machines hostname to a name that does not contain "." (e.g. "ihavenodotinmyhostname")
* Make sure you have no name or email configured in your global git config
* Create a new repository and "git add" a file
* Run: git commit -m "Test" --author "My Name <my@email.com>"
What did you expect to happen? (Expected behavior)
A commit should be created with author name "My Name", and author email "my@email.com"
What happened instead? (Actual behavior)
An error is thrown, complaining about not being able to determine the email address
What's different between what you expected and what actually happened?
The email should have been taken from the "--author" argument, but instead the commit failed.
Anything else you want to add:
This does not happen if your hostname contains a ".", e.g. "myhostname.local"
[System Info]
git version:
git version 2.40.1
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-15-generic #15-Ubuntu SMP PREEMPT_DYNAMIC Tue Jan 9 17:03:36 UTC 2024 x86_64
compiler info: gnuc: 12.3
libc info: glibc: 2.38
$SHELL (typically, interactive shell): /bin/bash
^ permalink raw reply
* Re: Bug: Commit fails when no global email address is set even though --author is used
From: Phillip Wood @ 2024-02-08 15:50 UTC (permalink / raw)
To: Marcus Tillmanns, git@vger.kernel.org
In-Reply-To: <F7D40DCD-2331-44D8-B4BF-8E6CD9EE64A6@qt.io>
Hi Marcus
On 08/02/2024 15:26, Marcus Tillmanns wrote:
> What did you do before the bug happened? (Steps to reproduce your issue)
>
> * Set your machines hostname to a name that does not contain "." (e.g. "ihavenodotinmyhostname")
> * Make sure you have no name or email configured in your global git config
> * Create a new repository and "git add" a file
> * Run: git commit -m "Test" --author "My Name <my@email.com>"
>
> What did you expect to happen? (Expected behavior)
>
> A commit should be created with author name "My Name", and author email "my@email.com"
>
> What happened instead? (Actual behavior)
>
> An error is thrown, complaining about not being able to determine the email address
This is expected as "git commit" needs an identity to use for the
committer as well as for the author. To set the committer you can use
the GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL environment variables if
you don't have the relevant config set and git cannot extract a domain
from your hostname.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 2/2] bisect: document command line arguments for "bisect start"
From: Jean-Noël Avila @ 2024-02-08 16:18 UTC (permalink / raw)
To: git
In-Reply-To: <20240207214436.538586-3-gitster@pobox.com>
Le 07/02/2024 à 22:44, Junio C Hamano a écrit :
> The syntax commonly used for alternatives is --opt-(a|b), not
> --opt-{a,b}.
>
> List bad/new and good/old consistently in this order, to be
> consistent with the description for "git bisect terms". Clarify
> <term> to either <term-old> or <term-new> to make them consistent
> with the description of "git bisect (good|bad)" subcommands.
>
> Suggested-by: Matthieu Moy <git@matthieu-moy.fr>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/git-bisect.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index 3d813f9c77..73f889b97b 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -16,7 +16,7 @@ DESCRIPTION
> The command takes various subcommands, and different options depending
> on the subcommand:
>
> - git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]
> + git bisect start [--term-(bad|new)=<term-new> --term-(good|old)=<term-old>]
> [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
> git bisect (bad|new|<term-new>) [<rev>]
> git bisect (good|old|<term-old>) [<rev>...]
LGTM
^ permalink raw reply
* Re: [PATCH v2 22/30] rev-parse: add an --output-object-format parameter
From: Jean-Noël Avila @ 2024-02-08 16:25 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git
In-Reply-To: <20231002024034.2611-22-ebiederm@gmail.com>
Le 02/10/2023 à 04:40, Eric W. Biederman a écrit :
> + else die(_("unsupported object format: %s"), arg);
"unsupported object format '%s'" already exist in translated strings.
One less similar string to translate.
Thank you.
> + }
> if (opt_with_value(arg, "--short", &arg)) {
> filter &= ~(DO_FLAGS|DO_NOREV);
^ permalink raw reply
* Re: [PATCH 1/2] bisect: document "terms" subcommand more fully
From: Junio C Hamano @ 2024-02-08 16:48 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Matthieu Moy, git
In-Reply-To: <a8e67945-153b-43bb-b1b0-ea24fa786097@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Wed, Feb 7, 2024, at 22:44, Junio C Hamano wrote:
>> The documentation for "git bisect terms", although it did not hide
>> any information, was a bit incomplete and forced readers to fill in
>> the blanks to get the complete picture.
>>
>> Acked-by: Matthieu Moy <git@matthieu-moy.fr>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>
> Past tense? How about:
>
> The documentation for "git bisect terms"---although it does not hide
> any information---is a bit incomplete and forces readers to fill in
> the blanks to get the complete picture.
Right.
Thanks.
^ permalink raw reply
* [PATCH] prune: mark rebase autostash and orig-head as reachable
From: Phillip Wood via GitGitGadget @ 2024-02-08 17:00 UTC (permalink / raw)
To: git; +Cc: Orgad Shaneh, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Rebase records the oid of HEAD before rebasing and the commit created by
"--autostash" in files in the rebase state directory. This means that
the autostash commit is never reachable from any ref or reflog and when
rebasing a detached HEAD the original HEAD can become unreachable if the
user expires HEAD's the reflog while the rebase is running. Fix this by
reading the relevant files when marking reachable commits.
Note that it is possible for the commit recorded in
.git/rebase-merge/amend to be unreachable but pruning that object does
not affect the operation of "git rebase --continue" as we're only
interested in the object id, not in the object itself.
Reported-by: Orgad Shaneh <orgads@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
prune: mark rebase autostash and orig-head as reachable
Thanks for reporting this Orgad, it reminded me that I've been meaning
to clean up this patch for a while.
I've got a patch on top of this that marks selected pseudorefs such as
ORIG_HEAD and AUTO_MERGE as reachable but I've held off sending that as
I think it would be better to rebase it on kn/for-all-refs once that has
stabilized as that will hopefully allows us to mark all pseudorefs as
reachable rather than using a hard-coded list.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1656%2Fphillipwood%2Fprune-protect-rebase-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1656/phillipwood/prune-protect-rebase-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1656
reachable.c | 51 +++++++++++++++++++++++++++++++++++++
t/t3407-rebase-abort.sh | 17 ++++++++++++-
t/t3420-rebase-autostash.sh | 10 ++++++++
3 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/reachable.c b/reachable.c
index f29b06a5d05..fef29422d4a 100644
--- a/reachable.c
+++ b/reachable.c
@@ -17,6 +17,7 @@
#include "pack-mtimes.h"
#include "config.h"
#include "run-command.h"
+#include "sequencer.h"
struct connectivity_progress {
struct progress *progress;
@@ -30,6 +31,53 @@ static void update_progress(struct connectivity_progress *cp)
display_progress(cp->progress, cp->count);
}
+static int add_one_file(const char *path, struct rev_info *revs)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct object_id oid;
+ struct object *object;
+
+ if (!read_oneliner(&buf, path, READ_ONELINER_SKIP_IF_EMPTY)) {
+ strbuf_release(&buf);
+ return 0;
+ }
+ strbuf_trim(&buf);
+ if (!get_oid_hex(buf.buf, &oid)) {
+ object = parse_object_or_die(&oid, buf.buf);
+ add_pending_object(revs, object, "");
+ }
+ return 0;
+}
+
+/* Mark objects recored in rebase state files as reachable. */
+static int add_rebase_files(struct rev_info *revs)
+{
+ struct strbuf buf = STRBUF_INIT;
+ size_t len;
+ const char *path[] = {
+ "rebase-apply/autostash",
+ "rebase-apply/orig-head",
+ "rebase-merge/autostash",
+ "rebase-merge/orig-head",
+ };
+ struct worktree **worktrees = get_worktrees();
+
+ for (struct worktree **wt = worktrees; *wt; wt++) {
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, get_worktree_git_dir(*wt));
+ strbuf_complete(&buf, '/');
+ len = buf.len;
+ for (size_t i = 0; i < ARRAY_SIZE(path); i++) {
+ strbuf_setlen(&buf, len);
+ strbuf_addstr(&buf, path[i]);
+ add_one_file(buf.buf, revs);
+ }
+ }
+ strbuf_release(&buf);
+ free_worktrees(worktrees);
+ return 0;
+}
+
static int add_one_ref(const char *path, const struct object_id *oid,
int flag, void *cb_data)
{
@@ -322,6 +370,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
head_ref(add_one_ref, revs);
other_head_refs(add_one_ref, revs);
+ /* rebase autostash and orig-head */
+ add_rebase_files(revs);
+
/* Add all reflog info */
if (mark_reflog)
add_reflogs_to_pending(revs, 0);
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index ebbaed147a6..9f49c4228b6 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -40,9 +40,24 @@ testrebase() {
test_path_is_missing "$state_dir"
'
+ test_expect_success "pre rebase$type head is marked as reachable" '
+ # Clean up the state from the previous one
+ git checkout -f --detach pre-rebase &&
+ test_tick &&
+ git commit --amend --only -m "reworded" &&
+ orig_head=$(git rev-parse HEAD) &&
+ test_must_fail git rebase$type main &&
+ # Stop ORIG_HEAD marking $state_dir/orig-head as reachable
+ git update-ref -d ORIG_HEAD &&
+ git reflog expire --expire="$GIT_COMMITTER_DATE" --all &&
+ git prune --expire=now &&
+ git rebase --abort &&
+ test_cmp_rev $orig_head HEAD
+ '
+
test_expect_success "rebase$type --abort after --skip" '
# Clean up the state from the previous one
- git reset --hard pre-rebase &&
+ git checkout -B to-rebase pre-rebase &&
test_must_fail git rebase$type main &&
test_path_is_dir "$state_dir" &&
test_must_fail git rebase --skip &&
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 693934ee8be..1a820f14815 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -333,4 +333,14 @@ test_expect_success 'never change active branch' '
test_cmp_rev not-the-feature-branch unrelated-onto-branch
'
+test_expect_success 'autostash commit is marked as reachable' '
+ echo changed >file0 &&
+ git rebase --autostash --exec "git prune --expire=now" \
+ feature-branch^ feature-branch &&
+ # git rebase succeeds if the stash cannot be applied so we need to check
+ # the contents of file0
+ echo changed >expect &&
+ test_cmp expect file0
+'
+
test_done
base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Junio C Hamano @ 2024-02-08 17:04 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, Phillip Wood, phillip.wood, git
In-Reply-To: <ZcSVx4slikt4xB3D@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> ```
> if (!starts_with(iter->ref.refname, "refs/") &&
> !(flags & INCLUDE_ROOT_REFS || is_pseudoref(iter->ref.refname)))
> continue;
> ```
>
> The problem I have is that it still wouldn't end up surfacing all refs
> which exist in the ref backend while being computationally more
> expensive. So the original usecase I had in mind when pitching this
> topic isn't actually addressed.
The reftable format, as a database format, may be capable of having
"refs/heads/master" and "refs/heads/master/1" at the same time, but
to be used as a ref backend for Git, it must refrain from surfacing
both at the same time. I think it is the same deal that it should
only allow "refs/*", "HEAD", and so called pseudorefs to be stored.
So INCLUDE_ROOT_REFS should be sufficient as long as the "ref
creation and update" side is not letting random cruft (e.g.,
"config") in. Isn't that sufficient?
> I know that in theory, the reftable backend shouldn't contain refs other
> than "refs/" or pseudo-refs anyway. But regardless of that, I think that
> formulating this in the form of "root refs" is too restrictive and too
> much focussed on the "files" backend.
It is not "focused on". The ref namespace of Git is tree-shaped,
period. The shape may have originated from its first ref backend
implementation's limitation, but as we gain other backends, we are
not planning to lift such limitations, are we? So we may still say
"when there is a master branch, you cannot have master/1 branch (due
to D/F conflict)", even if there is no notion of directory or file
in a backend implementation backed by a databasy file format. "HEAD"
and "CHERRY_PICK_HEAD", unlike "refs/tags/v1.0", are at the "root
level", not only when they are stored in a files backend, but always
when they are presented to end-users, who can tell that they are not
inside "refs/".
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Junio C Hamano @ 2024-02-08 17:07 UTC (permalink / raw)
To: Phillip Wood; +Cc: Karthik Nayak, phillip.wood, git, ps
In-Reply-To: <613b85a3-677d-40d8-84b5-69dd5c27cafe@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> This is a bit of a tangent but I've been wondering what the practical
> benefit of distinguishing between "HEAD" and "pseudoref" is.
I hate that distinction too. The practical difference they gave me
was that we historically never made FOO_HEAD to be a symref, and we
do not want them to be in the future, but for HEAD, it is perfectly
natural to be a symref.
^ permalink raw reply
* Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
From: Junio C Hamano @ 2024-02-08 17:20 UTC (permalink / raw)
To: Vegard Nossum, Phillip Wood; +Cc: Kristoffer Haugsbakk, git, Jonathan Nieder
In-Reply-To: <eaf511ff-f9e0-47ac-ae2e-3de0efa928dd@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> I think that typically for small suggestions like that we just add a
> Helped-by: trailer but feel free to add my SOB if you want.
Thanks, both. Here is what I assembled from the pieces.
----- >8 --------- >8 --------- >8 --------- >8 -----
From: Vegard Nossum <vegard.nossum@oracle.com>
Date: Fri, 2 Feb 2024 10:18:50 +0100
Subject: [PATCH] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
Running "git cherry-pick" as an x-command in the rebase plan loses
the original authorship information.
To fix this, unset GIT_CHERRY_PICK_HELP for 'exec' commands.
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
sequencer.c | 1 +
t/t3404-rebase-interactive.sh | 12 ++++++++++++
2 files changed, 13 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed..ed30ceaf8b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3647,6 +3647,7 @@ static int do_exec(struct repository *r, const char *command_line)
fprintf(stderr, _("Executing: %s\n"), command_line);
cmd.use_shell = 1;
strvec_push(&cmd.args, command_line);
+ strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP");
status = run_command(&cmd);
/* force re-reading of the cache */
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c5f30554c6..84a92d6da0 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
git rebase --continue
'
+test_expect_success 'cherry-pick works with rebase --exec' '
+ test_when_finished "git cherry-pick --abort; \
+ git rebase --abort; \
+ git checkout primary" &&
+ echo "exec git cherry-pick G" >todo &&
+ (
+ set_replace_editor todo &&
+ test_must_fail git rebase -i D D
+ ) &&
+ test_cmp_rev G CHERRY_PICK_HEAD
+'
+
test_expect_success 'rebase -x with empty command fails' '
test_when_finished "git rebase --abort ||:" &&
test_must_fail env git rebase -x "" @ 2>actual &&
--
2.43.0-561-g235986be82
^ permalink raw reply related
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