* Re: What's cooking in git.git (Sep 2023, #05; Fri, 15)
From: Linus Arver @ 2023-09-20 19:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqjzskdh9t.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>>> Whatever improvements you have in mind, if they are
>>> minor, letting the above graduate (they have been in 'next' for a
>>> week without anybody complaining) and doing them as a follow-up
>>> series would be sensible, I would think.
>>>
>>> Thanks.
>>
>> Hmm, I don't think they are minor? See
>> https://github.com/listx/git/tree/trailer-libification-prep for the
>> current state of things.
>
> They do not look like "oops the patches I sent to the list was
> totally bogus and they need replacing"; aren't they rather "these
> are not yet complete and there are some of the follow-on work on top
> of them"?
Yes, precisely.
>> I need to still follow up to your last comment on "trailer: rename
>> *_DEFAULT enums to *_UNSPECIFIED" [2] (I was going to see if we needed
>> the "obvious solution" as you described).
>>
>> If it's too painful to move this out of 'next' now, I'm OK with it
>> graduating as is and doing a separate follow-up (I expect several more
>> of these to happen anyway). Up to you.
>
> Let's mark the topic as "waiting for follow-up updates" and keep it
> in 'next' until the follow-up patches become ready, then.
Sounds good.
>> Sorry for not noticing that this was in 'next' sooner and communicating
>> accordingly.
>
> Gotchas happen. Let's try to communicate better the next time.
>
> Thanks.
Thanks!
^ permalink raw reply
* Re: [PATCH] fix: check parameters in json-write.c
From: Jeff Hostetler @ 2023-09-20 20:02 UTC (permalink / raw)
To: Taylor Blau, mark via GitGitGadget; +Cc: git, mark, wangsirun, Jeff Hostetler
In-Reply-To: <ZQne3ThSw6HVmNJc@nand.local>
On 9/19/23 1:48 PM, Taylor Blau wrote:
> [+cc Jeff Hostetler]
>
> On Tue, Sep 19, 2023 at 11:54:58AM +0000, mark via GitGitGadget wrote:
>> diff --git a/json-writer.c b/json-writer.c
>> index 005c820aa42..23ba7046e5d 100644
>> --- a/json-writer.c
>> +++ b/json-writer.c
>> @@ -20,6 +20,11 @@ static void append_quoted_string(struct strbuf *out, const char *in)
>> {
>> unsigned char c;
>>
>> + if (!in || !*in) {
>> + strbuf_addstr(out, "\"\"");
>> + return;
>> + }
>
> From reading the implementation of append_quoted_string(), I think that
> the case where "in" is the empty string is already covered. IOW, doing
> something like:
>
> struct strbuf buf = STRBUF_INIT;
> append_quoted_string(&out, "");
> warning("'%s'", buf.buf);
>
> would print out something like:
>
> warning: '""'
>
> as expected. Handling a NULL "in" argument is new behavior, but I am not
> sure if it is appropriate to coerce a NULL input into the empty string.
> I've CC'd the author of this code, whose opinion I trust more than my
> own here.
>
> Thanks,
> Taylor
There are three callers of `append_quoted_string()` and it is static
to the json-writer.c code.
Basically, in a JSON object, we have 2 uses:
{
"<key>" : "<string-value>",
"<key>" : <integer>,
...
}
And in a JSON array, we have the other:
[
"<string-value>",
...
]
I suppose it is OK for the 2 string-value cases to assume a NULL pointer
could be written as "" in the JSON output. Although, I kinda think a
NULL pointer should call BUG() as we have in the various assert_*()
routines. It really is a kind of logic error in the caller.
Regardless what we decide for the <string-value> case, in the <key>
case, the resulting JSON would not be valid. We need for the key to
be a non-empty string. For example { "" : 1 } is not valid JSON.
So the key case should call BUG() and not try to hide it.
So I'm leaning towards just making it a BUG() in all cases, but I'm
open to the other mixed handling.
Jeff
^ permalink raw reply
* Re: [PATCH] fix: check parameters in json-write.c
From: Junio C Hamano @ 2023-09-20 20:10 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Taylor Blau, mark via GitGitGadget, git, mark, wangsirun,
Jeff Hostetler
In-Reply-To: <dc45106c-d569-3438-d2ff-c3c94b6161d7@jeffhostetler.com>
Jeff Hostetler <git@jeffhostetler.com> writes:
> I suppose it is OK for the 2 string-value cases to assume a NULL pointer
> could be written as "" in the JSON output. Although, I kinda think a
> NULL pointer should call BUG() as we have in the various assert_*()
> routines. It really is a kind of logic error in the caller.
FWIW, that is my preference, too.
> Regardless what we decide for the <string-value> case, in the <key>
> case, the resulting JSON would not be valid. We need for the key to
> be a non-empty string. For example { "" : 1 } is not valid JSON.
> So the key case should call BUG() and not try to hide it.
I do not have a strong opinion on this side, and leave it up to the
area experts ;-)
>
> So I'm leaning towards just making it a BUG() in all cases, but I'm
> open to the other mixed handling.
>
> Jeff
^ permalink raw reply
* Re: [PATCH] pkt-line: do not chomp EOL for sideband progress info
From: Jonathan Tan @ 2023-09-20 21:08 UTC (permalink / raw)
To: Jiang Xin; +Cc: Jonathan Tan, Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <20230919071956.14015-1-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> In the protocol negotiation stage, we need to turn on the flag
> "PACKET_READ_CHOMP_NEWLINE" to chomp EOL for each packet line from
> client or server. But when receiving data and progress information
> using sideband, we will turn off the flag "PACKET_READ_CHOMP_NEWLINE"
> to prevent mangling EOLs from data and progress information.
>
> When both the server and the client support "sideband-all" capability,
> we have a dilemma that EOLs in negotiation packets should be trimmed,
> but EOLs in progress infomation should be leaved as is.
>
> Move the logic of chomping EOLs from "packet_read_with_status()" to
> "packet_reader_read()" can resolve this dilemma.
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
I think the summary is that when we use the struct packet_reader with
sideband and newline chomping, we want the chomping to occur only on
sideband 1, but the current code also chomps on sidebands 2 and 3 (3
is for fatal errors so it doesn't matter as much, but for 2, it really
matters).
This makes sense to fix.
As for how this is fixed, one issue is that we now have 2 places in
which newlines can be chomped (in packet_read_with_status() and with
this patch, packet_reader_read()). The issue is that we need to check
the sideband indicator before we chomp, and packet_read_with_status()
only knows how to chomp. So we either teach packet_read_with_status()
how to sideband, or tell packet_read_with_status() not to chomp and
chomp it ourselves (like in this patch).
Of the two, I would prefer it if packet_read_with_status() was taught
how to sideband - as it is, packet_read_with_status() is used 3 times
in pkt-line.c and 1 time in remote-curl.c, and 2 of those times (in
pkt-line.c) are used with sideband. Doing this does not only solve the
problem here, but reduces code duplication.
Having said that, let me look at the code anyway.
> @@ -597,12 +597,18 @@ void packet_reader_init(struct packet_reader *reader, int fd,
> enum packet_read_status packet_reader_read(struct packet_reader *reader)
> {
> struct strbuf scratch = STRBUF_INIT;
> + int options = reader->options;
>
> if (reader->line_peeked) {
> reader->line_peeked = 0;
> return reader->status;
> }
>
> + /* Do not chomp newlines for sideband progress and error messages */
> + if (reader->use_sideband && options & PACKET_READ_CHOMP_NEWLINE) {
> + options &= ~PACKET_READ_CHOMP_NEWLINE;
> + }
> +
This needs a better explanation (than what's in the comment), I think.
What this code is doing is disabling chomping because we have code that
conditionally does it later.
> /*
> * Consume all progress packets until a primary payload packet is
> * received
> @@ -615,7 +621,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
> reader->buffer,
> reader->buffer_size,
> &reader->pktlen,
> - reader->options);
> + options);
OK, we're using our own custom options that may have
PACKET_READ_CHOMP_NEWLINE unset.
> @@ -624,12 +630,19 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
> break;
> }
>
> - if (reader->status == PACKET_READ_NORMAL)
> + if (reader->status == PACKET_READ_NORMAL) {
> /* Skip the sideband designator if sideband is used */
> reader->line = reader->use_sideband ?
> reader->buffer + 1 : reader->buffer;
> - else
> +
> + if ((reader->options & PACKET_READ_CHOMP_NEWLINE) &&
> + reader->buffer[reader->pktlen - 1] == '\n') {
> + reader->buffer[reader->pktlen - 1] = 0;
> + reader->pktlen--;
> + }
When we reach here, we have skipped all sideband-2 pkt-lines, so
unconditionally chomping it here is good. Might be better if there was
also a check that use_sideband is set, just for symmetry with the code
near the start of this function.
^ permalink raw reply
* Re: [PATCH 1/2] t/t6300: introduce test_bad_atom()
From: Junio C Hamano @ 2023-09-20 22:56 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma
In-Reply-To: <20230920191654.6133-2-five231003@gmail.com>
Kousik Sanagavarapu <five231003@gmail.com> writes:
> Introduce a new function "test_bad_atom()", which is similar to
> "test_atom()" but should be used to check whether the correct error
> message is shown on stderr.
>
> Like "test_atom()", the new function takes three arguments. The three
> arguments specify the ref, the format and the expected error message
> respectively, with an optional fourth argument for tweaking
> "test_expect_*" (which is by default "success").
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Hariom Verma <hariom18599@gmail.com>
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> ---
> t/t6300-for-each-ref.sh | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 7b943fd34c..15b4622f57 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -267,6 +267,26 @@ test_expect_success 'arguments to %(objectname:short=) must be positive integers
> test_must_fail git for-each-ref --format="%(objectname:short=foo)"
> '
>
> +test_bad_atom() {
Style: have SP on both sides of "()".
> + case "$1" in
> + head) ref=refs/heads/main ;;
> + tag) ref=refs/tags/testtag ;;
> + sym) ref=refs/heads/sym ;;
> + *) ref=$1 ;;
> + esac
Somehow this indirection makes the two examples we see below harder
to understand. Why shouldn't we just write the full refname on th
command line of test_bad_atom? That would make it crystal clear
which ref each test works on. It does not help that both 'head' and
'sym' refer to a local branch (if the former referred to .git/HEAD
or .git/remotes/origin/HEAD it may have been an excellent choice of
the name, but that is not what is going on).
> + printf '%s\n' "$3">expect
Style: need SP before (but not after) '>'.
> + test_expect_${4:-success} $PREREQ "err basic atom: $1 $2" "
> + test_must_fail git for-each-ref --format='%($2)' $ref 2>actual &&
> + test_cmp expect actual
> + "
It is error prone to have the executable part of test_expect_{success,failure}
inside a pair of double quotes and have $variable interpolated
_before_ even the arguments to test_expect_{success,failure} are
formed. It is much more preferrable to write
test_bad_atom () {
ref=$1 format=$2
printf '%s\n' "$3" >expect
$test_do=test_expect_${4:-success}
$test_do $PREREQ "err basic atom: $ref $format" '
test_must_fail git for-each-ref \
--format="%($format)" "$ref" 2>error &&
test_cmp expect error
'
}
This is primarily because you cannot control what is in "$2" to
ensure the correctness of the test we see above locally (i.e. if
your caller has a single-quote in "$2", the shell script you create
for running test_expect_{success,failure} would be syntactically
incorrect). By enclosing the executable part inside a pair of
single quotes, and having the $variables interpolated when that
executable part is `eval`ed when test_expect_{success,failure} runs,
you will avoid such problems, and those reading the above can locally
know that you are aware of and correctly avoiding such problems.
I guess three among four problems I just pointed out you blindly
copied from test_atom. But let's not spread badness (preliminary
clean-up to existing badness would be welcome instead).
> +}
> +
> +test_bad_atom head 'authoremail:foo' \
> + 'fatal: unrecognized %(authoremail) argument: foo'
> +
> +test_bad_atom tag 'taggeremail:localpart trim' \
> + 'fatal: unrecognized %(taggeremail) argument: trim'
It is strange to see double SP before 'trim' in this error message.
Are we etching a code mistake in stone here? Wouldn't the error
message say "...argument: localpart trim" instead, perhaps?
> test_date () {
> f=$1 &&
> committer_date=$2 &&
^ permalink raw reply
* Re: [PATCH 1/2] t/t6300: introduce test_bad_atom()
From: Junio C Hamano @ 2023-09-20 23:09 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma
In-Reply-To: <xmqqy1h078tf.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> + case "$1" in
>> + head) ref=refs/heads/main ;;
>> + tag) ref=refs/tags/testtag ;;
>> + sym) ref=refs/heads/sym ;;
>> + *) ref=$1 ;;
>> + esac
>
> Somehow this indirection makes the two examples we see below harder
> to understand. ... It does not help that both 'head' and
> 'sym' refer to a local branch ...
Ah, this "sym" thing is a (rather unnatural) symbolic ref inside
refs/heads/ hierarchy, so the naming makes halfway sense. As it is
also used in test_atom, I no longer find it all that much disturbing.
Everything else I said in my review still stands, I would think.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] t/t6300: introduce test_bad_atom()
From: Junio C Hamano @ 2023-09-20 23:21 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma
In-Reply-To: <xmqqy1h078tf.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> +test_bad_atom tag 'taggeremail:localpart trim' \
>> + 'fatal: unrecognized %(taggeremail) argument: trim'
>
> It is strange to see double SP before 'trim' in this error message.
> Are we etching a code mistake in stone here? Wouldn't the error
> message say "...argument: localpart trim" instead, perhaps?
I tried. The fatal message does say ...argument: localpart trim" as
I suspected, when you ask for 'taggeremail:localpart trim'.
I think I know what is going on. With the [PATCH 1/2] as-is, this
piece does not pass. But because the error message from parsing
gets broken by [PATCH 2/2], after applying [PATCH 2/2], the error
message will become what the above test expects, hiding the new
breakage in the code. And it probably was not noticed before you
sent the patches, because you did not test [PATCH 1/2] alone.
^ permalink raw reply
* Re: [PATCH v2 0/6] RFC: simple reftable backend
From: Patrick Steinhardt @ 2023-09-21 10:01 UTC (permalink / raw)
To: Han-Wen Nienhuys via GitGitGadget; +Cc: git, Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.v2.git.git.1695214968.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 27876 bytes --]
On Wed, Sep 20, 2023 at 01:02:42PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:
> This series comes from a conversation with Patrick Steinhardt at Gitlab, who
> have an interest in a more scalable ref storage system.
>
> I unfortunately don't have business reasons to spend much time on this
> project anymore, and the original, sweeping migration has too many open
> questions.
>
> I thought of an alternate approach, and I wanted to show it as input to
> discussions at the contributor summit.
>
> I think the first part (everything before "refs: alternate reftable ref
> backend implementation") is a good improvement overall, and could be landed
> separately without much tweaking.
>
> The second part ("refs: alternate reftable ref backend implementation" and
> follow-on) is something open for discussion: the alternative "packed
> storage" is based on reftable, but it could conceivably be a different type
> of database/file format too. I think it's attractive to use reftable here,
> because over time more data (symrefs, reflog) could be moved into reftable.
>
> I'm offering this series as a RFC. I hope that someone else (Patrick?) can
> look into getting the bits and pieces of this merged.
I've added this topic to the Contributor's summit such that the project
can discuss what to do about the different proposals that exist around
next-gen reference backends. If the conclusion is that reftables and
this proposal here is still likely to land, then I'm happy to pick it up
and try to get it upstreamed.
I'll have a look at this RFC early next week.
Thanks!
Patrick
> Han-Wen Nienhuys (6):
> refs: construct transaction using a _begin callback
> refs: wrap transaction in a debug-specific transaction
> refs: push lock management into packed backend
> refs: move is_packed_transaction_needed out of packed-backend.c
> refs: alternate reftable ref backend implementation
> refs: always try to do packed transactions for reftable
>
> Makefile | 1 +
> config.mak.uname | 2 +-
> contrib/workdir/git-new-workdir | 2 +-
> refs.c | 13 +-
> refs/debug.c | 87 +-
> refs/files-backend.c | 216 ++--
> refs/packed-backend.c | 149 +--
> refs/packed-backend.h | 19 -
> refs/refs-internal.h | 5 +
> refs/reftable-backend.c | 1658 +++++++++++++++++++++++++++++++
> refs/reftable-backend.h | 8 +
> 11 files changed, 1929 insertions(+), 231 deletions(-)
> create mode 100644 refs/reftable-backend.c
> create mode 100644 refs/reftable-backend.h
>
>
> base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1574%2Fhanwen%2Fsimple-reftable-backend-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1574/hanwen/simple-reftable-backend-v2
> Pull-Request: https://github.com/git/git/pull/1574
>
> Range-diff vs v1:
>
> -: ----------- > 1: e99f3d20056 refs: construct transaction using a _begin callback
> -: ----------- > 2: 9a459259330 refs: wrap transaction in a debug-specific transaction
> 1: dea0fbb139a ! 3: 8dedb23eb69 refs: push lock management into packed backend
> @@ Metadata
> ## Commit message ##
> refs: push lock management into packed backend
>
> - Introduces a ref backend method transaction_begin, which for the
> - packed backend takes packed-refs.lock.
> + Take packed-refs.lock in the transaction_begin of the packed backend.
>
> This decouples the files backend from the packed backend, allowing the
> latter to be swapped out by another ref backend.
> @@ Commit message
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
>
> ## refs.c ##
> -@@ refs.c: struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
> - struct strbuf *err)
> - {
> - struct ref_transaction *tr;
> -+ int ret = 0;
> - assert(err);
> -
> - CALLOC_ARRAY(tr, 1);
> - tr->ref_store = refs;
> -+
> -+ if (refs->be->transaction_begin)
> -+ ret = refs->be->transaction_begin(refs, tr, err);
> -+ if (ret) {
> -+ free(tr);
> -+ return NULL;
> -+ }
> - return tr;
> - }
> -
> -
> - ## refs/debug.c ##
> -@@ refs/debug.c: static int debug_init_db(struct ref_store *refs, struct strbuf *err)
> - return res;
> - }
> -
> -+static int debug_transaction_begin(struct ref_store *refs,
> -+ struct ref_transaction *transaction,
> -+ struct strbuf *err)
> -+{
> -+ struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
> -+ int res;
> -+ transaction->ref_store = drefs->refs;
> -+ res = drefs->refs->be->transaction_begin(drefs->refs, transaction,
> -+ err);
> -+ trace_printf_key(&trace_refs, "transaction_begin: %d \"%s\"\n", res,
> -+ err->buf);
> -+ return res;
> -+}
> -+
> - static int debug_transaction_prepare(struct ref_store *refs,
> - struct ref_transaction *transaction,
> - struct strbuf *err)
> -@@ refs/debug.c: struct ref_storage_be refs_be_debug = {
> - * has a function we should also have a wrapper for it here.
> - * Test the output with "GIT_TRACE_REFS=1".
> - */
> -+ .transaction_begin = debug_transaction_begin,
> - .transaction_prepare = debug_transaction_prepare,
> - .transaction_finish = debug_transaction_finish,
> - .transaction_abort = debug_transaction_abort,
> +@@ refs.c: int ref_transaction_abort(struct ref_transaction *transaction,
> + int ret = 0;
> +
> + switch (transaction->state) {
> +- case REF_TRANSACTION_OPEN:
> +- /* No need to abort explicitly. */
> +- break;
> + case REF_TRANSACTION_PREPARED:
> ++ case REF_TRANSACTION_OPEN:
> ++ /* an OPEN transactions may have a lock. */
> + ret = refs->be->transaction_abort(refs, transaction, err);
> + break;
> + case REF_TRANSACTION_CLOSED:
>
> ## refs/files-backend.c ##
> @@ refs/files-backend.c: static int files_pack_refs(struct ref_store *ref_store,
> @@ refs/files-backend.c: static int files_transaction_finish(struct ref_store *ref_
> - backend_data->packed_transaction = NULL;
> - if (ret)
> - goto cleanup;
> -+ if (backend_data->packed_transaction) {
> ++ if (backend_data->packed_transaction) {
> + if (backend_data->packed_transaction_needed) {
> + ret = ref_transaction_commit(packed_transaction, err);
> + if (ret)
> + goto cleanup;
> -+ /* TODO: leaks on error path. */
> ++
> + ref_transaction_free(packed_transaction);
> + packed_transaction = NULL;
> + backend_data->packed_transaction = NULL;
> @@ refs/files-backend.c: static int files_initial_transaction_commit(struct ref_sto
> ref_transaction_free(packed_transaction);
>
> ## refs/packed-backend.c ##
> +@@ refs/packed-backend.c: static int write_packed_entry(FILE *fh, const char *refname,
> + return 0;
> + }
> +
> +-int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
> ++static int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
> + {
> + struct packed_ref_store *refs =
> + packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
> +@@ refs/packed-backend.c: int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
> + return 0;
> + }
> +
> +-void packed_refs_unlock(struct ref_store *ref_store)
> ++static void packed_refs_unlock(struct ref_store *ref_store)
> + {
> + struct packed_ref_store *refs = packed_downcast(
> + ref_store,
> +@@ refs/packed-backend.c: void packed_refs_unlock(struct ref_store *ref_store)
> + rollback_lock_file(&refs->lock);
> + }
> +
> +-int packed_refs_is_locked(struct ref_store *ref_store)
> +-{
> +- struct packed_ref_store *refs = packed_downcast(
> +- ref_store,
> +- REF_STORE_READ | REF_STORE_WRITE,
> +- "packed_refs_is_locked");
> +-
> +- return is_lock_file_locked(&refs->lock);
> +-}
> +-
> + /*
> + * The packed-refs header line that we write out. Perhaps other traits
> + * will be added later.
> @@ refs/packed-backend.c: int is_packed_transaction_needed(struct ref_store *ref_store,
>
> struct packed_transaction_backend_data {
> @@ refs/packed-backend.c: static void packed_transaction_cleanup(struct packed_ref_
> transaction->state = REF_TRANSACTION_CLOSED;
> }
>
> -+static int packed_transaction_begin(struct ref_store *ref_store,
> -+ struct ref_transaction *transaction,
> +-
> + static struct ref_transaction *packed_transaction_begin(struct ref_store *ref_store,
> +- struct strbuf *err) {
> +- struct ref_transaction *tr;
> +- CALLOC_ARRAY(tr, 1);
> +- return tr;
> + struct strbuf *err)
> +{
> + struct packed_ref_store *refs = packed_downcast(
> @@ refs/packed-backend.c: static void packed_transaction_cleanup(struct packed_ref_
> + REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,
> + "ref_transaction_begin");
> + struct packed_transaction_backend_data *data;
> -+
> -+ CALLOC_ARRAY(data, 1);
> -+ string_list_init_nodup(&data->updates);
> ++ struct ref_transaction *transaction;
> +
> + if (!is_lock_file_locked(&refs->lock)) {
> + if (packed_refs_lock(ref_store, 0, err))
> -+ /* todo: leaking data */
> -+ return -1;
> ++ return NULL;
> + }
> ++ CALLOC_ARRAY(transaction, 1);
> ++ CALLOC_ARRAY(data, 1);
> ++ string_list_init_nodup(&data->updates);
> + transaction->backend_data = data;
> -+ return 0;
> -+}
> -+
> ++ return transaction;
> + }
> +
> static int packed_transaction_prepare(struct ref_store *ref_store,
> - struct ref_transaction *transaction,
> - struct strbuf *err)
> @@ refs/packed-backend.c: static int packed_transaction_prepare(struct ref_store *ref_store,
> ref_store,
> REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,
> @@ refs/packed-backend.c: static int packed_transaction_prepare(struct ref_store *r
> if (write_with_updates(refs, &data->updates, err))
> goto failure;
>
> -@@ refs/packed-backend.c: struct ref_storage_be refs_be_packed = {
> - .name = "packed",
> - .init = packed_ref_store_create,
> - .init_db = packed_init_db,
> -+ .transaction_begin = packed_transaction_begin,
> - .transaction_prepare = packed_transaction_prepare,
> - .transaction_finish = packed_transaction_finish,
> - .transaction_abort = packed_transaction_abort,
>
> ## refs/packed-backend.h ##
> @@ refs/packed-backend.h: struct ref_store *packed_ref_store_create(struct repository *repo,
> @@ refs/packed-backend.h: struct ref_store *packed_ref_store_create(struct reposito
> /*
> * Return true if `transaction` really needs to be carried out against
> * the specified packed_ref_store, or false if it can be skipped
> -
> - ## refs/refs-internal.h ##
> -@@ refs/refs-internal.h: typedef struct ref_store *ref_store_init_fn(struct repository *repo,
> -
> - typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);
> -
> -+typedef int ref_transaction_begin_fn(struct ref_store *refs,
> -+ struct ref_transaction *transaction,
> -+ struct strbuf *err);
> -+
> - typedef int ref_transaction_prepare_fn(struct ref_store *refs,
> - struct ref_transaction *transaction,
> - struct strbuf *err);
> -@@ refs/refs-internal.h: struct ref_storage_be {
> - ref_store_init_fn *init;
> - ref_init_db_fn *init_db;
> -
> -+ ref_transaction_begin_fn *transaction_begin;
> - ref_transaction_prepare_fn *transaction_prepare;
> - ref_transaction_finish_fn *transaction_finish;
> - ref_transaction_abort_fn *transaction_abort;
> 2: a5cb3adc662 ! 4: 0b8919b05c4 refs: move is_packed_transaction_needed out of packed-backend.c
> @@ refs/files-backend.c: out:
> return ret;
> }
>
> -+
> +/*
> + * Return true if `transaction` really needs to be carried out against
> + * the specified packed_ref_store, or false if it can be skipped
> @@ refs/files-backend.c: out:
> struct files_transaction_backend_data {
> struct ref_transaction *packed_transaction;
> int packed_transaction_needed;
> -@@ refs/files-backend.c: struct ref_storage_be refs_be_files = {
> - .delete_reflog = files_delete_reflog,
> - .reflog_expire = files_reflog_expire
> - };
> -+
>
> ## refs/packed-backend.c ##
> @@ refs/packed-backend.c: error:
> 3: 559c04cee99 ! 5: d5669da57e3 refs: alternate reftable ref backend implementation
> @@ Commit message
> refs: alternate reftable ref backend implementation
>
> This introduces the reftable backend as an alternative to the packed
> - backend. This simplifies the code, because we now longer have to worry
> - about:
> + backend.
> +
> + This is an alternative to the approach which was explored in
> + https://github.com/git/git/pull/1215/. This alternative is simpler,
> + because we now longer have to worry about:
>
> - pseudorefs and the HEAD ref
> - worktrees
> - commands that blur together files and references (cherry-pick, rebase)
>
> This deviates from the spec that in
> - Documentation/technical/reftable.txt. It might be possible to update
> - the code such that all writes by default go to reftable directly. Then
> - the result would be compatible with an implementation that writes only
> - reftable (the reftable lock would still prevent races) relative to an
> - implementation that disregards loose refs. Or, JGit could be adapted
> - to follow this implementation.
> + Documentation/technical/reftable.txt. It might be possible to update the
> + code such that all writes by default go to reftable directly. Then the
> + result would be compatible with an implementation that writes only
> + reftable (the reftable lock would still prevent races), but something
> + must be done about the HEAD ref (which JGit keeps in reftable too.)
> + Alternatively, JGit could be adapted to follow this implementation:
> + despite the code being there for 4 years now, I haven't heard of anyone
> + using it in production (exactly because CGit does not support it).
>
> For this incremental path, the reftable format is arguably more
> complex than necessary, as
>
> - packed-refs doesn't support symrefs
> - - reflogs aren't moved into reftable/
> + - reflogs aren't moved into reftable
>
> on the other hand, the code is already there, and it's well-structured
> and well-tested.
>
> - This implementation is a prototype. To test, you need to do `export
> - GIT_TEST_REFTABLE=1`. Doing so creates a handful of errors in the
> - test-suite, most seemingly related to the new behavior of pack-refs
> - (which creates a reftable/ dir and not a packed-refs file.), but it
> - seems overseeable.
> + refs/reftable-backend.c was created by cannibalizing the first version
> + of reftable support (github PR 1215 as mentioned above). It supports
> + reflogs and symrefs, even though those features are never exercised.
> +
> + This implementation is a prototype, for the following reasons:
> +
> + - no considerations of backward compatibility and configuring an
> + extension
> + - no support for converting between packed-refs and reftable
> + - no documentation
> + - test failures when setting GIT_TEST_REFTABLE=1.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
>
> @@ contrib/workdir/git-new-workdir: trap cleanup $siglist
> case $x in
>
> ## refs/files-backend.c ##
> +@@
> + #include "../git-compat-util.h"
> ++#include "../abspath.h"
> + #include "../config.h"
> + #include "../copy.h"
> + #include "../environment.h"
> @@
> #include "refs-internal.h"
> #include "ref-cache.h"
> @@ refs/files-backend.c
> #include "../iterator.h"
> #include "../dir-iterator.h"
> @@ refs/files-backend.c: static struct ref_store *files_ref_store_create(struct repository *repo,
> + struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
> + struct ref_store *ref_store = (struct ref_store *)refs;
> + struct strbuf sb = STRBUF_INIT;
> ++ int has_reftable, has_packed;
> +
> + base_ref_store_init(ref_store, repo, gitdir, &refs_be_files);
> refs->store_flags = flags;
> get_common_dir_noenv(&sb, gitdir);
> refs->gitcommondir = strbuf_detach(&sb, NULL);
> -+
> -+ /* TODO: should look at the repo to decide whether to use packed-refs or
> -+ * reftable. */
> - refs->packed_ref_store =
> +- refs->packed_ref_store =
> - packed_ref_store_create(repo, refs->gitcommondir, flags);
> -+ git_env_bool("GIT_TEST_REFTABLE", 0)
> ++
> ++ strbuf_addf(&sb, "%s/reftable", refs->gitcommondir);
> ++ has_reftable = is_directory(sb.buf);
> ++ strbuf_reset(&sb);
> ++ strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
> ++ has_packed = file_exists(sb.buf);
> ++
> ++ if (!has_packed && !has_reftable)
> ++ has_reftable = git_env_bool("GIT_TEST_REFTABLE", 0);
> ++
> ++ refs->packed_ref_store = has_reftable
> + ? git_reftable_ref_store_create(repo, refs->gitcommondir, flags)
> + : packed_ref_store_create(repo, refs->gitcommondir, flags);
>
> chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir);
> chdir_notify_reparent("files-backend $GIT_COMMONDIR",
>
> - ## refs/packed-backend.c ##
> -@@ refs/packed-backend.c: static int write_packed_entry(FILE *fh, const char *refname,
> - return 0;
> - }
> -
> --int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
> -+static int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
> - {
> - struct packed_ref_store *refs =
> - packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
> -@@ refs/packed-backend.c: int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
> - return 0;
> - }
> -
> --void packed_refs_unlock(struct ref_store *ref_store)
> -+static void packed_refs_unlock(struct ref_store *ref_store)
> - {
> - struct packed_ref_store *refs = packed_downcast(
> - ref_store,
> -@@ refs/packed-backend.c: void packed_refs_unlock(struct ref_store *ref_store)
> - rollback_lock_file(&refs->lock);
> - }
> -
> --int packed_refs_is_locked(struct ref_store *ref_store)
> --{
> -- struct packed_ref_store *refs = packed_downcast(
> -- ref_store,
> -- REF_STORE_READ | REF_STORE_WRITE,
> -- "packed_refs_is_locked");
> --
> -- return is_lock_file_locked(&refs->lock);
> --}
> --
> - /*
> - * The packed-refs header line that we write out. Perhaps other traits
> - * will be added later.
> -
> ## refs/refs-internal.h ##
> @@ refs/refs-internal.h: struct ref_storage_be {
> };
> @@ refs/reftable-backend.c (new)
> +#include "../worktree.h"
> +#include "refs-internal.h"
> +#include "reftable-backend.h"
> ++#include "../repository.h"
> +
> +extern struct ref_storage_be refs_be_reftable;
> +
> @@ refs/reftable-backend.c (new)
> + return &ri->base;
> +}
> +
> -+static int fixup_symrefs(struct ref_store *ref_store,
> -+ struct ref_transaction *transaction)
> -+{
> -+ struct strbuf referent = STRBUF_INIT;
> -+ int i = 0;
> -+ int err = 0;
> -+
> -+ for (i = 0; i < transaction->nr; i++) {
> -+ struct ref_update *update = transaction->updates[i];
> -+ struct object_id old_oid;
> -+ int failure_errno;
> -+
> -+ err = git_reftable_read_raw_ref(ref_store, update->refname,
> -+ &old_oid, &referent,
> -+ /* mutate input, like
> -+ files-backend.c */
> -+ &update->type, &failure_errno);
> -+ if (err < 0 && failure_errno == ENOENT &&
> -+ is_null_oid(&update->old_oid)) {
> -+ err = 0;
> -+ }
> -+ if (err < 0)
> -+ goto done;
> -+
> -+ if (!(update->type & REF_ISSYMREF))
> -+ continue;
> -+
> -+ if (update->flags & REF_NO_DEREF) {
> -+ /* what should happen here? See files-backend.c
> -+ * lock_ref_for_update. */
> -+ } else {
> -+ /*
> -+ If we are updating a symref (eg. HEAD), we should also
> -+ update the branch that the symref points to.
> -+
> -+ This is generic functionality, and would be better
> -+ done in refs.c, but the current implementation is
> -+ intertwined with the locking in files-backend.c.
> -+ */
> -+ int new_flags = update->flags;
> -+ struct ref_update *new_update = NULL;
> -+
> -+ /* if this is an update for HEAD, should also record a
> -+ log entry for HEAD? See files-backend.c,
> -+ split_head_update()
> -+ */
> -+ new_update = ref_transaction_add_update(
> -+ transaction, referent.buf, new_flags,
> -+ &update->new_oid, &update->old_oid,
> -+ update->msg);
> -+ new_update->parent_update = update;
> -+
> -+ /* files-backend sets REF_LOG_ONLY here. */
> -+ update->flags |= REF_NO_DEREF | REF_LOG_ONLY;
> -+ update->flags &= ~REF_HAVE_OLD;
> -+ }
> -+ }
> -+
> -+done:
> -+ assert(err != REFTABLE_API_ERROR);
> -+ strbuf_release(&referent);
> -+ return err;
> -+}
> -+
> -+static int git_reftable_transaction_prepare(struct ref_store *ref_store,
> -+ struct ref_transaction *transaction,
> -+ struct strbuf *errbuf)
> ++static struct ref_transaction *git_reftable_transaction_begin(struct ref_store *ref_store,
> ++ struct strbuf *errbuf)
> +{
> + struct git_reftable_ref_store *refs =
> + (struct git_reftable_ref_store *)ref_store;
> + struct reftable_addition *add = NULL;
> -+ struct reftable_stack *stack = stack_for(
> -+ refs,
> -+ transaction->nr ? transaction->updates[0]->refname : NULL);
> -+ int i;
> ++ struct ref_transaction *transaction = NULL;
> ++ struct reftable_stack *stack = refs->main_stack;
> +
> + int err = refs->err;
> + if (err < 0) {
> @@ refs/reftable-backend.c (new)
> +
> + err = reftable_stack_new_addition(&add, stack);
> + if (err) {
> ++ strbuf_addf(errbuf, "reftable: transaction begin: %s",
> ++ reftable_error_str(err));
> + goto done;
> + }
> +
> -+ for (i = 0; i < transaction->nr; i++) {
> -+ struct ref_update *u = transaction->updates[i];
> -+ if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) &&
> -+ !(u->flags & REF_SKIP_OID_VERIFICATION) &&
> -+ !(u->flags & REF_LOG_ONLY)) {
> -+ struct object *o =
> -+ parse_object(refs->base.repo, &u->new_oid);
> -+ if (!o) {
> -+ strbuf_addf(
> -+ errbuf,
> -+ "trying to write ref '%s' with nonexistent object %s",
> -+ u->refname, oid_to_hex(&u->new_oid));
> -+ err = -1;
> -+ goto done;
> -+ }
> -+ }
> -+ }
> -+
> -+ err = fixup_symrefs(ref_store, transaction);
> -+ if (err) {
> -+ goto done;
> -+ }
> -+
> ++ CALLOC_ARRAY(transaction, 1);
> + transaction->backend_data = add;
> -+ transaction->state = REF_TRANSACTION_PREPARED;
> -+
> +done:
> -+ assert(err != REFTABLE_API_ERROR);
> -+ if (err < 0) {
> -+ if (add) {
> -+ reftable_addition_destroy(add);
> -+ add = NULL;
> -+ }
> -+ transaction->state = REF_TRANSACTION_CLOSED;
> -+ if (!errbuf->len)
> -+ strbuf_addf(errbuf, "reftable: transaction prepare: %s",
> -+ reftable_error_str(err));
> -+ }
> ++ return transaction;
> ++}
> +
> -+ return err;
> ++static int git_reftable_transaction_prepare(struct ref_store *ref_store,
> ++ struct ref_transaction *transaction,
> ++ struct strbuf *errbuf)
> ++{
> ++ transaction->state = REF_TRANSACTION_PREPARED;
> ++ return 0;
> +}
> +
> +static int git_reftable_transaction_abort(struct ref_store *ref_store,
> @@ refs/reftable-backend.c (new)
> + struct ref_transaction *transaction,
> + struct strbuf *errmsg)
> +{
> ++ struct git_reftable_ref_store *refs =
> ++ (struct git_reftable_ref_store *)transaction->ref_store;
> + struct reftable_addition *add =
> + (struct reftable_addition *)transaction->backend_data;
> + int err = 0;
> @@ refs/reftable-backend.c (new)
> +
> + err = reftable_addition_commit(add);
> +
> ++ if (!err)
> ++ err = reftable_stack_auto_compact(refs->main_stack);
> +done:
> + assert(err != REFTABLE_API_ERROR);
> + reftable_addition_destroy(add);
> @@ refs/reftable-backend.c (new)
> + .name = "reftable",
> + .init = git_reftable_ref_store_create,
> + .init_db = git_reftable_init_db,
> ++ .transaction_begin = git_reftable_transaction_begin,
> + .transaction_prepare = git_reftable_transaction_prepare,
> + .transaction_finish = git_reftable_transaction_finish,
> + .transaction_abort = git_reftable_transaction_abort,
> -: ----------- > 6: 2cf743031ab refs: always try to do packed transactions for reftable
>
> --
> gitgitgadget
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
From: Patrick Steinhardt @ 2023-09-21 10:05 UTC (permalink / raw)
To: git; +Cc: Christian Couder
[-- Attachment #1: Type: text/plain, Size: 6215 bytes --]
When reading revisions from stdin via git-rev-list(1)'s `--stdin` option
then these revisions never honor flags like `--not` which have been
passed on the command line. Thus, an invocation like e.g. `git rev-list
--all --not --stdin` will not treat all revisions read from stdin as
uninteresting. While this behaviour may be surprising to a user, it's
been this way ever since it has been introduced via 42cabc341c4 (Teach
rev-list an option to read revs from the standard input., 2006-09-05).
With that said, in c40f0b7877 (revision: handle pseudo-opts in `--stdin`
mode, 2023-06-15) we have introduced a new mode to read pseudo opts from
standard input where this behaviour is a lot more confusing. If you pass
`--not` via stdin, it will:
- Influence subsequent revisions or pseudo-options passed on the
command line.
- Influence pseudo-options passed via standard input.
- _Not_ influence normal revisions passed via standard input.
This behaviour is extremely inconsistent and bound to cause confusion.
While it would be nice to retroactively change the behaviour for how
`--not` and `--stdin` behave together, chances are quite high that this
would break existing scripts that expect the current behaviour that has
been around for many years by now. This is thus not really a viable
option to explore to fix the inconsistency.
Instead, we change the behaviour of how pseudo-opts read via standard
input influence the flags such that the effect is fully localized. With
this change, when reading `--not` via standard input, it will:
- _Not_ influence subsequent revisions or pseudo-options passed on
the command line, which is a change in behaviour.
- Influence pseudo-options passed via standard input.
- Influence normal revisions passed via standard input, which is a
change in behaviour.
Thus, all flags read via standard input are fully self-contained to that
standard input, only.
While this is a breaking change as well, the behaviour has only been
recently introduced with Git v2.42.0. Furthermore, the current behaviour
can be regarded as a simple bug. With that in mind it feels like the
right thing to do retroactively change it and make the behaviour sane.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Reported-by: Christian Couder <christian.couder@gmail.com>
---
Documentation/rev-list-options.txt | 6 +++++-
revision.c | 10 +++++-----
t/t6017-rev-list-stdin.sh | 21 +++++++++++++++++++++
3 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a4a0cb93b2..9bf13bac53 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -151,6 +151,8 @@ endif::git-log[]
--not::
Reverses the meaning of the '{caret}' prefix (or lack thereof)
for all following revision specifiers, up to the next `--not`.
+ When used on the command line before --stdin, the revisions passed
+ through stdin will not be affected by it.
--all::
Pretend as if all the refs in `refs/`, along with `HEAD`, are
@@ -240,7 +242,9 @@ endif::git-rev-list[]
them from standard input as well. This accepts commits and
pseudo-options like `--all` and `--glob=`. When a `--` separator
is seen, the following input is treated as paths and used to
- limit the result.
+ limit the result. Flags like `--not` which are read via standard input
+ are only respected for arguments passed in the same way and will not
+ influence any subsequent command line arguments.
ifdef::git-rev-list[]
--quiet::
diff --git a/revision.c b/revision.c
index 2f4c53ea20..a1f573ca06 100644
--- a/revision.c
+++ b/revision.c
@@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
}
static void read_revisions_from_stdin(struct rev_info *revs,
- struct strvec *prune,
- int *flags)
+ struct strvec *prune)
{
struct strbuf sb;
int seen_dashdash = 0;
int seen_end_of_options = 0;
int save_warning;
+ int flags = 0;
save_warning = warn_on_object_refname_ambiguity;
warn_on_object_refname_ambiguity = 0;
@@ -2817,13 +2817,13 @@ static void read_revisions_from_stdin(struct rev_info *revs,
continue;
}
- if (handle_revision_pseudo_opt(revs, argv, flags) > 0)
+ if (handle_revision_pseudo_opt(revs, argv, &flags) > 0)
continue;
die(_("invalid option '%s' in --stdin mode"), sb.buf);
}
- if (handle_revision_arg(sb.buf, revs, 0,
+ if (handle_revision_arg(sb.buf, revs, flags,
REVARG_CANNOT_BE_FILENAME))
die("bad revision '%s'", sb.buf);
}
@@ -2906,7 +2906,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (revs->read_from_stdin++)
die("--stdin given twice?");
- read_revisions_from_stdin(revs, &prune_data, &flags);
+ read_revisions_from_stdin(revs, &prune_data);
continue;
}
diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh
index a57f1ae2ba..4821b90e74 100755
--- a/t/t6017-rev-list-stdin.sh
+++ b/t/t6017-rev-list-stdin.sh
@@ -68,6 +68,7 @@ check --glob=refs/heads
check --glob=refs/heads --
check --glob=refs/heads -- file-1
check --end-of-options -dashed-branch
+check --all --not refs/heads/main
test_expect_success 'not only --stdin' '
cat >expect <<-EOF &&
@@ -127,4 +128,24 @@ test_expect_success 'unknown option without --end-of-options' '
test_cmp expect error
'
+test_expect_success '--not on command line does not influence revisions read via --stdin' '
+ cat >input <<-EOF &&
+ refs/heads/main
+ EOF
+ git rev-list refs/heads/main >expect &&
+
+ git rev-list refs/heads/main --not --stdin <input >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--not via stdin does not influence revisions from command line' '
+ cat >input <<-EOF &&
+ --not
+ EOF
+ git rev-list refs/heads/main >expect &&
+
+ git rev-list refs/heads/main --stdin refs/heads/main <input >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH] am: fix error message in parse_opt_show_current_patch()
From: Oswald Buddenhagen @ 2023-09-21 11:07 UTC (permalink / raw)
To: git; +Cc: Jean-Noël Avila, Johannes Sixt, René Scharfe,
Junio C Hamano
In-Reply-To: <fff19abd-263d-48c7-81fd-35a2766b6b16@web.de>
The argument order was incorrect. This was introduced by 246cac8505
(i18n: turn even more messages into "cannot be used together" ones,
2022-01-05).
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
fwiw, this is currently the only message that actually uses the %s=%s
format, so as of now, factoring out the argument names has only
theoretical value.
Cc: Jean-Noël Avila <jn.avila@free.fr>
Cc: Johannes Sixt <j6t@kdbg.org>
Cc: René Scharfe <l.s.r@web.de>
Cc: Junio C Hamano <gitster@pobox.com>
---
builtin/am.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/am.c b/builtin/am.c
index 202040b62e..6655059a57 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2303,7 +2303,8 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
return error(_("options '%s=%s' and '%s=%s' "
"cannot be used together"),
- "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
+ "--show-current-patch", arg,
+ "--show-current-patch", valid_modes[resume->sub_mode]);
resume->mode = RESUME_SHOW_PATCH;
resume->sub_mode = new_value;
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Oswald Buddenhagen @ 2023-09-21 10:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <f778bc6f-dbe1-4df6-95ff-c9e9f36a3cc9@web.de>
On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:
> MSVC warns about all combinations.
>
yes, though that's not a problem: after we established that the
underlying type is int, we can just have a cast in the initializer
macro.
>> so how about simply adding a (configure) test to ensure that there is
>> actually no problem, and calling it a day?
> If we base it on type size then we're making assumptions that I find
> hard to justify.
>
the only one i can think of is signedness. i think this can be safely
ignored as long as we use only small positive integers.
regards
^ permalink raw reply
* Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision
From: Junio C Hamano @ 2023-09-21 8:52 UTC (permalink / raw)
To: Jeff King; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <20230921041545.GA2338791@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> In an empty repository, "git log" will die anyway. So I think the more
> interesting case is "I have a repository with stuff in it, but HEAD
> points to an unborn branch". So:
>
> git --attr-source=HEAD diff foo^ foo
This still looks like a made-up example. Who in the right mind
would specify HEAD when both of the revs involved in the operation
are from branch 'foo'? The history of HEAD may not have anything
common with the operand of the operation 'foo' (or its parent), or
worse, it may not even exist.
But your "in this repository we never trust attributes from working
tree, take it instead from this file or from this blob" example does
make a lot more sense as a use case.
> And there you really are saying "if there are attributes in HEAD, use
> them; otherwise, don't worry about it". This is exactly what we do with
> mailmap.blob: in a bare repository it is set to HEAD by default, but if
> HEAD does not resolve, we just ignore it (just like a HEAD that does not
> contain a .mailmap file). And those match the non-bare cases, where we'd
> read those files from the working tree instead.
"HEAD" -> "HEAD:.mailmap" if I recall correctly.
And if HEAD does not resolve, we pretend as if HEAD is an empty
tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting
to do the same for the attribute sources and treat unborn HEAD as if
it specifies an empty tree-ish, without any configuration or an
extra option.
Such a change would be an end-user observable behaviour change, but
nobody sane would be running "git --attr-source=HEAD diff HEAD^ HEAD"
to check and detect an unborn HEAD for its error exit code, so I do
not think it is a horribly wrong thing to do.
But again, as you said, --attr-source=<tree-ish> does not sound like
a good fit for bare-repository hosted environment and a tentative
hack waiting for a proper attr.blob support, or something like that,
to appear.
> But what is weird about this patch is that we are using a config option
> to change how a command-line option is interpreted. If the idea is that
> some invocations care about the validity of the source and some do not,
> then the config option is much too blunt. It is set once long ago, but
> it can't distinguish between times you care about invalid sources and
> times you don't.
>
> It would make much more sense to me to have another command-line option,
> like:
>
> git --attr-source=HEAD --allow-invalid-attr-source
Yeah, if we were to make it configurable without changing the
default behaviour, I agree that would be more correct approach. A
configuration does not sound like a good fit.
> ... And I really think attr.blob is a better match for what GitLab
> is trying to do here, because it is set once and applies to all
> commands, rather than having to teach every invocation to pass it
> (though I guess maybe they use it as an environment variable).
True, too.
Thanks.
^ permalink raw reply
* Re: [PATCH v4] revision: add `--ignore-missing-links` user option
From: Karthik Nayak @ 2023-09-21 10:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, me
In-Reply-To: <xmqqh6noc13c.fsf@gitster.g>
On Wed, Sep 20, 2023 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> There needs an explanation of interaction with --missing=<action>
> option here, no? "--missing=allow-any" and "--missing=print" are
> sensible choices, I presume. The former allows the traversal to
> proceed, as you described in one of your responses. Also with
> "--missing=print", the user can more directly find out which are the
> missing objects, even without using the "--boundary" that requires
> them to sift between missing objects and the objects that are truly
> on boundary.
>
> Here is my attempt:
>
> --ignore-missing-links::
> During traversal, if an object that is referenced does not
> exist, instead of dying of a repository corruption, allow
> `--missing=<missing-action>` to decide what to do.
> +
> `--missing=print` will make the command print a list of missing
> objects, prefixed with a "?" character.
> +
> `--missing=allow-any` will make the command proceed without doing
> anything special. Used with `--boundary`, output these missing
> objects mixed with the commits on the edge of revision ranges,
> prefixed with a "-" character.
>
> It might make sense to add
>
> +
> Use of this option with other 'missing-action' may probably not
> give useful behaviour.
>
> at the end, but it may not be useful to the readers to say "we allow
> even more extra flexibility but haven't thought through what good
> they would do".
>
I was thinking about this, but mostly didn't do this, because the
interaction with `--missing`
is only for non-commit objects. Because for missing commits,
`--ignore-missing-links` skips
the commit and the value of `--missing` doesn't make any difference.
It's only for non-commit objects that `--missing` comes into play. So
perhaps change the current
explanation to:
--ignore-missing-links::
During traversal, if a commit that is referenced does not
exist, instead of dying of a repository corruption, pretend as
if the commit itself does not exist. Running the command
with the `--boundary` option makes these missing commits,
together with the commits on the edge of revision ranges
(i.e. true boundary objects), appear on the output, prefixed
with '-'.
This way `--ignore-missing-links` is specific to commits, combining
this with `--missing=...` for
non-commit objects is left to the user. What do you think?
> > +# With `--ignore-missing-links`, we stop the traversal when we encounter a
> > +# missing link. The boundary commit is not listed as we haven't used the
> > +# `--boundary` options.
> > +test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '
> > + hide_alternates &&
> > +
> > + git -C alt rev-list --objects --no-object-names \
> > + --ignore-missing-links --missing=allow-any HEAD >actual.raw &&
> > + git -C alt cat-file --batch-check="%(objectname)" \
> > + --batch-all-objects >expect.raw &&
> > +
> > + sort actual.raw >actual &&
> > + sort expect.raw >expect &&
> > + test_cmp expect actual
> > +'
>
> This gives a good baseline. "--missing=print" without "--boundary"
> may have more obvious use cases, but is there a practical use case
> for the output from an invocation with "--missing=allow-any" without
> "--boundary"? Just being curious if I am missing something obvious.
>
Not really, but it's easier to build up the testing, here without
boundary we can
use cat-file to test all objects (commits and others) that are output
by rev-list.
Then we can build on top of this in the next test, where we can also ensure that
boundary commits are printed. This however is very simplistic, as
you've mentioned.
There could be other objects and we don't really check.
> Perhaps add another test that uses "--missing=print" instead, and
> check that the "? missing" output matches what we expect to be
> missing? The same comment applies to the other test that uses
> "--missing=allow-any" without "--boundary" we see later.
>
Sure, we can add that too!
> > +# With `--ignore-missing-links` and `--boundary`, we can even print those boundary
> > +# commits.
> > +test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '
> > + git -C alt rev-list --ignore-missing-links --boundary HEAD >got &&
> > + grep "^-$(git rev-parse HEAD)" got
> > +'
>
> This makes sure what we expect to appear in 'got' actually is in
> 'got', but we should also make sure 'got' does not have anything
> unexpected.
>
Yeah, I can add that in too.
> > +test_expect_success "setup for rev-list --ignore-missing-links with missing objects" '
> > + show_alternates &&
> > + test_commit -C alt 11
> > +'
> > +
> > +for obj in "HEAD^{tree}" "HEAD:11.t"
> > +do
> > + # The `--ignore-missing-links` option should ensure that git-rev-list(1)
> > + # doesn't fail when used alongside `--objects` when a tree/blob is
> > + # missing.
> > + test_expect_success "rev-list --ignore-missing-links with missing $type" '
> > + oid="$(git -C alt rev-parse $obj)" &&
> > + path="alt/.git/objects/$(test_oid_to_path $oid)" &&
> > +
> > + mv "$path" "$path.hidden" &&
> > + test_when_finished "mv $path.hidden $path" &&
>
> In the first iteration, we check without the tree object and we only
> ensure that removed tree does not appear in the output---but we know
> the blob that is referenced by that removed tree will not appear in
> the output, either, don't we? Don't we want to check that, too?
>
> In the second iteration, we have resurrected the tree but removed
> the blob that is referenced by the tree, so we would not see that
> blob in the output, which makes sense.
>
I was implementing this change and just realized that for missing
trees, show_object() is
never called (that is --missing=print has no effect).
That means we only call show_object() when there is a missing blob.
So this effectively means:
1. missing commits: --ignore-missing-links works, --missing=... has no effect
2. missing trees: --ignore-missing-links works, --missing=... has no effect
3. missing blobs: --ignore-missing-links works in conjunction
with --missing=...
I now think it does make even more sense to hardcode the skipping of
`finish_object__ma` this way
we can state that `--ignore-missing-links` and `--missing` are
incompatible, wherein `--ignore-missing-links`
ignores any missing object (irrelevant of type) and `--missing` is
used to specifically handle missing blobs
and provides options.
This is also how currently `--boundary` and `--missing=print` is
specific to commits and blobs respectively.
What do you think?
^ permalink raw reply
* Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
From: Taylor Blau @ 2023-09-21 18:45 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder
In-Reply-To: <b93d4c8c23552abab64084b62f27944e7e192c0c.1695290733.git.ps@pks.im>
On Thu, Sep 21, 2023 at 12:05:57PM +0200, Patrick Steinhardt wrote:
> When reading revisions from stdin via git-rev-list(1)'s `--stdin` option
> then these revisions never honor flags like `--not` which have been
> passed on the command line. Thus, an invocation like e.g. `git rev-list
> --all --not --stdin` will not treat all revisions read from stdin as
> uninteresting. While this behaviour may be surprising to a user, it's
> been this way ever since it has been introduced via 42cabc341c4 (Teach
> rev-list an option to read revs from the standard input., 2006-09-05).
I'm not sure I agree that `--all --not --stdin` marking the tips given
on stdin as uninteresting would be surprising to users. It feels like
`--stdin` introduces its own "scope" that `--not` should apply to.
I might be biased or looking at this differently than how other users
might, but `--all --not --stdin` reads like "traverse everything except
what I give you over stdin", and deviating from that behavior feels more
surprising than not.
Either way, since this comes from as far back as 42cabc341c4, I think
that we're stuck with this behavior one way or the other ;-).
> With that said, in c40f0b7877 (revision: handle pseudo-opts in `--stdin`
> mode, 2023-06-15) we have introduced a new mode to read pseudo opts from
> standard input where this behaviour is a lot more confusing. If you pass
> `--not` via stdin, it will:
>
> - Influence subsequent revisions or pseudo-options passed on the
> command line.
I agree here that this behavior is legitimately surprising and should
probably be considered a bug.
> - Influence pseudo-options passed via standard input.
>
> - _Not_ influence normal revisions passed via standard input.
>
> This behaviour is extremely inconsistent and bound to cause confusion.
>
> While it would be nice to retroactively change the behaviour for how
> `--not` and `--stdin` behave together, chances are quite high that this
> would break existing scripts that expect the current behaviour that has
> been around for many years by now. This is thus not really a viable
> option to explore to fix the inconsistency.
>
> Instead, we change the behaviour of how pseudo-opts read via standard
> input influence the flags such that the effect is fully localized. With
> this change, when reading `--not` via standard input, it will:
>
> - _Not_ influence subsequent revisions or pseudo-options passed on
> the command line, which is a change in behaviour.
>
> - Influence pseudo-options passed via standard input.
>
> - Influence normal revisions passed via standard input, which is a
> change in behaviour.
These all same very sane to me. Let's read on...
> Thus, all flags read via standard input are fully self-contained to that
> standard input, only.
>
> While this is a breaking change as well, the behaviour has only been
> recently introduced with Git v2.42.0. Furthermore, the current behaviour
> can be regarded as a simple bug. With that in mind it feels like the
> right thing to do retroactively change it and make the behaviour sane.
I agree. I am not so sympathetic to scripts that rely on this behavior,
which feels like it is obviously broken.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> Reported-by: Christian Couder <christian.couder@gmail.com>
> ---
> Documentation/rev-list-options.txt | 6 +++++-
> revision.c | 10 +++++-----
> t/t6017-rev-list-stdin.sh | 21 +++++++++++++++++++++
> 3 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index a4a0cb93b2..9bf13bac53 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -151,6 +151,8 @@ endif::git-log[]
> --not::
> Reverses the meaning of the '{caret}' prefix (or lack thereof)
> for all following revision specifiers, up to the next `--not`.
> + When used on the command line before --stdin, the revisions passed
> + through stdin will not be affected by it.
Hmmph. This seems to change the behavior introduced by 42cabc341c4. Am I
reading this correctly that tips on stdin with '--not --stdin' would not
be marked as UNINTERESTING?
(Reading this back, there are a lot of double-negatives in what I just
wrote making it confusing for me at least. What I'm getting at here is
that I think `--not --stdin` should mark tips given via stdin as
UNINTERESTING).
> --all::
> Pretend as if all the refs in `refs/`, along with `HEAD`, are
> @@ -240,7 +242,9 @@ endif::git-rev-list[]
> them from standard input as well. This accepts commits and
> pseudo-options like `--all` and `--glob=`. When a `--` separator
> is seen, the following input is treated as paths and used to
> - limit the result.
> + limit the result. Flags like `--not` which are read via standard input
> + are only respected for arguments passed in the same way and will not
> + influence any subsequent command line arguments.
>
> ifdef::git-rev-list[]
> --quiet::
> diff --git a/revision.c b/revision.c
> index 2f4c53ea20..a1f573ca06 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
> }
>
> static void read_revisions_from_stdin(struct rev_info *revs,
> - struct strvec *prune,
> - int *flags)
> + struct strvec *prune)
> {
> struct strbuf sb;
> int seen_dashdash = 0;
> int seen_end_of_options = 0;
> int save_warning;
> + int flags = 0;
OK, I think this confirms my assumption that the `--not` in `--not
--stdin` does not propagate across to the input given over stdin. I am
not sure that we are safely able to change that behavior.
I wonder if we could instead do something like:
- inherit the current set of psuedo-opts when processing input over
`--stdin`
- allow pseudo-opts within the context of `--stdin` arbitrarily
- prevent those psuedo-opts applied while processing `--stdin` from
leaking over to subsequent command-line arguments.
Here's one approach for doing that, where we make a copy of the current
set of flags when calling `read_revisions_from_stdin()` instead of
passing a pointer to the global state.
--- 8< ---
diff --git a/revision.c b/revision.c
index a1f573ca06..d8dad35d52 100644
--- a/revision.c
+++ b/revision.c
@@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
}
static void read_revisions_from_stdin(struct rev_info *revs,
- struct strvec *prune)
+ struct strvec *prune,
+ int flags)
{
struct strbuf sb;
int seen_dashdash = 0;
int seen_end_of_options = 0;
int save_warning;
- int flags = 0;
save_warning = warn_on_object_refname_ambiguity;
warn_on_object_refname_ambiguity = 0;
@@ -2906,7 +2906,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (revs->read_from_stdin++)
die("--stdin given twice?");
- read_revisions_from_stdin(revs, &prune_data);
+ read_revisions_from_stdin(revs, &prune_data,
+ flags);
continue;
}
--- >8 ---
Thanks,
^ permalink raw reply related
* Re: [PATCH v5 1/1] range-diff: treat notes like `log`
From: Johannes Schindelin @ 2023-09-21 12:30 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, Denton Liu, Jeff King
In-Reply-To: <6e114271a2e7d2323193bd58bb307f60101942ce.1695154855.git.code@khaugsbakk.name>
[-- Attachment #1: Type: text/plain, Size: 6670 bytes --]
Hi Kristoffer,
this iteration looks good to me!
Thank you so much,
Johannes
On Tue, 19 Sep 2023, Kristoffer Haugsbakk wrote:
> Currently, `range-diff` shows the default notes if no notes-related
> arguments are given. This is also how `log` behaves. But unlike
> `range-diff`, `log` does *not* show the default notes if
> `--notes=<custom>` are given. In other words, this:
>
> git log --notes=custom
>
> is equivalent to this:
>
> git log --no-notes --notes=custom
>
> While:
>
> git range-diff --notes=custom
>
> acts like this:
>
> git log --notes --notes-custom
>
> This can’t be how the user expects `range-diff` to behave given that the
> man page for `range-diff` under `--[no-]notes[=<ref>]` says:
>
> > This flag is passed to the `git log` program (see git-log(1)) that
> > generates the patches.
>
> This behavior also affects `format-patch` since it uses `range-diff` for
> the cover letter. Unlike `log`, though, `format-patch` is not supposed
> to show the default notes if no notes-related arguments are given.[1]
> But this promise is broken when the range-diff happens to have something
> to say about the changes to the default notes, since that will be shown
> in the cover letter.
>
> Remedy this by introducing `--show-notes-by-default` that `range-diff` can
> use to tell the `log` subprocess what to do.
>
> § Authors
>
> • Fix by Johannes
> • Tests by Kristoffer
>
> † 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about
> showing notes, 2010-01-20).
>
> Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> Documentation/pretty-options.txt | 4 ++++
> range-diff.c | 2 +-
> revision.c | 7 +++++++
> revision.h | 1 +
> t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
> 5 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
> index dc685be363a..335395b727f 100644
> --- a/Documentation/pretty-options.txt
> +++ b/Documentation/pretty-options.txt
> @@ -87,6 +87,10 @@ being displayed. Examples: "--notes=foo" will show only notes from
> "--notes --notes=foo --no-notes --notes=bar" will only show notes
> from "refs/notes/bar".
>
> +--show-notes-by-default::
> + Show the default notes unless options for displaying specific
> + notes are given.
> +
> --show-notes[=<ref>]::
> --[no-]standard-notes::
> These options are deprecated. Use the above --notes/--no-notes
> diff --git a/range-diff.c b/range-diff.c
> index ca5493984a5..c45b6d849cb 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -60,7 +60,7 @@ static int read_patches(const char *range, struct string_list *list,
> "--output-indicator-context=#",
> "--no-abbrev-commit",
> "--pretty=medium",
> - "--notes",
> + "--show-notes-by-default",
> NULL);
> strvec_push(&cp.args, range);
> if (other_arg)
> diff --git a/revision.c b/revision.c
> index 2f4c53ea207..49d385257ac 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2484,6 +2484,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> revs->break_bar = xstrdup(optarg);
> revs->track_linear = 1;
> revs->track_first_time = 1;
> + } else if (!strcmp(arg, "--show-notes-by-default")) {
> + revs->show_notes_by_default = 1;
> } else if (skip_prefix(arg, "--show-notes=", &optarg) ||
> skip_prefix(arg, "--notes=", &optarg)) {
> if (starts_with(arg, "--show-notes=") &&
> @@ -3054,6 +3056,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> if (revs->expand_tabs_in_log < 0)
> revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
>
> + if (!revs->show_notes_given && revs->show_notes_by_default) {
> + enable_default_display_notes(&revs->notes_opt, &revs->show_notes);
> + revs->show_notes_given = 1;
> + }
> +
> return left;
> }
>
> diff --git a/revision.h b/revision.h
> index 82ab400139d..50091bbd13f 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -253,6 +253,7 @@ struct rev_info {
> shown_dashes:1,
> show_merge:1,
> show_notes_given:1,
> + show_notes_by_default:1,
> show_signature:1,
> pretty_given:1,
> abbrev_commit:1,
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index b5f4d6a6530..b33afa1c6aa 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -662,6 +662,20 @@ test_expect_success 'range-diff with multiple --notes' '
> test_cmp expect actual
> '
>
> +# `range-diff` should act like `log` with regards to notes
> +test_expect_success 'range-diff with --notes=custom does not show default notes' '
> + git notes add -m "topic note" topic &&
> + git notes add -m "unmodified note" unmodified &&
> + git notes --ref=custom add -m "topic note" topic &&
> + git notes --ref=custom add -m "unmodified note" unmodified &&
> + test_when_finished git notes remove topic unmodified &&
> + test_when_finished git notes --ref=custom remove topic unmodified &&
> + git range-diff --notes=custom main..topic main..unmodified \
> + >actual &&
> + ! grep "## Notes ##" actual &&
> + grep "## Notes (custom) ##" actual
> +'
> +
> test_expect_success 'format-patch --range-diff does not compare notes by default' '
> git notes add -m "topic note" topic &&
> git notes add -m "unmodified note" unmodified &&
> @@ -679,6 +693,20 @@ test_expect_success 'format-patch --range-diff does not compare notes by default
> ! grep "note" 0000-*
> '
>
> +test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' '
> + git notes add -m "topic note" topic &&
> + git notes --ref=custom add -m "topic note (custom)" topic &&
> + git notes add -m "unmodified note" unmodified &&
> + git notes --ref=custom add -m "unmodified note (custom)" unmodified &&
> + test_when_finished git notes remove topic unmodified &&
> + test_when_finished git notes --ref=custom remove topic unmodified &&
> + git format-patch --notes=custom --cover-letter --range-diff=$prev \
> + main..unmodified >actual &&
> + test_when_finished "rm 000?-*" &&
> + grep "## Notes (custom) ##" 0000-* &&
> + ! grep "## Notes ##" 0000-*
> +'
> +
> test_expect_success 'format-patch --range-diff with --no-notes' '
> git notes add -m "topic note" topic &&
> git notes add -m "unmodified note" unmodified &&
> --
> 2.42.0
>
>
^ permalink raw reply
* Re: [PATCH] am: fix error message in parse_opt_show_current_patch()
From: Oswald Buddenhagen @ 2023-09-21 19:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jean-Noël Avila, Johannes Sixt, René Scharfe
In-Reply-To: <xmqqh6nn5oo9.fsf@gitster.g>
On Thu, Sep 21, 2023 at 12:09:10PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>> fwiw, this is currently the only message that actually uses the %s=%s
>> format, so as of now, factoring out the argument names has only
>> theoretical value.
>
>I am not sure I follow, if you mean that the programmer needs to
>pass "--show-current-patch" only once if we used something like
>"%1$s=%2s and %1$s=%3s", I agree that it probably has little value.
>
no, i mean that that the usual pattern is just "options '%s' and '%s'
cannot be used together". this format string is indeed used many times,
so it makes sense to factor out the option names to avoid duplication of
translatable strings. not so here. but this particular case is still a
lot less specialized than many of the other strings replaced by the
referenced patch, and it's at least plausible that further uses would be
added at some point, so i left it as-is.
i thought about the duplication of the option string as well, but
compilers should merge the string constants, so that part is indeed
de-duplicated. the cost of the extra pointer push could be avoided by
use of the %1$s syntax, but afaict that's unprecedented in git, and i
kind of expect that some printf implementation would throw up from it.
also, it might reduce the chance of the format being used in another
place, but who knows.
regards
^ permalink raw reply
* Re: [PATCH] am: fix error message in parse_opt_show_current_patch()
From: Junio C Hamano @ 2023-09-21 19:09 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: git, Jean-Noël Avila, Johannes Sixt, René Scharfe
In-Reply-To: <20230921110727.789156-1-oswald.buddenhagen@gmx.de>
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> The argument order was incorrect. This was introduced by 246cac8505
> (i18n: turn even more messages into "cannot be used together" ones,
> 2022-01-05).
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> fwiw, this is currently the only message that actually uses the %s=%s
> format, so as of now, factoring out the argument names has only
> theoretical value.
I am not sure I follow, if you mean that the programmer needs to
pass "--show-current-patch" only once if we used something like
"%1$s=%2s and %1$s=%3s", I agree that it probably has little value.
The patch looks good. It seems that we are seeing a crack in test
coverage, by the way?
Will queue. Thanks.
> diff --git a/builtin/am.c b/builtin/am.c
> index 202040b62e..6655059a57 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2303,7 +2303,8 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
> if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
> return error(_("options '%s=%s' and '%s=%s' "
> "cannot be used together"),
> - "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
> + "--show-current-patch", arg,
> + "--show-current-patch", valid_modes[resume->sub_mode]);
>
> resume->mode = RESUME_SHOW_PATCH;
> resume->sub_mode = new_value;
^ permalink raw reply
* Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
From: Junio C Hamano @ 2023-09-21 19:04 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder
In-Reply-To: <b93d4c8c23552abab64084b62f27944e7e192c0c.1695290733.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Instead, we change the behaviour of how pseudo-opts read via standard
> input influence the flags such that the effect is fully localized. With
> this change, when reading `--not` via standard input, it will:
>
> - _Not_ influence subsequent revisions or pseudo-options passed on
> the command line, which is a change in behaviour.
>
> - Influence pseudo-options passed via standard input.
>
> - Influence normal revisions passed via standard input, which is a
> change in behaviour.
>
> Thus, all flags read via standard input are fully self-contained to that
> standard input, only.
I have to wonder what the most natural expectation by end-users be,
when "cmd --opt1 --stdin --opt3 arg2" is run and its stdin is fed
"--opt2 arg1". One interpretation may be to act as if "--stdin" on
the command line is replaced with what was read, but taken literally
that would make "cmd --opt1 --opt2 arg1 --opt3 arg2" that does not
make sense (i.e. options must come before arguments). We could
declare "--stdin is replaced by options read from there, and
non-options read from the standard input are handled separately",
but then it could be argued "cmd --opt1 --opt2 --opt3 arg2 arg1"
and "cmd --opt1 --opt2 --opt3 arg1 arg2" are equally plausible.
So in a sense, "what is read from --stdin is self contained" may be
the easiest to explain position to take.
> While this is a breaking change as well, the behaviour has only been
> recently introduced with Git v2.42.0. Furthermore, the current behaviour
> can be regarded as a simple bug. With that in mind it feels like the
> right thing to do retroactively change it and make the behaviour sane.
While I also appreciate your cautious approach to consider the risk
that this "fix" may have negative consequence, I tend to agree that
the behaviour is simply buggy and deserves to be fixed on the
'maint' track.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> Reported-by: Christian Couder <christian.couder@gmail.com>
> ---
> Documentation/rev-list-options.txt | 6 +++++-
> revision.c | 10 +++++-----
> t/t6017-rev-list-stdin.sh | 21 +++++++++++++++++++++
> 3 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index a4a0cb93b2..9bf13bac53 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -151,6 +151,8 @@ endif::git-log[]
> --not::
> Reverses the meaning of the '{caret}' prefix (or lack thereof)
> for all following revision specifiers, up to the next `--not`.
> + When used on the command line before --stdin, the revisions passed
> + through stdin will not be affected by it.
Do we also need to say "when read from --stdin, the revisions passed
on the command line are not affected" as well? I know you have it
where you explian "--stdin" in the next hunk, but since you are
adding one-half of the interaction, it may be less confusing to also
mention the other half at the same time.
> @@ -240,7 +242,9 @@ endif::git-rev-list[]
> them from standard input as well. This accepts commits and
> pseudo-options like `--all` and `--glob=`. When a `--` separator
> is seen, the following input is treated as paths and used to
> - limit the result.
> + limit the result. Flags like `--not` which are read via standard input
> + are only respected for arguments passed in the same way and will not
> + influence any subsequent command line arguments.
Other than that, looking good, and the changes to the code look all
sensible.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] t/t6300: introduce test_bad_atom()
From: Kousik Sanagavarapu @ 2023-09-21 18:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Hariom Verma
In-Reply-To: <xmqqy1h078tf.fsf@gitster.g>
Sorry for the late reply.
On Wed, Sep 20, 2023 at 03:56:28PM -0700, Junio C Hamano wrote:
> Kousik Sanagavarapu <five231003@gmail.com> writes:
>
> > Introduce a new function "test_bad_atom()", which is similar to
> > "test_atom()" but should be used to check whether the correct error
> > message is shown on stderr.
> >
> > Like "test_atom()", the new function takes three arguments. The three
> > arguments specify the ref, the format and the expected error message
> > respectively, with an optional fourth argument for tweaking
> > "test_expect_*" (which is by default "success").
> >
> > Mentored-by: Christian Couder <christian.couder@gmail.com>
> > Mentored-by: Hariom Verma <hariom18599@gmail.com>
> > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> > ---
> > t/t6300-for-each-ref.sh | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> > index 7b943fd34c..15b4622f57 100755
> > --- a/t/t6300-for-each-ref.sh
> > +++ b/t/t6300-for-each-ref.sh
> > @@ -267,6 +267,26 @@ test_expect_success 'arguments to %(objectname:short=) must be positive integers
> > test_must_fail git for-each-ref --format="%(objectname:short=foo)"
> > '
> >
> > +test_bad_atom() {
>
> Style: have SP on both sides of "()".
>
> [...]
>
> > + printf '%s\n' "$3">expect
>
> Style: need SP before (but not after) '>'.
I'll make these style changes, they slipped by.
> > + test_expect_${4:-success} $PREREQ "err basic atom: $1 $2" "
> > + test_must_fail git for-each-ref --format='%($2)' $ref 2>actual &&
> > + test_cmp expect actual
> > + "
>
> It is error prone to have the executable part of test_expect_{success,failure}
> inside a pair of double quotes and have $variable interpolated
> _before_ even the arguments to test_expect_{success,failure} are
> formed. It is much more preferrable to write
>
> test_bad_atom () {
> ref=$1 format=$2
> printf '%s\n' "$3" >expect
> $test_do=test_expect_${4:-success}
>
> $test_do $PREREQ "err basic atom: $ref $format" '
> test_must_fail git for-each-ref \
> --format="%($format)" "$ref" 2>error &&
> test_cmp expect error
> '
> }
>
> This is primarily because you cannot control what is in "$2" to
> ensure the correctness of the test we see above locally (i.e. if
> your caller has a single-quote in "$2", the shell script you create
> for running test_expect_{success,failure} would be syntactically
> incorrect). By enclosing the executable part inside a pair of
> single quotes, and having the $variables interpolated when that
> executable part is `eval`ed when test_expect_{success,failure} runs,
> you will avoid such problems, and those reading the above can locally
> know that you are aware of and correctly avoiding such problems.
I see.
> I guess three among four problems I just pointed out you blindly
> copied from test_atom. But let's not spread badness (preliminary
> clean-up to existing badness would be welcome instead).
Yeah, I had copied it from test_atom. Although I didn't realize that it
was bad to implement test_bad_atom the way I did. Thanks for such a nice
explanation. So I guess we can include the test_atom cleanup in this
series?
> > +}
> > +
> > +test_bad_atom head 'authoremail:foo' \
> > + 'fatal: unrecognized %(authoremail) argument: foo'
> > +
> > +test_bad_atom tag 'taggeremail:localpart trim' \
> > + 'fatal: unrecognized %(taggeremail) argument: trim'
>
> It is strange to see double SP before 'trim' in this error message.
> Are we etching a code mistake in stone here? Wouldn't the error
> message say "...argument: localpart trim" instead, perhaps?
>
> > test_date () {
> > f=$1 &&
> > committer_date=$2 &&
So I read the the other replies and it seems that it indeed hides a
breakage and yeah I hadn't tested PATCH 1/2 independently. I'll change
this too.
Thanks
^ permalink raw reply
* Re: [PATCH v4] revision: add `--ignore-missing-links` user option
From: Junio C Hamano @ 2023-09-21 19:16 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, me
In-Reply-To: <CAOLa=ZT_DVw4Na-rj4mk2ULkrqDyVP2_cOpgdLozFjn3RJbTvg@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> I was thinking about this, but mostly didn't do this, because the
> interaction with `--missing` is only for non-commit
> objects. Because for missing commits, `--ignore-missing-links`
> skips the commit and the value of `--missing` doesn't make any
> difference.
Hmph, somehow that smells like an existing bug. So does the "trees
are not shown by --missing=print, and show_object() is never called
for missing objects unless they are blobs" you mention. When the
user asks "instead of dying, list them so that I can ask around and
fetch them to repair this repository", shouldn't we do just that?
I wonder if these bugs are something people may be taking advatage
of and cannot be fixed retroactively? If we can fix these and nobody
complains, that would give us the ideal outcome, I would think.
Thanks.
^ permalink raw reply
* Re: [REGRESSION] uninitialized value $address in git send-email
From: Bagas Sanjaya @ 2023-09-21 7:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Strawbridge, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Emily Shaffer,
Doug Anderson, Git Mailing List
In-Reply-To: <xmqq7cokc0kj.fsf@gitster.g>
On 20/09/2023 22:43, Junio C Hamano wrote:
> Bagas Sanjaya <bagasdotme@gmail.com> writes:
>
>> Originally, I was intended to report regression on handling multiple
>> addresses passed in a single --to/--cc/--bcc option.
>
> You refer to v2.40 and v2.41 in the message I am responding to, but
> do you have a bisection? There seem to have been five topics around
> send-email during that timeperiod.
>
> $ git log --oneline --first-parent v2.40.0..v2.41.0 git-send-email.perl
> b04671b638 Merge branch 'jc/send-email-pre-process-fix'
> 64477d20d7 Merge branch 'mc/send-email-header-cmd'
> b6e9521956 Merge branch 'ms/send-email-feed-header-to-validate-hook'
> c4c9d5586f Merge branch 'rj/send-email-validate-hook-count-messages'
> 647a2bb3ff Merge branch 'jc/spell-id-in-both-caps-in-message-id'
I'll make one on the separate report.
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply
* [PATCH] test-lib: set UBSAN_OPTIONS to match ASan
From: Jeff King @ 2023-09-21 4:18 UTC (permalink / raw)
To: git
For a long time we have used ASAN_OPTIONS to set abort_on_error. This is
important because we want to notice detected problems even in programs
which are expected to fail. But we never did the same for UBSAN_OPTIONS.
This means that our UBSan test suite runs might silently miss some
cases.
It also causes a more visible effect, which is that t4058 complains
about unexpected "fixes" (and this is how I noticed the issue):
$ make SANITIZE=undefined CC=gcc && (cd t && ./t4058-*)
...
ok 8 - git read-tree does not segfault # TODO known breakage vanished
ok 9 - reset --hard does not segfault # TODO known breakage vanished
ok 10 - git diff HEAD does not segfault # TODO known breakage vanished
The tests themselves aren't that interesting. We have a known bug where
these programs segfault, and they do when compiled without sanitizers.
With UBSan, when the test runs:
test_might_fail git read-tree --reset base
it gets:
cache-tree.c:935:9: runtime error: member access within misaligned address 0x5a5a5a5a5a5a5a5a for type 'struct cache_entry', which requires 8 byte alignment
So that's garbage memory which would _usually_ cause us to segfault, but
UBSan catches it and complains first about the alignment. That makes
sense, but the weird thing is that UBSan then exits instead of aborting,
so our test_might_fail call considers that an acceptable outcome and the
test "passes".
Curiously, this historically seems to have aborted, because I've run
"make test" with UBSan many times (and so did our CI) and we never saw
the problem. Even more curiously, I see an abort if I use clang with
ASan and UBSan together, like:
# this aborts!
make SANITIZE=undefined,address CC=clang
But not with just UBSan, and not with both when used with gcc:
# none of these do
make SANITIZE=undefined CC=gcc
make SANITIZE=undefined CC=clang
make SANITIZE=undefined,address CC=gcc
Likewise moving to older versions of gcc (I tried gcc-11 and gcc-12 on
my Debian system) doesn't abort. Nor does moving around in Git's
history. Neither this test nor the relevant code have been touched in a
while, and going back to v2.41.0 produces the same outcome (even though
many UBSan CI runs have passed in the meantime).
So _something_ changed on my system (and likely will soon on other
people's, since this is stock Debian unstable), but I didn't track
it further. I don't know why it ever aborted in the past, but we
definitely should be explicit here and tell UBSan what we want to
happen.
Signed-off-by: Jeff King <peff@peff.net>
---
t/lib-httpd/apache.conf | 1 +
t/test-lib.sh | 3 +++
2 files changed, 4 insertions(+)
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index a22d138605..022276a6b9 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -92,6 +92,7 @@ PassEnv GIT_VALGRIND_OPTIONS
PassEnv GNUPGHOME
PassEnv ASAN_OPTIONS
PassEnv LSAN_OPTIONS
+PassEnv UBSAN_OPTIONS
PassEnv GIT_TRACE
PassEnv GIT_CONFIG_NOSYSTEM
PassEnv GIT_TEST_SIDEBAND_ALL
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5ea5d1d62a..1656c9eed0 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -89,6 +89,9 @@ prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0
export LSAN_OPTIONS
+prepend_var UBSAN_OPTIONS : $GIT_SAN_OPTIONS
+export UBSAN_OPTIONS
+
if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
then
echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
--
2.42.0.711.g968f0256ae
^ permalink raw reply related
* Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision
From: Jeff King @ 2023-09-21 4:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <xmqqfs38akx5.fsf@gitster.g>
On Wed, Sep 20, 2023 at 09:06:46AM -0700, Junio C Hamano wrote:
> > With empty repositories however, HEAD does not point to a valid treeish,
> > causing Git to die. This means we would need to check for a valid
> > treeish each time.
>
> Naturally.
>
> > To avoid this, let's add a configuration that allows
> > Git to simply ignore --attr-source if it does not resolve to a valid
> > tree.
>
> Not convincing at all as to the reason why we want to do anything
> "to avoid this". "git log" in a repository whose HEAD does not
> point to a valid treeish. "git blame" dies with "no such ref:
> HEAD". An empty repository (more precisely, an unborn history)
> needs special casing if you want to present it if you do not want to
> spew underlying error messages to the end users *anyway*. It is
> unclear why seeing what commit the HEAD pointer points at (or which
> branch it points at for that matter) is *an* *extra* and *otherwise*
> *unnecessary* overhead that need to be avoided.
In an empty repository, "git log" will die anyway. So I think the more
interesting case is "I have a repository with stuff in it, but HEAD
points to an unborn branch". So:
git --attr-source=HEAD diff foo^ foo
And there you really are saying "if there are attributes in HEAD, use
them; otherwise, don't worry about it". This is exactly what we do with
mailmap.blob: in a bare repository it is set to HEAD by default, but if
HEAD does not resolve, we just ignore it (just like a HEAD that does not
contain a .mailmap file). And those match the non-bare cases, where we'd
read those files from the working tree instead.
So I think the same notion applies here. You want to be able to point it
at HEAD by default, but if there is no HEAD, that is the same as if HEAD
simply did not contain any attributes. If we had attr.blob, that is
exactly how I would expect it to work.
My gut feeling is that --attr-source should do the same, and just
quietly ignore a ref that does not resolve. But I think an argument can
be made that because the caller explicitly gave us a ref, they expect it
to work (and that would catch misspellings, etc). Like:
git --attr-source=my-barnch diff foo^ foo
So I'm OK with not changing that behavior.
But what is weird about this patch is that we are using a config option
to change how a command-line option is interpreted. If the idea is that
some invocations care about the validity of the source and some do not,
then the config option is much too blunt. It is set once long ago, but
it can't distinguish between times you care about invalid sources and
times you don't.
It would make much more sense to me to have another command-line option,
like:
git --attr-source=HEAD --allow-invalid-attr-source
Obviously that is horrible to type, but I think the point is that you'd
only do this from a script anyway (because it's those automated cases
where you want to say "use HEAD only if it exists").
If there were an attr.blob config option and it complained about an
invalid HEAD, _then_ I think attr.allowInvalidSource might make sense
(though again, I would just argue for switching the behavior by
default). And I really think attr.blob is a better match for what GitLab
is trying to do here, because it is set once and applies to all
commands, rather than having to teach every invocation to pass it
(though I guess maybe they use it as an environment variable).
Of course I would think that, as the person who solved GitHub's exact
same problem for mailmap by adding mailmap.blob. So you may ingest the
appropriate grain of salt. :)
-Peff
^ permalink raw reply
* Re: Git in Outreachy? (December, 2023)
From: Taylor Blau @ 2023-09-21 20:46 UTC (permalink / raw)
To: Christian Couder; +Cc: Kaartic Sivaraam, git, Hariom verma, Victoria Dye
In-Reply-To: <CAP8UFD2CGf8efBHquS=LJP8uQ5ivuDryqGz96PZ81oDtrNgNXw@mail.gmail.com>
On Tue, Aug 29, 2023 at 10:38:45PM +0200, Christian Couder wrote:
> By the way, Kaartic, do you still want to be an org admin? And Taylor
> are you Ok with Kaartic being an org admin?
Sorry that this dropped off of my queue. FWIW, no issues from me.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 1/4] config: split out config_parse_options
From: Josh Steadmon @ 2023-09-21 21:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jonathantanmy, calvinwan, glencbz
In-Reply-To: <xmqq4jkpe3we.fsf@gitster.g>
On 2023.08.23 16:26, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > From: Glen Choo <chooglen@google.com>
> >
> > "struct config_options" is a disjoint set of options options used by the
> > config parser (e.g. event listners) and options used by
> > config_with_options() (e.g. to handle includes, choose which config
> > files to parse).
>
> There is some punctuation missing on the first line. Perhaps an em-dash
> between "options---options" or something like that?
Cleaned up this and an additional typo in the description.
> > Split parser-only options into config_parse_options.
> >
> > Signed-off-by: Glen Choo <chooglen@google.com>
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> > bundle-uri.c | 2 +-
> > config.c | 14 +++++++-------
> > config.h | 37 ++++++++++++++++++++-----------------
> > fsck.c | 2 +-
> > 4 files changed, 29 insertions(+), 26 deletions(-)
>
> > diff --git a/bundle-uri.c b/bundle-uri.c
> > index 4b5c49b93d..f93ca6a486 100644
> > --- a/bundle-uri.c
> > +++ b/bundle-uri.c
> > @@ -237,7 +237,7 @@ int bundle_uri_parse_config_format(const char *uri,
> > struct bundle_list *list)
> > {
> > int result;
> > - struct config_options opts = {
> > + struct config_parse_options opts = {
> > .error_action = CONFIG_ERROR_ERROR,
> > };
>
> OK, and this one only needs the parse_options half, and presumably
> all hunks (other than the one that splits the struct into two in
> config.h) are about turning the users of config_options that only
> need config_parse_options half.
>
> As we do not see any funny casts in the patch text, compilers should
> catch all questionable conversion in this step, if there were any.
>
> OK.
^ 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