Git development
 help / color / mirror / Atom feed
* Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Junio C Hamano @ 2023-08-31 21:52 UTC (permalink / raw)
  To: Wesley Schwengle; +Cc: git
In-Reply-To: <xmqqbkenszfa.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> I am not commenting on the tests, as the above code probably needs
> to be corrected first so that folks who want to squelch the message
> and want the "forkpoint behaviour by default when rebuilding on the
> usual upstream" behaviour can do so by setting the variable to true.
>
> And that obviously need to be tested, too.

Another worrysome thing about rebase.forkpoint is that it will be
inevitable for folks to start complaining that it does not work the
way other configuration variables do.  Setting the variable to
'true' is not the same as passing '--fork-point=true' from the
command line.

I actually think it would be a lot larger behaviour change with a
huge potential to be received as a regression if we start making the
variable to mean the same thing as passing '--fork-point=true'.
People may like the current "if you are rebuilding your branch on
its usual upstream, pay attention to the rebase and rewind of the
upstream itself, but if you are giving an explicit upstream from the
command line, the tool does not second guess you with the fork-point
heuristics" behaviour and prefer to set it to true.  We would be
breaking them big time if suddenly the rebase.forkpoint=true they
set previously starts triggering the fork-point heuristics when they
run "git rebase upstream".  So that needs to be kept in mind when/if
we fix the "setting the variable, even to 'true', will squelch the
warning".


^ permalink raw reply

* RE: [PATCH 10/10] lower core.maxTreeDepth default to 2048
From: rsbecker @ 2023-08-31 21:59 UTC (permalink / raw)
  To: 'Junio C Hamano', 'Jeff King'
  Cc: 'Oswald Buddenhagen', git
In-Reply-To: <xmqq7cpaudke.fsf@gitster.g>

On Thursday, August 31, 2023 5:06 PM, Junio C Hamano wrote:
>Jeff King <peff@peff.net> writes:
>
>> On Thu, Aug 31, 2023 at 12:39:37PM +0200, Oswald Buddenhagen wrote:
>>
>>> On Thu, Aug 31, 2023 at 02:23:20AM -0400, Jeff King wrote:
>>> > But I thought that
>>> > following the sequence of logic (from "4096 is probably OK" to
>>> > "whoops, it's not") had some value to share.
>>> >
>>> of course, but you can just integrate that into the squashed commit
message.
>>> having it all in one place makes it easier to follow.
>>
>> Yes, though I think having it as a separate patch makes it easier to
>> revisit later (e.g., by reverting or by replacing the patch during a
>> re-roll).
>
>I am on the fence.  Having it squashed into the same step as it was
introduced may
>reduce the patch count, but then it would not be easy to explain why 2048
is a
>reasonable default at that step when no code actually uses the variable, so
the end
>result is not all that easier to follow and read, as that earlier step
would be
>handwaving
>"2048 is good at the end of the series, trust me", unlike having it at the
end.  When
>4096 is introduced as a "random number that seems larger than large enough"
in the
>earlier step, it might be worth mentioning that it is a tentative default
and may turn
>out to be larger than necessary in which case we may want to shrink it ;-)

I have been trying to figure out the implications of this and went down the
wrong rabbit hole. Are we taking about the tree depth of the underlying
Merkel Tree (no) or the tree-ish thing representing the file system
(apparently yes). In this case, a practical depth of 2048 hits the exact max
path size on the NonStop platform, so I have no issue there. My concern is
one of terminology. My assumption of what maxTreeDepth meant, from other
terminology used in git, seemed (wrongly) to align with the use of --depth=n
where n<maxTreeDepth parameters for commands like fetch. From a user
intuition (arguably, if I have any here) is that the parameter should be
more of a path nomenclature, like maxPathHeight or maxHierarchyHeight rather
than what is currently in flight. Just my opinion and I'm fine no matter
which way.

--Randall
--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.




^ permalink raw reply

* Re: [PATCH v4] format-patch: --rfc honors what --subject-prefix sets
From: Junio C Hamano @ 2023-08-31 22:04 UTC (permalink / raw)
  To: Jeff King; +Cc: Drew DeVault, git
In-Reply-To: <20230831212950.GA949706@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Wed, Aug 30, 2023 at 12:28:15PM -0700, Junio C Hamano wrote:
>
>> Will queue.  Let's wait to see if others find something fishy for a
>> day or two and then merge it down to 'next'.
>
> It looks good to me, and I'm much happier with where the refactoring
> ended up compared to the earlier versions. I did have two nits, but I'm
> content if neither is addressed.
>
> One is that the commit message doesn't really describe the refactoring
> of --subject-prefix. I'm OK with that rationale being in the list
> archive, though.

Yeah, I agree that the reason for the change deserves to be
recorded.

>> >  static int subject_prefix_callback(const struct option *opt, const char *arg,
>> >  			    int unset)
>> >  {
>> > +	struct strbuf *sprefix;
>> > +
>> >  	BUG_ON_OPT_NEG(unset);
>> > +	sprefix = (struct strbuf *)opt->value;
>> >  	subject_prefix = 1;
>> > -	((struct rev_info *)opt->value)->subject_prefix = arg;
>> > +	strbuf_reset(sprefix);
>> > +	strbuf_addstr(sprefix, arg);
>> >  	return 0;
>> >  }
>> 
>> OK.
>
> The cast is unnecessary here, since opt->value is a void pointer which
> allows implicit casts. Just:
>
>   struct strbuf *sprefix = opt->value;
>
> is IMHO a little more readable. But as we're just passing it along to
> strbuf functions anyway, it would also work to do:
>
>   strbuf_reset(opt->value);
>   strbuf_addstr(opt->value, arg);
>
> I think we're deep into questions of style / preference here, so I'm OK
> with any of them. It's probably only that I've recently been refactoring
> so many parseopt callbacks with the same pattern that I have opinions at
> all. ;)

Sure.  FWIW, I like giving a meaningful name to a thing that is used
more than once, so my preference is the original minus excess cast.

Thanks for a review.  Here is a tentative rewrite

--- >8 ---
From e0d7db7423a91673c001aaa5e580c815ce2f7f92 Mon Sep 17 00:00:00 2001
From: Drew DeVault <sir@cmpwn.com>
Date: Wed, 30 Aug 2023 08:43:33 +0200
Subject: [PATCH v5] format-patch: --rfc honors what --subject-prefix sets

Rather than replacing the configured subject prefix (either through the
git config or command line) entirely with "RFC PATCH", this change
prepends RFC to whatever subject prefix was already in use.

This is useful, for example, when a user is working on a repository that
has a subject prefix considered to disambiguate patches:

	git config format.subjectPrefix 'PATCH my-project'

Prior to this change, formatting patches with --rfc would lose the
'my-project' information.

The data flow for the subject-prefix was that rev.subject_prefix
were to be kept the authoritative version of the subject prefix even
while parsing command line options, and sprefix variable was used as
a temporary area to futz with it.  Now, the parsing code has been
refactored to build the subject prefix into the sprefix variable and
assigns its value at the end to rev.subject_prefix, which makes the
flow easier to grasp.

Signed-off-by: Drew DeVault <sir@cmpwn.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Range-diff:
1:  fcd7aa53d2 ! 1:  e0d7db7423 format-patch: --rfc honors what --subject-prefix sets
    @@ Commit message
         Prior to this change, formatting patches with --rfc would lose the
         'my-project' information.
     
    +    The data flow for the subject-prefix was that rev.subject_prefix
    +    were to be kept the authoritative version of the subject prefix even
    +    while parsing command line options, and sprefix variable was used as
    +    a temporary area to futz with it.  Now, the parsing code has been
    +    refactored to build the subject prefix into the sprefix variable and
    +    assigns its value at the end to rev.subject_prefix, which makes the
    +    flow easier to grasp.
    +
         Signed-off-by: Drew DeVault <sir@cmpwn.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    @@ builtin/log.c: static int subject_prefix = 0;
     +	struct strbuf *sprefix;
     +
      	BUG_ON_OPT_NEG(unset);
    -+	sprefix = (struct strbuf *)opt->value;
    ++	sprefix = opt->value;
      	subject_prefix = 1;
     -	((struct rev_info *)opt->value)->subject_prefix = arg;
     +	strbuf_reset(sprefix);

 Documentation/git-format-patch.txt | 18 +++++++++++------
 builtin/log.c                      | 31 +++++++++++++++---------------
 t/t4014-format-patch.sh            | 22 ++++++++++++++++++++-
 3 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 373b46fc0d..62345ed764 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -217,9 +217,15 @@ populated with placeholder text.
 
 --subject-prefix=<subject prefix>::
 	Instead of the standard '[PATCH]' prefix in the subject
