* Re: [RFC PATCH v2 0/6] Noobify format for status, add, restore
From: Dragan Simic @ 2023-10-28 17:41 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <ZT1GWw886XuXwqlw.jacob@initialcommit.io>
On 2023-10-28 19:35, Jacob Stopak wrote:
> On Sat, Oct 28, 2023 at 06:20:41PM +0200, Dragan Simic wrote:
>> I agree, there should be a well-placed hint, but we'd need to think
>> really
>> well where to place it, so we don't annoy experienced git users too
>> much,
>> while we also inform the less experienced users.
>
> Yes, hmm, I wonder if maybe we could add the hint for the extended
> option
> only in the case that the user uses the --verbose option either on the
> command line or via the config setting. Since using the verbose option
> tells us the user is asking for more details, that might be a good time
> to inform them of the _even more detailed_ option, but of course that
> hint could be disabled easily if they preferred the "basic" verbosity.
That's something we can keep thinking about, until we find a good
solution. Also, a detailed review of the current logic behind
displaying the hints should be performed first, if you agree.
>> Sounds good, thank you. If you agree, I'll go ahead and implement
>> support
>> for a few "<command>.verbose" configuration options during the next
>> week or
>> so, and submit the patches. I'll most probably come to some more
>> important
>> conclusions while implementing that, which I'll relay over, of course.
>
> Yes I agree, that sounds great! Maybe I'll just wait then until seeing
> your implementation of that before I poke around on mine more. Then
> I'll
> apply your patches locally to add my extended option.
Great, thanks. I'll start working on the patches tomorrow or so, and
I'll get back with any important conclusions or open questions arising
from that, so we can discuss them further.
^ permalink raw reply
* Re: [RFC PATCH v2 0/6] Noobify format for status, add, restore
From: Jacob Stopak @ 2023-10-28 17:35 UTC (permalink / raw)
To: Dragan Simic; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <37e7bd8f6f4b75aa3b31dc98804b1334@manjaro.org>
On Sat, Oct 28, 2023 at 06:20:41PM +0200, Dragan Simic wrote:
> > Hehe ok, maybe there is room in some hints to notify users of the
> > extended option...
>
> I agree, there should be a well-placed hint, but we'd need to think really
> well where to place it, so we don't annoy experienced git users too much,
> while we also inform the less experienced users.
Yes, hmm, I wonder if maybe we could add the hint for the extended option
only in the case that the user uses the --verbose option either on the
command line or via the config setting. Since using the verbose option
tells us the user is asking for more details, that might be a good time
to inform them of the _even more detailed_ option, but of course that
hint could be disabled easily if they preferred the "basic" verbosity.
> > > > Any thoughts on what the section in the config for a more general
> > > > setting like this might be named?
> > >
> > > Maybe "core.verbose"? We already have "core.pager", which kind of
> > > affects the way all command outputs look like.
> >
> > Ok! The idea of using "core" came to mind but I wasn't sure if that was
> > more for lower-level settings or more general things.
>
> I also considered the "core" section to be reserved for the very low-level
> internal things, but having "core.pager" clearly allows other appropriate
> configuration options to be placed here.
Ok awesome!
> > Great. Thanks a lot for all the feedback. Let me doctor up the patch
> > series to take these things into account and submit an RFC v3 :D
>
> Sounds good, thank you. If you agree, I'll go ahead and implement support
> for a few "<command>.verbose" configuration options during the next week or
> so, and submit the patches. I'll most probably come to some more important
> conclusions while implementing that, which I'll relay over, of course.
Yes I agree, that sounds great! Maybe I'll just wait then until seeing
your implementation of that before I poke around on mine more. Then I'll
apply your patches locally to add my extended option.
^ permalink raw reply
* Re: [RFC PATCH v2 0/6] Noobify format for status, add, restore
From: Dragan Simic @ 2023-10-28 16:20 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <ZT0m68HWZS/tDGtH.jacob@initialcommit.io>
On 2023-10-28 17:21, Jacob Stopak wrote:
> On Sat, Oct 28, 2023 at 07:55:31AM +0200, Dragan Simic wrote:
>>> Basically what I'm asking is if commands that already have a
>>> --verbose
>>> flag would just get a config setting that does the existing thing by
>>> default?
>>
>> Well, not by default. The default values would remain "false", so
>> nothing
>> jumps out of nowhere.
>
> Right, sorry, I worded that poorly.
No worries, just wanted to make sure we're on the same page.
>> Yes, that's what I propose. It also looks like a logical choice to
>> me, and
>> it would leave space for some possible later changes to the
>> "<command>.verbose = extended" verbosity, without tying it to the
>> tables.
>> We'd also leave some space that way for even maybe an additional level
>> of
>> verbosity, be it "<command>.verbose = simple", "<command>.verbose =
>> graphical" or whatever.
>>
>> Perhaps this scheme should also support "<command>.verbose = basic",
>> which
>> would be an alias for "<command>.verbose = true", for additional
>> clarity.
>>
> Sounds good!
I'm glad you agree.
>> Perhaps it would also be good to nudge the newbies a bit by requesting
>> them
>> to enable the extended verbosity for each command by hand. That way
>> they
>> would both learn a bit about the way git configuration works, which
>> they
>> ultimately can't escape from, and they would be excited to learn new
>> git
>> commands. Or I at least hope so. :)
>>
> Hehe ok, maybe there is room in some hints to notify users of the
> extended option...
I agree, there should be a well-placed hint, but we'd need to think
really well where to place it, so we don't annoy experienced git users
too much, while we also inform the less experienced users.
>> > Any thoughts on what the section in the config for a more general
>> > setting like this might be named?
>>
>> Maybe "core.verbose"? We already have "core.pager", which kind of
>> affects
>> the way all command outputs look like.
>
> Ok! The idea of using "core" came to mind but I wasn't sure if that was
> more for lower-level settings or more general things.
I also considered the "core" section to be reserved for the very
low-level internal things, but having "core.pager" clearly allows other
appropriate configuration options to be placed here.
> Great. Thanks a lot for all the feedback. Let me doctor up the patch
> series to take these things into account and submit an RFC v3 :D
Sounds good, thank you. If you agree, I'll go ahead and implement
support for a few "<command>.verbose" configuration options during the
next week or so, and submit the patches. I'll most probably come to
some more important conclusions while implementing that, which I'll
relay over, of course.
^ permalink raw reply
* [Bugs] Hello,I can't build git by ./configure
From: h hm @ 2023-10-28 15:39 UTC (permalink / raw)
To: git@vger.kernel.org
[-- Attachment #1.1: Type: text/plain, Size: 521 bytes --]
I want to build commit 2e8e77c in the master
Step:
cd git
make configure
./configure
Out:
configure: Setting lib to 'lib' (the default)
configure: Will try -pthread then -lpthread to enable POSIX Threads.
configure: CHECKS for site configuration
checking for gcc... gcc
checking whether the C compiler works... no
configure: error: in `/home/haomi/git':
configure: error: C compiler cannot create executables
See `config.log' for more details
But before that commit ,build ok
There is my config.log
[-- Attachment #1.2: Type: text/html, Size: 2696 bytes --]
[-- Attachment #2: config.log --]
[-- Type: application/octet-stream, Size: 6380 bytes --]
This file contains any messages produced by compilers while
running configure, to aid debugging if configure makes a mistake.
It was created by git configure 2.42.GIT, which was
generated by GNU Autoconf 2.71. Invocation command line was
$ ./configure prefix=/usr/local/git
## --------- ##
## Platform. ##
## --------- ##
hostname = DESKTOP-4TTPHMR
uname -m = x86_64
uname -r = 3.4.9-1.x86_64
uname -s = CYGWIN_NT-10.0-25982
uname -v = 2023-09-06 11:19 UTC
/usr/bin/uname -p = unknown
/bin/uname -X = unknown
/bin/arch = x86_64
/usr/bin/arch -k = unknown
/usr/convex/getsysinfo = unknown
/usr/bin/hostinfo = unknown
/bin/machine = unknown
/usr/bin/oslevel = unknown
/bin/universe = unknown
PATH: /usr/local/bin/
PATH: /usr/bin/
PATH: /cygdrive/c/WINDOWS/system32/
PATH: /cygdrive/c/WINDOWS/
PATH: /cygdrive/c/WINDOWS/System32/Wbem/
PATH: /cygdrive/c/WINDOWS/System32/WindowsPowerShell/v1.0/
PATH: /cygdrive/c/WINDOWS/System32/OpenSSH/
PATH: /cygdrive/c/Program Files/dotnet/
PATH: /cygdrive/c/Program Files/Bandizip/
PATH: /cygdrive/c/Users/haomi/AppData/Local/Microsoft/WindowsApps/
## ----------- ##
## Core tests. ##
## ----------- ##
configure:2628: Setting lib to 'lib' (the default)
configure:2652: Will try -pthread then -lpthread to enable POSIX Threads.
configure:2688: CHECKS for site configuration
configure:2842: checking for gcc
configure:2863: found /usr/bin/gcc
configure:2874: result: gcc
configure:3227: checking for C compiler version
configure:3236: gcc --version >&5
gcc (GCC) 11.4.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
configure:3247: $? = 0
configure:3236: gcc -v >&5
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-cygwin/11/lto-wrapper.exe
Target: x86_64-pc-cygwin
Configured with: /mnt/share/cygpkgs/gcc/gcc.x86_64/src/gcc-11.4.0/configure --srcdir=/mnt/share/cygpkgs/gcc/gcc.x86_64/src/gcc-11.4.0 --prefix=/usr --exec-prefix=/usr --localstatedir=/var --sysconfdir=/etc --docdir=/usr/share/doc/gcc --htmldir=/usr/share/doc/gcc/html -C --build=x86_64-pc-cygwin --host=x86_64-pc-cygwin --target=x86_64-pc-cygwin --without-libiconv-prefix --without-libintl-prefix --libexecdir=/usr/lib --with-gcc-major-version-only --enable-shared --enable-shared-libgcc --enable-static --enable-version-specific-runtime-libs --enable-bootstrap --enable-__cxa_atexit --with-dwarf2 --with-tune=generic --enable-languages=ada,c,c++,d,fortran,lto,objc,obj-c++,jit --enable-graphite --enable-threads=posix --enable-libatomic --enable-libgomp --enable-libquadmath --enable-libquadmath-support --disable-libssp --enable-libada --disable-symvers --disable-multilib --with-gnu-ld --with-gnu-as --with-cloog-include=/usr/include/cloog-isl --without-libiconv-prefix --without-libintl-prefix --with-system-zlib --enable-linker-build-id --with-default-libstdcxx-abi=gcc4-compatible --enable-libstdcxx-filesystem-ts
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.4.0 (GCC)
configure:3247: $? = 0
configure:3236: gcc -V >&5
gcc: error: unrecognized command-line option '-V'
gcc: fatal error: no input files
compilation terminated.
configure:3247: $? = 1
configure:3236: gcc -qversion >&5
gcc: error: unrecognized command-line option '-qversion'; did you mean '--version'?
gcc: fatal error: no input files
compilation terminated.
configure:3247: $? = 1
configure:3236: gcc -version >&5
gcc: error: unrecognized command-line option '-version'
gcc: fatal error: no input files
compilation terminated.
configure:3247: $? = 1
configure:3267: checking whether the C compiler works
configure:3289: gcc conftest.c >&5
configure:3293: $? = 1
configure:3333: result: no
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME "git"
| #define PACKAGE_TARNAME "git"
| #define PACKAGE_VERSION "2.42.GIT"
| #define PACKAGE_STRING "git 2.42.GIT"
| #define PACKAGE_BUGREPORT "git@vger.kernel.org"
| #define PACKAGE_URL ""
| /* end confdefs.h. */
|
| int
| main (void)
| {
|
| ;
| return 0;
| }
configure:3338: error: in `/home/haomi/git':
configure:3340: error: C compiler cannot create executables
See `config.log' for more details
## ---------------- ##
## Cache variables. ##
## ---------------- ##
ac_cv_env_CC_set=
ac_cv_env_CC_value=
ac_cv_env_CFLAGS_set=set
ac_cv_env_CFLAGS_value=
ac_cv_env_CPPFLAGS_set=
ac_cv_env_CPPFLAGS_value=
ac_cv_env_CPP_set=
ac_cv_env_CPP_value=
ac_cv_env_LDFLAGS_set=
ac_cv_env_LDFLAGS_value=
ac_cv_env_LIBS_set=set
ac_cv_env_LIBS_value=
ac_cv_env_build_alias_set=
ac_cv_env_build_alias_value=
ac_cv_env_host_alias_set=
ac_cv_env_host_alias_value=
ac_cv_env_target_alias_set=
ac_cv_env_target_alias_value=
ac_cv_prog_ac_ct_CC=gcc
## ----------------- ##
## Output variables. ##
## ----------------- ##
ALLOCA=''
AR=''
ASCIIDOC=''
CC='gcc'
CFLAGS=''
CPP=''
CPPFLAGS=''
CURL_CONFIG=''
DEFS=''
DIFF=''
ECHO_C=''
ECHO_N='-n'
ECHO_T=''
EGREP=''
EXEEXT=''
GREP=''
LDFLAGS=''
LIBOBJS=''
LIBS=''
LTLIBOBJS=''
OBJEXT=''
PACKAGE_BUGREPORT='git@vger.kernel.org'
PACKAGE_NAME='git'
PACKAGE_STRING='git 2.42.GIT'
PACKAGE_TARNAME='git'
PACKAGE_URL=''
PACKAGE_VERSION='2.42.GIT'
PATH_SEPARATOR=':'
SHELL='/bin/sh'
TAR=''
TCLTK_PATH=''
ac_ct_AR=''
ac_ct_CC='gcc'
bindir='${exec_prefix}/bin'
build_alias=''
datadir='${datarootdir}'
datarootdir='${prefix}/share'
docdir='${datarootdir}/doc/${PACKAGE_TARNAME}'
dvidir='${docdir}'
exec_prefix='NONE'
host_alias=''
htmldir='${docdir}'
includedir='${prefix}/include'
infodir='${datarootdir}/info'
libdir='${exec_prefix}/lib'
libexecdir='${exec_prefix}/libexec'
localedir='${datarootdir}/locale'
localstatedir='${prefix}/var'
mandir='${datarootdir}/man'
oldincludedir='/usr/include'
pdfdir='${docdir}'
prefix='/usr/local/git'
program_transform_name='s,x,x,'
psdir='${docdir}'
runstatedir='${localstatedir}/run'
sbindir='${exec_prefix}/sbin'
sharedstatedir='${prefix}/com'
sysconfdir='${prefix}/etc'
target_alias=''
## ----------- ##
## confdefs.h. ##
## ----------- ##
/* confdefs.h */
#define PACKAGE_NAME "git"
#define PACKAGE_TARNAME "git"
#define PACKAGE_VERSION "2.42.GIT"
#define PACKAGE_STRING "git 2.42.GIT"
#define PACKAGE_BUGREPORT "git@vger.kernel.org"
#define PACKAGE_URL ""
configure: exit 77
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Oswald Buddenhagen @ 2023-10-28 15:27 UTC (permalink / raw)
To: Liam Beguin; +Cc: Kousik Sanagavarapu, git
In-Reply-To: <20231028022048.GA1784118@shaak>
On Fri, Oct 27, 2023 at 10:20:48PM -0400, Liam Beguin wrote:
>I agree, %aa isn't the best, I'm definitly opened to suggestions.
>My preference would've been for something like %ad, but that's already
>taken.
>
H for host would be available. (not to be confused with h for human.)
in retrospect i'd say that it was unwise to use separate letters for the
various forms of dates - a set of qualifiers that would be applied to a
single specifier would be nicer. ah well ...
regards
^ permalink raw reply
* Re: [RFC PATCH v2 0/6] Noobify format for status, add, restore
From: Jacob Stopak @ 2023-10-28 15:21 UTC (permalink / raw)
To: Dragan Simic; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <2a0ba4c8e96cb7d2ea66dd1e78cdd39c@manjaro.org>
On Sat, Oct 28, 2023 at 07:55:31AM +0200, Dragan Simic wrote:
> > So I assume an "add.verbose" config option would just always print that
> > without having to specify the -v, --verbose flag when running the
> > command?
>
> Yes, that's how I see it. Setting "add.verbose" to "true", to be precise,
> or to "basic", which I'll explain a bit further later in my response.
Ok, gotcha!
> > Basically what I'm asking is if commands that already have a --verbose
> > flag
> > would just get a config setting that does the existing thing by default?
>
> Well, not by default. The default values would remain "false", so nothing
> jumps out of nowhere.
Right, sorry, I worded that poorly.
> > > > So it seems like currently "verbose" is used for various things among
> > > > the command set...
> > >
> > > Yes, that's the basic verbosity, as I named it above.
Ok.
> > Would it make sense to try to define a more consistent type of output or
> > format style for "verbosity" across different commands? As it stands it
> > seems each command treats verbosity in its own way which makes it hard
> > to interpret exactly what it will do each time...
>
> We'd have to follow the already established behavior of the commands, and
> there are the man pages to describe what's going on with the verbosity for
> each command. In other words, nothing would get changed, just some more
> knobs would be added, for those who prefer to have the additional verbosity
> enabled.
Ok I see.
> > Ok so it sounds like you prefer to use "verbose" as the setting key?
> > I guess at this point that might make more sense since commit.verbose
> > already exists, and existing options could be packed into it like you
> > said instead of just true or false.
>
> It looks like a logical choice to me.
>
Ok.
> > And then my thing here would just be called "command.verbose =
> > extended"?
>
> Yes, that's what I propose. It also looks like a logical choice to me, and
> it would leave space for some possible later changes to the
> "<command>.verbose = extended" verbosity, without tying it to the tables.
> We'd also leave some space that way for even maybe an additional level of
> verbosity, be it "<command>.verbose = simple", "<command>.verbose =
> graphical" or whatever.
>
> Perhaps this scheme should also support "<command>.verbose = basic", which
> would be an alias for "<command>.verbose = true", for additional clarity.
>
Sounds good!
>
> Perhaps it would also be good to nudge the newbies a bit by requesting them
> to enable the extended verbosity for each command by hand. That way they
> would both learn a bit about the way git configuration works, which they
> ultimately can't escape from, and they would be excited to learn new git
> commands. Or I at least hope so. :)
>
Hehe ok, maybe there is room in some hints to notify users of the
extended option...
> > And it's true that some users might only want the extended (or any
> > format) for specific commands. I think a happy medium then is to have
> > the command-specific settings like you mention, plus one toplevel
> > option that enables a specific type of output format among all commands
> > (and overrides the command-specific settings), so that the user can
> > choose which they prefer.
>
> That's something we can consider as an additional configuration option.
> That way, users could also enable the basic verbosity for all commands,
> which may also be usable.
>
Cool!
> > Any thoughts on what the section in the config for a more general
> > setting like this might be named?
>
> Maybe "core.verbose"? We already have "core.pager", which kind of affects
> the way all command outputs look like.
Ok! The idea of using "core" came to mind but I wasn't sure if that was
more for lower-level settings or more general things.
Great. Thanks a lot for all the feedback. Let me doctor up the patch
series to take these things into account and submit an RFC v3 :D
^ permalink raw reply
* Re: [RFC][Outreachy] Seeking Git Community Feedback on My Application
From: Isoken Ibizugbe @ 2023-10-28 14:07 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD1+aWGymjssk5CotPjEmhu5sMcTy-b7eJc4fw-UA41Qig@mail.gmail.com>
On Sat, Oct 28, 2023 at 1:38 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Sat, Oct 28, 2023 at 12:41 PM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
> >
> > On Sat, Oct 28, 2023 at 9:07 AM Christian Couder
> > <christian.couder@gmail.com> wrote:
>
> > > > #define DIGIT "0123456789"
> > > >
> > > > static void t_digit_type(void)
> > > > {
> > > > int i;
> > > > const char *digits = DIGIT;
> > > > for (i = 0; digits[i]; i++)
> > > > {
> > > > check_int(isdigit(digits[i]), ==, 0);
> > > > }
> > >
> > > This tests that isdigit() returns 0 for each of the characters in
> > > "0123456789", but first I think isdigit() should return 1, not 0 for
> > > those characters.
> >
> > yes, that is true. should I send a re-roll?
>
> Yes, please.
#include "test-lib.h"
#include "ctype.h"
static void t_digit_type(void)
{
int i;
for (i = 0; i < 256; i++)
{
if (i < '0' || i > '9')
check_int(isdigit(i), ==, 0);
else
check_int(isdigit(i), ==, 1);
}
}
int main(void)
{
TEST(t_digit_type(), "Character is a digit");
return test_done();
}
^ permalink raw reply
* Re: [OUTREACHY] Final Application For Git Internshhip
From: Naomi Ibe @ 2023-10-28 13:17 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD34gre7Xxw=kFop93tLsGXc3RxasstBWpa6RaJ6VyEpFQ@mail.gmail.com>
On Sat, Oct 28, 2023 at 9:25 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Oct 25, 2023 at 2:14 PM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
> >>
> >> It could help to say if your contribution has been merged to 'master',
> >> 'next', 'seen' or not at all.
> >
> > My microproject contribution has been scheduled to be merged to the master branch.
>
> Great, please add this information to your application.
>
> >> I think that one of the important tasks to be done early is to
> >> identify what code in t/helper is unit testing C code and what code is
> >> really about helping other tests in the t/t*.sh scripts. It would be
> >> nice if you could give an example of each kind of code.
> >
> > In my opinion, helper/test-ctype.c is a unit test file containing a set of unit tests for character classification functions,
>
> Right.
>
> > while helper/test-dir-iterator.c is a unit test file which works together with the t/t0066-dir-iterator.sh file to iterate through a directory and give details on its contents. It likely is used for testing and inspecting directory structures and file types within a specified path.
>
> Actually "t/helper/test-dir-iterator.c" and "t/t0066-dir-iterator.sh"
> are used together to test the code in "dir-iterator.h" and
> "dir-iterator.c", so it's kind of special. Ideally this could be
> ported to the unit test framework as the goal is to test quite low
> level code (instead of a full git command for example), but I am not
> sure how easy it would be, and if it would even be worth it.
>
> >> An example of how you would migrate parts of a test, or how a migrated
> >> test would look like, would be nice.
> >
> > I'd first create a new test file, then include "test-libtap/tap.h" and "test-tool.h" header files, then I would initialize TAP with this command plan_tests(x), where x defines the number of tests to be run inside that file.
> > Below the plan_tests();, I'd migrate and edit my specific test function and requirements, and after that, I'd add my "done_testing();" and then "return exit_status();"
>
> These are quite good guidelines, but not quite an example.
>
Using the test-ctype.c file as an example, the migrated unit test file
would look like this:
#include "test-tool.h"
#include "test-libtap/tap.h"
#include <stdio.h>
#include <string.h>
static int is_in(const char *s, int ch) {
if (ch == '\0')
return ch == *s;
if (*s == '\0')
s++;
return !!strchr(s, ch);
}
int main() {
// Init TAP with the number of planned tests (1 test per character class)
plan(12);
// Character classes
char *classes[] = {
"DIGIT", " \n\r\t",
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ",
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789",
"*?[\\", "$()*+.?[\\^{|", "!\"#%&',-/:;<=>@_`~",
"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f"
"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f"
"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f"
"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f"
"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f"
"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f"
"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f"
"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f",
};
// Test each character class
for (int i = 0; i < 12; i++) {
char description[50];
sprintf(description, "Testing %s character class", classes[i]);
for (int ch = 0; ch < 256; ch++) {
int result = is_in(classes[i], ch);
if (ch == EOF)
ok(result == is_in(classes[i], ch), description);
else
is_int(result, is_in(classes[i], ch), description);
}
}
// After testing all
done_testing();
return exit_status();
}
> Thanks,
> Christian.
^ permalink raw reply
* Re: [RFC][Outreachy] Seeking Git Community Feedback on My Application
From: Christian Couder @ 2023-10-28 12:37 UTC (permalink / raw)
To: Isoken Ibizugbe; +Cc: git
In-Reply-To: <CAJHH8bGK28Fc+VG3uxgC5sGgFEAw6_6AEtusgmw7c4Vz0iGF_g@mail.gmail.com>
On Sat, Oct 28, 2023 at 12:41 PM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
>
> On Sat, Oct 28, 2023 at 9:07 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> > > #define DIGIT "0123456789"
> > >
> > > static void t_digit_type(void)
> > > {
> > > int i;
> > > const char *digits = DIGIT;
> > > for (i = 0; digits[i]; i++)
> > > {
> > > check_int(isdigit(digits[i]), ==, 0);
> > > }
> >
> > This tests that isdigit() returns 0 for each of the characters in
> > "0123456789", but first I think isdigit() should return 1, not 0 for
> > those characters.
>
> yes, that is true. should I send a re-roll?
Yes, please.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #03; Fri, 6)
From: René Scharfe @ 2023-10-28 12:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqcyxn3gas.fsf@gitster.g>
Am 09.10.23 um 18:35 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Am 07.10.23 um 10:20 schrieb Junio C Hamano:
>>> * rs/parse-options-value-int (2023-09-18) 2 commits
>>> - parse-options: use and require int pointer for OPT_CMDMODE
>>> - parse-options: add int value pointer to struct option
>>> ...
>> I don't mind removing this topic from seen for now; it's not ready, yet.
>
> After seeing the discussion moved to giving a more type safe enum
> support and then somehow convesation seemed to have petered out, I
> was wondering if we figured out the problem space enough to see an
> updated version with well defined scope and good problem
> description.
It's a simple idea with a broad scope: Replace void pointers or fortify
them by tracking type information across their use in order to get
compile time type checks for the entire code base. Give developers the
basic guardrails they already have everywhere else also in generic code.
Adding such type checks is easy, but fixing all the places using
incompatible pointers is a lot of churn. Option parsing, sorting and
many other callback functions would have to be fortified. The
OPT_CMDMODE patches were a test balloon for a small subset of the void
space. Extending them to cover more or even all of the int options may
give a better picture of the price for such a feature.
Will it uncover user-visible bugs? So far it hasn't, not directly.
Perhaps ASan suffices and we don't need to care about exotic platforms
that would punish our type sins.
Dealing with the OPT_CMDMODE code prompted to me to come up with ways
to improve its error messages, though. These patches took me
surprisingly long to prepare (plus my Git time is quite limited these
days). I'll send them separately.
> I do not mind keeping it on 'seen', especially if
> these two early steps are not expected to change. It is not like
> they are causing any maintenance burden.
The OPT_CMDMODE error message improvements change the code a lot and
conflict with these parse-options patches. Please drop the latter and
let's discuss the former first, as their user-visible changes are more
tangible and useful. I'll send an adjusted and perhaps bigger type
safety test balloon eventually.
René
^ permalink raw reply
* [PATCH] reflog: fix expire --single-worktree
From: René Scharfe @ 2023-10-28 11:58 UTC (permalink / raw)
To: Git List; +Cc: John Cai
33d7bdd645 (builtin/reflog.c: use parse-options api for expire, delete
subcommands, 2022-01-06) broke the option --single-worktree of git
reflog expire and added a non-printable short flag for it, presumably by
accident. While before it set the variable "all_worktrees" to 0, now it
sets it to 1, its default value. --no-single-worktree is required now
to set it to 0.
Fix it by replacing the variable with one that has the opposite meaning,
to avoid the negation and its potential for confusion. The new variable
"single_worktree" directly captures whether --single-worktree was given.
Also remove the unprintable short flag SOH (start of heading) because it
is undocumented, hard to use and is likely to have been added by mistake
in connection with the negation bug above.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/reflog.c | 6 +++---
t/t1410-reflog.sh | 23 +++++++++++++++++++++++
2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index df63a5892e..21337292f5 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -243,7 +243,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
{
struct cmd_reflog_expire_cb cmd = { 0 };
timestamp_t now = time(NULL);
- int i, status, do_all, all_worktrees = 1;
+ int i, status, do_all, single_worktree = 0;
unsigned int flags = 0;
int verbose = 0;
reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
@@ -268,7 +268,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "stale-fix", &cmd.stalefix,
N_("prune any reflog entries that point to broken commits")),
OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
- OPT_BOOL(1, "single-worktree", &all_worktrees,
+ OPT_BOOL(0, "single-worktree", &single_worktree,
N_("limits processing to reflogs from the current worktree only")),
OPT_END()
};
@@ -318,7 +318,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
worktrees = get_worktrees();
for (p = worktrees; *p; p++) {
- if (!all_worktrees && !(*p)->is_current)
+ if (single_worktree && !(*p)->is_current)
continue;
collected.worktree = *p;
refs_for_each_reflog(get_worktree_ref_store(*p),
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 6c45965b1e..09e7f3cdac 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -446,6 +446,29 @@ test_expect_success 'expire with multiple worktrees' '
)
'
+test_expect_success 'expire one of multiple worktrees' '
+ git init main-wt2 &&
+ (
+ cd main-wt2 &&
+ test_tick &&
+ test_commit foo &&
+ git worktree add link-wt &&
+ test_tick &&
+ test_commit -C link-wt foobar &&
+ test_tick &&
+ test-tool ref-store worktree:link-wt for-each-reflog-ent HEAD \
+ >expect-link-wt &&
+ git reflog expire --verbose --all --expire=$test_tick \
+ --single-worktree &&
+ test-tool ref-store worktree:main for-each-reflog-ent HEAD \
+ >actual-main &&
+ test-tool ref-store worktree:link-wt for-each-reflog-ent HEAD \
+ >actual-link-wt &&
+ test_must_be_empty actual-main &&
+ test_cmp expect-link-wt actual-link-wt
+ )
+'
+
test_expect_success REFFILES 'empty reflog' '
test_when_finished "rm -rf empty" &&
git init empty &&
--
2.42.0
^ permalink raw reply related
* [PATCH] am, rebase: fix arghelp syntax of --empty
From: René Scharfe @ 2023-10-28 11:58 UTC (permalink / raw)
To: Git List; +Cc: Elijah Newren, 徐沛文 (Aleen)
Use parentheses and pipes to present alternatives in the argument help
for the --empty options of git am and git rebase, like in the rest of
the documentation.
While at it remove a stray use of the enum empty_action value
STOP_ON_EMPTY_COMMIT to indicate that no short option is present.
While it has a value of 0 and thus there is no user-visible change,
that enum is not meant to hold short option characters. Hard-code 0,
like we do for other options without a short option.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Documentation/git-rebase.txt | 4 ++--
builtin/am.c | 2 +-
builtin/rebase.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e7b39ad244..b4526ca246 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -289,7 +289,7 @@ See also INCOMPATIBLE OPTIONS below.
+
See also INCOMPATIBLE OPTIONS below.
---empty={drop,keep,ask}::
+--empty=(drop|keep|ask)::
How to handle commits that are not empty to start and are not
clean cherry-picks of any upstream commit, but which become
empty after rebasing (because they contain a subset of already
@@ -695,7 +695,7 @@ be dropped automatically with `--no-keep-empty`).
Similar to the apply backend, by default the merge backend drops
commits that become empty unless `-i`/`--interactive` is specified (in
which case it stops and asks the user what to do). The merge backend
-also has an `--empty={drop,keep,ask}` option for changing the behavior
+also has an `--empty=(drop|keep|ask)` option for changing the behavior
of handling commits that become empty.
Directory rename detection
diff --git a/builtin/am.c b/builtin/am.c
index 6655059a57..9bb73d1671 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2420,7 +2420,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
{ OPTION_STRING, 'S', "gpg-sign", &state.sign_commit, N_("key-id"),
N_("GPG-sign commits"),
PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
- OPT_CALLBACK_F(STOP_ON_EMPTY_COMMIT, "empty", &state.empty_type, "{stop,drop,keep}",
+ OPT_CALLBACK_F(0, "empty", &state.empty_type, "(stop|drop|keep)",
N_("how to handle empty patches"),
PARSE_OPT_NONEG, am_option_parse_empty),
OPT_HIDDEN_BOOL(0, "rebasing", &state.rebasing,
diff --git a/builtin/rebase.c b/builtin/rebase.c
index f990811614..ad7baedd4a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1147,7 +1147,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
"instead of ignoring them"),
1, PARSE_OPT_HIDDEN),
OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
- OPT_CALLBACK_F(0, "empty", &options, "{drop,keep,ask}",
+ OPT_CALLBACK_F(0, "empty", &options, "(drop|keep|ask)",
N_("how to handle commits that become empty"),
PARSE_OPT_NONEG, parse_opt_empty),
OPT_CALLBACK_F('k', "keep-empty", &options, NULL,
--
2.42.0
^ permalink raw reply related
* [PATCH 2/2] am: simplify --show-current-patch handling
From: René Scharfe @ 2023-10-28 11:57 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
In-Reply-To: <cefdba32-db0b-4f68-954e-9d31fc12b1a0@web.de>
Let the parse-options code detect and handle the use of options that are
incompatible with --show-current-patch. This requires exposing the
distinction between the "raw" and "diff" sub-modes. Do that by
splitting the mode RESUME_SHOW_PATCH into RESUME_SHOW_PATCH_RAW and
RESUME_SHOW_PATCH_DIFF and stop tracking sub-modes in a separate struct.
The result is a simpler callback function and more precise error
messages. The original reports a spurious argument or a NULL pointer:
$ git am --show-current-patch --show-current-patch=diff
error: options '--show-current-patch=diff' and '--show-current-patch=raw' cannot be used together
$ git am --show-current-patch=diff --show-current-patch
error: options '--show-current-patch=(null)' and '--show-current-patch=diff' cannot be used together
With this patch we get the more precise:
$ git am --show-current-patch --show-current-patch=diff
error: --show-current-patch=diff is incompatible with --show-current-patch
$ git am --show-current-patch=diff --show-current-patch
error: --show-current-patch is incompatible with --show-current-patch=diff
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/am.c | 112 ++++++++++++++++++++-------------------------------
1 file changed, 44 insertions(+), 68 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 6655059a57..ef5b9459af 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -92,9 +92,16 @@ enum signoff_type {
SIGNOFF_EXPLICIT /* --signoff was set on the command-line */
};
-enum show_patch_type {
- SHOW_PATCH_RAW = 0,
- SHOW_PATCH_DIFF = 1,
+enum resume_type {
+ RESUME_FALSE = 0,
+ RESUME_APPLY,
+ RESUME_RESOLVED,
+ RESUME_SKIP,
+ RESUME_ABORT,
+ RESUME_QUIT,
+ RESUME_SHOW_PATCH_RAW,
+ RESUME_SHOW_PATCH_DIFF,
+ RESUME_ALLOW_EMPTY,
};
enum empty_action {
@@ -2191,7 +2198,7 @@ static void am_abort(struct am_state *state)
am_destroy(state);
}
-static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
+static int show_patch(struct am_state *state, enum resume_type resume_mode)
{
struct strbuf sb = STRBUF_INIT;
const char *patch_path;
@@ -2206,11 +2213,11 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
return run_command(&cmd);
}
- switch (sub_mode) {
- case SHOW_PATCH_RAW:
+ switch (resume_mode) {
+ case RESUME_SHOW_PATCH_RAW:
patch_path = am_path(state, msgnum(state));
break;
- case SHOW_PATCH_DIFF:
+ case RESUME_SHOW_PATCH_DIFF:
patch_path = am_path(state, "patch");
break;
default:
@@ -2257,57 +2264,25 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int
return 0;
}
-enum resume_type {
- RESUME_FALSE = 0,
- RESUME_APPLY,
- RESUME_RESOLVED,
- RESUME_SKIP,
- RESUME_ABORT,
- RESUME_QUIT,
- RESUME_SHOW_PATCH,
- RESUME_ALLOW_EMPTY,
-};
-
-struct resume_mode {
- enum resume_type mode;
- enum show_patch_type sub_mode;
-};
-
static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
{
int *opt_value = opt->value;
- struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
+ BUG_ON_OPT_NEG(unset);
+
+ if (!arg)
+ *opt_value = opt->defval;
+ else if (!strcmp(arg, "raw"))
+ *opt_value = RESUME_SHOW_PATCH_RAW;
+ else if (!strcmp(arg, "diff"))
+ *opt_value = RESUME_SHOW_PATCH_DIFF;
/*
* Please update $__git_showcurrentpatch in git-completion.bash
* when you add new options
*/
- const char *valid_modes[] = {
- [SHOW_PATCH_DIFF] = "diff",
- [SHOW_PATCH_RAW] = "raw"
- };
- int new_value = SHOW_PATCH_RAW;
-
- BUG_ON_OPT_NEG(unset);
-
- if (arg) {
- for (new_value = 0; new_value < ARRAY_SIZE(valid_modes); new_value++) {
- if (!strcmp(arg, valid_modes[new_value]))
- break;
- }
- if (new_value >= ARRAY_SIZE(valid_modes))
- return error(_("invalid value for '%s': '%s'"),
- "--show-current-patch", arg);
- }
-
- if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
- return error(_("options '%s=%s' and '%s=%s' "
- "cannot be used together"),
- "--show-current-patch", arg,
- "--show-current-patch", valid_modes[resume->sub_mode]);
-
- resume->mode = RESUME_SHOW_PATCH;
- resume->sub_mode = new_value;
+ else
+ return error(_("invalid value for '%s': '%s'"),
+ "--show-current-patch", arg);
return 0;
}
@@ -2317,7 +2292,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
int binary = -1;
int keep_cr = -1;
int patch_format = PATCH_FORMAT_UNKNOWN;
- struct resume_mode resume = { .mode = RESUME_FALSE };
+ enum resume_type resume_mode = RESUME_FALSE;
int in_progress;
int ret = 0;
@@ -2388,27 +2363,27 @@ int cmd_am(int argc, const char **argv, const char *prefix)
PARSE_OPT_NOARG),
OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL,
N_("override error message when patch failure occurs")),
- OPT_CMDMODE(0, "continue", &resume.mode,
+ OPT_CMDMODE(0, "continue", &resume_mode,
N_("continue applying patches after resolving a conflict"),
RESUME_RESOLVED),
- OPT_CMDMODE('r', "resolved", &resume.mode,
+ OPT_CMDMODE('r', "resolved", &resume_mode,
N_("synonyms for --continue"),
RESUME_RESOLVED),
- OPT_CMDMODE(0, "skip", &resume.mode,
+ OPT_CMDMODE(0, "skip", &resume_mode,
N_("skip the current patch"),
RESUME_SKIP),
- OPT_CMDMODE(0, "abort", &resume.mode,
+ OPT_CMDMODE(0, "abort", &resume_mode,
N_("restore the original branch and abort the patching operation"),
RESUME_ABORT),
- OPT_CMDMODE(0, "quit", &resume.mode,
+ OPT_CMDMODE(0, "quit", &resume_mode,
N_("abort the patching operation but keep HEAD where it is"),
RESUME_QUIT),
- { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
+ { OPTION_CALLBACK, 0, "show-current-patch", &resume_mode,
"(diff|raw)",
N_("show the patch being applied"),
PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
- parse_opt_show_current_patch, RESUME_SHOW_PATCH },
- OPT_CMDMODE(0, "allow-empty", &resume.mode,
+ parse_opt_show_current_patch, RESUME_SHOW_PATCH_RAW },
+ OPT_CMDMODE(0, "allow-empty", &resume_mode,
N_("record the empty patch as an empty commit"),
RESUME_ALLOW_EMPTY),
OPT_BOOL(0, "committer-date-is-author-date",
@@ -2463,12 +2438,12 @@ int cmd_am(int argc, const char **argv, const char *prefix)
* intend to feed us a patch but wanted to continue
* unattended.
*/
- if (argc || (resume.mode == RESUME_FALSE && !isatty(0)))
+ if (argc || (resume_mode == RESUME_FALSE && !isatty(0)))
die(_("previous rebase directory %s still exists but mbox given."),
state.dir);
- if (resume.mode == RESUME_FALSE)
- resume.mode = RESUME_APPLY;
+ if (resume_mode == RESUME_FALSE)
+ resume_mode = RESUME_APPLY;
if (state.signoff == SIGNOFF_EXPLICIT)
am_append_signoff(&state);
@@ -2482,7 +2457,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
* stray directories.
*/
if (file_exists(state.dir) && !state.rebasing) {
- if (resume.mode == RESUME_ABORT || resume.mode == RESUME_QUIT) {
+ if (resume_mode == RESUME_ABORT || resume_mode == RESUME_QUIT) {
am_destroy(&state);
am_state_release(&state);
return 0;
@@ -2493,7 +2468,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
state.dir);
}
- if (resume.mode)
+ if (resume_mode)
die(_("Resolve operation not in progress, we are not resuming."));
for (i = 0; i < argc; i++) {
@@ -2511,7 +2486,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
strvec_clear(&paths);
}
- switch (resume.mode) {
+ switch (resume_mode) {
case RESUME_FALSE:
am_run(&state, 0);
break;
@@ -2520,7 +2495,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
break;
case RESUME_RESOLVED:
case RESUME_ALLOW_EMPTY:
- am_resolve(&state, resume.mode == RESUME_ALLOW_EMPTY ? 1 : 0);
+ am_resolve(&state, resume_mode == RESUME_ALLOW_EMPTY ? 1 : 0);
break;
case RESUME_SKIP:
am_skip(&state);
@@ -2532,8 +2507,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
am_rerere_clear();
am_destroy(&state);
break;
- case RESUME_SHOW_PATCH:
- ret = show_patch(&state, resume.sub_mode);
+ case RESUME_SHOW_PATCH_RAW:
+ case RESUME_SHOW_PATCH_DIFF:
+ ret = show_patch(&state, resume_mode);
break;
default:
BUG("invalid resume value");
--
2.42.0
^ permalink raw reply related
* [PATCH 1/2] parse-options: make CMDMODE errors more precise
From: René Scharfe @ 2023-10-28 11:53 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Only a single PARSE_OPT_CMDMODE option can be specified for the same
variable at the same time. This is enforced by get_value(), but the
error messages are imprecise in three ways:
1. If a non-PARSE_OPT_CMDMODE option changes the value variable of a
PARSE_OPT_CMDMODE option then an ominously vague message is shown:
$ t/helper/test-tool parse-options --set23 --mode1
error: option `mode1' : incompatible with something else
Worse: If the order of options is reversed then no error is reported at
all:
$ t/helper/test-tool parse-options --mode1 --set23
boolean: 0
integer: 23
magnitude: 0
timestamp: 0
string: (not set)
abbrev: 7
verbose: -1
quiet: 0
dry run: no
file: (not set)
Fortunately this can currently only happen in the test helper; actual
Git commands don't share the same variable for the value of options with
and without the flag PARSE_OPT_CMDMODE.
2. If there are multiple options with the same value (synonyms), then
the one that is defined first is shown rather than the one actually
given on the command line, which is confusing:
$ git am --resolved --quit
error: option `quit' is incompatible with --continue
3. Arguments of PARSE_OPT_CMDMODE options are not handled by the
parse-option machinery. This is left to the callback function. We
currently only have a single affected option, --show-current-patch of
git am. Errors for it can show an argument that was not actually given
on the command line:
$ git am --show-current-patch --show-current-patch=diff
error: options '--show-current-patch=diff' and '--show-current-patch=raw' cannot be used together
The options --show-current-patch and --show-current-patch=raw are
synonyms, but the error accuses the user of input they did not actually
made. Or it can awkwardly print a NULL pointer:
$ git am --show-current-patch=diff --show-current-patch
error: options '--show-current-patch=(null)' and '--show-current-patch=diff' cannot be used together
The reasons for these shortcomings is that the current code checks
incompatibility only when encountering a PARSE_OPT_CMDMODE option at the
command line, and that it searches the previous incompatible option by
value.
Fix the first two points by checking all PARSE_OPT_CMDMODE variables
after parsing each option and by storing all relevant details if their
value changed. Do that whether or not the changing options has the flag
PARSE_OPT_CMDMODE set. Report an incompatibility only if two options
change the variable to different values and at least one of them is a
PARSE_OPT_CMDMODE option. This changes the output of the first three
examples above to:
$ t/helper/test-tool parse-options --set23 --mode1
error: --mode1 is incompatible with --set23
$ t/helper/test-tool parse-options --mode1 --set23
error: --set23 is incompatible with --mode1
$ git am --resolved --quit
error: --quit is incompatible with --resolved
Store the argument of PARSE_OPT_CMDMODE options of type OPTION_CALLBACK
as well to allow taking over the responsibility for compatibility
checking from the callback function. The next patch will use this
capability to fix the messages for git am --show-current-patch.
Use a linked list for storing the PARSE_OPT_CMDMODE variables. This
somewhat outdated data structure is simple and suffices, as the number
of elements per command is currently only zero or one. We do support
multiple different command modes variables per command, but I don't
expect that we'd ever use a significant number of them. Once we do we
can switch to a hashmap.
Since we no longer need to search the conflicting option, the all_opts
parameter of get_value() is no longer used. Remove it.
Extend the tests to check for both conflicting option names, but don't
insist on a particular order.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
parse-options.c | 144 ++++++++++++++++++++++------------
parse-options.h | 3 +
t/helper/test-parse-options.c | 16 ++++
t/t0040-parse-options.sh | 33 ++++++--
4 files changed, 139 insertions(+), 57 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 093eaf2db8..e0c94b0546 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -70,42 +70,10 @@ static void fix_filename(const char *prefix, char **file)
*file = prefix_filename_except_for_dash(prefix, *file);
}
-static enum parse_opt_result opt_command_mode_error(
- const struct option *opt,
- const struct option *all_opts,
- enum opt_parsed flags)
-{
- const struct option *that;
- struct strbuf that_name = STRBUF_INIT;
-
- /*
- * Find the other option that was used to set the variable
- * already, and report that this is not compatible with it.
- */
- for (that = all_opts; that->type != OPTION_END; that++) {
- if (that == opt ||
- !(that->flags & PARSE_OPT_CMDMODE) ||
- that->value != opt->value ||
- that->defval != *(int *)opt->value)
- continue;
-
- if (that->long_name)
- strbuf_addf(&that_name, "--%s", that->long_name);
- else
- strbuf_addf(&that_name, "-%c", that->short_name);
- error(_("%s is incompatible with %s"),
- optname(opt, flags), that_name.buf);
- strbuf_release(&that_name);
- return PARSE_OPT_ERROR;
- }
- return error(_("%s : incompatible with something else"),
- optname(opt, flags));
-}
-
-static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
- const struct option *opt,
- const struct option *all_opts,
- enum opt_parsed flags)
+static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
+ const struct option *opt,
+ enum opt_parsed flags,
+ const char **argp)
{
const char *s, *arg;
const int unset = flags & OPT_UNSET;
@@ -118,14 +86,6 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))
return error(_("%s takes no value"), optname(opt, flags));
- /*
- * Giving the same mode option twice, although unnecessary,
- * is not a grave error, so let it pass.
- */
- if ((opt->flags & PARSE_OPT_CMDMODE) &&
- *(int *)opt->value && *(int *)opt->value != opt->defval)
- return opt_command_mode_error(opt, all_opts, flags);
-
switch (opt->type) {
case OPTION_LOWLEVEL_CALLBACK:
return opt->ll_callback(p, opt, NULL, unset);
@@ -200,6 +160,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
p_unset = 0;
p_arg = arg;
}
+ if (opt->flags & PARSE_OPT_CMDMODE)
+ *argp = p_arg;
if (opt->callback)
return (*opt->callback)(opt, p_arg, p_unset) ? (-1) : 0;
else
@@ -247,16 +209,91 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
}
}
+struct parse_opt_cmdmode_list {
+ int value, *value_ptr;
+ const struct option *opt;
+ const char *arg;
+ enum opt_parsed flags;
+ struct parse_opt_cmdmode_list *next;
+};
+
+static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
+ const struct option *opts)
+{
+ ctx->cmdmode_list = NULL;
+
+ for (; opts->type != OPTION_END; opts++) {
+ struct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;
+ int *value_ptr = opts->value;
+
+ if (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)
+ continue;
+
+ while (elem && elem->value_ptr != value_ptr)
+ elem = elem->next;
+ if (elem)
+ continue;
+
+ CALLOC_ARRAY(elem, 1);
+ elem->value_ptr = value_ptr;
+ elem->value = *value_ptr;
+ elem->next = ctx->cmdmode_list;
+ ctx->cmdmode_list = elem;
+ }
+}
+
+static char *optnamearg(const struct option *opt, const char *arg,
+ enum opt_parsed flags)
+{
+ if (flags & OPT_SHORT)
+ return xstrfmt("-%c%s", opt->short_name, arg ? arg : "");
+ return xstrfmt("--%s%s%s%s", flags & OPT_UNSET ? "no-" : "",
+ opt->long_name, arg ? "=" : "", arg ? arg : "");
+}
+
+static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
+ const struct option *opt,
+ enum opt_parsed flags)
+{
+ const char *arg = NULL;
+ enum parse_opt_result result = do_get_value(p, opt, flags, &arg);
+ struct parse_opt_cmdmode_list *elem = p->cmdmode_list;
+ char *opt_name, *other_opt_name;
+
+ for (; elem; elem = elem->next) {
+ if (*elem->value_ptr == elem->value)
+ continue;
+
+ if (elem->opt &&
+ (elem->opt->flags | opt->flags) & PARSE_OPT_CMDMODE)
+ break;
+
+ elem->opt = opt;
+ elem->arg = arg;
+ elem->flags = flags;
+ elem->value = *elem->value_ptr;
+ }
+
+ if (result || !elem)
+ return result;
+
+ opt_name = optnamearg(opt, arg, flags);
+ other_opt_name = optnamearg(elem->opt, elem->arg, elem->flags);
+ error(_("%s is incompatible with %s"), opt_name, other_opt_name);
+ free(opt_name);
+ free(other_opt_name);
+ return -1;
+}
+
static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
const struct option *options)
{
- const struct option *all_opts = options;
const struct option *numopt = NULL;
for (; options->type != OPTION_END; options++) {
if (options->short_name == *p->opt) {
p->opt = p->opt[1] ? p->opt + 1 : NULL;
- return get_value(p, options, all_opts, OPT_SHORT);
+ return get_value(p, options, OPT_SHORT);
}
/*
@@ -318,7 +355,6 @@ static enum parse_opt_result parse_long_opt(
struct parse_opt_ctx_t *p, const char *arg,
const struct option *options)
{
- const struct option *all_opts = options;
const char *arg_end = strchrnul(arg, '=');
const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
@@ -387,7 +423,7 @@ static enum parse_opt_result parse_long_opt(
continue;
p->opt = rest + 1;
}
- return get_value(p, options, all_opts, flags ^ opt_flags);
+ return get_value(p, options, flags ^ opt_flags);
}
if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
@@ -405,7 +441,7 @@ static enum parse_opt_result parse_long_opt(
return PARSE_OPT_HELP;
}
if (abbrev_option)
- return get_value(p, abbrev_option, all_opts, abbrev_flags);
+ return get_value(p, abbrev_option, abbrev_flags);
return PARSE_OPT_UNKNOWN;
}
@@ -413,13 +449,11 @@ static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p,
const char *arg,
const struct option *options)
{
- const struct option *all_opts = options;
-
for (; options->type != OPTION_END; options++) {
if (!(options->flags & PARSE_OPT_NODASH))
continue;
if (options->short_name == arg[0] && arg[1] == '\0')
- return get_value(p, options, all_opts, OPT_SHORT);
+ return get_value(p, options, OPT_SHORT);
}
return PARSE_OPT_ERROR;
}
@@ -574,6 +608,7 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
(flags & PARSE_OPT_KEEP_ARGV0))
BUG("Can't keep argv0 if you don't have it");
parse_options_check(options);
+ build_cmdmode_list(ctx, options);
}
void parse_options_start(struct parse_opt_ctx_t *ctx,
@@ -1006,6 +1041,11 @@ int parse_options(int argc, const char **argv,
precompose_argv_prefix(argc, argv, NULL);
free_preprocessed_options(real_options);
free(ctx.alias_groups);
+ for (struct parse_opt_cmdmode_list *elem = ctx.cmdmode_list; elem;) {
+ struct parse_opt_cmdmode_list *next = elem->next;
+ free(elem);
+ elem = next;
+ }
return parse_options_end(&ctx);
}
diff --git a/parse-options.h b/parse-options.h
index 4a66ec3bf5..bd62e20268 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -445,6 +445,8 @@ static inline void die_for_incompatible_opt3(int opt1, const char *opt1_name,
/*----- incremental advanced APIs -----*/
+struct parse_opt_cmdmode_list;
+
/*
* It's okay for the caller to consume argv/argc in the usual way.
* Other fields of that structure are private to parse-options and should not
@@ -459,6 +461,7 @@ struct parse_opt_ctx_t {
unsigned has_subcommands;
const char *prefix;
const char **alias_groups; /* must be in groups of 3 elements! */
+ struct parse_opt_cmdmode_list *cmdmode_list;
};
void parse_options_start(struct parse_opt_ctx_t *ctx,
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index a4f6e24b0c..ded8116cc5 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -21,6 +21,19 @@ static struct {
int unset;
} length_cb;
+static int mode34_callback(const struct option *opt, const char *arg, int unset)
+{
+ if (unset)
+ *(int *)opt->value = 0;
+ else if (!strcmp(arg, "3"))
+ *(int *)opt->value = 3;
+ else if (!strcmp(arg, "4"))
+ *(int *)opt->value = 4;
+ else
+ return error("invalid value for '%s': '%s'", "--mode34", arg);
+ return 0;
+}
+
static int length_callback(const struct option *opt, const char *arg, int unset)
{
length_cb.called = 1;
@@ -124,6 +137,9 @@ int cmd__parse_options(int argc, const char **argv)
OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
OPT_CMDMODE(0, "mode1", &integer, "set integer to 1 (cmdmode option)", 1),
OPT_CMDMODE(0, "mode2", &integer, "set integer to 2 (cmdmode option)", 2),
+ OPT_CALLBACK_F(0, "mode34", &integer, "(3|4)",
+ "set integer to 3 or 4 (cmdmode option)",
+ PARSE_OPT_CMDMODE, mode34_callback),
OPT_CALLBACK('L', "length", &integer, "str",
"get length of <str>", length_callback),
OPT_FILENAME('F', "file", &file, "set file to <file>"),
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index a0ad6192d6..06fb9e6457 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -28,6 +28,7 @@ usage: test-tool parse-options <options>
--[no-]set23 set integer to 23
--mode1 set integer to 1 (cmdmode option)
--mode2 set integer to 2 (cmdmode option)
+ --[no-]mode34 (3|4) set integer to 3 or 4 (cmdmode option)
-L, --[no-]length <str>
get length of <str>
-F, --[no-]file <file>
@@ -366,19 +367,41 @@ test_expect_success 'OPT_NEGBIT() works' '
'
test_expect_success 'OPT_CMDMODE() works' '
- test-tool parse-options --expect="integer: 1" --mode1
+ test-tool parse-options --expect="integer: 1" --mode1 &&
+ test-tool parse-options --expect="integer: 3" --mode34=3
'
-test_expect_success 'OPT_CMDMODE() detects incompatibility' '
+test_expect_success 'OPT_CMDMODE() detects incompatibility (1)' '
test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&
test_must_be_empty output &&
- test_i18ngrep "incompatible with --mode" output.err
+ test_i18ngrep "mode1" output.err &&
+ test_i18ngrep "mode2" output.err &&
+ test_i18ngrep "is incompatible with" output.err
'
-test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '
+test_expect_success 'OPT_CMDMODE() detects incompatibility (2)' '
test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&
test_must_be_empty output &&
- test_i18ngrep "incompatible with something else" output.err
+ test_i18ngrep "mode2" output.err &&
+ test_i18ngrep "set23" output.err &&
+ test_i18ngrep "is incompatible with" output.err
+'
+
+test_expect_success 'OPT_CMDMODE() detects incompatibility (3)' '
+ test_must_fail test-tool parse-options --mode2 --set23 >output 2>output.err &&
+ test_must_be_empty output &&
+ test_i18ngrep "mode2" output.err &&
+ test_i18ngrep "set23" output.err &&
+ test_i18ngrep "is incompatible with" output.err
+'
+
+test_expect_success 'OPT_CMDMODE() detects incompatibility (4)' '
+ test_must_fail test-tool parse-options --mode2 --mode34=3 \
+ >output 2>output.err &&
+ test_must_be_empty output &&
+ test_i18ngrep "mode2" output.err &&
+ test_i18ngrep "mode34.3" output.err &&
+ test_i18ngrep "is incompatible with" output.err
'
test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '
--
2.42.0
^ permalink raw reply related
* Re: [RFC][Outreachy] Seeking Git Community Feedback on My Application
From: Isoken Ibizugbe @ 2023-10-28 10:40 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD16OAPiRFJfjZN=soAe3WzDBteyvzv-b3CD67jz6Haqyg@mail.gmail.com>
On Sat, Oct 28, 2023 at 9:07 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Oct 25, 2023 at 2:46 PM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
>
> > Thank you for the review. I have made changes to the project plan and
> > it emphasizes the critical tasks of identifying, selecting, and
> > porting tests, making it more concise and aligned with the project's
> > scope.
>
> Good.
>
> > - Community Bonding (Oct 2 - Nov 20): Microproject contribution, Git
> > project application, get familiar with the Git codebase and testing
> > ecosystem.
> > -Identify and Select Tests: Identify and prioritize tests worth
> > porting, and document the selected tests. (I would classify tests that
> > are worth porting according to the following for now;
> >
> > Relevance: Prioritize tests that are relevant to the current Git codebase.
> > Coverage: Focus on tests that cover core functionality or critical code paths.
> > Usage Frequency: Port tests that are frequently used or run in Git's
> > development process.
> > Isolation: Choose tests that can be easily ported and run independently.
>
> I think the main issue with identification is that now in t/helper/ we
> have both:
>
> 1) code that implements helpers that are used by the end-to-end
> tests scripts written in shell and named "t/tXXXX-*.sh" where XXXX is
> a number, and
> 2) code that implements unit tests for some C code in the code base.
>
> So I think only 2) should be ported to the unit test framework, and 1)
> should not be ported and stay in t/helper/.
>
> > - Write Unit Tests: Write unit tests for the identified test cases
> > using the Git custom test framework.
> > - Port Existing Tests: Port selected test cases from the t/helper
> > directory to the unit testing framework, by modifying them to work
> > within the custom TAP framework.
> > - Test Execution and Debugging: Execute the newly written unit tests
> > and the ported tests using the test framework.
> > - Seek Feedback: Share the progress with mentors and the Git
> > community, and address any concerns or suggestions provided by the
> > community.
> > - Documentation and Reporting: Document the entire process of
> > migrating Git's tests to the unit testing framework, and prepare a
> > final project report summarizing the work done, challenges faced, and
> > lessons learned.
> >
> > What is the custom TAP framework?
> >
> > According to this patch
> > (https://lore.kernel.org/git/ca284c575ece0aee7149641d5fb1977ccd7e7873.1692229626.git.steadmon@google.com/)
> > by Phillip Wood, which contains an example implementation for writing
> > unit tests with TAP output. The custom TAP framework is a Test
> > Anything Protocol (TAP) framework that allows for clear reporting of
> > test results, aiding in debugging and troubleshooting.
>
> Ok. Our end-to-end tests scripts written in shell already use TAP,
> that's why it's nice to have unit tests also using TAP.
>
> > The framework contains the following features:
> >
> > - Test Structure: Unit tests are defined as functions containing
> > multiple checks. The tests are run using the TEST() macro. If any
> > checks within a test fail, the entire test is marked as failed.
> > - Output Format: The output of the test program follows the TAP
> > format. It includes a series of messages describing the test's status.
> > For passed tests, it reports "ok," and for failed tests, it reports
> > "not ok." Each test is numbered, e.g., "ok 1 - static initialization
> > works," to indicate success or failure.
> > - Check Functions: Several check functions are available, including
> > check() for boolean conditions, check_int(), check_uint(), and
> > check_char() for comparing values using various operators. check_str()
> > is used to compare strings.
> > - Skipping Tests: Tests can be skipped using test_skip() and can
> > include a reason for skipping, which is printed as part of the report.
> > - Diagnostic Messages: Tests can generate diagnostic messages using
> > test_msg() to provide additional context or explanations for test
> > failures.
> > - Planned Failing Tests: Tests that are known to fail can be marked
> > with TEST_TODO(). These tests will still run, and the failures will be
> > reported, but they will not cause the entire suite to fail.
> > - Building and Running: The unit tests can be built with "make
> > unit-tests" (with some additional Makefile changes), and they can be
> > executed manually or using a tool like prove.
>
> Ok.
>
> > Using the formerly given criteria, test-ctype.c is suitable for
> > porting because it tests character type checks used extensively in
> > Git. These tests cover various character types and their expected
> > behaviour, ensuring the correctness and reliability of Git's
> > operations, and test-ctype.c isolation makes it suitable for porting
> > without relying on multiple libraries.
>
> Ok.
>
> > Here is a sample of the implementation of how I would write the unit
> > test following the custom TAP framework taking t/helper/test-ctype.c
> >
> > - Create and rename the new .c file;
> > I would rename it according to the convention done in the t/unit-test
> > directory, by starting the name with a “t-” prefix e.g t-ctype.c
> >
> > - Document the tests and include the necessary headers:
> > /**
> > *Tests the behavior of ctype
> > *functions
> > */
> > #include "test-lib.h"
> > #include "ctype.h"
> >
> > - Define test functions:
> > #define DIGIT "0123456789"
> >
> > static void t_digit_type(void)
> > {
> > int i;
> > const char *digits = DIGIT;
> > for (i = 0; digits[i]; i++)
> > {
> > check_int(isdigit(digits[i]), ==, 0);
> > }
>
> This tests that isdigit() returns 0 for each of the characters in
> "0123456789", but first I think isdigit() should return 1, not 0 for
> those characters.
yes, that is true. should I send a re-roll?
>
> And second, I think the test should check the value returned by
> isdigit() for each of the 256 possible values of a char, not just for
> the characters in "0123456789".
>
> test-ctype.c is doing the right thing regarding those 2 issues.
>
> > - Include main function which will call the test functions using the TEST macro;
> > int main(void)
> > {
> > TEST(t_digit_type(), "Character is a digit");
> > return test_done();
> > }
> >
> > - Run the tests:
> > ‘make && make’ unit-tests can be used build and run the unit tests
> > Or run the test binaries directly:
> > ./t/unit-tests/t-ctype.c
> >
> > The Makefile will be modified to add the file;
> > UNIT_TEST_PROGRAMS += t-ctype
> > The test output will be in the TAP format and will indicate which
> > tests passed(ok) and which failed(not ok), along with diagnostic
> > messages in case of failures.
> >
> > ok 1 - Character is a digit
> >
> > 1..1
>
> Yeah, this looks right.
>
> Thanks,
> Christian.
^ permalink raw reply
* Re: [OUTREACHY] Final Application For Git Internshhip
From: Christian Couder @ 2023-10-28 8:24 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <CACS=G2yGj2FYp9XBCknKqEXh5ZWXiQFLWrWk+SAmWzDMrjJwQg@mail.gmail.com>
On Wed, Oct 25, 2023 at 2:14 PM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>>
>> It could help to say if your contribution has been merged to 'master',
>> 'next', 'seen' or not at all.
>
> My microproject contribution has been scheduled to be merged to the master branch.
Great, please add this information to your application.
>> I think that one of the important tasks to be done early is to
>> identify what code in t/helper is unit testing C code and what code is
>> really about helping other tests in the t/t*.sh scripts. It would be
>> nice if you could give an example of each kind of code.
>
> In my opinion, helper/test-ctype.c is a unit test file containing a set of unit tests for character classification functions,
Right.
> while helper/test-dir-iterator.c is a unit test file which works together with the t/t0066-dir-iterator.sh file to iterate through a directory and give details on its contents. It likely is used for testing and inspecting directory structures and file types within a specified path.
Actually "t/helper/test-dir-iterator.c" and "t/t0066-dir-iterator.sh"
are used together to test the code in "dir-iterator.h" and
"dir-iterator.c", so it's kind of special. Ideally this could be
ported to the unit test framework as the goal is to test quite low
level code (instead of a full git command for example), but I am not
sure how easy it would be, and if it would even be worth it.
>> An example of how you would migrate parts of a test, or how a migrated
>> test would look like, would be nice.
>
> I'd first create a new test file, then include "test-libtap/tap.h" and "test-tool.h" header files, then I would initialize TAP with this command plan_tests(x), where x defines the number of tests to be run inside that file.
> Below the plan_tests();, I'd migrate and edit my specific test function and requirements, and after that, I'd add my "done_testing();" and then "return exit_status();"
These are quite good guidelines, but not quite an example.
Thanks,
Christian.
^ permalink raw reply
* Re: [RFC][Outreachy] Seeking Git Community Feedback on My Application
From: Christian Couder @ 2023-10-28 8:07 UTC (permalink / raw)
To: Isoken Ibizugbe; +Cc: git
In-Reply-To: <CAJHH8bH0gp9tbDJ4DYk3jkNPD5_dZ9s62D9ae3q33aBP0ZL9Lg@mail.gmail.com>
On Wed, Oct 25, 2023 at 2:46 PM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
> Thank you for the review. I have made changes to the project plan and
> it emphasizes the critical tasks of identifying, selecting, and
> porting tests, making it more concise and aligned with the project's
> scope.
Good.
> - Community Bonding (Oct 2 - Nov 20): Microproject contribution, Git
> project application, get familiar with the Git codebase and testing
> ecosystem.
> -Identify and Select Tests: Identify and prioritize tests worth
> porting, and document the selected tests. (I would classify tests that
> are worth porting according to the following for now;
>
> Relevance: Prioritize tests that are relevant to the current Git codebase.
> Coverage: Focus on tests that cover core functionality or critical code paths.
> Usage Frequency: Port tests that are frequently used or run in Git's
> development process.
> Isolation: Choose tests that can be easily ported and run independently.
I think the main issue with identification is that now in t/helper/ we
have both:
1) code that implements helpers that are used by the end-to-end
tests scripts written in shell and named "t/tXXXX-*.sh" where XXXX is
a number, and
2) code that implements unit tests for some C code in the code base.
So I think only 2) should be ported to the unit test framework, and 1)
should not be ported and stay in t/helper/.
> - Write Unit Tests: Write unit tests for the identified test cases
> using the Git custom test framework.
> - Port Existing Tests: Port selected test cases from the t/helper
> directory to the unit testing framework, by modifying them to work
> within the custom TAP framework.
> - Test Execution and Debugging: Execute the newly written unit tests
> and the ported tests using the test framework.
> - Seek Feedback: Share the progress with mentors and the Git
> community, and address any concerns or suggestions provided by the
> community.
> - Documentation and Reporting: Document the entire process of
> migrating Git's tests to the unit testing framework, and prepare a
> final project report summarizing the work done, challenges faced, and
> lessons learned.
>
> What is the custom TAP framework?
>
> According to this patch
> (https://lore.kernel.org/git/ca284c575ece0aee7149641d5fb1977ccd7e7873.1692229626.git.steadmon@google.com/)
> by Phillip Wood, which contains an example implementation for writing
> unit tests with TAP output. The custom TAP framework is a Test
> Anything Protocol (TAP) framework that allows for clear reporting of
> test results, aiding in debugging and troubleshooting.
Ok. Our end-to-end tests scripts written in shell already use TAP,
that's why it's nice to have unit tests also using TAP.
> The framework contains the following features:
>
> - Test Structure: Unit tests are defined as functions containing
> multiple checks. The tests are run using the TEST() macro. If any
> checks within a test fail, the entire test is marked as failed.
> - Output Format: The output of the test program follows the TAP
> format. It includes a series of messages describing the test's status.
> For passed tests, it reports "ok," and for failed tests, it reports
> "not ok." Each test is numbered, e.g., "ok 1 - static initialization
> works," to indicate success or failure.
> - Check Functions: Several check functions are available, including
> check() for boolean conditions, check_int(), check_uint(), and
> check_char() for comparing values using various operators. check_str()
> is used to compare strings.
> - Skipping Tests: Tests can be skipped using test_skip() and can
> include a reason for skipping, which is printed as part of the report.
> - Diagnostic Messages: Tests can generate diagnostic messages using
> test_msg() to provide additional context or explanations for test
> failures.
> - Planned Failing Tests: Tests that are known to fail can be marked
> with TEST_TODO(). These tests will still run, and the failures will be
> reported, but they will not cause the entire suite to fail.
> - Building and Running: The unit tests can be built with "make
> unit-tests" (with some additional Makefile changes), and they can be
> executed manually or using a tool like prove.
Ok.
> Using the formerly given criteria, test-ctype.c is suitable for
> porting because it tests character type checks used extensively in
> Git. These tests cover various character types and their expected
> behaviour, ensuring the correctness and reliability of Git's
> operations, and test-ctype.c isolation makes it suitable for porting
> without relying on multiple libraries.
Ok.
> Here is a sample of the implementation of how I would write the unit
> test following the custom TAP framework taking t/helper/test-ctype.c
>
> - Create and rename the new .c file;
> I would rename it according to the convention done in the t/unit-test
> directory, by starting the name with a “t-” prefix e.g t-ctype.c
>
> - Document the tests and include the necessary headers:
> /**
> *Tests the behavior of ctype
> *functions
> */
> #include "test-lib.h"
> #include "ctype.h"
>
> - Define test functions:
> #define DIGIT "0123456789"
>
> static void t_digit_type(void)
> {
> int i;
> const char *digits = DIGIT;
> for (i = 0; digits[i]; i++)
> {
> check_int(isdigit(digits[i]), ==, 0);
> }
This tests that isdigit() returns 0 for each of the characters in
"0123456789", but first I think isdigit() should return 1, not 0 for
those characters.
And second, I think the test should check the value returned by
isdigit() for each of the 256 possible values of a char, not just for
the characters in "0123456789".
test-ctype.c is doing the right thing regarding those 2 issues.
> - Include main function which will call the test functions using the TEST macro;
> int main(void)
> {
> TEST(t_digit_type(), "Character is a digit");
> return test_done();
> }
>
> - Run the tests:
> ‘make && make’ unit-tests can be used build and run the unit tests
> Or run the test binaries directly:
> ./t/unit-tests/t-ctype.c
>
> The Makefile will be modified to add the file;
> UNIT_TEST_PROGRAMS += t-ctype
> The test output will be in the TAP format and will indicate which
> tests passed(ok) and which failed(not ok), along with diagnostic
> messages in case of failures.
>
> ok 1 - Character is a digit
>
> 1..1
Yeah, this looks right.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Andy Koppe @ 2023-10-28 7:02 UTC (permalink / raw)
To: Liam Beguin, Jeff King; +Cc: Junio C Hamano, Kousik Sanagavarapu, git
In-Reply-To: <9a1e3e90-3e94-41fa-897d-5c64c4a42871@gmail.com>
On 28/10/2023 07:58, Andy Koppe wrote:
> On 28/10/2023 04:22, Liam Beguin wrote:
>> On Fri, Oct 27, 2023 at 10:13:01PM -0400, Jeff King wrote:
>>> One way you could directly use this is in shortlog, which these days
>>> lets you group by specific formats. So:
>>>
>>> git shortlog -ns --group=format:%aA
>>
>> That's exactly what I implemented this for :-)
>
> Another potential use case is custom log formats where one might want to
> color the local-part separately from the domain part.
>
>>> It also feels like a symmetric match to "%al", which already exists.
>
> Speaking of symmetry, I think it would need "%c" counterparts for the
> coloring use case.
D'oh, it always helps to read the actual patches, which do have the %c
variants.
Thanks,
Andy
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Andy Koppe @ 2023-10-28 6:58 UTC (permalink / raw)
To: Liam Beguin, Jeff King; +Cc: Junio C Hamano, Kousik Sanagavarapu, git
In-Reply-To: <20231028032221.GB1784118@shaak>
On 28/10/2023 04:22, Liam Beguin wrote:
> On Fri, Oct 27, 2023 at 10:13:01PM -0400, Jeff King wrote:
>> One way you could directly use this is in shortlog, which these days
>> lets you group by specific formats. So:
>>
>> git shortlog -ns --group=format:%aA
>
> That's exactly what I implemented this for :-)
Another potential use case is custom log formats where one might want to
color the local-part separately from the domain part.
>> It also feels like a symmetric match to "%al", which already exists.
Speaking of symmetry, I think it would need "%c" counterparts for the
coloring use case.
> I chose the "a" for "address", but I'm not sold on %aa either.
> I just couldn't find anything better that wasn't already taken.
>
> What about "a@"?
Makes sense, and I suppose there's "%G?" as precedent for using a symbol
rather than letter in these.
If that's not suitable though, how about "m" for "mail domain"? It also
immediately follows "l" for "local-part" in the alphabet.
Regards,
Andy
^ permalink raw reply
* Re: [RFC PATCH v2 0/6] Noobify format for status, add, restore
From: Dragan Simic @ 2023-10-28 5:55 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <ZTx3fIGpdGl4JpaV.jacob@initialcommit.io>
On 2023-10-28 04:52, Jacob Stopak wrote:
>> They currently don't exist, but that's something I've planned to
>> implement,
>> e.g. to "add.verbose" as a new configuration option. It should be
>> usable,
>> while not being messy or intrusive as a new feature.
>
> "git add" already has the -v, --verbose flag available for the command
> itself like:
>
> $ git add -v foo.txt
> add 'foo.txt'
>
> But like you said the config option add.verbose doesn't seem to exist
> yet.
>
> So I assume an "add.verbose" config option would just always print that
> without having to specify the -v, --verbose flag when running the
> command?
Yes, that's how I see it. Setting "add.verbose" to "true", to be
precise, or to "basic", which I'll explain a bit further later in my
response.
> Basically what I'm asking is if commands that already have a --verbose
> flag
> would just get a config setting that does the existing thing by
> default?
Well, not by default. The default values would remain "false", so
nothing jumps out of nowhere.
>>> So it seems like currently "verbose" is used for various things among
>>> the command set...
>>
>> Yes, that's the basic verbosity, as I named it above.
>
> Would it make sense to try to define a more consistent type of output
> or
> format style for "verbosity" across different commands? As it stands it
> seems each command treats verbosity in its own way which makes it hard
> to
> interpret exactly what it will do each time...
We'd have to follow the already established behavior of the commands,
and there are the man pages to describe what's going on with the
verbosity for each command. In other words, nothing would get changed,
just some more knobs would be added, for those who prefer to have the
additional verbosity enabled.
>>> Another thing is that commands like status have multiple flags that
>>> can be
>>> used to specify the output format, such as --short, --long,
>>> --porcelain,
>>> etc, but only --short seems to be configurable as a git config
>>> setting.
>>> Is there a reason (besides backward compatibility I guess) that these
>>> aren't rolled into a single thing like --format=<type>? This seems
>>> like
>>> it would be the easiest way to future proof for new formats like
>>> --format=verbose, --format=noob, --format=extended, etc.
>>
>> That's a good question, but I'd need to go through the commit history
>> to be
>> able to provide some kind of an explanation. It could also be all
>> packed
>> into "status.verbose" as a new configuration option.
>
> Ok so it sounds like you prefer to use "verbose" as the setting key?
> I guess at this point that might make more sense since commit.verbose
> already exists, and existing options could be packed into it like you
> said instead of just true or false.
It looks like a logical choice to me.
> And then my thing here would just be called "command.verbose =
> extended"?
Yes, that's what I propose. It also looks like a logical choice to me,
and it would leave space for some possible later changes to the
"<command>.verbose = extended" verbosity, without tying it to the
tables. We'd also leave some space that way for even maybe an
additional level of verbosity, be it "<command>.verbose = simple",
"<command>.verbose = graphical" or whatever.
Perhaps this scheme should also support "<command>.verbose = basic",
which would be an alias for "<command>.verbose = true", for additional
clarity.
>>> From a noob's perspective though, does adding a config setting for
>>> each
>>> command really make sense? I'm kindof envisioning this setting now as
>>> a
>>> "mode" that is either enabled for all commands it affects or for
>>> none.
>>> And it's highly unlikely a newish user would individually discover
>>> which
>>> commands this "extended" format is available for, and run "git config
>>> <command>.verbose = extended" for every one. I mean we could do that
>>> in case there are folks who only want it for specific commands, but
>>> to
>>> fulfill it's purpose I think there should definetely be some general
>>> way
>>> to enable the setting for all commands that have it.
>>
>> Quite frankly, we shouldn't expect that all users are noobs, and as a
>> result
>> dumb everything down just to make them as comfortable as possible. On
>> the
>> other hand, perhaps not everyone would like to have extended verbosity
>> enabled for all commands, just as not everyone uses "-v" for all
>> commands.
>
> I agree with this, and I think it's important to cater to both newbies
> and
> experienced users alike. That's why I said I never dreamed of making
> this
> new format the default.
Perhaps it would also be good to nudge the newbies a bit by requesting
them to enable the extended verbosity for each command by hand. That
way they would both learn a bit about the way git configuration works,
which they ultimately can't escape from, and they would be excited to
learn new git commands. Or I at least hope so. :)
> And it's true that some users might only want the extended (or any
> format)
> for specific commands. I think a happy medium then is to have the
> command-
> specific settings like you mention, plus one toplevel option that
> enables a
> specific type of output format among all commands (and overrides the
> command-specific settings), so that the user can choose which they
> prefer.
That's something we can consider as an additional configuration option.
That way, users could also enable the basic verbosity for all commands,
which may also be usable.
> Any thoughts on what the section in the config for a more general
> setting
> like this might be named? If "status.verbose = extended" would already
> be
> taken specifically for the status command, what terminology could we
> use
> to mean something like "global.verbose = extended" or "global.extended
> =
> true"? Although the former seems better to me since other format values
> could be implemented, like "global.verbose = standard"...
Maybe "core.verbose"? We already have "core.pager", which kind of
affects the way all command outputs look like.
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Liam Beguin @ 2023-10-28 3:22 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Kousik Sanagavarapu, git
In-Reply-To: <20231028021301.GA35796@coredump.intra.peff.net>
Hi Junio, Peff,
On Fri, Oct 27, 2023 at 10:13:01PM -0400, Jeff King wrote:
> On Sat, Oct 28, 2023 at 09:12:06AM +0900, Junio C Hamano wrote:
>
> > Grouping @gmail.com addresses do not smell all that useful, though.
While I agree with you, I think that's more an exception that the rule.
> > More importantly, it is not clear what "Many reports" refers to. If
> > they are *not* verbatim output from "git log" family of commands,
> > iow, they are produced by post-processing output from "git log"
> > family of commands, then I do not quite see why %aa is useful at
> > all.
I might've been a bit generous with "many report", I was mostly thinking
of the ones published by lwn.net, and U-Boot for example.
To some extent, "git shortlog" could be considered a part of that
post-processing chain.
> One way you could directly use this is in shortlog, which these days
> lets you group by specific formats. So:
>
> git shortlog -ns --group=format:%aA
That's exactly what I implemented this for :-)
> is potentially useful.
>
> I say "potentially" because it really depends on your project and its
> contributors. In git.git the results are mostly either too broad
> ("gmail.com" covers many unrelated people) or too narrow (I'll assume
> I'm the only contributor from "peff.net"). There are a few possibly
> useful ones ("microsoft.com", "gitlab.com", though even those are
> misleading because email domains don't always correspond to
> affiliations).
I agree with your comment here, while grouping everything under
"gmail.com" for example doesn't provide anything really useful we can
rely on mailmap to fix that when appropriate. I think it would otherwise
count as unaffiliated.
I don't claim this to be foolproof, but I do think that it gives a good
overall view of which companies are involved in the project for the most
part.
> So I don't find it useful myself, but I see how it could be in the right
> circumstances. It also feels like a symmetric match to "%al", which
> already exists. I do find "aa" as the identifier a little hard to
> remember. I guess it's "a" for "address", though I'd have called the
> whole local@domain thing an address thing that. Of course "d" for domain
> would make sense, but that is already taken. If we could spell it as
> %(authoremail:domain) that would remove the question. But given the
> existence of "%al", I'm not too sad to see another letter allocated to
> this purpose in the meantime.
I chose the "a" for "address", but I'm not sold on %aa either.
I just couldn't find anything better that wasn't already taken.
What about "a@"?
It's a bit easier to remember, being the first character of the
domain-part.
> Just my two cents as a shortlog --format afficionado. ;) (Of course,
> shortlog itself is the ultimate "you could really just post-process log
> output" example).
I'm a big fan of shortlog --format (and --group) as well!
Taking it a step further, it's also possible to pass in whatever mailmap
you want to generate a "report". Let's say there's mapping that only
makes sense for a single release something like this could be used:
git -c mailmap.file=git-mailmap-v2.42 shortlog -sn --group=format:%aA
> -Peff
Thanks for your time.
Cheers,
Liam
^ permalink raw reply
* Re: [RFC PATCH v2 0/6] Noobify format for status, add, restore
From: Jacob Stopak @ 2023-10-28 2:52 UTC (permalink / raw)
To: Dragan Simic; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <9b93115810ca269c87ec08f72fdc9c12@manjaro.org>
> They currently don't exist, but that's something I've planned to implement,
> e.g. to "add.verbose" as a new configuration option. It should be usable,
> while not being messy or intrusive as a new feature.
"git add" already has the -v, --verbose flag available for the command
itself like:
$ git add -v foo.txt
add 'foo.txt'
But like you said the config option add.verbose doesn't seem to exist yet.
So I assume an "add.verbose" config option would just always print that
without having to specify the -v, --verbose flag when running the command?
Basically what I'm asking is if commands that already have a --verbose flag
would just get a config setting that does the existing thing by default?
> Yes, those are the basic per-command verbosity modes or levels, as I call
> them. The way I see it, your patches would add new, extended per-command
> verbosity levels.
Ok, I see.
> > So it seems like currently "verbose" is used for various things among
> > the command set...
> Yes, that's the basic verbosity, as I named it above.
Would it make sense to try to define a more consistent type of output or
format style for "verbosity" across different commands? As it stands it
seems each command treats verbosity in its own way which makes it hard to
interpret exactly what it will do each time...
> > Another thing is that commands like status have multiple flags that can
> > be
> > used to specify the output format, such as --short, --long, --porcelain,
> > etc, but only --short seems to be configurable as a git config setting.
> > Is there a reason (besides backward compatibility I guess) that these
> > aren't rolled into a single thing like --format=<type>? This seems like
> > it would be the easiest way to future proof for new formats like
> > --format=verbose, --format=noob, --format=extended, etc.
>
> That's a good question, but I'd need to go through the commit history to be
> able to provide some kind of an explanation. It could also be all packed
> into "status.verbose" as a new configuration option.
Ok so it sounds like you prefer to use "verbose" as the setting key?
I guess at this point that might make more sense since commit.verbose
already exists, and existing options could be packed into it like you
said instead of just true or false.
And then my thing here would just be called "command.verbose = extended"?
> > From a noob's perspective though, does adding a config setting for each
> > command really make sense? I'm kindof envisioning this setting now as a
> > "mode" that is either enabled for all commands it affects or for none.
> > And it's highly unlikely a newish user would individually discover which
> > commands this "extended" format is available for, and run "git config
> > <command>.verbose = extended" for every one. I mean we could do that
> > in case there are folks who only want it for specific commands, but to
> > fulfill it's purpose I think there should definetely be some general way
> > to enable the setting for all commands that have it.
>
> Quite frankly, we shouldn't expect that all users are noobs, and as a result
> dumb everything down just to make them as comfortable as possible. On the
> other hand, perhaps not everyone would like to have extended verbosity
> enabled for all commands, just as not everyone uses "-v" for all commands.
I agree with this, and I think it's important to cater to both newbies and
experienced users alike. That's why I said I never dreamed of making this
new format the default.
And it's true that some users might only want the extended (or any format)
for specific commands. I think a happy medium then is to have the command-
specific settings like you mention, plus one toplevel option that enables a
specific type of output format among all commands (and overrides the
command-specific settings), so that the user can choose which they prefer.
Any thoughts on what the section in the config for a more general setting
like this might be named? If "status.verbose = extended" would already be
taken specifically for the status command, what terminology could we use
to mean something like "global.verbose = extended" or "global.extended =
true"? Although the former seems better to me since other format values
could be implemented, like "global.verbose = standard"...
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Liam Beguin @ 2023-10-28 2:20 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git
In-Reply-To: <20231027184357.21049-1-five231003@gmail.com>
Hi Kousik,
On Sat, Oct 28, 2023 at 12:10:30AM +0530, Kousik Sanagavarapu wrote:
> Hi Liam,
>
> Liam Beguin <liambeguin@gmail.com> wrote:
> > Subject: Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
>
> Since we are adding both '%aa' and '%aA', it would be better to
> to include both in the commit subject, but since it is already long
> enough, in my opinion
>
> pretty: add formats for domain-part of email address
>
> would convey the gist of the commit to the reader better.
That reads better, I'll update the commit message.
> > Many reports use the email domain to keep track of organizations
> > contributing to projects.
> > Add support for formatting the domain-part of a contributor's address so
> > that this can be done using git itself, with something like:
> >
> > git shortlog -sn --group=format:%aA v2.41.0..v2.42.0
> >
> > Signed-off-by: Liam Beguin <liambeguin@gmail.com>
>
> A very very very minor nit but the commit message would read better as
>
> ... contributing to projects, so add support for ...
>
> Feel free to ignore it.
>
> > ---
> > Documentation/pretty-formats.txt | 6 ++++++
> > pretty.c | 13 ++++++++++++-
> > t/t4203-mailmap.sh | 28 ++++++++++++++++++++++++++++
> > t/t6006-rev-list-format.sh | 6 ++++--
> > 4 files changed, 50 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> > index a22f6fceecdd..72102a681c3a 100644
> > --- a/Documentation/pretty-formats.txt
> > +++ b/Documentation/pretty-formats.txt
> > @@ -195,6 +195,9 @@ The placeholders are:
> > '%al':: author email local-part (the part before the '@' sign)
> > '%aL':: author email local-part (see '%al') respecting .mailmap, see
> > linkgit:git-shortlog[1] or linkgit:git-blame[1])
> > +'%aa':: author email domain-part (the part after the '@' sign)
> > +'%aA':: author email domain-part (see '%al') respecting .mailmap, see
> > + linkgit:git-shortlog[1] or linkgit:git-blame[1])
> > '%ad':: author date (format respects --date= option)
> > '%aD':: author date, RFC2822 style
> > '%ar':: author date, relative
> > @@ -213,6 +216,9 @@ The placeholders are:
> > '%cl':: committer email local-part (the part before the '@' sign)
> > '%cL':: committer email local-part (see '%cl') respecting .mailmap, see
> > linkgit:git-shortlog[1] or linkgit:git-blame[1])
> > +'%ca':: committer email domain-part (the part before the '@' sign)
> > +'%cA':: committer email domain-part (see '%cl') respecting .mailmap, see
> > + linkgit:git-shortlog[1] or linkgit:git-blame[1])
> > '%cd':: committer date (format respects --date= option)
> > '%cD':: committer date, RFC2822 style
> > '%cr':: committer date, relative
> > diff --git a/pretty.c b/pretty.c
> > index cf964b060cd1..4f5d081589ea 100644
> > --- a/pretty.c
> > +++ b/pretty.c
> > @@ -791,7 +791,7 @@ static size_t format_person_part(struct strbuf *sb, char part,
> > mail = s.mail_begin;
> > maillen = s.mail_end - s.mail_begin;
> >
> > - if (part == 'N' || part == 'E' || part == 'L') /* mailmap lookup */
> > + if (part == 'N' || part == 'E' || part == 'L' || part == 'A') /* mailmap lookup */
> > mailmap_name(&mail, &maillen, &name, &namelen);
> > if (part == 'n' || part == 'N') { /* name */
> > strbuf_add(sb, name, namelen);
> > @@ -808,6 +808,17 @@ static size_t format_person_part(struct strbuf *sb, char part,
> > strbuf_add(sb, mail, maillen);
> > return placeholder_len;
> > }
> > + if (part == 'a' || part == 'A') { /* domain-part */
> > + const char *at = memchr(mail, '@', maillen);
> > + if (at) {
> > + at += 1;
> > + maillen -= at - mail;
> > + strbuf_add(sb, at, maillen);
> > + } else {
> > + strbuf_add(sb, mail, maillen);
> > + }
> > + return placeholder_len;
> > + }
> >
> > if (!s.date_begin)
> > goto skip;
>
> So, if we have a domain-name, we grab it, else (the case where we don't
> have '@') we grab it as-is. Looks good.
>
> > diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> > index 2016132f5161..35bf7bb05bea 100755
> > --- a/t/t4203-mailmap.sh
> > +++ b/t/t4203-mailmap.sh
> > @@ -624,6 +624,34 @@ test_expect_success 'Log output (local-part email address)' '
> > test_cmp expect actual
> > '
> >
> > +test_expect_success 'Log output (domain-part email address)' '
> > + cat >expect <<-EOF &&
> > + Author email cto@coompany.xx has domain-part coompany.xx
> > + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> > +
> > + Author email me@company.xx has domain-part company.xx
> > + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> > +
> > + Author email me@company.xx has domain-part company.xx
> > + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> > +
> > + Author email nick2@company.xx has domain-part company.xx
> > + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> > +
> > + Author email bugs@company.xx has domain-part company.xx
> > + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> > +
> > + Author email bugs@company.xx has domain-part company.xx
> > + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> > +
> > + Author email author@example.com has domain-part example.com
> > + Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
> > + EOF
> > +
> > + git log --pretty=format:"Author email %ae has domain-part %aa%nCommitter email %ce has domain-part %ca%n" >actual &&
> > + test_cmp expect actual
> > +'
> > +
> > test_expect_success 'Log output with --use-mailmap' '
> > test_config mailmap.file complex.map &&
> >
> > diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
> > index 573eb97a0f7f..34c686becf2d 100755
> > --- a/t/t6006-rev-list-format.sh
> > +++ b/t/t6006-rev-list-format.sh
> > @@ -163,11 +163,12 @@ commit $head1
> > EOF
> >
> > # we don't test relative here
> > -test_format author %an%n%ae%n%al%n%ad%n%aD%n%at <<EOF
> > +test_format author %an%n%ae%n%al%aa%n%ad%n%aD%n%at <<EOF
> > commit $head2
> > $GIT_AUTHOR_NAME
> > $GIT_AUTHOR_EMAIL
> > $TEST_AUTHOR_LOCALNAME
> > +$TEST_AUTHOR_DOMAIN
> > Thu Apr 7 15:13:13 2005 -0700
> > Thu, 7 Apr 2005 15:13:13 -0700
> > 1112911993
> > @@ -180,11 +181,12 @@ Thu, 7 Apr 2005 15:13:13 -0700
> > 1112911993
> > EOF
> >
> > -test_format committer %cn%n%ce%n%cl%n%cd%n%cD%n%ct <<EOF
> > +test_format committer %cn%n%ce%n%cl%ca%n%cd%n%cD%n%ct <<EOF
> > commit $head2
> > $GIT_COMMITTER_NAME
> > $GIT_COMMITTER_EMAIL
> > $TEST_COMMITTER_LOCALNAME
> > +$TEST_COMMITTER_DOMAIN
> > Thu Apr 7 15:13:13 2005 -0700
> > Thu, 7 Apr 2005 15:13:13 -0700
> > 1112911993
> >
> > --
> > 2.39.0
>
> The tests look good too.
>
> I should say I'm skeptical of the new format's name though. I know '%ad' is
> taken... but maybe it's just me.
>
> Thanks
I agree, %aa isn't the best, I'm definitly opened to suggestions.
My preference would've been for something like %ad, but that's already
taken.
Thanks for reviewing.
Cheers,
Liam
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Jeff King @ 2023-10-28 2:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kousik Sanagavarapu, Liam Beguin, git
In-Reply-To: <xmqq7cn7obah.fsf@gitster.g>
On Sat, Oct 28, 2023 at 09:12:06AM +0900, Junio C Hamano wrote:
> Grouping @gmail.com addresses do not smell all that useful, though.
>
> More importantly, it is not clear what "Many reports" refers to. If
> they are *not* verbatim output from "git log" family of commands,
> iow, they are produced by post-processing output from "git log"
> family of commands, then I do not quite see why %aa is useful at
> all.
One way you could directly use this is in shortlog, which these days
lets you group by specific formats. So:
git shortlog -ns --group=format:%aA
is potentially useful.
I say "potentially" because it really depends on your project and its
contributors. In git.git the results are mostly either too broad
("gmail.com" covers many unrelated people) or too narrow (I'll assume
I'm the only contributor from "peff.net"). There are a few possibly
useful ones ("microsoft.com", "gitlab.com", though even those are
misleading because email domains don't always correspond to
affiliations).
So I don't find it useful myself, but I see how it could be in the right
circumstances. It also feels like a symmetric match to "%al", which
already exists. I do find "aa" as the identifier a little hard to
remember. I guess it's "a" for "address", though I'd have called the
whole local@domain thing an address thing that. Of course "d" for domain
would make sense, but that is already taken. If we could spell it as
%(authoremail:domain) that would remove the question. But given the
existence of "%al", I'm not too sad to see another letter allocated to
this purpose in the meantime.
Just my two cents as a shortlog --format afficionado. ;) (Of course,
shortlog itself is the ultimate "you could really just post-process log
output" example).
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Junio C Hamano @ 2023-10-28 0:12 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: Liam Beguin, git
In-Reply-To: <20231027184357.21049-1-five231003@gmail.com>
Kousik Sanagavarapu <five231003@gmail.com> writes:
> Liam Beguin <liambeguin@gmail.com> wrote:
>> Subject: Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
>
> Since we are adding both '%aa' and '%aA', it would be better to
> to include both in the commit subject, but since it is already long
> enough, in my opinion
>
> pretty: add formats for domain-part of email address
>
> would convey the gist of the commit to the reader better.
;-). Very good.
>> Many reports use the email domain to keep track of organizations
>> contributing to projects.
Grouping @gmail.com addresses do not smell all that useful, though.
More importantly, it is not clear what "Many reports" refers to. If
they are *not* verbatim output from "git log" family of commands,
iow, they are produced by post-processing output from "git log"
family of commands, then I do not quite see why %aa is useful at
all.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox