Git development
 help / color / mirror / Atom feed
* [PATCH 1/3] diff-format.adoc: remove mention of diff-tree specific output
From: Philippe Blain via GitGitGadget @ 2026-05-15 15:48 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.2304.git.git.1778860091.gitgitgadget@gmail.com>

From: Philippe Blain <levraiphilippeblain@gmail.com>

In the "Raw output format" section, we start by mentioning that 'git
diff-tree' prints the hashes of what is being compared. This is only
true in --stdin mode, and is already mentioned in the description of
'--stdin' in git-diff-tree.adoc. Remove this sentence such that we only
focus on the common output between diff-tree, diff-index, diff-files and
diff --raw.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/diff-format.adoc | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/Documentation/diff-format.adoc b/Documentation/diff-format.adoc
index 9f7e988241..7f18c64f1e 100644
--- a/Documentation/diff-format.adoc
+++ b/Documentation/diff-format.adoc
@@ -19,9 +19,7 @@ compared differs:
 `git-diff-files [<pattern>...]`::
         compares the index and the files on the filesystem.
 
-The `git-diff-tree` command begins its output by printing the hash of
-what is being compared. After that, all the commands print one output
-line per changed file.
+All the commands print one output line per changed file.
 
 An output line is formatted this way:
 
-- 
gitgitgadget


^ permalink raw reply related

* [PATCH 2/3] diff-format.adoc: 'git diff-files' prints two lines for unmerged files
From: Philippe Blain via GitGitGadget @ 2026-05-15 15:48 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.2304.git.git.1778860091.gitgitgadget@gmail.com>

From: Philippe Blain <levraiphilippeblain@gmail.com>

Since 10637b84d9 (diff-files: -1/-2/-3 to diff against unmerged stage.,
2005-11-29), for unmerged entries 'git diff-files' print both an
"unmerged" line ('U'), as well as an "in-place edit" line ('M')
comparing stage 2 (by default) with the working tree. The "Raw output
format" documentation however mentions that all commands print a single
line per changed file. Adjust diff-format.adoc to also mention this
special case, for completeness.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/diff-format.adoc | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-format.adoc b/Documentation/diff-format.adoc
index 7f18c64f1e..43d91ef868 100644
--- a/Documentation/diff-format.adoc
+++ b/Documentation/diff-format.adoc
@@ -19,7 +19,9 @@ compared differs:
 `git-diff-files [<pattern>...]`::
         compares the index and the files on the filesystem.
 
-All the commands print one output line per changed file.
+All the commands print one output line per changed file,
+except `git diff-files` in the case of an unmerged file, which prints
+both an "unmerged" and an "in-place edit" line.
 
 An output line is formatted this way:
 
-- 
gitgitgadget


^ permalink raw reply related

* [PATCH 3/3] diff-format.adoc: mode and hash are 0* for unmerged paths from index only
From: Philippe Blain via GitGitGadget @ 2026-05-15 15:48 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.2304.git.git.1778860091.gitgitgadget@gmail.com>

From: Philippe Blain <levraiphilippeblain@gmail.com>

In the "Raw output format" section, we mention that the 'mode' and
'sha1' for "src" and "dst" are 0* if "(creation|deletion) or unmerged".
For unmerged entries, 'mode' and 'sha1' are in fact 0* only when we are
looking at the index, i.e. on the left side for 'git diff-files' and on
the right side for 'git diff-index --cached'. Be more precise by
mentioning this, and while at it uniformize the wording of the "work
tree out of sync with the index" case.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/diff-format.adoc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-format.adoc b/Documentation/diff-format.adoc
index 43d91ef868..ef5df140fe 100644
--- a/Documentation/diff-format.adoc
+++ b/Documentation/diff-format.adoc
@@ -37,13 +37,13 @@ unmerged       :000000 000000 0000000 0000000 U file6
 That is, from the left to the right:
 
 . a colon.
-. mode for "src"; 000000 if creation or unmerged.
+. mode for "src"; 000000 if creation, or if "src" is from the index and is unmerged.
 . a space.
-. mode for "dst"; 000000 if deletion or unmerged.
+. mode for "dst"; 000000 if deletion, or if "dst" is from the index and is unmerged.
 . a space.
-. sha1 for "src"; 0\{40\} if creation or unmerged.
+. sha1 for "src"; 0\{40\} if creation, or if "src" is from the index and is unmerged.
 . a space.
-. sha1 for "dst"; 0\{40\} if deletion, unmerged or "work tree out of sync with the index".
+. sha1 for "dst"; 0\{40\} if deletion, if "dst" is from the index and is unmerged, or if "dst" is from the work tree and is out of sync with the index.
 . a space.
 . status, followed by optional "score" number.
 . a tab or a NUL when `-z` option is used.
-- 
gitgitgadget

^ permalink raw reply related

* Re: [PATCH v1 01/11] git-gui: allow specifying path '.' to the browser
From: Johannes Sixt @ 2026-05-15 15:54 UTC (permalink / raw)
  To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260514143322.865587-2-mlevedahl@gmail.com>

Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> Invoking "git-gui browser rev ." should show the file browser for the
> commitish rev, starting at the root directory. This errors out in
> normalize_relpath because the '.' is removed, yielding an empty list as
> argument to [file join ...]. Fix this.

Good catch!

The description isn't precise, though. '.' means to list the current
directory. The mentioned problem happens only if this is also the root
of the working tree.

> 
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 23fe76e..6048f92 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2965,7 +2965,11 @@ proc normalize_relpath {path} {
>  		}
>  		lappend elements $item
>  	}
> -	return [eval file join $elements]
> +	if {$elements ne {}} {
> +		return [eval file join $elements]
> +	} else {
> +		return {}
> +	}
>  }
>  
>  # -- Not a normal commit type invocation?  Do that instead!


-- Hannes


^ permalink raw reply

* Re: [PATCH v1 02/11] git-gui: refactor browser / blame argument parsing
From: Johannes Sixt @ 2026-05-15 15:56 UTC (permalink / raw)
  To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260514143322.865587-3-mlevedahl@gmail.com>

Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> git-gui has subcommands blame and browser, both of which accept a
> pathname, possibly preceded by a commit-ish item to specify a revision.
> Also, blame can take a first argument that gives a line number to focus.
> 
> The command line parser for the above is more complex than needed, and
> cannot work without a worktree as the pathname objects are checked
> against the current worktree for existence. This also precludes naming a
> directory or file that does not exist on the currently checked out
> branch.