-	line, instead use '[<subject prefix>]'. This
-	allows for useful naming of a patch series, and can be
-	combined with the `--numbered` option.
+	line, instead use '[<subject prefix>]'. This can be used
+	to name a patch series, and can be combined with the
+	`--numbered` option.
++
+The configuration variable `format.subjectPrefix` may also be used
+to configure a subject prefix to apply to a given repository for
+all patches. This is often useful on mailing lists which receive
+patches for several repositories and can be used to disambiguate
+the patches (with a value of e.g. "PATCH my-project").
 
 --filename-max-length=<n>::
 	Instead of the standard 64 bytes, chomp the generated output
@@ -229,9 +235,9 @@ populated with placeholder text.
 	variable, or 64 if unconfigured.
 
 --rfc::
-	Alias for `--subject-prefix="RFC PATCH"`. RFC means "Request For
-	Comments"; use this when sending an experimental patch for
-	discussion rather than application.
+	Prepends "RFC" to the subject prefix (producing "RFC PATCH" by
+	default). RFC means "Request For Comments"; use this when sending
+	an experimental patch for discussion rather than application.
 
 -v <n>::
 --reroll-count=<n>::
diff --git a/builtin/log.c b/builtin/log.c
index db3a88bfe9..75762b497d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1468,19 +1468,16 @@ static int subject_prefix = 0;
 static int subject_prefix_callback(const struct option *opt, const char *arg,
 			    int unset)
 {
+	struct strbuf *sprefix;
+
 	BUG_ON_OPT_NEG(unset);
+	sprefix = opt->value;
 	subject_prefix = 1;
-	((struct rev_info *)opt->value)->subject_prefix = arg;
+	strbuf_reset(sprefix);
+	strbuf_addstr(sprefix, arg);
 	return 0;
 }
 
-static int rfc_callback(const struct option *opt, const char *arg, int unset)
-{
-	BUG_ON_OPT_NEG(unset);
-	BUG_ON_OPT_ARG(arg);
-	return subject_prefix_callback(opt, "RFC PATCH", unset);
-}
-
 static int numbered_cmdline_opt = 0;
 
 static int numbered_callback(const struct option *opt, const char *arg,
@@ -1907,6 +1904,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct strbuf rdiff_title = STRBUF_INIT;
 	struct strbuf sprefix = STRBUF_INIT;
 	int creation_factor = -1;
+	int rfc = 0;
 
 	const struct option builtin_format_patch_options[] = {
 		OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
@@ -1930,13 +1928,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    N_("mark the series as Nth re-roll")),
 		OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
 			    N_("max length of output filename")),
-		OPT_CALLBACK_F(0, "rfc", &rev, NULL,
-			    N_("use [RFC PATCH] instead of [PATCH]"),
-			    PARSE_OPT_NOARG | PARSE_OPT_NONEG, rfc_callback),
+		OPT_BOOL(0, "rfc", &rfc, N_("use [RFC PATCH] instead of [PATCH]")),
 		OPT_STRING(0, "cover-from-description", &cover_from_description_arg,
 			    N_("cover-from-description-mode"),
 			    N_("generate parts of a cover letter based on a branch's description")),