While the old browser isn't simple, it implements the strategy "revs
before paths, no revs after the first path or '--'" that is applied by
every git command. The rewritten parser is only slightly simpler. It
should not ignore "--". Furthermore, the old parser ignored excessive
trailing arguments, while the new parser ignores excessive leading
arguments. Neither is desirable, and we should report an incorrect
argument list (if we don't, then I prefer the old behavior).

> 
> So, replace this with a simpler parser that looks at argument number and
> number of arguments to know what value to expect. The blame and browser
> backends already have error checking with diagnostic information, so
> defer most error checking to those. Also, allow a line-number selection
> to be given and silently ignored for the browser, further simplifying
> this code.

The line number selection isn't ignored by the browser, but reported as
an incorrect usage before and after this patch.

> 
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 66 +++++++++++++-----------------------------------------
>  1 file changed, 16 insertions(+), 50 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 6048f92..a951fcd 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2986,51 +2986,34 @@ blame {
>  	set head {}
>  	set path {}
>  	set jump_spec {}
> -	set is_path 0
> +	set nargs [llength $argv]
> +	if {$nargs < 1} {
> +		usage
> +	}
> +	set argn 0
>  	foreach a $argv {
> -		set p [file join $_prefix $a]
> +		set argn [expr {$argn + 1}]
>  
> -		if {$is_path || [file exists $p]} {
> -			if {$path ne {}} usage
> -			set path [normalize_relpath $p]
> -			break
> -		} elseif {$a eq {--}} {
> -			if {$path ne {}} {
> -				if {$head ne {}} usage
> -				set head $path
> -				set path {}
> +		if {$argn < $nargs} {
> +			# revision or line number
> +			if {[regexp {^--line=(\d+)$} $a a lnum]} {
> +				set jump_spec [list $lnum]
> +			} else {
> +				set head $a
>  			}
> -			set is_path 1
> -		} elseif {[regexp {^--line=(\d+)$} $a a lnum]} {
> -			if {$jump_spec ne {} || $head ne {}} usage
> -			set jump_spec [list $lnum]
> -		} elseif {$head eq {}} {
> -			if {$head ne {}} usage
> -			set head $a
> -			set is_path 1
> -		} else {
> -			usage
> -		}
> -	}
> -	unset is_path
> -
> -	if {$head ne {} && $path eq {}} {
> -		if {[string index $head 0] eq {/}} {
> -			set path [normalize_relpath $head]
> -			set head {}
>  		} else {
> -			set path [normalize_relpath $_prefix$head]
> -			set head {}
> +			set path [normalize_relpath $a]

This loses the capability to request a browser relative to a
subdirectory of the working tree. For example, "git gui browser main ."
now shows the top-level directory instead of the subdirectory when
invoked from a subdirectory.

>  		}
>  	}
>  
>  	if {$head eq {}} {
>  		load_current_branch
> +		set head $current_branch
>  	} else {
>  		if {[regexp [string map "@@ [expr $hashlength - 1]" {^[0-9a-f]{1,@@}$}] $head]} {
>  			if {[catch {
> -					set head [git rev-parse --verify $head]
> -				} err]} {
> +				set head [git rev-parse --verify $head]
> +			} err]} {

Please leave the indentation unchanged.

>  				if {[tk windowingsystem] eq "win32"} {
>  					tk_messageBox -icon error -title [mc Error] -message $err
>  				} else {
> @@ -3046,26 +3029,9 @@ blame {
>  	switch -- $subcommand {
>  	browser {
>  		if {$jump_spec ne {}} usage
> -		if {$head eq {}} {
> -			if {$path ne {} && [file isdirectory $path]} {
> -				set head $current_branch
> -			} else {
> -				set head $path
> -				set path {}
> -			}
> -		}
>  		browser::new $head $path
>  	}
>  	blame   {
> -		if {$head eq {} && ![file exists $path]} {
> -			catch {wm withdraw .}
> -			tk_messageBox \
> -				-icon error \
> -				-type ok \
> -				-title [mc "git-gui: fatal error"] \
> -				-message [mc "fatal: cannot stat path %s: No such file or directory" $path]
> -			exit 1
> -		}
>  		blame::new $head $path $jump_spec
>  	}
>  	}

The check for the existence of files is actually necessary to
disambiguate the meaning of the argument. If a file "maint" exists, then
the argument is to be interpreted as path, not as the ref "maint", even
if that exists, too.

I suggest to protect the "file exists" calls with ($_gitworktree ne {}
&& ...) or (![is_bare] && ...) to handle being invoked from a bare
repository. That is, in a bare repository we treat arguments the same as
files that do not exist in the currently checked-out branch.

-- Hannes


^ permalink raw reply

* Re: [PATCH v1 03/11] git-gui: guard set/unset of GIT_DIR and GIT_WORK_TREE
From: Johannes Sixt @ 2026-05-15 15:58 UTC (permalink / raw)
  To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260514143322.865587-4-mlevedahl@gmail.com>

Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> git-gui unconditionally exports GIT_DIR and GIT_WORK_TREE to the
> environment, and furthmore unconditionally unsets these in many places.
> But, GIT_WORK_TREE should be set only if it is not {} as the empty
> value, really meaning no work-tree is found, causes git to throw fatal
> errors (git-gui gets the error from branch --show-current).  Fixing this
> is required to allow blame and browser to operate from a repository
> without a worktree.
> 
> Establish a pair of functions to remove GIT_DIR and GIT_WORK_TREE from
> the environment, avoiding any error if they do not exist. Also, add a
> function to export these, but export GIT_WORK_TREE only if not empty.

Good. But as I said in a parallel thread, I actually concur with your
assessment in the coverletter of this patch series that GIT_WORK_TREE
should be not set at all. At least in the modes that require a working tree.

> 
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index a951fcd..387cad6 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1122,6 +1122,22 @@ unset argv0dir
>  ##
>  ## repository setup
>  
> +proc set_gitdir_vars {} {
> +	global _gitdir _gitworktree env
> +	if {$_gitdir ne {}} {
> +		set env(GIT_DIR) $_gitdir
> +	}
> +	if {$_gitworktree ne {}} {
> +		set env(GIT_WORK_TREE) $_gitworktree
> +	}
> +}
> +
> +proc unset_gitdir_vars {} {
> +	global env
> +	catch {unset env(GIT_DIR)}
> +	catch {unset env(GIT_WORK_TREE)}
> +}
> +
>  set picked 0
>  if {[catch {
>  		set _gitdir $env(GIT_DIR)
> @@ -1207,8 +1223,8 @@ if {[lindex $_reponame end] eq {.git}} {
>  	set _reponame [lindex $_reponame end]
>  }
>  
> -set env(GIT_DIR) $_gitdir
> -set env(GIT_WORK_TREE) $_gitworktree
> +# Export the final paths
> +set_gitdir_vars
>  
>  ######################################################################
>  ##
> @@ -2050,13 +2066,11 @@ proc do_gitk {revs {is_submodule false}} {
>  			# TODO we could make life easier (start up faster?) for gitk
>  			# by setting these to the appropriate values to allow gitk
>  			# to skip the heuristics to find their proper value
> -			unset env(GIT_DIR)
> -			unset env(GIT_WORK_TREE)
> +			unset_gitdir_vars
>  		}
>  		safe_exec_bg [concat $cmd $revs "--" "--"]
>  
> -		set env(GIT_DIR) $_gitdir
> -		set env(GIT_WORK_TREE) $_gitworktree
> +		set_gitdir_vars
>  		cd $pwd
>  
>  		if {[info exists main_status]} {
> @@ -2084,16 +2098,14 @@ proc do_git_gui {} {
>  
>  		# see note in do_gitk about unsetting these vars when
>  		# running tools in a submodule
> -		unset env(GIT_DIR)
> -		unset env(GIT_WORK_TREE)
> +		unset_gitdir_vars
>  
>  		set pwd [pwd]
>  		cd $current_diff_path
>  
>  		safe_exec_bg [concat $exe gui]
>  
> -		set env(GIT_DIR) $_gitdir
> -		set env(GIT_WORK_TREE) $_gitworktree
> +		set_gitdir_vars
>  		cd $pwd
>  
>  		set status_operation [$::main_status \

After these changes, a 'global env' probably becomes stale and could be
removed.

-- Hannes


^ permalink raw reply

* Re: [PATCH v1 04/11] git-gui: put choose_repository::pick in a proc
From: Johannes Sixt @ 2026-05-15 15:59 UTC (permalink / raw)
  To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260514143322.865587-5-mlevedahl@gmail.com>

Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> git-gui includes a 'repository picker', which allows creating a new
> repository + worktree, or selecting a worktree from a recent list.
> git-gui runs the picker when a valid git repository is not found. All of
> the code for this is embedded in the discovery process block, making the
> latter more difficult to read, and also making things more difficult if
> we want to have an explicit 'pick' subcommand to force this to run.

OK, let's see how useful this becomes.

> 
> Let's move this invocation and supporting code to a separate proc,
> aiding in subsequent refactoring. Assure GIT_DIR and GIT_WORK_TREE are
> unset, configuration is loaded, ant that _gitdir is correctly set

s/ant/and/

> afterwards. As this is invoked before worktree discovery, later code
> will set that anyway so need not be included here.
> 
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 387cad6..0b73c35 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1139,6 +1139,16 @@ proc unset_gitdir_vars {} {
>  }
>  
>  set picked 0
> +proc pick_repo {} {
> +	unset_gitdir_vars
> +	load_config 1
> +	apply_config
> +	choose_repository::pick
> +	set _gitdir [git rev-parse --absolute-git-dir]
> +	set _prefix {}
> +	set picked 1
> +}
> +

So, this isn't intended as a plain move of code? Since we set _gitdir
here, we could remove the corresonding lines from lib/choose_repository.tcl.

Is the variable "picked" only needed for this particular picker
invocation? Then it should not be set in the function, but at the call site.

>  if {[catch {
>  		set _gitdir $env(GIT_DIR)
>  		set _prefix {}
> @@ -1149,13 +1159,7 @@ if {[catch {
>  		set _gitdir [git rev-parse --git-dir]
>  		set _prefix [git rev-parse --show-prefix]
>  	} err]} {
> -	load_config 1
> -	apply_config
> -	choose_repository::pick
> -	if {![file isdirectory $_gitdir]} {
> -		exit 1
> -	}
> -	set picked 1
> +		pick_repo

The indentation is off here.

>  }
>  
>  # Use object format as hash algorithm (either "sha1" or "sha256")

-- Hannes


^ permalink raw reply

* Re: [PATCH v1 05/11] git-gui: use --absolute-git-dir
From: Johannes Sixt @ 2026-05-15 16:00 UTC (permalink / raw)
  To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260514143322.865587-6-mlevedahl@gmail.com>

Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> git-gui uses git rev-parse --git-dir to get the pathname of the
> discovered git repository. The returned value can be relative, and is
> '.' if the current directory is the top of the repository directory
> itself.  git-gui has code to change '.' to [pwd] in this case so that
> subsequent logic runs.
> 
> But, git rev-parse supports --absolute-git-dir from fac60b8925
> ("rev-parse: add option for absolute or relative path formatting",
> 2020-12-13), and included in git 2.31. git-gui requires git >= 2.36, so
> this more useful form is always available. Use --absolute-git-dir to
> always get an absolute path, avoiding the need for other checks.

Nice!

However, the patch is incomplete. We set _gitdir also from
lib/choose_repository.tcl. I think it would be best to swap this patch
with patch 4/11, remove the _gitdir setters from the picker
implementation, and call `rev-parse --absolute-git-dir` like you did in
4/11. This depends on that the picker sets the current directory to the
top-level of the working tree with the embeded .git directory.

BTW, missing sign-off.

> ---
>  git-gui.sh | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 0b73c35..c2cf5f1 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1156,7 +1156,7 @@ if {[catch {
>  	&& [catch {
>  		# beware that from the .git dir this sets _gitdir to .
>  		# and _prefix to the empty string
> -		set _gitdir [git rev-parse --git-dir]
> +		set _gitdir [git rev-parse --absolute-git-dir]
>  		set _prefix [git rev-parse --show-prefix]
>  	} err]} {
>  		pick_repo
> @@ -1173,18 +1173,12 @@ if {$hashalgorithm eq "sha1"} {
>  	exit 1
>  }
>  
> -# we expand the _gitdir when it's just a single dot (i.e. when we're being
> -# run from the .git dir itself) lest the routines to find the worktree
> -# get confused
> -if {$_gitdir eq "."} {
> -	set _gitdir [pwd]
> -}
> -
>  if {![file isdirectory $_gitdir]} {
>  	catch {wm withdraw .}
>  	error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"]
>  	exit 1
>  }
> +
>  # _gitdir exists, so try loading the config
>  load_config 0
>  apply_config

-- Hannes


^ permalink raw reply

* Re: [PATCH v1 07/11] git-gui: use rev-parse exclusively to find a repository
From: Johannes Sixt @ 2026-05-15 16:06 UTC (permalink / raw)
  To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260514143322.865587-8-mlevedahl@gmail.com>

Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> git-gui attempts to use env(GIT_DIR) directly as the git repository,
> accepting GIT_DIR if it is a directory. Only if that fails is git
> rev-parse used to discover the repository.  But, this avoids all of
> git-core's validity checking on a repository, thus possibly deferring an
> error to a later step, possibly unexpected. Repository validation should
> be part of initial setup so that later processing does not need error
> trapping for configuration errors.

OK. If the user gave us GIT_DIR with our without GIT_WORK_TREE, then
that combination better be workable.

> 
> Let's just invoke rev-parse so all error checking is done. Stop here if
> the user set GIT_DIR or GIT_WORK_TREE. Otherwise, continue the existing
> behavior and show the repository picker.

OK. But the paragraph is confusing, because a big "If an error occurs"
is missing after the first sentence.

> 
> Also, remove a later check on whether _gitdir is a directory: that code
> cannot be reached without rev-parse having validating the repository.

Good.

> 
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 2e2ddc0..81789dd 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -374,6 +374,7 @@ set _gitdir {}
>  set _gitworktree {}
>  set _isbare {}
>  set _githtmldir {}
> +set _prefix {}
>  set _reponame {}
>  set _shellpath {@@SHELL_PATH@@}
>  
> @@ -1167,19 +1168,18 @@ proc pick_repo {} {
>  	set picked 1
>  }
>  
> +# find repository.
>  if {[catch {
> -		set _gitdir $env(GIT_DIR)
> -		set _prefix {}
> -		}]
> -	&& [catch {
> -		# beware that from the .git dir this sets _gitdir to .
> -		# and _prefix to the empty string
> -		set _gitdir [git rev-parse --absolute-git-dir]
> -		set _prefix [git rev-parse --show-prefix]
> -	} err]} {
> +	set _gitdir [git rev-parse --absolute-git-dir]

Please do also set _prefix. It should fix the bug that the file chooser
uses an empty prefix after

cd lib
GIT_DIR=$PWD/../.git GIT_WORK_TREE=$PWD/.. ../git-gui.sh browser master .

(this is an old bug.)

Please keep the additional indentation of the catch body.

> +} err]} {
> +	if {[is_gitvars_error $err]} {
> +		exit 1
> +	} else {
>  		pick_repo
> +	}

Treat the 'if' as an early exist without an else, and we don't need the
previously strange indentation of 'pick_repo'.

>  }
>  
> +
>  # Use object format as hash algorithm (either "sha1" or "sha256")
>  set hashalgorithm [git rev-parse --show-object-format]
>  if {$hashalgorithm eq "sha1"} {
> @@ -1191,12 +1191,6 @@ if {$hashalgorithm eq "sha1"} {
>  	exit 1
>  }
>  
> -if {![file isdirectory $_gitdir]} {
> -	catch {wm withdraw .}
> -	error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"]
> -	exit 1
> -}
> -
>  # _gitdir exists, so try loading the config
>  load_config 0
>  apply_config

(Stopping the review here for today.)

-- Hannes


^ permalink raw reply

* Re: [PATCH 2/2] use __builtin_add_overflow() in st_add() with Clang
From: René Scharfe @ 2026-05-15 16:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List, Jeff King
In-Reply-To: <fceded1f-60a2-48d2-91fc-5d2161272868@web.de>

On 5/14/26 10:17 PM, René Scharfe wrote:
> On 5/14/26 9:12 PM, Junio C Hamano wrote:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Provide a variant of st_add() that wraps __builtin_add_overflow() to
>>> help Clang optimize it.  Use it on all platforms for simplicity.
>>> ...
>>> +/* Help Clang; GCC generates the same code for both variants. */
>>> +#if defined(__clang__)
>>> +static inline size_t st_add(size_t a, size_t b)
>>> +{
>>> +	size_t sum;
>>> +	if (__builtin_add_overflow(a, b, &sum))
>>> +		die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
>>> +		    (uintmax_t)a, (uintmax_t)b);
>>> +	return sum;
>>> +}
>>> +#else
>>>  static inline size_t st_add(size_t a, size_t b)
>>>  {
>>>  	if (unsigned_add_overflows(a, b))
>>> @@ -621,6 +632,7 @@ static inline size_t st_add(size_t a, size_t b)
>>>  		    (uintmax_t)a, (uintmax_t)b);
>>>  	return a + b;
>>>  }
>>> +#endif
>>
>> Makes me wonder if we tweaked unsigned_add_overflows() to take an
>> extra *dst parameter to match __builtin_add_overflow(), which of
>> course requires us to all of 18 callsites, it might make the whole
>> thing a bit simpler.  New uses of unsigned_add_overflows(), if we
>> ever add them, would automatically benefit, right?
> 
> Hmm.  It sounds like a lot of churn, but it would make sure that
> we use the checked result and not check a + b and then go on and
> use x + y because the code de-synced at some point.
> 
> How to do it, though?  It needs to be generic and evaluate its
> arguments only once.  Perhaps like this?
> 
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index ca89cfb0b3..27fbb622d7 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -103,6 +103,21 @@ struct strbuf;
>  #define unsigned_add_overflows(a, b) \
>      ((b) > maximum_unsigned_value_of_type(a) - (a))
>  
> +static bool uint_add_overflow(uintmax_t a, uintmax_t b,
> +			      uintmax_t *out, size_t out_size)
> +{
> +	if (b > UINTMAX_MAX - a)
> +		return true;
> +	a += b;
> +	if (a > (UINTMAX_MAX >> (bitsizeof(uintmax_t) - CHAR_BIT * out_size)))
> +		return true;
> +	*out = a;
> +	return false;
> +}
> +
> +#define UINT_ADD_OVERFLOW(a, b, out) \
> +	uint_add_overflow((a), (b), (out), sizeof(a))
> +
>  /*
>   * Returns true if the multiplication of "a" and "b" will
>   * overflow. The types of "a" and "b" must match and must be unsigned.
> @@ -616,10 +631,11 @@ int git_open_cloexec(const char *name, int flags);
>  
>  static inline size_t st_add(size_t a, size_t b)
>  {
> -	if (unsigned_add_overflows(a, b))
> +	size_t ret;
> +	if (UINT_ADD_OVERFLOW(a, b, &ret))

Type mismatch of third argument: pointer to size_t given, pointer to
uintmax_t expected.

>  		die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
>  		    (uintmax_t)a, (uintmax_t)b);
> -	return a + b;
> +	return ret;
>  }
>  #define st_add3(a,b,c)   st_add(st_add((a),(b)),(c))
>  #define st_add4(a,b,c,d) st_add(st_add3((a),(b),(c)),(d))
Perhaps like this instead?


diff --git a/git-compat-util.h b/git-compat-util.h
index ae1bdc90a4..23ea42f373 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -103,6 +103,25 @@ struct strbuf;
 #define unsigned_add_overflows(a, b) \
     ((b) > maximum_unsigned_value_of_type(a) - (a))
 
+static inline uintmax_t uint_add_overflow(uintmax_t a, uintmax_t b,
+					  uintmax_t max, bool *overflow)
+{
+	*overflow = a > max || b > max - a;
+	return a + b;
+}
+
+#ifdef __clang__
+#define UINT_ADD_OVERFLOW(a, b, out, overflow) \
+	(*(overflow) = __builtin_add_overflow((a), (b), (out)))
+#else
+#define UINT_ADD_OVERFLOW(a, b, out, overflow) ( \
+	*(out) = uint_add_overflow((a), (b), \
+				   maximum_unsigned_value_of_type(*(out)), \
+				   (overflow)), \
+	*(overflow) \
+)
+#endif
+
 /*
  * Returns true if the multiplication of "a" and "b" will
  * overflow. The types of "a" and "b" must match and must be unsigned.
@@ -616,10 +635,12 @@ int git_open_cloexec(const char *name, int flags);
 
 static inline size_t st_add(size_t a, size_t b)
 {
-	if (unsigned_add_overflows(a, b))
+	bool overflow;
+	size_t ret;
+	if (UINT_ADD_OVERFLOW(a, b, &ret, &overflow))
 		die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
 		    (uintmax_t)a, (uintmax_t)b);
-	return a + b;
+	return ret;
 }
 #define st_add3(a,b,c)   st_add(st_add((a),(b)),(c))
 #define st_add4(a,b,c,d) st_add(st_add3((a),(b),(c)),(d))


^ permalink raw reply related

* Re: [PATCH 1/2] strbuf: use st_add3() in strbuf_grow()
From: Jeff King @ 2026-05-15 16:50 UTC (permalink / raw)
  To: René Scharfe; +Cc: Junio C Hamano, Git List
In-Reply-To: <459f5f2b-2565-4dae-9f9f-8848a5cb9d94@web.de>

On Fri, May 15, 2026 at 04:30:34PM +0200, René Scharfe wrote:

> > That's all assuming that no overflow happens before ALLOC_GROW() gets
> > the values. We also tend to do unchecked computions for the "nr" field
> > there, but it's usually just "nr_foo + 1", so the same logic applies:
> > you'd have to have an existing array consuming the entire address space
> > minus one byte to trigger an overflow.
>
> The use in read-cache.c::do_read_index() looks odd.  Has been present
> since commit one.  Is the point that it over-allocates to have room for
> additions right from the start?  For read-only commands this only wastes
> memory, no?

Hmm, yeah, that is weird, and unusual to use alloc_nr() directly. We are
presumably picking up istate->cache_nr from the on-disk file, so it
could be anything, and that alloc_nr() could overflow.

We'd store the too-small value in alloc, so we _know_ it's too small. So
later when we use ALLOC_GROW(), the problem would be resolved as we grow
the array. I'm not convinced the initial load might not overflow the
array, though.

-Peff

^ permalink raw reply

* Re: [PATCH 2/2] use __builtin_add_overflow() in st_add() with Clang
From: Jeff King @ 2026-05-15 16:53 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git List
In-Reply-To: <26b71f9c-0cc5-4bfd-9175-f45b584e202e@web.de>

On Fri, May 15, 2026 at 04:36:15PM +0200, René Scharfe wrote:

> > I guess the ideal primitive is probably a wrapper with the same
> > interface as __builtin_add_overflow(), which could then be used
> > everywhere that unsigned_add_overflows() with some minor conversion.
> 
> Junio said the same. :)
> > But it gets awkward to do as a macro, and using an inline function runs
> > into type questions.
> Indeed.  If it was easy then this wouldn't exist as a builtin.  We
> can approximate it somewhat, but will it be robust enough?

I always thought it was a builtin because the most efficient way
involves checking the carry flag, which can't be accessed from C.

But yeah, the type issues are real, too. ;)

I think we should take what you posted for now, and we can iterate on a
more general add-and-check interface later (or never if it's too
tricky).

-Peff

^ permalink raw reply

* Re: [PATCH] http: handle absolute-path alternates from server root
From: Jeff King @ 2026-05-15 17:01 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: Junio C Hamano, git, slonkazoid
In-Reply-To: <agbOEsZ8NmE8SyfV@pks.im>

On Fri, May 15, 2026 at 09:41:06AM +0200, Patrick Steinhardt wrote:

> > We talked about dropping it a few years ago, but Eric countered that
> > dumb clones are easier on the server in some cases (like gigantic
> > public-inbox repos that are packed to keep most of the old history in
> > one big pack that is never updated). The verbatim pack-reuse feature
> > tries to get smart clones closer to that, but it's hard to beat serving
> > a static file from the server's perspective. I haven't measured anything
> > in that area in a while, though.
> 
> In theory we can get much closer with packfile URIs, too, can't we? If
> the packfiles are directly accessible anyway the server could just
> announce these directly and have the client fetch them. That should
> significantly reduce the load on the server even further.

Packfile URIs help with the actual pack generation (even if we're
blitting out bits from the disk with verbatim packfile reuse, we still
have to handle gaps and compute the checksum over the output pack).

But it doesn't help with the server computing the set of objects the
client needs in the first place. IIRC, packfile URIs work by the server
saying "oh, I was going to send you object XYZ, but you can get it from
this stable pack instead". So the server still has to compute the set of
objects (and send any that are not mentioned in URI packs). Bitmaps
help, but there's still non-trivial computation and storage on the
server.

Contrast that with a client that instead pulls a packfile over dumb
storage on its own, and then comes to the server for a top-off fetch.
The server still has to do some computation, but it's usually quite
small, because both sides agree quickly that there's no need to dig down
further than the tips in that dumb packfile.

> Of course, the big downside is that "fetch.uriProtocols" is empty by
> default, so Git will not use them. Makes me wonder whether this is
> something we want to eventually change, but I guess the current default
> behaviour is somewhat insecure as it would allow the server to redirect
> clients to arbitrary locations. It would be great if we had a mechanism
> that only allowed packfile URIs that use the same host, which would make
> this a lot more reasonable to enable by default.

It's been a while since I've looked at it, but I seem to recall that the
server-side tools for specifying which packfile URIs to use were not
that mature. Maybe that has changed, though (I'm probably 5 years out of
date since the last time I really thought about these things).

-Peff

^ permalink raw reply

* Re: What's cooking in git.git (May 2026, #03)
From: Junio C Hamano @ 2026-05-15 17:06 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git
In-Reply-To: <CAOLa=ZS8a2R79+8hD-r1JpJmHUatHo4VEC=ybjf02jLEHWsRoQ@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

> Hello Junio,
>
> I've not been active on the list past few weeks, did we reach a
> consensus about
> 20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com ? Or was
> it missed, I thought it was in a ready state, but happy to reiterate as
> needed.
>
> Lore: https://lore.kernel.org/git/20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com/#t

I've not been active on the list past few weeks, either, so please
don't expect me to know anything that happened during my 3-week
absense ;-)

My understanding of the status of that thread is that after

https://lore.kernel.org/git/CAOLa=ZT1zE+MLeaYE_5jWmNzSvtTTBw3ZAopai+2Ei27kmYm2g@mail.gmail.com/

that said you "Will add ... locally", we are all waiting for you to
say either "after waiting for sufficient amount of time, there
wasn't any other major change necessary, so I won't add it locally
after all" or "we have waited for sufficient amount of time, so here
is the hopefully final edition that includes what I added locally
following Patrick's review".


^ permalink raw reply

* Re: [PATCH v3 5/8] environment: move "precomposed_unicode" into `struct repo_config_values`
From: Tian Yuchen @ 2026-05-15 17:15 UTC (permalink / raw)
  To: Olamide Caleb Bello, git
  Cc: phillip.wood123, gitster, christian.couder, usmanakinyemi202,
	kaartic.sivaraam, me
In-Reply-To: <20260423165432.143598-6-belkid98@gmail.com>

On 4/24/26 00:54, Olamide Caleb Bello wrote:
>   struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *prec_dir)
>   {
> +	struct repo_config_values *cfg = repo_config_values(the_repository);
>   	struct dirent *res;
> +
>   	res = readdir(prec_dir->dirp);
>   	if (res) {
>   		size_t namelenz = strlen(res->d_name) + 1; /* \0 */
This 'precompose_utf8_readdir()' appears to be a wrapper for 
'readdir()', so that Git thinks it is calling a standard POSIX function. 
Looking at it this way, I feel the architectural design here is a bit 
ambiguous. I think the role of 'readdir()' (or its wrapper) should be: 
"You give me a directory handle, and I give you a file entry". 
Conceptually, it shouldn’t even be related to Git: you can take a look 
at the rest of code of this wrapper.

Since there is a wrapper for 'readdir()', there must also be one for 
'opendir()'. To me, initializing a flag like 'perform_precompose' when 
the PREC_DIR handle is created in 'opendir()' , and pass it down through 
the handle looks a bit more optimal. In other words, If one of them has 
to handle the repo pointer, I think it would be better to leave that to 
'opendir()' ;)

Good night, yuchen

^ permalink raw reply

* [PATCH] commit-reach: use the decoration hash for tips_reachable_from_bases()
From: Kristofer Karlsson via GitGitGadget @ 2026-05-15 18:07 UTC (permalink / raw)
  To: git; +Cc: Kristofer Karlsson, Kristofer Karlsson

From: Kristofer Karlsson <krka@spotify.com>

tips_reachable_from_bases() walks the commit graph from a set of base
commits to find which tip commits are reachable.  The inner loop does
a linear scan over the tips array to check whether each visited commit
is a tip, making the overall cost O(C * T) where C is commits walked
and T is the number of tips.

Replace the linear scan with the decoration hash for lookups, reducing
the per-commit tip check from O(T) to O(1) and the overall cost from
O(C * T) to O(C + T).

This function is called by `git for-each-ref --merged` and
`git branch/tag --contains/--no-contains` via reach_filter() in
ref-filter.c.

Benchmark on a merge-heavy monorepo (2.3M commits, 10,000 refs):

  Command                           Before    After   Speedup
  for-each-ref --merged HEAD        6.64s     1.66s     4.0x
  for-each-ref --no-merged HEAD     6.75s     1.74s     3.9x
  branch --merged HEAD              0.68s     0.61s      10%
  branch --no-merged HEAD           0.65s     0.61s       8%
  tag --merged HEAD                 0.12s     0.12s       -

The large speedup for for-each-ref is because it checks all 10,000
refs as tips, making the O(T) inner loop expensive.  The branch
subcommand only checks local branches (fewer tips), so the improvement
is smaller.

Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
    commit-reach: use the decoration hash for tips_reachable_from_bases()
    
    This is a single small commit that replaces an O(C*T) linear scan in
    tips_reachable_from_bases() with an O(1) lookup using the decoration
    hash.
    
    The function is called by git for-each-ref --merged and git branch/tag
    --contains/--no-contains via reach_filter() in ref-filter.c. On a
    merge-heavy monorepo with 2.3M commits and 10,000 refs, git for-each-ref
    --merged HEAD goes from 6.6s to 1.7s (4x).
    
    The diff is intentionally minimal (+9/-6) to make the idea easy to
    discuss before polishing. Things I'm not fully happy about:
    
     * Extra block scope { } just to preserve indentation of the inner body
     * Hacking the array index into the decoration value as (void *)(i + 1)
       instead of storing a proper pointer
     * Relying on unsigned wraparound (- 1 on a size_t 0) to check for
       not-found via j < tips_nr
    
    Happy to clean all of these up in a follow-up commit if the approach
    makes sense.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2116%2Fspkrka%2Ftips-reachable-minimal-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2116/spkrka/tips-reachable-minimal-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2116

 commit-reach.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/commit-reach.c b/commit-reach.c
index d3a9b3ed6f..70b056eae0 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -1150,6 +1150,7 @@ void tips_reachable_from_bases(struct repository *r,
 	size_t min_generation_index = 0;
 	timestamp_t min_generation;
 	struct commit_list *stack = NULL;
+	struct decoration tip_index = { "tip_index" };
 
 	if (!bases || !tips || !tips_nr)
 		return;
@@ -1173,6 +1174,10 @@ void tips_reachable_from_bases(struct repository *r,
 	QSORT(commits, tips_nr, compare_commit_and_index_by_generation);
 	min_generation = commits[0].generation;
 
+	for (size_t i = 0; i < tips_nr; i++)
+		add_decoration(&tip_index, &commits[i].commit->object,
+			       (void *)(i + 1));
+
 	while (bases) {
 		repo_parse_commit(r, bases->item);
 		commit_list_insert(bases->item, &stack);
@@ -1183,14 +1188,11 @@ void tips_reachable_from_bases(struct repository *r,
 		int explored_all_parents = 1;
 		struct commit_list *p;
 		struct commit *c = stack->item;
-		timestamp_t c_gen = commit_graph_generation(c);
 
 		/* Does it match any of our tips? */
-		for (size_t j = min_generation_index; j < tips_nr; j++) {
-			if (c_gen < commits[j].generation)
-				break;
-
-			if (commits[j].commit == c) {
+		{
+			size_t j = (size_t)lookup_decoration(&tip_index, &c->object) - 1;
+			if (j < tips_nr) {
 				tips[commits[j].index]->object.flags |= mark;
 
 				if (j == min_generation_index) {
@@ -1232,6 +1234,7 @@ void tips_reachable_from_bases(struct repository *r,
 	}
 
 done:
+	clear_decoration(&tip_index, NULL);
 	free(commits);
 	repo_clear_commit_marks(r, SEEN);
 	commit_list_free(stack);

base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
-- 
gitgitgadget

^ permalink raw reply related

* [PATCH] evaluate the second argument of ALLOC_GROW only once
From: René Scharfe @ 2026-05-15 18:16 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

Allow the new element count passed to ALLOC_GROW to be a complex
expression with side-effects by evaluating it only once, as a parameter
to a new helper function.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 git-compat-util.h | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index ae1bdc90a4..2bc1f43f48 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -812,6 +812,16 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)
 
 #define alloc_nr(x) (((x)+16)*3/2)
 
+static inline bool st_alloc_nr(size_t nr, size_t alloc, size_t *outp)
+{
+	if (nr > alloc) {
+		size_t out = alloc_nr(alloc);
+		*outp = out < nr ? nr : out;
+		return true;
+	}
+	return false;
+}
+
 /**
  * Dynamically growing an array using realloc() is error prone and boring.
  *
@@ -857,12 +867,10 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)
  */
 #define ALLOC_GROW(x, nr, alloc) \
 	do { \
-		if ((nr) > alloc) { \
-			if (alloc_nr(alloc) < (nr)) \
-				alloc = (nr); \
-			else \
-				alloc = alloc_nr(alloc); \
-			REALLOC_ARRAY(x, alloc); \
+		size_t alloc_grow_new_alloc_; \
+		if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
+			alloc = alloc_grow_new_alloc_; \
+			REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
 		} \
 	} while (0)
 
-- 
2.54.0

^ permalink raw reply related

* Re: [PATCH] evaluate the second argument of ALLOC_GROW only once
From: Jeff King @ 2026-05-15 19:08 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <323f5677-301b-4d7a-b552-6606597c2b1f@web.de>

On Fri, May 15, 2026 at 08:16:50PM +0200, René Scharfe wrote:

> +		size_t alloc_grow_new_alloc_; \
> +		if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
> +			alloc = alloc_grow_new_alloc_; \
> +			REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
>  		} \

What happens if a caller passes in an argument that isn't a size_t?
We'll check for overflow in the size_t space, and then truncate it when
we assign to alloc, I think.

I think we generally try to hold allocations in size_t these days, but
I'd be surprised if there weren't a few "int" holdouts. Grepping around,
alloc_node() seems to be an example.

BTW, non-size_t arguments nullifies my earlier hand-waving around "nr +
1 overflowing implies we've filled up the address space". But we are
still protected in the existing code by the:

  if (alloc_nr(alloc) < (nr))
	alloc = (nr);

logic. But with your patch, that all happens in the size_t space, so I
think it would actually introduce possible array overflows when the
caller is using a smaller type.

-Peff

^ permalink raw reply

* Re: [PATCH] evaluate the second argument of ALLOC_GROW only once
From: Jeff King @ 2026-05-15 19:50 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <20260515190818.GA98370@coredump.intra.peff.net>

On Fri, May 15, 2026 at 03:08:18PM -0400, Jeff King wrote:

> On Fri, May 15, 2026 at 08:16:50PM +0200, René Scharfe wrote:
> 
> > +		size_t alloc_grow_new_alloc_; \
> > +		if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
> > +			alloc = alloc_grow_new_alloc_; \
> > +			REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
> >  		} \
> 
> What happens if a caller passes in an argument that isn't a size_t?
> We'll check for overflow in the size_t space, and then truncate it when
> we assign to alloc, I think.
> 
> I think we generally try to hold allocations in size_t these days, but
> I'd be surprised if there weren't a few "int" holdouts. Grepping around,
> alloc_node() seems to be an example.
> 
> BTW, non-size_t arguments nullifies my earlier hand-waving around "nr +
> 1 overflowing implies we've filled up the address space". But we are
> still protected in the existing code by the:
> 
>   if (alloc_nr(alloc) < (nr))
> 	alloc = (nr);
> 
> logic. But with your patch, that all happens in the size_t space, so I
> think it would actually introduce possible array overflows when the
> caller is using a smaller type.

Hmm, playing with it and looking a little closer, I think we don't end
up overflowing the buffer because you use the size_t for
REALLOC_ARRAY(). So the result is big, but then "alloc" is truncated.

And then on the next call, we think "oh no, the allocation is way too
small" because we are using the truncated value. So we try to size up
for every single allocation, even though it's actually big enough, and
the program slows to a crawl. ;)

For reference, IU was using this hack to play around and demonstrate:

diff --git a/git.c b/git.c
index 5a40eab8a2..638bbc69e4 100644
--- a/git.c
+++ b/git.c
@@ -969,6 +969,19 @@ int cmd_main(int argc, const char **argv)
 
 	cmd = argv[0];
 
+	if (!strcmp(cmd, "foo")) {
+		unsigned char *buf = NULL;
+		unsigned nr = 0, alloc = 0;
+		for (unsigned i = 0; i < UINT_MAX; i++) {
+			ALLOC_GROW(buf, nr + 1, alloc);
+			if (i % 313370 == 0)
+				warning("at i=%u, alloc=%u, nr=%u", i, alloc, nr);
+			buf[nr++] = i % 256;
+		}
+		printf("done, final nr=%u, alloc=%u\n", nr, alloc);
+		return 0;
+	}
+
 	/*
 	 * We use PATH to find git commands, but we prepend some higher
 	 * precedence paths: the "--exec-path" option, the GIT_EXEC_PATH


The same problem exists in several places in actual code, but I'm not
sure how practical it is to trigger. The alloc_node() is counting not
just objects, but blocks of objects. So you'd need 2*31 * 1024 objects
of one type, which is probably going to run afoul of other limitations.
Other cases are similar; for example "yes | git fetch-pack --stdin foo"
will grow an array indefinitely, but at one strbuf per line it starts
swapping on my 64GB machine at only 350M entries.

I think as long as the behavior remains "slow, but we do not overflow
any buffers" when you reach these limits, that's OK. Nobody is going to
do it in practice, and we just want to make sure that malicious inputs
cannot get out-of-bounds writes. It might be worth adding a comment,
though, to make sure nobody ever swaps "alloc_grow_new_alloc_" for
"alloc" in that macro.

-Peff

^ permalink raw reply related

* [RFC PATCH] approxidate: make "today" wrap to midnight
From: Tuomas Ahola @ 2026-05-15 20:58 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, Tuomas Ahola

Although some commands do reject invalid approxidate expressions,
in other cases those are simply evaluated as the current time.
Oftentimes that is a perfectly good compromise to handle silly
requests, but it isn't without rough edges.

Let's consider what "git log --since=today" should yield.
As it happens that "today" isn't actually a valid approxidate
format, the command currently tries to list commits with
*future* timestamps.  Perhaps it would make more sense if
it returned the commits made since midnight---that is,
during the current day.

Moreover, a revision parameter "@{today}" is currently outright
rejected.  Making "today" a valid approxidate time format could
make a natural way to specify the state of the ref at the start
of the current day.

Bind "today" to new function `date_today()` as an approxidate
special.  Make it return the last midnight if no specific time
is given; i.e. retain the old behavior of "noon today" and such.

Signed-off-by: Tuomas Ahola <taahol@utu.fi>
---

Notes:
    The "Jan 5 today" test is adapted from
    
      c27cc94fad (approxidate: handle pending number for "specials", 2018-11-02):
    
    > (saying "Jan 5 yesterday" should not respect the number at all).

 date.c          | 10 ++++++++++
 t/t0006-date.sh |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/date.c b/date.c
index 17a95077cf..343d6aab6f 100644
--- a/date.c
+++ b/date.c
@@ -1192,6 +1192,15 @@ static void date_never(struct tm *tm, struct tm *now UNUSED, int *num)
 	*num = 0;
 }
 
+static void date_today(struct tm *tm, struct tm *now, int *num UNUSED)
+{
+	if (tm->tm_hour == now->tm_hour &&
+	    tm->tm_min == now->tm_min &&
+	    tm->tm_sec == now->tm_sec)
+		date_time(tm, now, 0);
+	update_tm(tm, now, 0);
+}
+
 static const struct special {
 	const char *name;
 	void (*fn)(struct tm *, struct tm *, int *);
@@ -1204,6 +1213,7 @@ static const struct special {
 	{ "AM", date_am },
 	{ "never", date_never },
 	{ "now", date_now },
+	{ "today", date_today },
 	{ NULL }
 };
 
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 53ced36df4..07bf6115ab 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -164,6 +164,7 @@ check_approxidate() {
 }
 
 check_approxidate now '2009-08-30 19:20:00'
+check_approxidate today '2009-08-30 00:00:00'
 check_approxidate '5 seconds ago' '2009-08-30 19:19:55'
 check_approxidate 5.seconds.ago '2009-08-30 19:19:55'
 check_approxidate 10.minutes.ago '2009-08-30 19:10:00'
@@ -187,6 +188,7 @@ check_approxidate 'last tuesday' '2009-08-25 19:20:00'
 check_approxidate 'July 5th' '2009-07-05 19:20:00'
 check_approxidate '06/05/2009' '2009-06-05 19:20:00'
 check_approxidate '06.05.2009' '2009-05-06 19:20:00'
+check_approxidate 'Jan 5 today' '2009-01-30 00:00:00'
 
 check_approxidate 'Jun 6, 5AM' '2009-06-06 05:00:00'
 check_approxidate '5AM Jun 6' '2009-06-06 05:00:00'

base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
-- 
2.30.2


^ permalink raw reply related

* Re: [PATCH] commit-reach: use the decoration hash for tips_reachable_from_bases()
From: Jeff King @ 2026-05-15 21:14 UTC (permalink / raw)
  To: Kristofer Karlsson via GitGitGadget; +Cc: git, Kristofer Karlsson
In-Reply-To: <pull.2116.git.1778868463992.gitgitgadget@gmail.com>

On Fri, May 15, 2026 at 06:07:43PM +0000, Kristofer Karlsson via GitGitGadget wrote:

> From: Kristofer Karlsson <krka@spotify.com>
> 
> tips_reachable_from_bases() walks the commit graph from a set of base
> commits to find which tip commits are reachable.  The inner loop does
> a linear scan over the tips array to check whether each visited commit
> is a tip, making the overall cost O(C * T) where C is commits walked
> and T is the number of tips.
> 
> Replace the linear scan with the decoration hash for lookups, reducing
> the per-commit tip check from O(T) to O(1) and the overall cost from
> O(C * T) to O(C + T).
> 
> This function is called by `git for-each-ref --merged` and
> `git branch/tag --contains/--no-contains` via reach_filter() in
> ref-filter.c.
> 
> Benchmark on a merge-heavy monorepo (2.3M commits, 10,000 refs):
> 
>   Command                           Before    After   Speedup
>   for-each-ref --merged HEAD        6.64s     1.66s     4.0x
>   for-each-ref --no-merged HEAD     6.75s     1.74s     3.9x
>   branch --merged HEAD              0.68s     0.61s      10%
>   branch --no-merged HEAD           0.65s     0.61s       8%
>   tag --merged HEAD                 0.12s     0.12s       -
> 
> The large speedup for for-each-ref is because it checks all 10,000
> refs as tips, making the O(T) inner loop expensive.  The branch
> subcommand only checks local branches (fewer tips), so the improvement
> is smaller.

Hmm, I couldn't reproduce the speedup on something like linux.git (~1.4M
commits) with a lot of synthetic branches. I'd think that old branches
would be the most expensive, so I did:

  old=$(git rev-list --reverse HEAD | head -n1)
  seq --format="update refs/heads/branch%g $old" 10000 |
  git update-ref --stdin

Running "git for-each-ref --no-merged HEAD" takes ~650ms with stock Git.
But with your patch, it goes to ~830ms!

So what am I missing about your repo that it is so slow in the first
place?

>      * Hacking the array index into the decoration value as (void *)(i + 1)
>        instead of storing a proper pointer

The decoration API is not the most generic option here. There's an
oidmap type, but you have to embed the hashmap bits into your struct,
which is a lot of boilerplate if you're just storing an int. You can
define a khash with a custom value type, and I think the existing
oid_pos uses an int, which might be enough. All of those will store an
extra copy of the oid, though for the sizes we're talking about that's
not the end of the world.

Since we're always mapping commits, you could define a commit-slab (each
commit struct gets a unique id which we then index into a big array).
See commit-slab.h for an example.

I'm not very familiar with this code, but I wonder if we actually need
to map at all. It looks like we are mostly interested in set inclusion,
so perhaps an oidset() would work. Or even a bit in the object-flags.

-Peff

^ permalink raw reply

* Re: [PATCH] evaluate the second argument of ALLOC_GROW only once
From: René Scharfe @ 2026-05-15 23:01 UTC (permalink / raw)
  To: Jeff King; +Cc: Git List, Junio C Hamano
In-Reply-To: <20260515195049.GA149960@coredump.intra.peff.net>

On 5/15/26 9:50 PM, Jeff King wrote:
> On Fri, May 15, 2026 at 03:08:18PM -0400, Jeff King wrote:
> 
>> On Fri, May 15, 2026 at 08:16:50PM +0200, René Scharfe wrote:
>>
>>> +		size_t alloc_grow_new_alloc_; \
>>> +		if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
>>> +			alloc = alloc_grow_new_alloc_; \
>>> +			REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
>>>  		} \
>>
>> What happens if a caller passes in an argument that isn't a size_t?
>> We'll check for overflow in the size_t space, and then truncate it when
>> we assign to alloc, I think.
>>
>> I think we generally try to hold allocations in size_t these days, but
>> I'd be surprised if there weren't a few "int" holdouts. Grepping around,
>> alloc_node() seems to be an example.
>>
>> BTW, non-size_t arguments nullifies my earlier hand-waving around "nr +
>> 1 overflowing implies we've filled up the address space". But we are
>> still protected in the existing code by the:
>>
>>   if (alloc_nr(alloc) < (nr))
>> 	alloc = (nr);
>>
>> logic. But with your patch, that all happens in the size_t space, so I
>> think it would actually introduce possible array overflows when the
>> caller is using a smaller type.
> 
> Hmm, playing with it and looking a little closer, I think we don't end
> up overflowing the buffer because you use the size_t for
> REALLOC_ARRAY(). So the result is big, but then "alloc" is truncated.
> 
> And then on the next call, we think "oh no, the allocation is way too
> small" because we are using the truncated value. So we try to size up
> for every single allocation, even though it's actually big enough, and
> the program slows to a crawl. ;)
> 
> For reference, IU was using this hack to play around and demonstrate:
> 
> diff --git a/git.c b/git.c
> index 5a40eab8a2..638bbc69e4 100644
> --- a/git.c
> +++ b/git.c
> @@ -969,6 +969,19 @@ int cmd_main(int argc, const char **argv)
>  
>  	cmd = argv[0];
>  
> +	if (!strcmp(cmd, "foo")) {
> +		unsigned char *buf = NULL;
> +		unsigned nr = 0, alloc = 0;
> +		for (unsigned i = 0; i < UINT_MAX; i++) {
> +			ALLOC_GROW(buf, nr + 1, alloc);
> +			if (i % 313370 == 0)
> +				warning("at i=%u, alloc=%u, nr=%u", i, alloc, nr);
> +			buf[nr++] = i % 256;
> +		}
> +		printf("done, final nr=%u, alloc=%u\n", nr, alloc);
> +		return 0;
> +	}
> +
>  	/*
>  	 * We use PATH to find git commands, but we prepend some higher
>  	 * precedence paths: the "--exec-path" option, the GIT_EXEC_PATH
> 
> 
> The same problem exists in several places in actual code, but I'm not
> sure how practical it is to trigger. The alloc_node() is counting not
> just objects, but blocks of objects. So you'd need 2*31 * 1024 objects
> of one type, which is probably going to run afoul of other limitations.
> Other cases are similar; for example "yes | git fetch-pack --stdin foo"
> will grow an array indefinitely, but at one strbuf per line it starts
> swapping on my 64GB machine at only 350M entries.
> 
> I think as long as the behavior remains "slow, but we do not overflow
> any buffers" when you reach these limits, that's OK. Nobody is going to
> do it in practice, and we just want to make sure that malicious inputs
> cannot get out-of-bounds writes. It might be worth adding a comment,
> though, to make sure nobody ever swaps "alloc_grow_new_alloc_" for
> "alloc" in that macro.
There is no overflow check in either version (yet), so neither is safe
to operate close to the boundary.  Close meaning the intermediate term
(alloc + 16) * 3 being bigger than the maximum value.

Does the size_t arithmetic make matters worse?  The only change I can
see is that it interprets negative values as big unsigned ones and
then doesn't reallocate.  The outcome for positive values is the same,
overflow and all, no?

Here's a demo program exercising the arithmetic part of the macros:


#include <limits.h>
#include <stdbool.h>
#include <stdio.h>

#define alloc_nr(x) (((x)+16)*3/2)

#define ALLOC_GROW1(x, nr, alloc) \
	do { \
		if ((nr) > alloc) { \
			if (alloc_nr(alloc) < (nr)) \
				alloc = (nr); \
			else \
				alloc = alloc_nr(alloc); \
			x = true; \
		} \
	} while (0)

static inline bool st_alloc_nr(size_t nr, size_t alloc, size_t *outp)
{
	if (nr > alloc) {
		size_t out = alloc_nr(alloc);
		*outp = out < nr ? nr : out;
		return true;
	}
	return false;
}


#define ALLOC_GROW2(x, nr, alloc) \
	do { \
		size_t alloc_grow_new_alloc_; \
		if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
			alloc = alloc_grow_new_alloc_; \
			x = true; \
		} \
	} while (0)

#define T signed char
#define MIN 0
#define MAX SCHAR_MAX

int main(int argc, char **argv)
{
	for (T i = 0;; i++) {
		for (T j = MIN;; j++) {
			T alloc1 = j, alloc2 = j;
			bool allocated1 = false, allocated2 = false;
			ALLOC_GROW1(allocated1, i, alloc1);
			ALLOC_GROW2(allocated2, i, alloc2);
			if (alloc1 != alloc2 || allocated1 != allocated2)
				printf("%zu %zu %d %zu %d %zu\n",
				       (size_t)i, (size_t)j,
				       allocated1, (size_t)alloc1,
				       allocated2, (size_t)alloc2);
			if (j == MAX)
				break;
		}
		if (i == MAX)
			break;
	}
	return 0;
}



^ permalink raw reply

* [PATCH v7] revision.c: implement --max-count-oldest
From: Mirko Faina @ 2026-05-15 23:29 UTC (permalink / raw)
  To: git
  Cc: Mirko Faina, Junio C Hamano, Jeff King, Jean-Noël Avila,
	Patrick Steinhardt, Tian Yuchen, Ben Knoble, Johannes Sixt,
	Chris Torek
In-Reply-To: <ce8d1ff49ef418ae3720265a124ef53a959d289e.1778017966.git.mroik@delayed.space>

--max-count is a commit limiting option sets a maximum amount of commits
to be shown. If a user wants to see only the first N commits of the
history (the oldest commits) they'd have to do something like

    git log $(git rev-list HEAD | tail -n N | head -n 1)

This is not very user-friendly.

Teach get_revision() the --max-count-oldest option.

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
Since v6 I've simplified the docs, replaced die messages with
die_for_incompatible_opt2 and fixed graph output when used with
--boundary.

 Documentation/rev-list-options.adoc |   5 +-
 revision.c                          | 103 +++++++++++++++++++++++++++-
 revision.h                          |   2 +
 t/t4202-log.sh                      |  33 +++++++++
 4 files changed, 139 insertions(+), 4 deletions(-)

diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index 2d195a1474..e8c88d0f1c 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -16,7 +16,10 @@ ordering and formatting options, such as `--reverse`.
 `-<number>`::
 `-n <number>`::
 `--max-count=<number>`::
-	Limit the output to _<number>_ commits.
+	Limit the output to the first _<number>_ commits that would be shown.
+
+`--max-count-oldest=<number>`::
+	Limit the output to the last _<number>_ commits that would be shown.
 
 `--skip=<number>`::
 	Skip _<number>_ commits before starting to show the commit output.
diff --git a/revision.c b/revision.c
index 599b3a66c3..7fc79049b2 100644
--- a/revision.c
+++ b/revision.c
@@ -2339,10 +2339,28 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	}
 
 	if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
+		if (revs->max_count_type == 1)
+			die_for_incompatible_opt2(1, "--max-count", 1,
+						  "--max-count-oldest");
 		revs->max_count = parse_count(optarg);
 		revs->no_walk = 0;
+		revs->max_count_type = 0;
 		return argcount;
+	} else if ((argcount = parse_long_opt("max-count-oldest", argv, &optarg))) {
+		if (revs->max_count_type == 0 && revs->max_count != -1)
+			die_for_incompatible_opt2(1, "--max-count", 1,
+						  "--max-count-oldest");
+		if (revs->skip_count > 0)
+			die_for_incompatible_opt2(1, "--skip", 1,
+						  "--max-count-oldest");
+		revs->max_count = parse_count(optarg);
+		revs->no_walk = 0;
+		revs->max_count_type = 1;
+		revs->max_count_stage = 0;
 	} else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
+		if (revs->max_count_type == 1)
+			die_for_incompatible_opt2(1, "--skip", 1,
+						  "--max-count-oldest");
 		revs->skip_count = parse_count(optarg);
 		return argcount;
 	} else if ((*arg == '-') && isdigit(arg[1])) {
@@ -4521,15 +4539,83 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 	return c;
 }
 
+static void retrieve_oldest_commits(struct rev_info *revs,
+				    struct commit_list **queue)
+{
+	struct commit *c;
+	int max_count = revs->max_count;
+	int queuei_count = 0;
+	int queueo_count = 0;
+	struct commit_list *queueo = NULL;
+	struct commit_list *queuei = NULL;
+	struct commit_list *reversed_queue = NULL;
+
+	revs->max_count = -1;
+	while ((c = get_revision_internal(revs))) {
+		/*
+		 * We need to reset SHOWN status otherwise --graph breaks.
+		 * It is fine to do, get_revision_internal() doesn't consider
+		 * children commits as they have been already processed and the
+		 * traversal happens only child to parent.
+		 *
+		 * We do this because the --graph machinery relies on the status
+		 * of the parents to decide how the printing will happen.
+		 *
+		 * We can't simply replace this instruction with a
+		 * graph_update() as it doesn't do the actualy printing, we'd
+		 * have to remove any commit that goes over the
+		 * --max-count-oldest limit from revs->graph.
+		 */
+		c->object.flags &= ~(SHOWN | CHILD_SHOWN);
+		commit_list_insert(c, &queuei);
+		queuei_count++;
+		while (queuei_count + queueo_count > max_count) {
+			if (!queueo_count) {
+				while (queuei_count > 0) {
+					c = pop_commit(&queuei);
+					queuei_count--;
+					commit_list_insert(c, &queueo);
+					queueo_count++;
+				}
+			}
+			pop_commit(&queueo);
+			queueo_count--;
+		}
+	}
+
+	while ((c = pop_commit(&queueo)))
+		commit_list_insert(c, &reversed_queue);
+	while ((c = pop_commit(&queuei)))
+		commit_list_insert(c, &queueo);
+	while ((c = pop_commit(&queueo)))
+		commit_list_insert(c, &reversed_queue);
+
+	while ((c = pop_commit(&reversed_queue)))
+		commit_list_insert(c, queue);
+}
+
 struct commit *get_revision(struct rev_info *revs)
 {
 	struct commit *c;
 	struct commit_list *reversed;
+	struct commit_list *queue = NULL;
+	struct commit_list *p;
+
+	if (revs->max_count_type == 1 && !revs->max_count_stage) {
+		retrieve_oldest_commits(revs, &queue);
+		commit_list_free(revs->commits);
+		revs->commits = queue;
+		revs->max_count_stage = 1;
+	}
 
 	if (revs->reverse) {
 		reversed = NULL;
-		while ((c = get_revision_internal(revs)))
-			commit_list_insert(c, &reversed);
+		if (revs->max_count_type == 1)
+			while ((c = pop_commit(&revs->commits)))
+				commit_list_insert(c, &reversed);
+		else
+			while ((c = get_revision_internal(revs)))
+				commit_list_insert(c, &reversed);
 		commit_list_free(revs->commits);
 		revs->commits = reversed;
 		revs->reverse = 0;
@@ -4543,7 +4629,18 @@ struct commit *get_revision(struct rev_info *revs)
 		return c;
 	}
 
-	c = get_revision_internal(revs);
+	if (revs->max_count_stage) {
+		c = pop_commit(&revs->commits);
+		if (c) {
+			c->object.flags |= SHOWN;
+			if (!(c->object.flags & BOUNDARY))
+				for (p = c->parents; p; p = p->next)
+					p->item->object.flags |= CHILD_SHOWN;
+		}
+	} else {
+		c = get_revision_internal(revs);
+	}
+
 	if (c && revs->graph)
 		graph_update(revs->graph, c);
 	if (!c) {
diff --git a/revision.h b/revision.h
index 584f1338b5..e157463cb1 100644
--- a/revision.h
+++ b/revision.h
@@ -309,6 +309,8 @@ struct rev_info {
 	/* special limits */
 	int skip_count;
 	int max_count;
+	unsigned int max_count_type:1;
+	unsigned int max_count_stage:1;
 	timestamp_t max_age;
 	timestamp_t max_age_as_filter;
 	timestamp_t min_age;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 05cee9e41b..8f2471e7e4 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1882,6 +1882,39 @@ test_expect_success 'log --graph with --name-status' '
 	test_cmp_graph --name-status tangle..reach
 '
 
+test_expect_success 'log --max-count-oldest=3 --oneline' '
+	test_when_finished rm expect &&
+	git log --oneline | tail -n3 >expect &&
+	git log --oneline --max-count-oldest=3 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
+	test_when_finished rm expect &&
+	git log --oneline --reverse | head -n3 >expect &&
+	git log --oneline --max-count-oldest=3 --reverse >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --max-count-oldest with --max-count' '
+	test_when_finished rm stderr &&
+	test_must_fail git log --max-count-oldest=3 --max-count=3 2>stderr &&
+	test_grep "cannot be used together" stderr
+'
+
+test_expect_success 'log --max-count-oldest with --skip' '
+	test_when_finished rm stderr &&
+	test_must_fail git log --max-count-oldest=3 --skip=1 2>stderr &&
+	test_grep "cannot be used together" stderr
+'
+
+test_expect_success 'log --max-count-oldest=1000 --graph --boundary' '
+	test_when_finished rm expect actual &&
+	git log --graph --boundary >expect &&
+	git log --max-count-oldest=1000 --graph --boundary >actual &&
+	test_cmp expect actual
+'
+
 cat >expect <<-\EOF
 * reach
 |
-- 
2.54.0


^ permalink raw reply related

* UBSan failing on expensive test(s)
From: Junio C Hamano @ 2026-05-15 23:43 UTC (permalink / raw)
  To: git

This started happening on 'next' that runs EXPENSIVE tests thanks to
Dscho's recent updates to enable them in CI.

https://github.com/git/git/actions/runs/25896439353/job/76110441841#step:10:2172

It claims that """

    commit.c:1574:6: runtime error: signed integer overflow:
    -2147483648 - 1 cannot be represented in type 'int'

""".

Another is related in the sense that it used to be hidden behind
EXPENSIVE prerequisite, but is probably unrelated.

https://github.com/git/git/actions/runs/25896439353/job/76110441842#step:10:156


^ permalink raw reply

* Re: [RFC PATCH] approxidate: make "today" wrap to midnight
From: Junio C Hamano @ 2026-05-16  0:03 UTC (permalink / raw)
  To: Tuomas Ahola; +Cc: git, Jeff King
In-Reply-To: <20260515205803.26211-1-taahol@utu.fi>

Tuomas Ahola <taahol@utu.fi> writes:

> Although some commands do reject invalid approxidate expressions,
> in other cases those are simply evaluated as the current time.
> Oftentimes that is a perfectly good compromise to handle silly
> requests, but it isn't without rough edges.
>
> Let's consider what "git log --since=today" should yield.
> As it happens that "today" isn't actually a valid approxidate
> format, the command currently tries to list commits with
> *future* timestamps.  Perhaps it would make more sense if
> it returned the commits made since midnight---that is,
> during the current day.

I actually am of two minds about this.  

What should "git log --until=today" do when you run it in the late
afternoon?  Wouldn't you want to see what you did in the morning and
early afternoon?

Because we cannot define "today" as "--since=today means the latest
midnight and later, while --until=today means until the end of today
[*]" without introducing an extra hint to calls to approxidate() to
tell it who is calling for what, it is impossible to give these two
sensible behavior at the same time.

    Side note: but because the existing history is all about the past,
    "until the end of today" is by definition a synonym for "up to
    now", so defining "today" the same as "now" would make "until"
    behave just as sensibly as if we define it as "the end of today".

In practice, using "--until" to *not* truncate at all (which is what
--until=now or --until=end.of.today would essentially mean) has no
practical value, while "--since=beginning.of.today" does have more
utility, allowing you to specify "the last 8 hours and 50 minutes"
without knowing that it is now at 08:50 in the morning.  So I am
still in favor of interpreting "today" as "the latest midnight, the
beginning of today" because that would give us a more useful
behavior than other possible definitions.  We may want to strengthen
the justification behind the chosen definition of why we chose what
we chose over any other time in today with something like what I
said above, mentioning "--until".

Thanks.

^ 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