-		OPT_CALLBACK_F(0, "subject-prefix", &rev, N_("prefix"),
+		OPT_CALLBACK_F(0, "subject-prefix", &sprefix, N_("prefix"),
 			    N_("use [<prefix>] instead of [PATCH]"),
 			    PARSE_OPT_NONEG, subject_prefix_callback),
 		OPT_CALLBACK_F('o', "output-directory", &output_directory,
@@ -2016,11 +2012,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.max_parents = 1;
 	rev.diffopt.flags.recursive = 1;
 	rev.diffopt.no_free = 1;
-	rev.subject_prefix = fmt_patch_subject_prefix;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.def = "HEAD";
 	s_r_opt.revarg_opt = REVARG_COMMITTISH;
 
+	strbuf_addstr(&sprefix, fmt_patch_subject_prefix);
 	if (format_no_prefix)
 		diff_set_noprefix(&rev.diffopt);
 
@@ -2048,13 +2044,16 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_from_description_arg)
 		cover_from_description_mode = parse_cover_from_description(cover_from_description_arg);
 
+	if (rfc)
+		strbuf_insertstr(&sprefix, 0, "RFC ");
+
 	if (reroll_count) {
-		strbuf_addf(&sprefix, "%s v%s",
-			    rev.subject_prefix, reroll_count);
+		strbuf_addf(&sprefix, " v%s", reroll_count);
 		rev.reroll_count = reroll_count;
-		rev.subject_prefix = sprefix.buf;
 	}
 
+	rev.subject_prefix = sprefix.buf;
+
 	for (i = 0; i < extra_hdr.nr; i++) {
 		strbuf_addstr(&buf, extra_hdr.items[i].string);
 		strbuf_addch(&buf, '\n');
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 3cf2b7a7fb..9fa1f3bc7a 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1373,7 +1373,27 @@ test_expect_success '--rfc' '
 	Subject: [RFC PATCH 1/1] header with . in it
 	EOF
 	git format-patch -n -1 --stdout --rfc >patch &&
-	grep ^Subject: patch >actual &&
+	grep "^Subject:" patch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--rfc does not overwrite prefix' '
+	cat >expect <<-\EOF &&
+	Subject: [RFC PATCH foobar 1/1] header with . in it
+	EOF
+	git -c format.subjectPrefix="PATCH foobar" \
+		format-patch -n -1 --stdout --rfc >patch &&
+	grep "^Subject:" patch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--rfc is argument order independent' '
+	cat >expect <<-\EOF &&
+	Subject: [RFC PATCH foobar 1/1] header with . in it
+	EOF
+	git format-patch -n -1 --stdout --rfc \
+		--subject-prefix="PATCH foobar" >patch &&
+	grep "^Subject:" patch >actual &&
 	test_cmp expect actual
 '
 
-- 
2.42.0-100-g3525f1dbc1


^ permalink raw reply related

* Re: [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile
From: Junio C Hamano @ 2023-08-31 22:12 UTC (permalink / raw)
  To: Jeff King; +Cc: git, René Scharfe
In-Reply-To: <20230831212051.GD949469@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Using --stage=all requires writing to tempfiles, since we cannot put
> multiple stages into a single file. So --stage=all implies --temp.

This one is new, and it does read really well.  Thanks.

^ permalink raw reply

* Re: [PATCH 10/10] lower core.maxTreeDepth default to 2048
From: Jeff King @ 2023-08-31 22:31 UTC (permalink / raw)
  To: rsbecker; +Cc: 'Junio C Hamano', 'Oswald Buddenhagen', git
In-Reply-To: <096e01d9dc56$6d9f12a0$48dd37e0$@nexbridge.com>

On Thu, Aug 31, 2023 at 05:59:28PM -0400, rsbecker@nexbridge.com wrote:

> I have been trying to figure out the implications of this and went down the
> wrong rabbit hole. Are we taking about the tree depth of the underlying
> Merkel Tree (no) or the tree-ish thing representing the file system
> (apparently yes). In this case, a practical depth of 2048 hits the exact max
> path size on the NonStop platform, so I have no issue there. My concern is
> one of terminology. My assumption of what maxTreeDepth meant, from other
> terminology used in git, seemed (wrongly) to align with the use of --depth=n
> where n<maxTreeDepth parameters for commands like fetch. From a user
> intuition (arguably, if I have any here) is that the parameter should be
> more of a path nomenclature, like maxPathHeight or maxHierarchyHeight rather
> than what is currently in flight. Just my opinion and I'm fine no matter
> which way.

The documentation from the patch is:

  +core.maxTreeDepth::
  +       The maximum depth Git is willing to recurse while traversing a
  +       tree (e.g., "a/b/cde/f" has a depth of 4). This is a fail-safe
  +       to allow Git to abort cleanly, and should not generally need to
  +       be adjusted. The default is 4096.

Does reading that answer your question and make the meaning clear? If
not, can you suggest any changes?

I'd like to stick with "depth" here as it's commonly used in other
places to mean the same thing (e.g., git-grep's --max-depth option).

I also think this is something that most people will remain completely
oblivious to, as you'd only hit it for absurd cases.

-Peff

^ permalink raw reply

* Re: [PATCH v4] format-patch: --rfc honors what --subject-prefix sets
From: Jeff King @ 2023-08-31 22:32 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Drew DeVault, git
In-Reply-To: <xmqqv8cuswah.fsf@gitster.g>

On Thu, Aug 31, 2023 at 03:04:54PM -0700, Junio C Hamano wrote:

> Sure.  FWIW, I like giving a meaningful name to a thing that is used
> more than once, so my preference is the original minus excess cast.
> 
> Thanks for a review.  Here is a tentative rewrite

Thanks, looks good to me.

-Peff

^ permalink raw reply

* Re: [REGRESSION] Can't clone GitHub repos (fetch-pack error) due to avoiding deprecated OpenSSL SHA-1 routines
From: brian m. carlson @ 2023-08-31 23:19 UTC (permalink / raw)
  To: Bagas Sanjaya; +Cc: Git Mailing List, Eric Wong, Junio C Hamano, Jonathan Tan
In-Reply-To: <ZPCL11k38PXTkFga@debian.me>

[-- Attachment #1: Type: text/plain, Size: 2875 bytes --]

On 2023-08-31 at 12:47:19, Bagas Sanjaya wrote:
> Hi,
> 
> I built Git v2.42.0 on Debian testing, linked with OpenSSL (v3.0.10 from
> distribution) with Makefile knob `OPENSSL_SHA1=YesPlease 
> OPENSSL_SHA256=YesPlease`. I tried to shallow clone git.git repository:

I should point out that using OpenSSL's SHA-1 support is insecure
because it doesn't check for collisions.  As a practical matter, no
distro builds that way, and if you distributed that build, it would
probably qualify for a CVE.

However, OPENSSL_SHA256 being set is fine for a local build or a build
where you're not distributing OpenSSL itself.

> ```
> $ git clone https://github.com/git/git --depth=1 git-scm
> ```
> 
> All the necessary objects were fetched but the clone errored instead with:
> 
> ```
> fatal: fetch-pack: invalid index-pack output
> ```
> 
> This issue is a regression since v2.41.0 doesn't have it. Bisecting, the
> culprit is commit bda9c12073e7 (avoid SHA-1 functions deprecated in OpenSSL 3+,
> 2023-08-01). AFAIK, the culprit doesn't touch `fetch-pack.c` as I hoped.

I also see this with that configuration on Debian sid, and it appears to
affect SHA-256 as well.  The testsuite fails in a variety of spectacular
ways.  For example, t0410 is broken in exactly this way.

A simple git index-pack on Git's codebase shows a segfault in
`EVP_DigestUpdate` after calling `flush`.  It's not clear to me why this
would occur, but I did note that the context is a static variable.  I
wonder if there's something about this configuration that results in
breakage if a context is reused, although looking at the code, nothing
jumps out to me.

I did, however, apply this patch, which I think makes the problem really
clear:

----
diff --git a/sha1/openssl.h b/sha1/openssl.h
index 006c1f4ba5..0390ba9da6 100644
--- a/sha1/openssl.h
+++ b/sha1/openssl.h
@@ -32,6 +32,7 @@ static inline void openssl_SHA1_Final(unsigned char *digest,
 {
 	EVP_DigestFinal_ex(ctx->ectx, digest, NULL);
 	EVP_MD_CTX_free(ctx->ectx);
+	ctx->ectx = NULL;
 }
 
 static inline void openssl_SHA1_Clone(struct openssl_SHA1_CTX *dst,
----

Now we see that when the segfault happens, `input_ctx.sha1.ectx` is
NULL.  I'm not sure why that is, or what needs to be fixed, but I think
it's clear that _someone_ isn't calling the `init_fn` method before
re-using the context, and they definitely should be.

Hopefully this gives someone a good push in the right direction on
solving the problem.

If someone wants to pick up the above patch to help make this problem
more obvious in the future (don't forget to do the same for SHA-256),
please do so with my blessing.  I wouldn't say you need my sign-off
since it's so trivial, but feel free to forge it if it makes you feel
better.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

^ permalink raw reply related

* Re: [REGRESSION] Can't clone GitHub repos (fetch-pack error) due to avoiding deprecated OpenSSL SHA-1 routines
From: Eric Wong @ 2023-09-01  0:57 UTC (permalink / raw)
  To: brian m. carlson, Bagas Sanjaya, git, Junio C Hamano,
	Jonathan Tan
In-Reply-To: <ZPEf8kbBUFqLO25W@tapette.crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> wrote:
> Hopefully this gives someone a good push in the right direction on
> solving the problem.

Thanks.  Here's my WIP which fixes clones on SHA-1 and SHA-256
 <https://80x24.org/sha256test.git> and also t1050-large.sh, but
t1514-rev-parse-push.sh is still broken...

That said, I don't much understand some of the code I'm modifying
and just poking at it until tests pass and valgrind is happy :x

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 006ffdc9c5..dda94a9f46 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1166,6 +1166,7 @@ static void parse_pack_objects(unsigned char *hash)
 	struct ofs_delta_entry *ofs_delta = ofs_deltas;
 	struct object_id ref_delta_oid;
 	struct stat st;
+	git_hash_ctx tmp_ctx;
 
 	if (verbose)
 		progress = start_progress(
@@ -1202,7 +1203,9 @@ static void parse_pack_objects(unsigned char *hash)
 
 	/* Check pack integrity */
 	flush();
-	the_hash_algo->final_fn(hash, &input_ctx);
+	the_hash_algo->init_fn(&tmp_ctx);
+	the_hash_algo->clone_fn(&tmp_ctx, &input_ctx);
+	the_hash_algo->final_fn(hash, &tmp_ctx);
 	if (!hasheq(fill(the_hash_algo->rawsz), hash))
 		die(_("pack is corrupted (SHA1 mismatch)"));
 	use(the_hash_algo->rawsz);
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 73bff3a23d..92b9c8598b 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -268,6 +268,7 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,
 					  type, size);
 	the_hash_algo->init_fn(&ctx);
 	the_hash_algo->update_fn(&ctx, obuf, header_len);
+	the_hash_algo->init_fn(&checkpoint.ctx);
 
 	/* Note: idx is non-NULL when we are writing */
 	if ((flags & HASH_WRITE_OBJECT) != 0)
diff --git a/csum-file.c b/csum-file.c
index cd01713244..870748e016 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -207,7 +207,7 @@ int hashfile_truncate(struct hashfile *f, struct hashfile_checkpoint *checkpoint
 	    lseek(f->fd, offset, SEEK_SET) != offset)
 		return -1;
 	f->total = offset;
-	f->ctx = checkpoint->ctx;
+	the_hash_algo->clone_fn(&f->ctx, &checkpoint->ctx);
 	f->offset = 0; /* hashflush() was called in checkpoint */
 	return 0;
 }

^ permalink raw reply related

* [PATCH] treewide: fix various bugs w/ OpenSSL 3+ EVP API
From: Eric Wong @ 2023-09-01  2:09 UTC (permalink / raw)
  To: brian m. carlson, Bagas Sanjaya, git, Junio C Hamano,
	Jonathan Tan
In-Reply-To: <20230901005742.M783359@dcvr>

The OpenSSL 3+ EVP API for SHA-* cannot support our prior use cases
supported by other SHA-* implementations.  It has the following
differences:

1. ->init_fn is required before all use
2. struct assignments don't work and requires ->clone_fn
3. can't support ->update_fn after ->final_*fn

While fixing cases 1 and 2 is merely the matter of calling ->init_fn and
->clone_fn as appropriate, fixing case 3 requires calling ->final_*fn on
a temporary context that's cloned from the primary context.

Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Link: https://lore.kernel.org/ZPCL11k38PXTkFga@debian.me/
Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
Fixes: 3e440ea0aba0 ("sha256: avoid functions deprecated in OpenSSL 3+")
Fixes: bda9c12073e7 ("avoid SHA-1 functions deprecated in OpenSSL 3+")
Signed-off-by: Eric Wong <e@80x24.org>
---
 Ugh, I wonder if I setup my config.mak incorrectly when testing
 3e440ea0aba0 and bda9c12073e7 :x

 There may be other misuses not exposed by the test suite.  Making
 git_hash_ctx opaque could flush out some of them (but I dislike
 APIs which force heap allocations in the first place).  In any case,
 I really wish git relied less on globals so object lifetimes could be
 more obvious and really wish all C projects could rely on
 gcc/tinycc/clang-supported __attribute__((__cleanup__)) to make
 lifetimes easier-to-manage...

 builtin/fast-import.c    | 1 +
 builtin/index-pack.c     | 5 ++++-
 builtin/unpack-objects.c | 5 ++++-
 bulk-checkin.c           | 1 +
 csum-file.c              | 2 +-
 5 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 4dbb10aff3..444f41cf8c 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -1102,6 +1102,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
 		|| (pack_size + PACK_SIZE_THRESHOLD + len) < pack_size)
 		cycle_packfile();
 
+	the_hash_algo->init_fn(&checkpoint.ctx);
 	hashfile_checkpoint(pack_file, &checkpoint);
 	offset = checkpoint.offset;
 
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 006ffdc9c5..dda94a9f46 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1166,6 +1166,7 @@ static void parse_pack_objects(unsigned char *hash)
 	struct ofs_delta_entry *ofs_delta = ofs_deltas;
 	struct object_id ref_delta_oid;
 	struct stat st;
+	git_hash_ctx tmp_ctx;
 
 	if (verbose)
 		progress = start_progress(
@@ -1202,7 +1203,9 @@ static void parse_pack_objects(unsigned char *hash)
 
 	/* Check pack integrity */
 	flush();
-	the_hash_algo->final_fn(hash, &input_ctx);
+	the_hash_algo->init_fn(&tmp_ctx);
+	the_hash_algo->clone_fn(&tmp_ctx, &input_ctx);
+	the_hash_algo->final_fn(hash, &tmp_ctx);
 	if (!hasheq(fill(the_hash_algo->rawsz), hash))
 		die(_("pack is corrupted (SHA1 mismatch)"));
 	use(the_hash_algo->rawsz);
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 32505255a0..fef7423448 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -609,6 +609,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
 {
 	int i;
 	struct object_id oid;
+	git_hash_ctx tmp_ctx;
 
 	disable_replace_refs();
 
@@ -669,7 +670,9 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
 	the_hash_algo->init_fn(&ctx);
 	unpack_all();
 	the_hash_algo->update_fn(&ctx, buffer, offset);
-	the_hash_algo->final_oid_fn(&oid, &ctx);
+	the_hash_algo->init_fn(&tmp_ctx);
+	the_hash_algo->clone_fn(&tmp_ctx, &ctx);
+	the_hash_algo->final_oid_fn(&oid, &tmp_ctx);
 	if (strict) {
 		write_rest();
 		if (fsck_finish(&fsck_options))
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 73bff3a23d..92b9c8598b 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -268,6 +268,7 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,
 					  type, size);
 	the_hash_algo->init_fn(&ctx);
 	the_hash_algo->update_fn(&ctx, obuf, header_len);
+	the_hash_algo->init_fn(&checkpoint.ctx);
 
 	/* Note: idx is non-NULL when we are writing */
 	if ((flags & HASH_WRITE_OBJECT) != 0)
diff --git a/csum-file.c b/csum-file.c
index cd01713244..870748e016 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -207,7 +207,7 @@ int hashfile_truncate(struct hashfile *f, struct hashfile_checkpoint *checkpoint
 	    lseek(f->fd, offset, SEEK_SET) != offset)
 		return -1;
 	f->total = offset;
-	f->ctx = checkpoint->ctx;
+	the_hash_algo->clone_fn(&f->ctx, &checkpoint->ctx);
 	f->offset = 0; /* hashflush() was called in checkpoint */
 	return 0;
 }

^ permalink raw reply related

* Re: [PATCH] treewide: fix various bugs w/ OpenSSL 3+ EVP API
From: Junio C Hamano @ 2023-09-01  5:32 UTC (permalink / raw)
  To: Eric Wong; +Cc: brian m. carlson, Bagas Sanjaya, git, Jonathan Tan
In-Reply-To: <20230901020928.M610756@dcvr>

Eric Wong <e@80x24.org> writes:

> The OpenSSL 3+ EVP API for SHA-* cannot support our prior use cases
> supported by other SHA-* implementations.  It has the following
> differences:
>
> 1. ->init_fn is required before all use
> 2. struct assignments don't work and requires ->clone_fn
> 3. can't support ->update_fn after ->final_*fn
>
> While fixing cases 1 and 2 is merely the matter of calling ->init_fn and
> ->clone_fn as appropriate, fixing case 3 requires calling ->final_*fn on
> a temporary context that's cloned from the primary context.
>
> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> Link: https://lore.kernel.org/ZPCL11k38PXTkFga@debian.me/
> Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
> Fixes: 3e440ea0aba0 ("sha256: avoid functions deprecated in OpenSSL 3+")
> Fixes: bda9c12073e7 ("avoid SHA-1 functions deprecated in OpenSSL 3+")
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
>  Ugh, I wonder if I setup my config.mak incorrectly when testing
>  3e440ea0aba0 and bda9c12073e7 :x

The third kind looks like a fun one to diagnoise and fix.

Thanks.  Will queue.

>  There may be other misuses not exposed by the test suite.  Making
>  git_hash_ctx opaque could flush out some of them (but I dislike
>  APIs which force heap allocations in the first place).  In any case,
>  I really wish git relied less on globals so object lifetimes could be
>  more obvious and really wish all C projects could rely on
>  gcc/tinycc/clang-supported __attribute__((__cleanup__)) to make
>  lifetimes easier-to-manage...
>
>  builtin/fast-import.c    | 1 +
>  builtin/index-pack.c     | 5 ++++-
>  builtin/unpack-objects.c | 5 ++++-
>  bulk-checkin.c           | 1 +
>  csum-file.c              | 2 +-
>  5 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 4dbb10aff3..444f41cf8c 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -1102,6 +1102,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
>  		|| (pack_size + PACK_SIZE_THRESHOLD + len) < pack_size)
>  		cycle_packfile();
>  
> +	the_hash_algo->init_fn(&checkpoint.ctx);
>  	hashfile_checkpoint(pack_file, &checkpoint);
>  	offset = checkpoint.offset;
>  
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 006ffdc9c5..dda94a9f46 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1166,6 +1166,7 @@ static void parse_pack_objects(unsigned char *hash)
>  	struct ofs_delta_entry *ofs_delta = ofs_deltas;
>  	struct object_id ref_delta_oid;
>  	struct stat st;
> +	git_hash_ctx tmp_ctx;
>  
>  	if (verbose)
>  		progress = start_progress(
> @@ -1202,7 +1203,9 @@ static void parse_pack_objects(unsigned char *hash)
>  
>  	/* Check pack integrity */
>  	flush();
> -	the_hash_algo->final_fn(hash, &input_ctx);
> +	the_hash_algo->init_fn(&tmp_ctx);
> +	the_hash_algo->clone_fn(&tmp_ctx, &input_ctx);
> +	the_hash_algo->final_fn(hash, &tmp_ctx);
>  	if (!hasheq(fill(the_hash_algo->rawsz), hash))
>  		die(_("pack is corrupted (SHA1 mismatch)"));
>  	use(the_hash_algo->rawsz);
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index 32505255a0..fef7423448 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -609,6 +609,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
>  {
>  	int i;
>  	struct object_id oid;
> +	git_hash_ctx tmp_ctx;
>  
>  	disable_replace_refs();
>  
> @@ -669,7 +670,9 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
>  	the_hash_algo->init_fn(&ctx);
>  	unpack_all();
>  	the_hash_algo->update_fn(&ctx, buffer, offset);
> -	the_hash_algo->final_oid_fn(&oid, &ctx);
> +	the_hash_algo->init_fn(&tmp_ctx);
> +	the_hash_algo->clone_fn(&tmp_ctx, &ctx);
> +	the_hash_algo->final_oid_fn(&oid, &tmp_ctx);
>  	if (strict) {
>  		write_rest();
>  		if (fsck_finish(&fsck_options))
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 73bff3a23d..92b9c8598b 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -268,6 +268,7 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,
>  					  type, size);
>  	the_hash_algo->init_fn(&ctx);
>  	the_hash_algo->update_fn(&ctx, obuf, header_len);
> +	the_hash_algo->init_fn(&checkpoint.ctx);
>  
>  	/* Note: idx is non-NULL when we are writing */
>  	if ((flags & HASH_WRITE_OBJECT) != 0)
> diff --git a/csum-file.c b/csum-file.c
> index cd01713244..870748e016 100644
> --- a/csum-file.c
> +++ b/csum-file.c
> @@ -207,7 +207,7 @@ int hashfile_truncate(struct hashfile *f, struct hashfile_checkpoint *checkpoint
>  	    lseek(f->fd, offset, SEEK_SET) != offset)
>  		return -1;
>  	f->total = offset;
> -	f->ctx = checkpoint->ctx;
> +	the_hash_algo->clone_fn(&f->ctx, &checkpoint->ctx);
>  	f->offset = 0; /* hashflush() was called in checkpoint */
>  	return 0;
>  }

^ permalink raw reply

* Re: [PATCH] treewide: fix various bugs w/ OpenSSL 3+ EVP API
From: Oswald Buddenhagen @ 2023-09-01  6:46 UTC (permalink / raw)
  To: Eric Wong
  Cc: brian m. carlson, Bagas Sanjaya, git, Junio C Hamano,
	Jonathan Tan
In-Reply-To: <20230901020928.M610756@dcvr>

On Fri, Sep 01, 2023 at 02:09:28AM +0000, Eric Wong wrote:
>@@ -1202,7 +1203,9 @@ static void parse_pack_objects(unsigned char *hash)
> 
> 	/* Check pack integrity */
> 	flush();
>+	the_hash_algo->init_fn(&tmp_ctx);
>
does it make sense (and doesn't it potentially even cause a leak) to 
init the target before cloning into it? at least the fallback simply 
memcpy()s over it.

>@@ -669,7 +670,9 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
> 	the_hash_algo->init_fn(&ctx);
> 	unpack_all();
> 	the_hash_algo->update_fn(&ctx, buffer, offset);
>+	the_hash_algo->init_fn(&tmp_ctx);
>
ditto

>+	the_hash_algo->clone_fn(&tmp_ctx, &ctx);
>+	the_hash_algo->final_oid_fn(&oid, &tmp_ctx);

regards

^ permalink raw reply

* Re: [PATCH v4] format-patch: --rfc honors what --subject-prefix sets
From: Drew DeVault @ 2023-09-01  7:29 UTC (permalink / raw)
  To: Junio C Hamano, Jeff King; +Cc: git
In-Reply-To: <xmqqv8cuswah.fsf@gitster.g>

+1 to proposed changes

^ permalink raw reply

* Re: Is there a way to get the "format-patch" formatted file name?
From: Vít Ondruch @ 2023-09-01  8:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqo7inw2na.fsf@gitster.g>


[-- Attachment #1.1: Type: text/plain, Size: 2663 bytes --]


Dne 31. 08. 23 v 19:19 Junio C Hamano napsal(a):
> Vít Ondruch <vondruch@redhat.com> writes:
>
>> My typical use case is to download patches from GH, e.g.:
>>
>> ~~~
>>
>> $ curl -OL https://github.com/rails/sprockets/pull/791.patch
> ...
>> The problem with this is that I end up with the "791.patch" file,
>> while I'd like have a file with similar name as if I have used the git
>> command:
>>
>>
>> ~~~
>>
>> $ git format-patch -1 6554b6d
>> 0001-Fix-Minitest-constant-name-in-tests.patch
>>
>> ~~~
>>
>>
>> So I wonder, is there a way to get such file name?
> Do you mean: GitHub should let me run this command instead
>
>    $ curl -OL https://github.com/rails/sprockets/pull/0001-Fix-Minitest-constant-name-in-tests.patch
>
> It may be nice for them to give a more meaningful name to their pull
> request (not just the output file name) than just an integer.


This URL would be hard to compile and it could potentially lead me to 
the same question ;) But maybe in combination with `-J` option, GH (but 
also all other forges) could probably do something about it.


> But
> that is not a question/request we can answer here (this is not a
> help forum for GitHub users).


Right.


>
> Something along the lines of
>
>      sed -ne '/^Subject: /{
>              s/^Subject: *\[PATCH[^]]*\] *//;
>              s/[^a-zA-Z0-9]/-/g;
>              s/--*/-/g;
>              s/$/\.patch/;
> 	    p;
> 	    q;
>      }' 791.patch
>
> should be doable, but I am not sure what the benefit is.


If this was provided as a some convenient script, then I would not need 
to know what the sanitation of the subject is done to get the file name. 
However, so far I was mostly fine with manually replacing spaces by 
dashes, so what is sanitized is not really that important to me. I just 
though that if Git has already some opinion about it and it is already 
implemented, it would be nice to reuse the functionality.


>    Once you
> get it in Git,


But I typically don't have a Git.


>   you'd park it on a branch with a useful name and we
> can forget about "791", so the "The files we get from GitHub are
> named in a way that makes it hard to identify them" does not sound
> like a Git issue, at least to me.
>
>
>

It does not seems I have convinced you (and that is fine). I'll probably 
look around. patchutils project could be probably good candidate for 
such script. I'll try to ask there. Maybe I'll also try to explore if GH 
would be open to provide some better filename, leveraging the `-J` curl 
option.


Thx for you help! Appreciate that.


Vít


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

^ permalink raw reply

* Re: Is there a way to get the "format-patch" formatted file name?
From: Oswald Buddenhagen @ 2023-09-01 11:00 UTC (permalink / raw)
  To: Vít Ondruch; +Cc: Junio C Hamano, git
In-Reply-To: <24e10903-29a0-497a-c8d8-b26c02de7336@redhat.com>

On Fri, Sep 01, 2023 at 10:56:28AM +0200, Vít Ondruch wrote:
>If this was provided as a some convenient script,
>
well, that's the key here: to be actually useful, it needs to be 
integrated into a highly specialized tool. the poor discoverability of 
that in conjunction with its somewhat minor utility is probably a 
sufficient argument for not going that route (beyond personal tool 
collections).
github & co. providing a sensible content-disposition otoh is a 
(somewhat) well-known, generic solution to this problem.

but i wouldn't be opposed to for example git-mailinfo learning to 
understand the --pretty argument, if the implementation doesn't turn out 
to be completely out of proportion.

regards

^ permalink raw reply

* Re: [PATCH] treewide: fix various bugs w/ OpenSSL 3+ EVP API
From: Bagas Sanjaya @ 2023-09-01 11:02 UTC (permalink / raw)
  To: Eric Wong, brian m. carlson, git, Junio C Hamano, Jonathan Tan
In-Reply-To: <20230901020928.M610756@dcvr>

[-- Attachment #1: Type: text/plain, Size: 3411 bytes --]

On Fri, Sep 01, 2023 at 02:09:28AM +0000, Eric Wong wrote:
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 4dbb10aff3..444f41cf8c 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -1102,6 +1102,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
>  		|| (pack_size + PACK_SIZE_THRESHOLD + len) < pack_size)
>  		cycle_packfile();
>  
> +	the_hash_algo->init_fn(&checkpoint.ctx);
>  	hashfile_checkpoint(pack_file, &checkpoint);
>  	offset = checkpoint.offset;
>  
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 006ffdc9c5..dda94a9f46 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1166,6 +1166,7 @@ static void parse_pack_objects(unsigned char *hash)
>  	struct ofs_delta_entry *ofs_delta = ofs_deltas;
>  	struct object_id ref_delta_oid;
>  	struct stat st;
> +	git_hash_ctx tmp_ctx;
>  
>  	if (verbose)
>  		progress = start_progress(
> @@ -1202,7 +1203,9 @@ static void parse_pack_objects(unsigned char *hash)
>  
>  	/* Check pack integrity */
>  	flush();
> -	the_hash_algo->final_fn(hash, &input_ctx);
> +	the_hash_algo->init_fn(&tmp_ctx);
> +	the_hash_algo->clone_fn(&tmp_ctx, &input_ctx);
> +	the_hash_algo->final_fn(hash, &tmp_ctx);
>  	if (!hasheq(fill(the_hash_algo->rawsz), hash))
>  		die(_("pack is corrupted (SHA1 mismatch)"));
>  	use(the_hash_algo->rawsz);
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index 32505255a0..fef7423448 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -609,6 +609,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
>  {
>  	int i;
>  	struct object_id oid;
> +	git_hash_ctx tmp_ctx;
>  
>  	disable_replace_refs();
>  
> @@ -669,7 +670,9 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
>  	the_hash_algo->init_fn(&ctx);
>  	unpack_all();
>  	the_hash_algo->update_fn(&ctx, buffer, offset);
> -	the_hash_algo->final_oid_fn(&oid, &ctx);
> +	the_hash_algo->init_fn(&tmp_ctx);
> +	the_hash_algo->clone_fn(&tmp_ctx, &ctx);
> +	the_hash_algo->final_oid_fn(&oid, &tmp_ctx);
>  	if (strict) {
>  		write_rest();
>  		if (fsck_finish(&fsck_options))
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 73bff3a23d..92b9c8598b 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -268,6 +268,7 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,
>  					  type, size);
>  	the_hash_algo->init_fn(&ctx);
>  	the_hash_algo->update_fn(&ctx, obuf, header_len);
> +	the_hash_algo->init_fn(&checkpoint.ctx);
>  
>  	/* Note: idx is non-NULL when we are writing */
>  	if ((flags & HASH_WRITE_OBJECT) != 0)
> diff --git a/csum-file.c b/csum-file.c
> index cd01713244..870748e016 100644
> --- a/csum-file.c
> +++ b/csum-file.c
> @@ -207,7 +207,7 @@ int hashfile_truncate(struct hashfile *f, struct hashfile_checkpoint *checkpoint
>  	    lseek(f->fd, offset, SEEK_SET) != offset)
>  		return -1;
>  	f->total = offset;
> -	f->ctx = checkpoint->ctx;
> +	the_hash_algo->clone_fn(&f->ctx, &checkpoint->ctx);
>  	f->offset = 0; /* hashflush() was called in checkpoint */
>  	return 0;
>  }

The regression gone away, thanks!

Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply

* Re: [REGRESSION] Can't clone GitHub repos (fetch-pack error) due to avoiding deprecated OpenSSL SHA-1 routines
From: Bagas Sanjaya @ 2023-09-01 11:09 UTC (permalink / raw)
  To: brian m. carlson, Git Mailing List, Eric Wong, Junio C Hamano,
	Jonathan Tan
In-Reply-To: <ZPEf8kbBUFqLO25W@tapette.crustytoothpaste.net>

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

On Thu, Aug 31, 2023 at 11:19:14PM +0000, brian m. carlson wrote:
> On 2023-08-31 at 12:47:19, Bagas Sanjaya wrote:
> > Hi,
> > 
> > I built Git v2.42.0 on Debian testing, linked with OpenSSL (v3.0.10 from
> > distribution) with Makefile knob `OPENSSL_SHA1=YesPlease 
> > OPENSSL_SHA256=YesPlease`. I tried to shallow clone git.git repository:
> 
> I should point out that using OpenSSL's SHA-1 support is insecure
> because it doesn't check for collisions.  As a practical matter, no
> distro builds that way, and if you distributed that build, it would
> probably qualify for a CVE.
> 
> However, OPENSSL_SHA256 being set is fine for a local build or a build
> where you're not distributing OpenSSL itself.

Thanks for the disclaimer. I did such build for myself since the distro
version always lagging.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply

* Re: [bug]fatal: fetch-pack: invalid index-pack output
From: Bagas Sanjaya @ 2023-09-01 11:15 UTC (permalink / raw)
  To: mingli zhang, git; +Cc: Jonathan Tan, brian m. carlson
In-Reply-To: <50B3E989-0405-4B05-9940-D3943C62260A@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]

On Sun, Aug 14, 2022 at 05:09:13AM +0800, mingli zhang wrote:
> HI,
> 
> At some point, I exec git pull/fetch/clone will got a fatal message:
> 
> ```
> 
> fatal: fetch-pack: invalid index-pack output
> 
> ```
> 
> I use git 2.36.1 on MacOS Monterey 12.3.1
> 
> ```
> git --version
> 04:51:36.776640 git.c:459 trace: built-in: git version
> git version 2.36.1
> 
> ```
> 
> Example:
> 
> ```
> 
> zml@localhashdata postgres % git pull -r
> 04:53:33.553655 git.c:459 trace: built-in: git pull -r
> 04:53:33.640927 run-command.c:654 trace: run_command: git merge-base --fork-point refs/remotes/origin/REL_14_STABLE REL_14_STABLE
> 04:53:33.651808 run-command.c:654 trace: run_command: git fetch --update-head-ok
> 04:53:33.658822 git.c:459 trace: built-in: git fetch --update-head-ok
> 04:53:33.683642 run-command.c:654 trace: run_command: unset GIT_PREFIX; GIT_PROTOCOL=version=2 ssh -o SendEnv=GIT_PROTOCOL git@github.com 'git-upload-pack '\''postgres/postgres.git'\'''
> remote: Enumerating objects: 16843, done.
> remote: Counting objects: 100% (16231/16231), done.
> remote: Compressing objects: 100% (4164/4164), done.
> 04:53:40.822310 run-command.c:654 trace: run_command: git index-pack --stdin -v --fix-thin '--keep=fetch-pack 70208 on localhashdata' --pack_header=2,10035
> 04:53:40.828465 git.c:459 trace: built-in: git index-pack --stdin -v --fix-thin '--keep=fetch-pack 70208 on localhashdata' --pack_header=2,10035
> remote: Total 10035 (delta 8554), reused 7150 (delta 5805), pack-reused 0
> Receiving objects: 100% (10035/10035), 6.93 MiB | 2.42 MiB/s, done.
> fatal: fetch-pack: invalid index-pack output
> 
> ```
> 
> I try to config memory or use git pull -r --depth 1, but didn’t help.
> 
> My git config is
> 
> ```
> [core]
> packedGitLimit = 512m
> packedGitWindowSize = 512m
> compression = 0
> 
> [pack]
> deltaCacheSize = 2047m
> packSizeLimit = 2047m
> windowMemory = 2047m
> ```
> 
> And the code dir permission is
> 
> ```
> drwxrwxrwx
> 
> ```
> 
> Any suggestions ?

Related discussion is in [1] with the fix for OpenSSL users is in [2].
Please test.

[1]: https://lore.kernel.org/git/ZPCL11k38PXTkFga@debian.me/
[2]: https://lore.kernel.org/git/20230901020928.M610756@dcvr/

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply

* move from gitscm to github with Eclipse
From: Chamberlain, Anthony @ 2023-09-01 12:28 UTC (permalink / raw)
  To: git@vger.kernel.org

Hi

I am currently using gitscm.  I have Eclipse with many git workspaces.  Several years ago I made the gitscm token and things work fine.
Now with switching to github this will be done gradually, so some projects will be github and some still gitscm.  I made my github token.
Now if in my Eclipse I fill out the login, password, etc., and check "Store in Secure Store" will this remove the login and password 
I am currently using?  Or will it keep two separate values depending on the base URL, or what will happen?

PS: I cannot insert the picture or it will think it is spam


NOTICE OF CONFIDENTIALITY: Information included in and/or attached to this transmission may be confidential. This transmission is intended for the addressee(s) only. Any unauthorized disclosure, reproduction, or distribution of and /or any unauthorized action taken in reliance on the information in this transmission is prohibited. If you believe that you received this transmission in error, please notify the sender by reply transmission and destroy the transmission without copying or disclosing it. We may also need to contact you for additional information as required by HIPAA or state law.

For questions about your privacy choices, please view our privacy policy on our website at: https://www.multiplan.us/ccpa/ .

^ permalink raw reply

* Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Phillip Wood @ 2023-09-01 13:19 UTC (permalink / raw)
  To: Wesley Schwengle, git; +Cc: Junio C Hamano
In-Reply-To: <20230819203528.562156-2-wesleys@opperschaap.net>

Hi Wesley

On 19/08/2023 21:34, Wesley Schwengle wrote:
> When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,
> 2021-09-16) was submitted there was a discussion on if the forkpoint
> behaviour of `git rebase' was sane. In my experience this wasn't sane.
> Git rebase doesn't work if you don't have an upstream branch configured
> (or something that says `merge = refs/heads/master' in the git config).
> The behaviour of `git rebase' was that if you supply an upstream on the
> command line that it behaves as if `--no-forkpoint' was supplied and if
> you don't supply an upstream, it behaves as if `--forkpoint' was
> supplied. This can result in a loss of commits if you don't know that
> and if you don't know about `git reflog' or have other copies of your
> changes. This can be seen with the following reproduction path:
> 
>      mkdir reproduction
>      cd reproduction
>      git init .
>      echo "commit a" > file.txt
>      git add file.txt
>      git commit -m "First commit" file.txt
>      echo "commit b" >> file.txt
>      git commit -m "Second commit" file.txt
> 
>      git switch -c foo
>      echo "commit c" >> file.txt"
>      git commit -m "Third commit" file.txt
>      git branch --set-upstream-to=master
> 
>      git status
>      On branch foo
>      Your branch is ahead of 'master' by 1 commit.
> 
>      git switch master
>      git merge foo

Here "git merge" fast-forwards I think, if instead it created a merge 
commit there would be no problem as the tip of branch "foo" would not 
end up in master's reflog.

>      git reset --hard HEAD^
>      git switch foo
>      Switched to branch 'foo'
>      Your branch is ahead of 'master' by 1 commit.
> 
>      git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'

For a reproduction recipe I think "git log --oneline" would suffice.

>      5f427e3 Third commit
>      03ad791 Second commit
>      411e6d4 First commit
> 
>      git rebase
>      git status
>      On branch foo
>      Your branch is up to date with 'master'.
> 
>      git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'
>      03ad791 Second commit
>      411e6d4 First commit

Thanks for the detailed reproduction recipe, I think it would be helpful 
to summarize what's happening in the commit message, especially as it 
seems to depend on "git merge" fast-forwarding. Do you often merge a 
branch into it's upstream and then reset the upstream branch?

I tend to agree with Junio that the current default is pretty 
reasonable. Looking through the links from the cover letter it seems 
that the current behavior came from a desire for

	git fetch && git rebase

to behave like

	git pull --rebase

I think the commit message for any change to the default should address 
why that is undesirable. Also we should consider what problems may arise 
from not defaulting to --fork-point when rebasing on an upstream branch 
that has itself been rebased or rewound.

Best Wishes

Phillip

^ permalink raw reply

* Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable
From: Phillip Wood @ 2023-09-01 13:24 UTC (permalink / raw)
  To: Jeff King, git; +Cc: Johannes Schindelin
In-Reply-To: <20230830194919.GA1709446@coredump.intra.peff.net>

Hi Peff

On 30/08/2023 20:49, Jeff King wrote:
> This is a more efficient way to do the same thing that
> ci/config/allow-ref does (which didn't exist back then).

I like the idea of a more efficient way to skip the ci for certain refs. 
I've got my allow-ref script set up to reject a bunch of refs and run 
the ci on everything else. It's not clear to me how to replicate that 
with the setup proposed here. Would it be possible to add a second 
variable that prevents the ci from being run if it contains ref being 
pushed?

Best Wishes

Phillip

> We should be able to do the same with ci/config/skip-concurrent (and
> just use vars.CI_SKIP_CONCURRENT) throughout the workflow. But I didn't
> test that at all.
> 
> After that, the only useful thing left in the "config" job would be the
> "skip-if-redundant" step. I'm not sure if it will be possible to get the
> same behavior there without spinning up a VM.
> 
>    [1/2]: ci: allow branch selection through "vars"
>    [2/2]: ci: deprecate ci/config/allow-ref script
> 
>   .github/workflows/main.yml | 10 +++++++---
>   ci/config/README           | 14 ++++++++++++++
>   ci/config/allow-ref.sample | 27 ---------------------------
>   3 files changed, 21 insertions(+), 30 deletions(-)
>   create mode 100644 ci/config/README
>   delete mode 100755 ci/config/allow-ref.sample
> 


^ permalink raw reply

* Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Phillip Wood @ 2023-09-01 13:33 UTC (permalink / raw)
  To: Junio C Hamano, Wesley Schwengle; +Cc: git
In-Reply-To: <xmqq1qfiubg5.fsf@gitster.g>

On 31/08/2023 22:52, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> I am not commenting on the tests, as the above code probably needs
>> to be corrected first so that folks who want to squelch the message
>> and want the "forkpoint behaviour by default when rebuilding on the
>> usual upstream" behaviour can do so by setting the variable to true.
>>
>> And that obviously need to be tested, too.
> 
> Another worrysome thing about rebase.forkpoint is that it will be
> inevitable for folks to start complaining that it does not work the
> way other configuration variables do.  Setting the variable to
> 'true' is not the same as passing '--fork-point=true' from the
> command line.

It does seem strange, it looks like the variable was really added as a 
way to turn off the current default. If we do change the default to 
--no-fork-point when no upstream is given on the commandline then I 
think we should consider allowing "auto" for rebase.forkpoint with the 
some meaning as "true" and recommend that instead.

Best Wishes

Phillip

> I actually think it would be a lot larger behaviour change with a
> huge potential to be received as a regression if we start making the
> variable to mean the same thing as passing '--fork-point=true'.
> People may like the current "if you are rebuilding your branch on
> its usual upstream, pay attention to the rebase and rewind of the
> upstream itself, but if you are giving an explicit upstream from the
> command line, the tool does not second guess you with the fork-point
> heuristics" behaviour and prefer to set it to true.  We would be
> breaking them big time if suddenly the rebase.forkpoint=true they
> set previously starts triggering the fork-point heuristics when they
> run "git rebase upstream".  So that needs to be kept in mind when/if
> we fix the "setting the variable, even to 'true', will squelch the
> warning".
> 


^ permalink raw reply

* [PATCH] ref-filter: sort numerically when ":size" is used
From: Kousik Sanagavarapu @ 2023-09-01 14:24 UTC (permalink / raw)
  To: git; +Cc: Kousik Sanagavarapu

Atoms like "raw" and "contents" have a ":size" option which can be used
to know the size of the data. Since these atoms have the cmp_type
FIELD_STR, they are sorted alphabetically from 'a' to 'z' and '0' to
'9'. Meaning, even when the ":size" option is used and what we
ultimatlely have is numbers, we still sort alphabetically.

For example, consider the the following case in a repo

refname			contents:size		raw:size
=======			=============		========
refs/heads/branch1	1130			1210
refs/heads/master	300			410
refs/tags/v1.0		140			260

Sorting with "--format="%(refname) %(contents:size) --sort=contents:size"
would give

refs/heads/branch1 1130
refs/tags/v1.0.0 140
refs/heads/master 300

which is an alphabetic sort, while what one might really expect is

refs/tags/v1.0.0 140
refs/heads/master 300
refs/heads/branch1 1130

which is a numeric sort (that is, a "$ sort -n file" as opposed to a
"$ sort file", where "file" contains only the "contents:size" or
"raw:size" info, each of which is on a newline).

Same is the case with "--sort=raw:size".

So, sort numerically whenever the sort is done with "contents:size" or
"raw:size" and do it the normal alphabetic way when "contents" or "raw"
are used with some other option (they are FIELD_STR anyways).

Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 ref-filter.c            | 20 +++++++++++++++-----
 t/t6300-for-each-ref.sh | 15 +++++++++++++--
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 1bfaf20fbf..5d7bea5f23 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -932,7 +932,13 @@ struct atom_value {
 	ssize_t s_size;
 	int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,
 		       struct strbuf *err);
-	uintmax_t value; /* used for sorting when not FIELD_STR */
+
+	/*
+	 * Used for sorting when not FIELD_STR or when FIELD_STR but the
+	 * sort should be numeric and not alphabetic.
+	 */
+	uintmax_t value;
+
 	struct used_atom *atom;
 };
 
@@ -1857,7 +1863,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
 				v->s = xmemdupz(buf, buf_size);
 				v->s_size = buf_size;
 			} else if (atom->u.raw_data.option == RAW_LENGTH) {
-				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)buf_size);
+				v->value = (uintmax_t)buf_size;
+				v->s = xstrfmt("%"PRIuMAX, v->value);
 			}
 			continue;
 		}
@@ -1883,8 +1890,10 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
 			v->s = strbuf_detach(&sb, NULL);
 		} else if (atom->u.contents.option == C_BODY_DEP)
 			v->s = xmemdupz(bodypos, bodylen);
-		else if (atom->u.contents.option == C_LENGTH)
-			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
+		else if (atom->u.contents.option == C_LENGTH) {
+			v->value = (uintmax_t)strlen(subpos);
+			v->s = xstrfmt("%"PRIuMAX, v->value);
+		}
 		else if (atom->u.contents.option == C_BODY)
 			v->s = xmemdupz(bodypos, nonsiglen);
 		else if (atom->u.contents.option == C_SIG)
@@ -2265,6 +2274,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 
 		v->s_size = ATOM_SIZE_UNSPECIFIED;
 		v->handler = append_atom;
+		v->value = 0;
 		v->atom = atom;
 
 		if (*name == '*') {
@@ -2986,7 +2996,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 		cmp_detached_head = 1;
 	} else if (s->sort_flags & REF_SORTING_VERSION) {
 		cmp = versioncmp(va->s, vb->s);
-	} else if (cmp_type == FIELD_STR) {
+	} else if (cmp_type == FIELD_STR && !va->value && !vb->value) {
 		if (va->s_size < 0 && vb->s_size < 0) {
 			int (*cmp_fn)(const char *, const char *);
 			cmp_fn = s->sort_flags & REF_SORTING_ICASE
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index aa3c7c03c4..7b943fd34c 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1017,16 +1017,16 @@ test_expect_success 'Verify sorts with raw' '
 test_expect_success 'Verify sorts with raw:size' '
 	cat >expected <<-EOF &&
 	refs/myblobs/blob8
-	refs/myblobs/first
 	refs/myblobs/blob7
-	refs/heads/main
 	refs/myblobs/blob4
 	refs/myblobs/blob1
 	refs/myblobs/blob2
 	refs/myblobs/blob3
 	refs/myblobs/blob5
 	refs/myblobs/blob6
+	refs/myblobs/first
 	refs/mytrees/first
+	refs/heads/main
 	EOF
 	git for-each-ref --format="%(refname)" --sort=raw:size \
 		refs/heads/main refs/myblobs/ refs/mytrees/first >actual &&
@@ -1138,6 +1138,17 @@ test_expect_success 'for-each-ref --format compare with cat-file --batch' '
 	test_cmp expected actual
 '
 
+test_expect_success 'verify sorts with contents:size' '
+	cat >expect <<-\EOF &&
+	refs/heads/main
+	refs/heads/newtag
+	refs/heads/ambiguous
+	EOF
+	git for-each-ref --format="%(refname)" \
+		--sort=contents:size refs/heads/ >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'set up multiple-sort tags' '
 	for when in 100000 200000
 	do
-- 
2.42.0.51.g5dc72c0fbc.dirty


^ permalink raw reply related

* Re: Is there a way to get the "format-patch" formatted file name?
From: Junio C Hamano @ 2023-09-01 15:53 UTC (permalink / raw)
  To: Oswald Buddenhagen; +Cc: Vít Ondruch, git
In-Reply-To: <ZPHEOYyyX+l3AGP1@ugly>

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> but i wouldn't be opposed to for example git-mailinfo learning to
> understand the --pretty argument, if the implementation doesn't turn
> out to be completely out of proportion.

Excellent suggestion.  I agree that 'mailinfo' would be the closest
place we have for such a new feature.  It's "info" output (i.e. what
comes out to the standard output of the command) is designed to be
extensible, and I vaguely recall that we indeed have added new
field(s) during its lifetime with existing users already.

We can just invent a new label (e.g. "Filesystem-safe-subject:"),
pass the subject string to pretty.c:format_sanitized_subject() and
emit the result next to the existing "Subject:" with that label, and
we can even do so unconditionally without breaking anybody.


Having it in 'mailinfo' may still not be a good solution to the
issue, given that Vit says

>> But I typically don't have a Git.

though.


On a related not-so-distant tangent, we probably should redo the
support for --message-id to emit it as an extra entry to the "info"
output, instead of contaminating the "message" output.  The option
was added only to support "git am --message-id", and as long as the
calling "am" and "mailinfo" are updated in sync to use the "info"
output to carry the Message-Id: information, we should be able to do
such a clean-up without changing the externally visible behaviour.

^ permalink raw reply

* Re: Is there a way to get the "format-patch" formatted file name?
From: Junio C Hamano @ 2023-09-01 16:04 UTC (permalink / raw)
  To: Oswald Buddenhagen; +Cc: Vít Ondruch, git
In-Reply-To: <xmqqwmx9ritx.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> We can just invent a new label (e.g. "Filesystem-safe-subject:"),
> pass the subject string to pretty.c:format_sanitized_subject() and
> emit the result next to the existing "Subject:" with that label, and
> we can even do so unconditionally without breaking anybody.

Such a change would be quite small and simple.

 mailinfo.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git c/mailinfo.c w/mailinfo.c
index 931505363c..b0eb646f4e 100644
--- c/mailinfo.c
+++ w/mailinfo.c
@@ -5,6 +5,7 @@
 #include "utf8.h"
 #include "strbuf.h"
 #include "mailinfo.h"
+#include "pretty.h"
 
 static void cleanup_space(struct strbuf *sb)
 {
@@ -1166,11 +1167,16 @@ static void handle_info(struct mailinfo *mi)
 		}
 
 		if (!strcmp(header[i], "Subject")) {
+			struct strbuf san = STRBUF_INIT;
+
 			if (!mi->keep_subject) {
 				cleanup_subject(mi, hdr);
 				cleanup_space(hdr);
 			}
 			output_header_lines(mi->output, "Subject", hdr);
+			format_sanitized_subject(&san, hdr->buf, hdr->len);
+			output_header_lines(mi->output, "Filesystem-Safe-Subject", &san);
+			strbuf_release(&san);
 		} else if (!strcmp(header[i], "From")) {
 			cleanup_space(hdr);
 			handle_from(mi, hdr);

^ permalink raw reply related

* [PATCH v2 0/3] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-01 16:18 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk
In-Reply-To: <cover.1686505920.git.code@khaugsbakk.name>

The following cover letter is identical to the previous one up to but not
including section “Changes since version 1”.

Cheers

🙛 🙙

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.

These changes are supposed to make `format-range` behave like `log` with
regards to notes.

These changes also fixes an issue with notes being shared in the cover
letter via `range-diff`, and that’s really the main motivation for
making these changes.

§ How `log` works

`log` shows the default notes if no notes arguments are given. But if
you give it specific notes to show then it forgets about the default
notes. Further, there is the convenience `--notes` option which will
show the default notes again. These options are cumulative. For example:

    git log --notes --notes=custom

Will show the default notes as well as the `custom` notes.

See discussion in: https://lore.kernel.org/git/20110329143357.GA10771@sigill.intra.peff.net/

§ How `range-format` works

`range-format` passes `--notes` to `log`, which means that it does not
have the default behavior of `log` (forget the default logs if you say
e.g. `--notes=custom`). However, the man page says that (under
`--[no-]notes[=<ref>]`):

> This flag is passed to the git log program (see git-log(1)) that generates the patches.

This makes me (at least) think that `range-format` is supposed to work
just like `log` with regards to notes.

§ `format-patch` and the difference between showing and sharing

`format-patch` has a different default: it shows no notes. This makes
sense in my opinion since `format-patch` is meant to be used to share
changes with others, and you might be surprised if your notes (which
might have only been notes to yourself) are sent out in your emails
(keep in mind that notes refs are *not* pushed by default).

But the slightly faulty behavior of `range-diff` bleeds through to
`format-patch` since the latter calls the former; if you have default
notes they can be shared in the range-diff on the cover letter, even
though `format-patch` isn’t supposed to show them.

§ Code layout and approach to the problem

As I’ve said, my focus was on fixing `format-patch`, so I’ve considered
how `format-patch` calls `range-diff` which in turn calls `log`.

`format-patch` is a command which is defined in `builtin/log.c`. For
notes in particular it in fact has some explicit logic for handling
notes based on the value of `rev`. (There seems to be no issues with
this part of the code; only the code in `range-diff.c` which passes the
default argument to `log`.) It then calls
`range-diff.c:show_range_diff`. That function on `master` loads some
default arguments, among them `--notes`. It then eventually calls `log`
as a subprocess.

My change consists of co-opting the deprecated `--standard-notes` and
changing its behavior so that it can be used in
`range-diff.c:show_range_diff`.

Using a special switch/option was the only way I found in order to fix
this problem.

I could have also created a new option but I thought that doing a
smaller change initially would be better.

§ Changes since version 1

Patch 3: change comment placement based on feedback.

§ Rebase on upstream

Rebased on `master` (fc6bba66bc (Merge branch
'js/allow-t4000-to-be-indented-with-spaces', 2023-08-14)).

§ CI

https://github.com/LemmingAvalanche/git/actions/runs/5868463450

Kristoffer Haugsbakk (3):
  range-diff: treat notes like `log`
  doc: pretty-options: remove documentation for deprecated options
  revision: comment `--no-standard-notes` as deprecated

 Documentation/pretty-options.txt |  1 -
 range-diff.c                     |  2 +-
 revision.c                       |  8 ++++++--
 t/t3206-range-diff.sh            | 28 ++++++++++++++++++++++++++++
 4 files changed, 35 insertions(+), 4 deletions(-)

Range-diff against v1:
1:  6a4fe606cf = 1:  e9a5910831 range-diff: treat notes like `log`
2:  fbc1c47b92 = 2:  f7308b7abf doc: pretty-options: remove documentation for deprecated options
3:  9141f5a86e ! 3:  80245bbb7e revision: comment `--no-standard-notes` as deprecated
    @@ Metadata
      ## Commit message ##
         revision: comment `--no-standard-notes` as deprecated

    -    We still use `--standard-notes` but this option has no use and is no
    -    longer documented anywhere.
    +    `--standard-notes` used to be deprecated but is now (since 6a4fe606cf[1])
    +    used internally. Its negation `--no-standard-notes`, however, is still
    +    deprecated even for internal use.
    +
    +    Mark this option as such.
    +
    +    † 1: range-diff: treat notes like `log`, 2023-05-19

         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>

    +
    + ## Notes (series) ##
    +    • Move comment: https://lore.kernel.org/git/xmqqbkhk724x.fsf@gitster.g/
    +    • Tweak commit message so that it's clearer why we are only commenting
    +      “no” as deprecated and not the other option as well
    +
      ## revision.c ##
     @@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
    - 		enable_default_display_notes(&revs->notes_opt,
      					     &revs->show_notes);
      		revs->notes_opt.use_default_notes = -1;
    -+		/* Deprecated */
      	} else if (!strcmp(arg, "--no-standard-notes")) {
    ++		/* Deprecated */
      		revs->notes_opt.use_default_notes = 0;
      	} else if (!strcmp(arg, "--oneline")) {
    + 		revs->verbose_header = 1;
--
2.41.0

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox