* [GSOC][PATCH] userdiff: add builtin patterns for JavaScript.
From: Sergius Nyah @ 2024-02-29 10:11 UTC (permalink / raw)
To: git, christian.couder, gitster; +Cc: pk, shyamthakkar001, Sergius Nyah
In-Reply-To: <xmqqttlsjvsi.fsf@gitster.g>
This commit introduces builtin patterns for JavaScript in userdiff.
It adds a new test case in t4018-diff-funcname.sh to verify the enhanced
JavaScript function detection in Git diffs.
Signed-off-by: Sergius Justus Chesami Nyah <sergiusnyah@gmail.com>
---
userdiff.c | 17 +++++++++++++++--
t/t4018-diff-funcname.sh | 25 ++++++++-
2 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/userdiff.c b/userdiff.c
index e399543823..12e31ff14d 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -18,40 +16,19 @@
#include "git-compat-util.h"
#include "config.h"
#include "userdiff.h"
#include "strbuf.h"
PATTERNS("javascript",
/* Looks for lines that start with optional whitespace, followed
* by 'function'* and any characters (for function declarations),
* or valid JavaScript identifiers, equals sign '=', 'function' keyword
* and any characters (for function expressions).
* Also considers functions defined inside blocks with '{...}'.
*/
"^[ \t]*(function[ \t]*.*|[a-zA-Z_$][0-9a-zA-Z_$]*[ \t]*=[ \t]*function[ \t]*.*|(\\{[ \t]*)?)\n",
/* This pattern matches JavaScript identifiers */
"[a-zA-Z_$][0-9a-zA-Z_$]*"
"|[-+0-9.eE]+|0[xX][0-9a-fA-F]+"
"|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\|"),
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 43593866bc..9c3b80665e 100644
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -18,40 +16,19 @@
test_expect_success 'identify builtin patterns in Javascript' '
# setup
echo "function myFunction() { return true; }" > test.js &&
echo "var myVar = function() { return false; }" >> test.js &&
git add test.js &&
git commit -m "add test.js" &&
# modify the file
echo "function myFunction() { return false; }" > test.js &&
echo "var myVar = function() { return true; }" >> test.js &&
# command under test
git diff >output &&
# check results
test_i18ngrep "function myFunction() { return true; }" output &&
test_i18ngrep "function myFunction() { return false; }" output &&
test_i18ngrep "var myVar = function() { return false; }" output &&
test_i18ngrep "var myVar = function() { return true; }" output
'
test_done
--
2.43.2
^ permalink raw reply related
* Re: [Question] How to parse range-diff output
From: ZheNing Hu @ 2024-02-29 11:00 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Junio C Hamano, Christian Couder, Johannes Schindelin, Git List
In-Reply-To: <11783b8e-472d-4b75-bb48-88138daf16ae@app.fastmail.com>
Kristoffer Haugsbakk <code@khaugsbakk.name> 于2024年2月26日周一 21:16写道：
>
> On Mon, Feb 26, 2024, at 08:25, ZheNing Hu wrote:
> > Hi,
> >
> > I am currently looking to implement a service that provides a version
> > range comparison based on git range-diff. I can easily parse out
> > commit pair headers like "3: 0bf6289 ! 3: a076e88 dev5," but I am
> > unsure how to parse the details in the subsequent diff patch body.
> >
> > It is not a standard diff output where one can parse out the filename
> > from the diff header, It should be called a diff of diffs. We can see
> > various headers with file names such as "@@ File1 (new)", "## File2
> > (new) ##", or "@@ File3: function3" in different formats. This is
> > confusing. How should we correctly parse a range-diff patch, and do
> > you have any good suggestions?
> >
> > Thanks for any help.
> > --
> > ZheNing Hu
>
> Hi
>
> Note that “Output Stability” says that this output is not meant to be
> machine-readable. It’s for human consumption. It’s not textually stable.
>
> So a new version of Git might break your implementation without warning.
>
It is very regrettable that some crucial metadata from the range-diff
patch cannot be extracted. I can only hope that future range-diff will have
the capability of being machine-readable. Thank you!
> --
> Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v5 2/3] git-std-lib: introduce Git Standard Library
From: Phillip Wood @ 2024-02-29 11:16 UTC (permalink / raw)
To: Calvin Wan, git; +Cc: Jonathan Tan, Junio C Hamano
In-Reply-To: <20240222175033.1489723-3-calvinwan@google.com>
Hi Calvin
On 22/02/2024 17:50, Calvin Wan wrote:
> This commit contains:
> - Makefile rules for git-std-lib.a
> - code and Makefile rules for git-stub-lib.a
> - description and rationale of the above in Documentation/
We tend to avoid lists like this in our commit messages. Starting with
the motivation for adding git-std-lib would be more helpful to the
reader I think.
> Quoting from documentation introduced in this commit:
>
> The Git Standard Library intends to serve as the foundational library
> and root dependency that other libraries in Git will be built off
> of. That is to say, suppose we have libraries X and Y; a user that
> wants to use X and Y would need to include X, Y, and this Git Standard
> Library.
>
> Code demonstrating the use of git-std-lib.a and git-stub-lib.a will be
> in a subsequent commit.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
I think virtually all the changes since the last version apart from
rewording the documentation come from my fixup at [1] I'm happy to offer
my Signed-off-by: for those. I cannot offer a review of the changes from
that fixup though I'm still happy with the approach. I do however think
I should have included git-compat-util.h in the stub implementations and
used BUG() instead of assert(). I've left some comments on the
documentation below.
[1]
https://github.com/phillipwood/git/commit/0f8393d2189a4c73d3f00f5ae74d3972677309d0
> diff --git a/Documentation/technical/git-std-lib.txt b/Documentation/technical/git-std-lib.txt
> new file mode 100644
> index 0000000000..3d9aa121ac
> --- /dev/null
> +++ b/Documentation/technical/git-std-lib.txt
> @@ -0,0 +1,170 @@
> += Git Standard Library
> +
> +The Git Standard Library intends to serve as the foundational library
> +and root dependency that other libraries in Git will be built off of.
> +That is to say, suppose we have libraries X and Y; a user that wants to
> +use X and Y would need to include X, Y, and this Git Standard Library.
> +This does not mean that the Git Standard Library will be the only
> +possible root dependency in the future, but rather the most significant
> +and widely used one. Git itself is also built off of the Git Standard
> +Library.
> +
> +== Dependency graph in libified Git
> +
> +Before the introduction of the Git Standard Library, all objects defined
> +in the Git library are compiled and archived into a singular file,
s/singular/single/ there are some instances of "singular" later on as
well all of which would be better as "single" I think.
> +libgit.a, which is then linked against by common-main.o with other
> +external dependencies and turned into the Git executable.
I found this description a bit confusing. As I understand it to build
git we link git.o against common-main.o, libgit.a, xdiff/lib.a,
reftable/libreftable.a and libpcre etc.
> In other
> +words, the Git executable has dependencies on libgit.a and a couple of
> +external libraries. The libfication of Git slightly alters this build
> +flow by separating out libgit.a into libgit.a and git-std-lib.a.
> +
> +With our current method of building Git, we can imagine the dependency
s/imagine/visualize/
> +graph as such:
> +
> + Git
> + /\
> + / \
> + / \
> + libgit.a ext deps
> +
> +We want to separate out potential libraries from libgit.a and have
> +libgit.a depend on them, which would possibly look like:
> +
> + Git
> + /\
> + / \
> + / \
> + libgit.a ext deps
> + /\
> + / \
> + / \
> +object-store.a (other lib)
> + | /
> + | /
> + | /
> + | /
> + | /
> + | /
> + | /
> +git-std-lib.a
> +
> +Instead of containing all objects in Git, libgit.a would contain objects
> +that are not built by libraries it links against. Consequently, if
s/by libraries/by the libraries/
> +someone wanted a custom build of Git with a custom implementation of the
> +object store, they would only have to swap out object-store.a rather
> +than do a hard fork of Git.
> +
> +== Rationale behind Git Standard Library
> +
> +The rationale behind the selected object files in the Git Standard
> +Library is the result of two observations within the Git
> +codebase:
> + 1. every file includes git-compat-util.h which defines functions
> + in a couple of different files
> + 2. wrapper.c + usage.c have difficult-to-separate circular
s/+/and/
> + dependencies with each other and other files.
> +
> +=== Ubiquity of git-compat-util.h and circular dependencies
> +
> +Every file in the Git codebase includes git-compat-util.h. It serves as
> +"a compatibility aid that isolates the knowledge of platform specific
> +inclusion order and what feature macros to define before including which
> +system header" (Junio[1]). Since every file includes git-compat-util.h,
> +and git-compat-util.h includes wrapper.h and usage.h, it would make
> +sense for wrapper.c and usage.c to be a part of the root library. They
> +have difficult to separate circular dependencies with each other so it
> +would impractical for them to be independent libraries. Wrapper.c has
> +dependencies on parse.c, abspath.c, strbuf.c, which in turn also have
> +dependencies on usage.c and wrapper.c - more circular dependencies.
> +
> +=== Tradeoff between swappability and refactoring
> +
> +From the above dependency graph, we can see that git-std-lib.a could be
> +many smaller libraries rather than a singular library. So why choose a
> +singular library when multiple libraries can be individually easier to
> +swap and are more modular? A singular library requires less work to
> +separate out circular dependencies within itself so it becomes a
> +tradeoff question between work and reward. While there may be a point in
> +the future where a file like usage.c would want its own library so that
> +someone can have custom die() or error(), the work required to refactor
> +out the circular dependencies in some files would be enormous due to
> +their ubiquity so therefore I believe it is not worth the tradeoff
I'm not sure if we want to use the first person in our technical
documentation, unlike the cover letter to a patch series it is not
immediately obvious to the reader who "I" is. This applies the passages
in the first person below as well.
> +currently. Additionally, we can in the future choose to do this refactor
> +and change the API for the library if there becomes enough of a reason
> +to do so (remember we are avoiding promising stability of the interfaces
> +of those libraries).
> +
> +=== Reuse of compatibility functions in git-compat-util.h
> +
> +Most functions defined in git-compat-util.h are implemented in compat/
> +and have dependencies limited to strbuf.h and wrapper.h so they can be
> +easily included in git-std-lib.a, which as a root dependency means that
> +higher level libraries do not have to worry about compatibility files in
> +compat/. The rest of the functions defined in git-compat-util.h are
> +implemented in top level files and are hidden behind
> +an #ifdef if their implementation is not in git-std-lib.a.
I think the reference to #ifdef is out of date now we've moved to more stubs
> +=== Rationale summary
> +
> +The Git Standard Library allows us to get the libification ball rolling
> +with other libraries in Git. By not spending many more months attempting
> +to refactor difficult circular dependencies and instead spending that
> +time getting to a state where we can test out swapping a library out
> +such as config or object store, we can prove the viability of Git
> +libification on a much faster time scale. Additionally the code cleanups
> +that have happened so far have been minor and beneficial for the
> +codebase. It is probable that making large movements would negatively
> +affect code clarity.
> +
> +== Git Standard Library boundary
> +
> +While I have described above some useful heuristics for identifying
> +potential candidates for git-std-lib.a, a standard library should not
> +have a shaky definition for what belongs in it.
Maybe "we need a more precise definition" rather than the "shaky
definition" bit
> + - Low-level files (aka operates only on other primitive types) that are
> + used everywhere within the codebase (wrapper.c, usage.c, strbuf.c)
> + - Dependencies that are low-level and widely used
> + (abspath.c, date.c, hex-ll.c, parse.c, utf8.c)
> + - low-level git/* files with functions defined in git-compat-util.h
> + (ctype.c)
> + - compat/*
> +
> +There are other files that might fit this definition, but that does not
> +mean it should belong in git-std-lib.a. Those files should start as
> +their own separate library since any file added to git-std-lib.a loses
> +its flexibility of being easily swappable.
> +
> +Wrapper.c and usage.c have dependencies on pager and trace2 that are
> +possible to remove at the cost of sacrificing the ability for standard Git
> +to be able to trace functions in those files and other files in git-std-lib.a.
> +In order for git-std-lib.a to compile with those dependencies, stubbed out
> +versions of those files are implemented and swapped in during compilation time
> +(see STUB_LIB_OBJS in the Makefile).
> +
> +== Files inside of Git Standard Library
> +
> +The set of files in git-std-lib.a can be found in STD_LIB_OBJS and COMPAT_OBJS
> +in the Makefile.
> +
> +When these files are compiled together with the files in STUB_LIB_OBJS (or
> +user-provided files that provide the same functions), they form a complete
> +library.
> +
> +== Pitfalls
> +
> +There are a small amount of files under compat/* that have dependencies
s/amount/number/ as files are countable
> +not inside of git-std-lib.a. While those functions are not called on
> +Linux, other OSes might call those problematic functions. I don't see
> +this as a major problem, just moreso an observation that libification in
> +general may also require some minor compatibility work in the future.
> +
> +== Testing
> +
> +Unit tests should catch any breakages caused by changes to files in
> +git-std-lib.a (i.e. introduction of a out of scope dependency) and new
> +functions introduced to git-std-lib.a will require unit tests written
s/test written/tests to be written/
> +for them.
> +
> +[1] https://lore.kernel.org/git/xmqqwn17sydw.fsf@gitster.g/
It is nice to see us compiling git using git-std-lib.
Best Wishes
Phillip
> diff --git a/Makefile b/Makefile
> index 4e255c81f2..d37ea9d34b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -669,6 +669,8 @@ FUZZ_PROGRAMS =
> GIT_OBJS =
> LIB_OBJS =
> SCALAR_OBJS =
> +STD_LIB_OBJS =
> +STUB_LIB_OBJS =
> OBJECTS =
> OTHER_PROGRAMS =
> PROGRAM_OBJS =
> @@ -923,6 +925,8 @@ TEST_SHELL_PATH = $(SHELL_PATH)
>
> LIB_FILE = libgit.a
> XDIFF_LIB = xdiff/lib.a
> +STD_LIB_FILE = git-std-lib.a
> +STUB_LIB_FILE = git-stub-lib.a
> REFTABLE_LIB = reftable/libreftable.a
> REFTABLE_TEST_LIB = reftable/libreftable_test.a
>
> @@ -962,7 +966,6 @@ COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
>
> LIB_H = $(FOUND_H_SOURCES)
>
> -LIB_OBJS += abspath.o
> LIB_OBJS += add-interactive.o
> LIB_OBJS += add-patch.o
> LIB_OBJS += advice.o
> @@ -1004,8 +1007,6 @@ LIB_OBJS += convert.o
> LIB_OBJS += copy.o
> LIB_OBJS += credential.o
> LIB_OBJS += csum-file.o
> -LIB_OBJS += ctype.o
> -LIB_OBJS += date.o
> LIB_OBJS += decorate.o
> LIB_OBJS += delta-islands.o
> LIB_OBJS += diagnose.o
> @@ -1046,7 +1047,6 @@ LIB_OBJS += hash-lookup.o
> LIB_OBJS += hashmap.o
> LIB_OBJS += help.o
> LIB_OBJS += hex.o
> -LIB_OBJS += hex-ll.o
> LIB_OBJS += hook.o
> LIB_OBJS += ident.o
> LIB_OBJS += json-writer.o
> @@ -1097,7 +1097,6 @@ LIB_OBJS += pack-write.o
> LIB_OBJS += packfile.o
> LIB_OBJS += pager.o
> LIB_OBJS += parallel-checkout.o
> -LIB_OBJS += parse.o
> LIB_OBJS += parse-options-cb.o
> LIB_OBJS += parse-options.o
> LIB_OBJS += patch-delta.o
> @@ -1152,7 +1151,6 @@ LIB_OBJS += sparse-index.o
> LIB_OBJS += split-index.o
> LIB_OBJS += stable-qsort.o
> LIB_OBJS += statinfo.o
> -LIB_OBJS += strbuf.o
> LIB_OBJS += streaming.o
> LIB_OBJS += string-list.o
> LIB_OBJS += strmap.o
> @@ -1189,21 +1187,32 @@ LIB_OBJS += unpack-trees.o
> LIB_OBJS += upload-pack.o
> LIB_OBJS += url.o
> LIB_OBJS += urlmatch.o
> -LIB_OBJS += usage.o
> LIB_OBJS += userdiff.o
> -LIB_OBJS += utf8.o
> LIB_OBJS += varint.o
> LIB_OBJS += version.o
> LIB_OBJS += versioncmp.o
> LIB_OBJS += walker.o
> LIB_OBJS += wildmatch.o
> LIB_OBJS += worktree.o
> -LIB_OBJS += wrapper.o
> LIB_OBJS += write-or-die.o
> LIB_OBJS += ws.o
> LIB_OBJS += wt-status.o
> LIB_OBJS += xdiff-interface.o
>
> +STD_LIB_OBJS += abspath.o
> +STD_LIB_OBJS += ctype.o
> +STD_LIB_OBJS += date.o
> +STD_LIB_OBJS += hex-ll.o
> +STD_LIB_OBJS += parse.o
> +STD_LIB_OBJS += strbuf.o
> +STD_LIB_OBJS += usage.o
> +STD_LIB_OBJS += utf8.o
> +STD_LIB_OBJS += wrapper.o
> +
> +STUB_LIB_OBJS += stubs/trace2.o
> +STUB_LIB_OBJS += stubs/pager.o
> +STUB_LIB_OBJS += stubs/misc.o
> +
> BUILTIN_OBJS += builtin/add.o
> BUILTIN_OBJS += builtin/am.o
> BUILTIN_OBJS += builtin/annotate.o
> @@ -1352,7 +1361,7 @@ UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
>
> # xdiff and reftable libs may in turn depend on what is in libgit.a
> -GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
> +GITLIBS = common-main.o $(STD_LIB_FILE) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
> EXTLIBS =
>
> GIT_USER_AGENT = git/$(GIT_VERSION)
> @@ -2693,6 +2702,8 @@ OBJECTS += $(XDIFF_OBJS)
> OBJECTS += $(FUZZ_OBJS)
> OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
> OBJECTS += $(UNIT_TEST_OBJS)
> +OBJECTS += $(STD_LIB_OBJS)
> +OBJECTS += $(STUB_LIB_OBJS)
>
> ifndef NO_CURL
> OBJECTS += http.o http-walker.o remote-curl.o
> @@ -3686,7 +3697,7 @@ clean: profile-clean coverage-clean cocciclean
> $(RM) git.res
> $(RM) $(OBJECTS)
> $(RM) headless-git.o
> - $(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
> + $(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB) $(STD_LIB_FILE) $(STUB_LIB_FILE)
> $(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
> $(RM) $(TEST_PROGRAMS)
> $(RM) $(FUZZ_PROGRAMS)
> @@ -3878,3 +3889,18 @@ $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
> build-unit-tests: $(UNIT_TEST_PROGS)
> unit-tests: $(UNIT_TEST_PROGS)
> $(MAKE) -C t/ unit-tests
> +
> +### Libified Git rules
> +
> +# git-std-lib.a
> +# Programs other than git should compile this with
> +# make NO_GETTEXT=YesPlease git-std-lib.a
> +# and link against git-stub-lib.a (if the default no-op functionality is fine)
> +# or a custom .a file with the same interface as git-stub-lib.a (if custom
> +# functionality is needed) as well.
> +$(STD_LIB_FILE): $(STD_LIB_OBJS) $(COMPAT_OBJS)
> + $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
> +
> +# git-stub-lib.a
> +$(STUB_LIB_FILE): $(STUB_LIB_OBJS)
> + $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
> diff --git a/stubs/misc.c b/stubs/misc.c
> new file mode 100644
> index 0000000000..92da76fd46
> --- /dev/null
> +++ b/stubs/misc.c
> @@ -0,0 +1,33 @@
> +#include <assert.h>
> +#include <stdlib.h>
> +
> +#ifndef NO_GETTEXT
> +/*
> + * NEEDSWORK: This is enough to link our unit tests against
> + * git-std-lib.a built with gettext support. We don't really support
> + * programs other than git using git-std-lib.a with gettext support
> + * yet. To do that we need to start using dgettext() rather than
> + * gettext() in our code.
> + */
> +int git_gettext_enabled = 0;
> +#endif
> +
> +int common_exit(const char *file, int line, int code);
> +
> +int common_exit(const char *file, int line, int code)
> +{
> + exit(code);
> +}
> +
> +#if !defined(__MINGW32__) && !defined(_MSC_VER)
> +int lstat_cache_aware_rmdir(const char *path);
> +
> +int lstat_cache_aware_rmdir(const char *path)
> +{
> + /*
> + * This function should not be called by programs linked
> + * against git-stub-lib.a
> + */
> + assert(0);
> +}
> +#endif
> diff --git a/stubs/pager.c b/stubs/pager.c
> new file mode 100644
> index 0000000000..4f575cada7
> --- /dev/null
> +++ b/stubs/pager.c
> @@ -0,0 +1,6 @@
> +#include "pager.h"
> +
> +int pager_in_use(void)
> +{
> + return 0;
> +}
> diff --git a/stubs/trace2.c b/stubs/trace2.c
> new file mode 100644
> index 0000000000..7d89482228
> --- /dev/null
> +++ b/stubs/trace2.c
> @@ -0,0 +1,27 @@
> +#include "git-compat-util.h"
> +#include "trace2.h"
> +
> +struct child_process { int stub; };
> +struct repository { int stub; };
> +struct json_writer { int stub; };
> +
> +void trace2_region_enter_fl(const char *file, int line, const char *category,
> + const char *label, const struct repository *repo, ...) { }
> +void trace2_region_leave_fl(const char *file, int line, const char *category,
> + const char *label, const struct repository *repo, ...) { }
> +void trace2_data_string_fl(const char *file, int line, const char *category,
> + const struct repository *repo, const char *key,
> + const char *value) { }
> +void trace2_cmd_ancestry_fl(const char *file, int line, const char **parent_names) { }
> +void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
> + va_list ap) { }
> +void trace2_cmd_name_fl(const char *file, int line, const char *name) { }
> +void trace2_thread_start_fl(const char *file, int line,
> + const char *thread_base_name) { }
> +void trace2_thread_exit_fl(const char *file, int line) { }
> +void trace2_data_intmax_fl(const char *file, int line, const char *category,
> + const struct repository *repo, const char *key,
> + intmax_t value) { }
> +int trace2_is_enabled(void) { return 0; }
> +void trace2_counter_add(enum trace2_counter_id cid, uint64_t value) { }
> +void trace2_collect_process_info(enum trace2_process_info_reason reason) { }
^ permalink raw reply
* Re: [GSOC][PATCH] userdiff: add builtin patterns for JavaScript.
From: Ghanshyam Thakkar @ 2024-02-29 12:01 UTC (permalink / raw)
To: Sergius Nyah, git, christian.couder, gitster; +Cc: pk
In-Reply-To: <20240229101131.445405-1-sergiusnyah@gmail.com>
On Thu Feb 29, 2024 at 3:41 PM IST, Sergius Nyah wrote:
> This commit introduces builtin patterns for JavaScript in userdiff.
"This commit introduces ..." -> "Introduce ...". I think describing which
specification this adheres to (i.e. ECMA-262) would be helpful for the
readers.
> It adds a new test case in t4018-diff-funcname.sh to verify the enhanced
> JavaScript function detection in Git diffs.
"It adds ..." -> "Add ..."
> Signed-off-by: Sergius Justus Chesami Nyah <sergiusnyah@gmail.com>
> ---
> userdiff.c | 17 +++++++++++++++--
> t/t4018-diff-funcname.sh | 25 ++++++++-
> 2 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/userdiff.c b/userdiff.c
> index e399543823..12e31ff14d 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -18,40 +16,19 @@
> #include "git-compat-util.h"
> #include "config.h"
> #include "userdiff.h"
> #include "strbuf.h"
>
> PATTERNS("javascript",
> /* Looks for lines that start with optional whitespace, followed
> * by 'function'* and any characters (for function declarations),
> * or valid JavaScript identifiers, equals sign '=', 'function' keyword
> * and any characters (for function expressions).
> * Also considers functions defined inside blocks with '{...}'.
> */
> "^[ \t]*(function[ \t]*.*|[a-zA-Z_$][0-9a-zA-Z_$]*[ \t]*=[ \t]*function[ \t]*.*|(\\{[ \t]*)?)\n",
> /* This pattern matches JavaScript identifiers */
> "[a-zA-Z_$][0-9a-zA-Z_$]*"
> "|[-+0-9.eE]+|0[xX][0-9a-fA-F]+"
> "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\|"),
It seems like you manually manipulated the diff/format-patch output that
is not the commit message. Usually all the new lines have '+' sign
prefixed and all the removed lines have '-' sign prefixed. git am gives
the following error:
Applying: userdiff: add builtin patterns for JavaScript.
error: corrupt patch at line 11
Patch failed at 0022 userdiff: add builtin patterns for JavaScript.
If you find it difficult to use send-email/format-patch, you can always
make a pull request on git/git or gitgitgadget/git on GitHub and
GitGitGadget[1] will take care of the rest. If you still want to use
format-patch, it is a good practice to version your patches
(i.e. [PATCH v2 1/2] ...) so that it is easy to follow.
(cf. Documentation/SubmittingPatches or Documentation/MyFirstContribution)
[1]: https://gitgitgadget.github.io/
Thanks.
> diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
> index 43593866bc..9c3b80665e 100644
> --- a/t/t4018-diff-funcname.sh
> +++ b/t/t4018-diff-funcname.sh
> @@ -18,40 +16,19 @@
> test_expect_success 'identify builtin patterns in Javascript' '
> # setup
> echo "function myFunction() { return true; }" > test.js &&
> echo "var myVar = function() { return false; }" >> test.js &&
> git add test.js &&
> git commit -m "add test.js" &&
>
> # modify the file
> echo "function myFunction() { return false; }" > test.js &&
> echo "var myVar = function() { return true; }" >> test.js &&
>
> # command under test
> git diff >output &&
>
> # check results
> test_i18ngrep "function myFunction() { return true; }" output &&
> test_i18ngrep "function myFunction() { return false; }" output &&
> test_i18ngrep "var myVar = function() { return false; }" output &&
> test_i18ngrep "var myVar = function() { return true; }" output
> '
>
> test_done
> --
> 2.43.2
^ permalink raw reply
* [PATCH] tests: modernize the test script t0010-racy-git.sh
From: Aryan Gupta via GitGitGadget @ 2024-02-29 12:23 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt [ ], Michal Suchánek [ ],
Jean-Noël AVILA [ ]
From: aryangupta701 <garyan447@gmail.com>
Modernize the formatting of the test script to align with current
standards and improve its overall readability.
Signed-off-by: Aryan Gupta <garyan447@gmail.com>
---
[GSOC] [PATCH] Modernize a test script
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1675%2Faryangupta701%2Ftest-modernize-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1675/aryangupta701/test-modernize-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1675
t/t0010-racy-git.sh | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh
index 837c8b7228b..04dc1cf3ff5 100755
--- a/t/t0010-racy-git.sh
+++ b/t/t0010-racy-git.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-test_description='racy GIT'
+test_description='racy git'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
@@ -16,19 +16,18 @@ do
echo xyzzy >infocom
files=$(git diff-files -p)
- test_expect_success \
- "Racy GIT trial #$trial part A" \
- 'test "" != "$files"'
+ test_expect_success 'Racy git trial #$trial part A' '
+ test "" != "$files"
+ '
sleep 1
echo xyzzy >cornerstone
git update-index --add cornerstone
files=$(git diff-files -p)
- test_expect_success \
- "Racy GIT trial #$trial part B" \
- 'test "" != "$files"'
-
+ test_expect_success 'Racy git trial #$trial part B' '
+ test "" != "$files"
+ '
done
test_done
base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
--
gitgitgadget
^ permalink raw reply related
* [question] How to customize git revert template
From: Wu Zhenyu @ 2024-02-29 12:52 UTC (permalink / raw)
To: git
In https://git-scm.com/docs/git-revert
> While git creates a basic commit message automatically
How can I customize git revert template? e.g.,
```sh
git config revert.template ~/.config/git/template
```
TIA!
--
Best Regards
Wu Zhenyu
^ permalink raw reply
* [PATCH] setup: clarify TODO comment about ignoring core.bare
From: Ghanshyam Thakkar @ 2024-02-29 13:41 UTC (permalink / raw)
To: shyamthakkar001
Cc: git, gitster, johannes.schindelin, newren, christian.couder
In-Reply-To: <85d4e83c-b6c4-4308-ac8c-a65c911c8a95@gmail.com>
The comment suggested to heed core.bare from template config if no
command line override given. However, it was clarified by Junio that
such values are ignored by intention and does not make sense to
propagate it from template config to the repository config[1].
Therefore, remove the TODO tag and update the comment to add
additional context if such functionality is desired in the future.
Also update the relevant test cases to not expect failure and remove
one redundant testcase.
[1]: https://lore.kernel.org/git/xmqqjzonpy9l.fsf@gitster.g/
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
setup.c | 16 ++++++++++------
t/t1301-shared-repo.sh | 15 ++-------------
t/t5606-clone-options.sh | 6 +++---
3 files changed, 15 insertions(+), 22 deletions(-)
diff --git a/setup.c b/setup.c
index b69b1cbc2a..92e0c3a121 100644
--- a/setup.c
+++ b/setup.c
@@ -1997,16 +1997,20 @@ static int create_default_files(const char *template_path,
if (init_shared_repository != -1)
set_shared_repository(init_shared_repository);
/*
- * TODO: heed core.bare from config file in templates if no
- * command-line override given
+ * Note: The below line simply checks the presence of worktree (the
+ * simplification of which is given after the line) and core.bare from
+ * config file is not taken into account when deciding if the worktree
+ * should be created or not, even if no command line override given.
+ * That is intentional. Therefore, if in future we want to heed
+ * core.bare from config file, we should do it before we create any
+ * subsequent directories for worktree or repo because until this point
+ * they should already be created.
*/
is_bare_repository_cfg = prev_bare_repository || !work_tree;
- /* TODO (continued):
+ /* Note (continued):
*
- * Unfortunately, the line above is equivalent to
+ * The line above is equivalent to
* is_bare_repository_cfg = !work_tree;
- * which ignores the config entirely even if no `--[no-]bare`
- * command line option was present.
*
* To see why, note that before this function, there was this call:
* prev_bare_repository = is_bare_repository()
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index b1eb5c01b8..29cf8a9661 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -52,7 +52,7 @@ test_expect_success 'shared=all' '
test 2 = $(git config core.sharedrepository)
'
-test_expect_failure 'template can set core.bare' '
+test_expect_success 'template cannot set core.bare' '
test_when_finished "rm -rf subdir" &&
test_when_finished "rm -rf templates" &&
test_config core.bare true &&
@@ -60,18 +60,7 @@ test_expect_failure 'template can set core.bare' '
mkdir -p templates/ &&
cp .git/config templates/config &&
git init --template=templates subdir &&
- test_path_exists subdir/HEAD
-'
-
-test_expect_success 'template can set core.bare but overridden by command line' '
- test_when_finished "rm -rf subdir" &&
- test_when_finished "rm -rf templates" &&
- test_config core.bare true &&
- umask 0022 &&
- mkdir -p templates/ &&
- cp .git/config templates/config &&
- git init --no-bare --template=templates subdir &&
- test_path_exists subdir/.git/HEAD
+ test_path_is_missing subdir/HEAD
'
test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository' '
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index a400bcca62..e93e0d0cc3 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -120,14 +120,14 @@ test_expect_success 'prefers -c config over --template config' '
'
-test_expect_failure 'prefers --template config even for core.bare' '
+test_expect_success 'ignore --template config for core.bare' '
template="$TRASH_DIRECTORY/template-with-bare-config" &&
mkdir "$template" &&
git config --file "$template/config" core.bare true &&
git clone "--template=$template" parent clone-bare-config &&
- test "$(git -C clone-bare-config config --local core.bare)" = "true" &&
- test_path_is_file clone-bare-config/HEAD
+ test "$(git -C clone-bare-config config --local core.bare)" = "false" &&
+ test_path_is_missing clone-bare-config/HEAD
'
test_expect_success 'prefers config "clone.defaultRemoteName" over default' '
--
2.44.0
^ permalink raw reply related
* [GSoC][PATCH 0/1] microproject: Use test_path_is_* functions in test scripts
From: shejialuo @ 2024-02-29 15:04 UTC (permalink / raw)
To: git; +Cc: shejialuo
Hello everyone,
My name is Jialuo She, mastering in the software engineering. This is my
last semester. And I will graduate this summer and works as a full time
employee. So I wanna make good use of my time by contributing to open
source software, and take this opportunity to continue contributing to
Git after I start working in the future.
My reason for choosing to participate in Git is actually quite simple.
It's because I once wrote a toy version of Git myself.Throught this
project, https://github.com/shejialuo/ugit-cpp. I came to understand
the magic of Git. I also want to do my part and contribute to something
meaningful.
For myself, the most attractive GSoC idea for me is "Implement
consistency checks for refs". I will dive into this idea soon.
At last, Wish everyone good health and happiness every day.
shejialuo (1):
[GSoC][PATCH] t3070: refactor test -e command
t/t3070-wildmatch.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
--
2.44.0
^ permalink raw reply
* [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command
From: shejialuo @ 2024-02-29 15:04 UTC (permalink / raw)
To: git; +Cc: shejialuo
In-Reply-To: <20240229150442.490649-1-shejialuo@gmail.com>
The "test_path_exists" function was proposed at 7e9055b. It provides
parameter number check and more robust error messages.
This patch converts all "test -e" into "test_path_exists" to improve
test debug when failure.
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
t/t3070-wildmatch.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index 4dd42df38c..d18ddc1a52 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -107,7 +107,7 @@ match_with_ls_files() {
if test "$match_expect" = 'E'
then
- if test -e .git/created_test_file
+ if test_path_exists .git/created_test_file
then
test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): match dies on '$pattern' '$text'" "
printf '%s' '$text' >expect &&
@@ -118,7 +118,7 @@ match_with_ls_files() {
fi
elif test "$match_expect" = 1
then
- if test -e .git/created_test_file
+ if test_path_exists .git/created_test_file
then
test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): match '$pattern' '$text'" "
printf '%s' '$text' >expect &&
@@ -130,7 +130,7 @@ match_with_ls_files() {
fi
elif test "$match_expect" = 0
then
- if test -e .git/created_test_file
+ if test_path_exists .git/created_test_file
then
test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): no match '$pattern' '$text'" "
>expect &&
@@ -175,7 +175,7 @@ match() {
fi
test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '
- if test -e .git/created_test_file
+ if test_path_exists .git/created_test_file
then
git reset &&
git clean -df
@@ -198,7 +198,7 @@ match() {
fi &&
git add -A &&
printf "%s" "$file" >.git/created_test_file
- elif test -e .git/created_test_file
+ elif test_path_exists .git/created_test_file
then
rm .git/created_test_file
fi
--
2.44.0
^ permalink raw reply related
* Re: [question] How to customize git revert template
From: Kristoffer Haugsbakk @ 2024-02-29 15:18 UTC (permalink / raw)
To: Wu Zhenyu; +Cc: git
In-Reply-To: <k4zvycdixqmntkyevdtsrswbfiklyj24mp6jhltnwwdswxromc@q3wtrivkbfbl>
Hi
On Thu, Feb 29, 2024, at 13:52, Wu Zhenyu wrote:
> In https://git-scm.com/docs/git-revert
>
>> While git creates a basic commit message automatically
>
> How can I customize git revert template? e.g.,
>
> ```sh
> git config revert.template ~/.config/git/template
> ```
>
> TIA!
>
> --
> Best Regards
> Wu Zhenyu
I see no way to customize this message. Your only choices seem to be
1. This default
2. The “reference” style using `--reference`
So there doesn’t seem to be a template alternative here.
--
Kristoffer Haugsbakk
^ permalink raw reply
* New Construction Project
From: larryclassicestimation @ 2024-02-29 15:19 UTC (permalink / raw)
To: git
Hi,
We are an estimating firm and offer our services to GC's and Sub Contractors. We perform commercial and residential estimates & takeoffs. We are using professional software to perform takeoffs.
If you have any job for an estimate please send over plans after reviewing the drawings we will give you a best price as compared to market price. You may also ask us for sample estimates.
Thanks.
Regards,
Larry
Estimating Dept
classic estimation
^ permalink raw reply
* Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it
From: Junio C Hamano @ 2024-02-29 16:34 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240229054239.GA1669526@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Sorry, this needs one tweak to pass under the sha256 CI job:
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 902e42c1c0..1ef540f73d 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -781,6 +781,7 @@ test_expect_success 'archive with custom path does not request v2' '
> test_expect_success 'reject client packfile-uris if not advertised' '
> {
> packetize command=fetch &&
> + packetize object-format=$(test_oid algo) &&
> printf 0001 &&
> packetize packfile-uris https &&
> packetize done &&
>
> Otherwise the server complains that the other side did not respect its
> advertised object-format (I sure am glad to have included the final
> "hey, this input works, right?" test there, as that is what caught it).
Ah, good finding. Will use it to amend.
I wonder if it is still worth testing if the command is happy with
an input that lacks object-format capability under SHA-1 mode. This
test piece is primarily about packfile-uris, so in that sense, we
are not all that interested in that unspecified client object-format
defaults to the initial value of serve.c:client_hash_algo (which is
SHA-1), and not testing that here is perfectly fine, I guess.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/2] commit: Avoid redundant scissor line with --cleanup=scissors -v
From: Junio C Hamano @ 2024-02-29 16:38 UTC (permalink / raw)
To: Josh Triplett; +Cc: git
In-Reply-To: <ZeAftoPPRsltswbS@localhost>
Josh Triplett <josh@joshtriplett.org> writes:
> I did add a test; v2 that you replied to has this:
>> Add a test for this.
Thanks. Let's mark it for 'next' then.
^ permalink raw reply
* Re: [PATCH v5 2/3] git-std-lib: introduce Git Standard Library
From: Junio C Hamano @ 2024-02-29 17:23 UTC (permalink / raw)
To: Phillip Wood; +Cc: Calvin Wan, git, Jonathan Tan
In-Reply-To: <db8b2398-c6d6-47a1-a4d4-7e4726d13312@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Calvin
> On 22/02/2024 17:50, Calvin Wan wrote:
Thanks for reviewing -- I held mine back as I expected this will see
another reroll soonish, but you already have raised many points I
also had trouble with, so I do not have to ;-)
Below, I'll liberally omit everything you wrote that I agree with.
>> +libgit.a, which is then linked against by common-main.o with other
>> +external dependencies and turned into the Git executable.
>
> I found this description a bit confusing. As I understand it to build
> git we link git.o against common-main.o, libgit.a, xdiff/lib.a,
> reftable/libreftable.a and libpcre etc.
In addition, there is no single "the Git executable", simply because
not everything is builtin command. The purpose of using libgit.a is
because we are too lazy to list and maintain all the internal
dependencies to link final executables like 'git' (which has all the
built-in command implementations) and 'git-remote-curl' (which is a
standalone program). Instead of feeding exact list of object files
to "$(CC) -o git" command line, we throw everything into libgit.a
and let the linker pick what is needed. To link "git", we may
include all builtin/*.o, git.o, common-main.o, libgit.a and the
external [*] library dependencies they have. To link "git-daemon",
we may not link builtin/*.o and git.o and link daemon.o instead.
Side note: here I am counting xdiff/lib.a as an external
library as it is mostly a borrowed code.
In other words, libgit.a is not a true library in the sense that it
was designed to be _used_ as a library. It was merely a detail of
how we implemented lazy dependency management in our build process,
which happend with 0a02ce72 (Clean up the Makefile a bit.,
2005-04-18) whose commit log message uses air-quotes around the word
"library".
>> +From the above dependency graph, we can see that git-std-lib.a could be
>> +many smaller libraries rather than a singular library. So why choose a
>> +singular library when multiple libraries can be individually easier to
>> +swap and are more modular? A singular library requires less work to
>> +separate out circular dependencies within itself so it becomes a
>> +tradeoff question between work and reward. While there may be a point in
>> +the future where a file like usage.c would want its own library so that
>> +someone can have custom die() or error(), the work required to refactor
>> +out the circular dependencies in some files would be enormous due to
>> +their ubiquity so therefore I believe it is not worth the tradeoff
>
> I'm not sure if we want to use the first person in our technical
> documentation, unlike the cover letter to a patch series it is not
> immediately obvious to the reader who "I" is. This applies the
> passages in the first person below as well.
I found it highly annoying while reading it, too. If the document
(not the commit that introduced the document) were signed and
written to state the position of one author, as opposed to spell out
the position the project will collectively take, it would have been
OK, but this document is meant to set a course of the project (and
discussion on it is the process to decide which course to take), the
first person singular "I" did not sit well for me.
Another thing to consider that I do not think you covered is the
name of the resulting .a archive. By starting it with "lib", a
customer can find your libstdgit.a with -lstdgit on the command
line, once libstdgit.a is installed at an appropriate location (or
the build-time library path is configured to point the location you
have libstdgit.a at). libgit.a that wasn't really designed to be
used as such a library did not have to follow the naming convention,
but if the thing being proposed is meant to be eventually used as a
library by external entities, git-std-lib.a is a rather poor name
for it.
PS.
I seem to have been hit by a power outage and am on UPS, so I'll
probably be offline until the power comes back.
^ permalink raw reply
* Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command
From: Eric Sunshine @ 2024-02-29 17:58 UTC (permalink / raw)
To: shejialuo; +Cc: git
In-Reply-To: <20240229150442.490649-2-shejialuo@gmail.com>
On Thu, Feb 29, 2024 at 10:05 AM shejialuo <shejialuo@gmail.com> wrote:
> t3070: refactor test -e command
>
> The "test_path_exists" function was proposed at 7e9055b. It provides
> parameter number check and more robust error messages.
>
> This patch converts all "test -e" into "test_path_exists" to improve
> test debug when failure.
Thanks for providing this GSoC submission. The aim of this patch makes
sense, but it turns out that t3070 is not a good choice for this
exercise. Before getting into that, though, a few minor comments about
the commit message.
This patch isn't actually refactoring the code, so using "refactor" in
the title is misleading.
Rather than mentioning only the object-ID, we normally reference other
commits like this (using `git log --pretty=reference -1 <object-id>`):
7e9055bb00 (t7406: prefer test_* helper functions to test -[feds],
2018-08-08)
In this case, it's not clear why you chose to reference that
particular commit over any of the others which make similar changes.
It probably would be simpler to drop mention of that commit and just
copy its reasoning into your commit message.
Taking all the above into account, a possible rewrite of the commit
message might be:
t3070: prefer test_path_exists helper function
test -e does not provide a nice error message when we hit test
failures, so use test_path_exists instead.
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
> @@ -107,7 +107,7 @@ match_with_ls_files() {
> if test "$match_expect" = 'E'
> then
> - if test -e .git/created_test_file
> + if test_path_exists .git/created_test_file
> then
> test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): match dies on '$pattern' '$text'" "
The point of functions such as test_path_exists() is to _assert_ that
some condition is true, thus allowing the test to succeed; if the
condition is not true, then the function prints an error message and
the test aborts and fails. Here is how test_path_exists() is defined:
test_path_exists () {
test "$#" -ne 1 && BUG "1 param"
if ! test -e "$1"
then
echo "Path $1 doesn't exist"
false
fi
}
It is meant to replace noisy code such as:
if ! test -e bloop
then
echo >&2 "error message" &&
exit 1
fi &&
other-code
with much simpler:
test_path_exists bloop &&
other-code
It is also meant to be used within `test_expect_success` (or
`test_expect_failure`) blocks. So, the changes made by this patch are
undesirable for a couple reasons...
First, this code is outside a `test_expect_success` (or
`test_expect_failure`) block.
Second, as noted above, test_path_exists() is an _assertion_ which
requires the file to exist, and aborts the test if the file does not
exist. But the `test -e` being changed here is part of the proper
control-flow of this logic; it is not asserting anything, but merely
branching to one or another part of the code depending upon the result
of the `test -e` test. Thus, replacing this control-flow check with
the assertion function test_path_exists() changes the logic in an
undesirable way.
The above comments are applicable to most of the changes made by this
patch. The only exceptions are the last two changes...
> @@ -175,7 +175,7 @@ match() {
> test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '
> - if test -e .git/created_test_file
> + if test_path_exists .git/created_test_file
> then
> git reset &&
... which _do_ use test_path_exists() within a `test_expect_success`
block. However, the changes are still undesirable because, as above,
this `test -e` is merely part of the normal control-flow; it's not
acting as an assertion, thus test_path_exists() -- which is an
assertion -- is not correct.
Unfortunately, none of the uses of`test -e` in t3070 are being used as
assertions worthy of replacement with test_path_exists(), thus this
isn't a good script in which to make such changes. If you reroll, you
may be able to find a good candidate script by searching for code
which looks something like this:
foo &&
test -e path &&
bar &&
and replacing it with:
foo &&
test_path_exists path &&
bar &&
^ permalink raw reply
* Re: [PATCH] tests: modernize the test script t0010-racy-git.sh
From: Eric Sunshine @ 2024-02-29 18:11 UTC (permalink / raw)
To: Aryan Gupta via GitGitGadget
Cc: git, Patrick Steinhardt [ ], Michal Suchánek [ ],
Jean-Noël AVILA [ ], aryangupta701
In-Reply-To: <pull.1675.git.1709209435242.gitgitgadget@gmail.com>
On Thu, Feb 29, 2024 at 7:24 AM Aryan Gupta via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> From: aryangupta701 <garyan447@gmail.com>
The name in the "From:" header should match the name in the
"Signed-off-by:" trailer.
> Modernize the formatting of the test script to align with current
> standards and improve its overall readability.
The aim of this patch makes sense, but the implementation isn't quite correct.
> Signed-off-by: Aryan Gupta <garyan447@gmail.com>
> ---
> diff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh> @@ -16,19 +16,18 @@ do
> files=$(git diff-files -p)
> - test_expect_success \
> - "Racy GIT trial #$trial part A" \
> - 'test "" != "$files"'
> + test_expect_success 'Racy git trial #$trial part A' '
> + test "" != "$files"
> + '
The variable `trial` takes on values 0-4, and that value is meant to
appear in the test title as the script runs:
Racy GIT trial #0 part A
Racy GIT trial #0 part B
Racy GIT trial #1 part A
Racy GIT trial #1 part B
...
However, by changing the title from a double-quote string to a
single-quote string, you inhibit interpolation of the `trial` variable
into the title, hence the test titles instead show up as:
Racy GIT trial #$trial part A
Racy GIT trial #$trial part B
Racy GIT trial #$trial part A
Racy GIT trial #$trial part B
which is undesirable.
So, the title should continue using double-quotes, and not be changed
to single-quotes.
The other change, which fixes the style of the test's body, appears correct.
^ permalink raw reply
* Re: [PATCH v5 2/3] git-std-lib: introduce Git Standard Library
From: Linus Arver @ 2024-02-29 18:27 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood; +Cc: Calvin Wan, git, Jonathan Tan
In-Reply-To: <xmqq1q8vi3pz.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> On 22/02/2024 17:50, Calvin Wan wrote:
>
>>> +libgit.a, which is then linked against by common-main.o with other
>>> +external dependencies and turned into the Git executable.
>>
>> I found this description a bit confusing. As I understand it to build
>> git we link git.o against common-main.o, libgit.a, xdiff/lib.a,
>> reftable/libreftable.a and libpcre etc.
>
> In addition, there is no single "the Git executable", simply because
> not everything is builtin command. The purpose of using libgit.a is
> because we are too lazy to list and maintain all the internal
> dependencies to link final executables like 'git' (which has all the
> built-in command implementations) and 'git-remote-curl' (which is a
> standalone program). Instead of feeding exact list of object files
> to "$(CC) -o git" command line, we throw everything into libgit.a
> and let the linker pick what is needed. To link "git", we may
> include all builtin/*.o, git.o, common-main.o, libgit.a and the
> external [*] library dependencies they have. To link "git-daemon",
> we may not link builtin/*.o and git.o and link daemon.o instead.
>
> Side note: here I am counting xdiff/lib.a as an external
> library as it is mostly a borrowed code.
>
> In other words, libgit.a is not a true library in the sense that it
> was designed to be _used_ as a library. It was merely a detail of
> how we implemented lazy dependency management in our build process,
> which happend with 0a02ce72 (Clean up the Makefile a bit.,
> 2005-04-18) whose commit log message uses air-quotes around the word
> "library".
Somehow I did not realize that this was going on. Thank you for pointing
this out!
It does make me wonder if we should stop being lazy and do the work that
the linker has been doing for us "for free" ourselves. IOW, stop linking
against a monolithic libgit.a. That way we would replace implicit
dependencies with explicit ones, which might help us understand which
things need what.
But maybe doing that is super painful, so, maybe it's not worth it. IDK.
^ permalink raw reply
* Re: [PATCH v5 2/3] git-std-lib: introduce Git Standard Library
From: Junio C Hamano @ 2024-02-29 18:54 UTC (permalink / raw)
To: Linus Arver; +Cc: Phillip Wood, Calvin Wan, git, Jonathan Tan
In-Reply-To: <owly34tb5dop.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> It does make me wonder if we should stop being lazy and do the
> work that the linker has been doing for us "for free"
> ourselves. IOW, stop linking against a monolithic libgit.a.
> ... which might help us understand which things need what.
Sorry, but I fail see a point in such an exercise. If a tool is
available to help us and if there is no downside of using the tool,
we should keep using it. If you are proposing to move away from the
current build practice because you have a concrete downside of the
approach and avoid that, then it might be a good proposal, though.
And "we do not learn otherwise" is not a downside of the approach;
"we do not learn" comes from your not learning, the tools do not
force you to be ignorant. We do not propose to use more __asm__ in
our C sources only because compilers were doing that for us "for
free" and because the compilers were somehow robbing us the
opportunity to learn micro-optimization techniques, do we?
A small downside I can immediately think of is possible in a
situation where we have been throwing an object file into libgit.a
archive that is no longer used by any final executable. In such a
scenario, if you change the source file that is compiled into such
an unused object file, your next "make" will update libgit.a to
replace the unused object file with its new version with your
updates, and that would cause the final build product to be linked
again with objects needed from libgit.a, but there shouldn't be any
change because we are talking about an object that is *not* used by
them but still is in libgit.a due to be listed on LIB_OBJS variable.
But that is a purely theoretical downside. It may be the case that
we haven't done our spring cleaning recently and we haven't noticed
that a source file or two are now unused but are still listed on
LIB_OBJS to be included in the libgit.a archive. But even if that
were the case, it is implausible that you are touching such an
unused source file in the first place.
^ permalink raw reply
* Re: [PATCH] branch: adjust documentation
From: Rubén Justo @ 2024-02-29 18:56 UTC (permalink / raw)
To: Junio C Hamano, Dragan Simic; +Cc: git
In-Reply-To: <xmqqttlsld4t.fsf@gitster.g>
On Wed, Feb 28, 2024 at 09:20:02 -0800, Junio C Hamano wrote:
> The current description section talks about option and its arguments
> without showing the syntax, making it unnecessarily extra verbose.
> For example, we see something like this:
>
> With a `-m` or `-M` option, <oldbranch> will be renamed to
> <newbranch>. If <oldbranch> had a corresponding reflog, it is
> renamed to match ...
>
> But in the final shape of the documentation, I would like to see the
> description section not talk about these arguments, and would read
> more like
>
> When given `-m` or `-M` options, the command renames an existing
> branch to a different name.
>
Good.
> among short descriptions made at the conceptual level on other modes
> like "listing" mode, "delete" mode, etc. [*3*]
>
> And the option description would become something like [*]:
>
> -m [<one>] <two>::
> Renames the branch <one> (the current branch is used when
> not given) to a new name <two>, together with its reflog and
> configuration settings for the branch. ...
> Now in such a context, <one> and <two> placeholders having actually
> the word "branch" in it would sound redundant and awkward to read,
Indeed.
But I'm on the fence. Do we have to use "Renames the branch <one>"?
If we wisely choose the placeholder, perhaps we can write:
-m [<one>] <two>::
Renames <one> (the current branch is used when not given) to
a new name <two>, together with its reflog and configuration
settings ...
And if <one> is _good enough_ then "current branch is used when ..."
should seem somewhat redundant. So it could be possible to end up
having something like:
-m [<one>] <two>::
Renames <one> to a new name <two>, together with its reflog
and configuration settings ...
Are we going to say "the current branch is used when ..." in the
description for the other options too? The description for "-c|-C",
"--edit-description", "--unset-upstream", ... Perhaps we are, and it
will sound repetitive. However, even if we do, with the _good enough_
placeholder the user will be able to fill the gaps we might leave in
the documentation, like the one that presumably has bring us here:
pull.1613.git.git.1701303891791.gitgitgadget@gmail.com.
And finally; Can't <one> and <two> be consistent with other bits we
have in the documentation like the descriptions of "git switch <one>",
"git checkout -b <two>" or "git init -b <three>"?
After the revamp, I'll be less happy (but happy :-) nonetheless) if we
end up having a SYNOPSIS similar to the one we have today:
- Documentation/git-branch.txt:
'git branch' (-c | -C) [<one>] <two>
'git branch' (-d | -D) [-r] <three>...
'git branch' --edit-description [<three>]
It seems to me to be made up of disconnected pieces.
And for reference:
- Documentation/git-switch.txt:
'git switch' [<options>] [--no-guess] <four>
'git switch' [<options>] --orphan <five>
- Documentation/git-init.txt:
'git init' [--initial-branch=<six>]
My apologies if I'm pushing too hard on this and being reiterative in
my messages in this series. My intention has been to explore the use
we want of the placeholders. Of course, this is not at odds with my
sympathy for the vision proposed. I agree on the direction.
> Even though the choice of words Rubén made in the patch under
> discussion may work well in the current document structure.
My patch is mainly about CodingGuideLines:
If a placeholder has multiple words, they are separated by dashes:
<new-branch-name>
--template=<template-directory>
^ permalink raw reply
* Re: [PATCH 3/4] completion: reflog with implicit "show"
From: Rubén Justo @ 2024-02-29 19:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <abe1c96d-48f7-4355-8688-e1fb05c67448@gmail.com>
On Wed, Feb 21, 2024 at 07:06:36PM +0100, Rubén Justo wrote:
> On Tue, Feb 20, 2024 at 17:46:20 -0800, Junio C Hamano wrote:
>
> > $ git reflog sho<TAB>
> > shot show
> >
> > That is what I would expect.
>
> Thank you for responding.
>
> Of course that's the logical expectation.
>
> However I'm not sure if it is sensible to offer completion for
> sub-commands mixed with branch names.
>
> Furthermore, I am also worried that such an approach implies making the
> user pay, probably unnecessarily many times, for __git_complete_refs in
> cases such as:
>
> $ git reflog <TAB><TAB> ;# the user may be just exploring the sub-commands
> $ git reflog s<TAB> ;# the user may be lazy and just want "show "
> $ git reflog show<TAB> ;# likewise, to complete the SP
> $ git reflog expir<TAB> ;# how often a expir... branch is expected here?
>
> The experienced user, if not most users, should be intuitive enough to
> circumvent the corner cases:
>
> $ git reflog <TAB><TAB>
> delete expire show
> ...
> $ git reflog s<TAB>
> ...
> $ git reflog show s<TAB>
> ...
> $ git reflog show shot
>
> This is why I choose to fallback to __git_complete_ref only when no
> other option is available.
>
> If you think, or anyone else, that these concerns don't make sense, I'm
> open to make it work as you described.
I am happy with the current iteration and I still think that mixing
branch names with options is a source of confusion.
However, this topic in the latest 'What's cooking' is marked with:
Expecting a reroll.
cf. <dd106d87-3363-426a-90a2-16e1f2d04661@gmail.com>
source: <98daf977-dbad-4d3b-a293-6a769895088f@gmail.com>
I am confused about what the expectation is.
Consider this message a ping as maybe the message I'm responding to has
been missed.
Thanks.
^ permalink raw reply
* [PATCH] docs: sort configuration variable groupings alphabetically
From: Eric Sunshine @ 2024-02-29 19:02 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Bruno Haible
In-Reply-To: <6139934.yKrmzQ4Hd0@nimes>
From: Eric Sunshine <sunshine@sunshineco.com>
By and large, variable groupings in Documentation/config.txt are sorted
alphabetically, though a few are not. Those outliers make it more
difficult to find a specific grouping when quickly running an eye over
the list to locate a variable of interest. Address this shortcoming by
sorting the groupings alphabetically.
NOTE: This change only sorts the top-level groupings (i.e. "core.*"
comes after "completion.*"); it does not touch the ordering of variables
within each group since variables within individual groups might
intentionally be ordered in some other fashion (such as
most-common-first or most-important-first).
Reported-by: Bruno Haible <bruno@clisp.org>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
Documentation/config.txt | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e3a74dd1c1..782c2bab90 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -369,20 +369,18 @@ inventing new variables for use in your own tool, make sure their
names do not conflict with those that are used by Git itself and
other popular tools, and describe them in your documentation.
-include::config/advice.txt[]
-
-include::config/attr.txt[]
-
-include::config/core.txt[]
-
include::config/add.txt[]
+include::config/advice.txt[]
+
include::config/alias.txt[]
include::config/am.txt[]
include::config/apply.txt[]
+include::config/attr.txt[]
+
include::config/blame.txt[]
include::config/branch.txt[]
@@ -405,10 +403,12 @@ include::config/commit.txt[]
include::config/commitgraph.txt[]
-include::config/credential.txt[]
-
include::config/completion.txt[]
+include::config/core.txt[]
+
+include::config/credential.txt[]
+
include::config/diff.txt[]
include::config/difftool.txt[]
@@ -421,10 +421,10 @@ include::config/feature.txt[]
include::config/fetch.txt[]
-include::config/format.txt[]
-
include::config/filter.txt[]
+include::config/format.txt[]
+
include::config/fsck.txt[]
include::config/fsmonitor--daemon.txt[]
@@ -435,10 +435,10 @@ include::config/gitcvs.txt[]
include::config/gitweb.txt[]
-include::config/grep.txt[]
-
include::config/gpg.txt[]
+include::config/grep.txt[]
+
include::config/gui.txt[]
include::config/guitool.txt[]
@@ -519,10 +519,10 @@ include::config/splitindex.txt[]
include::config/ssh.txt[]
-include::config/status.txt[]
-
include::config/stash.txt[]
+include::config/status.txt[]
+
include::config/submodule.txt[]
include::config/tag.txt[]
--
2.44.0
^ permalink raw reply related
* Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command
From: Junio C Hamano @ 2024-02-29 19:06 UTC (permalink / raw)
To: Eric Sunshine; +Cc: shejialuo, git
In-Reply-To: <CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
>> @@ -175,7 +175,7 @@ match() {
>> test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '
>> - if test -e .git/created_test_file
>> + if test_path_exists .git/created_test_file
>> then
>> git reset &&
>
> ... which _do_ use test_path_exists() within a `test_expect_success`
> block. However, the changes are still undesirable because, as above,
> this `test -e` is merely part of the normal control-flow; it's not
> acting as an assertion, thus test_path_exists() -- which is an
> assertion -- is not correct.
>
> Unfortunately, none of the uses of`test -e` in t3070 are being used as
> assertions worthy of replacement with test_path_exists(), thus this
> isn't a good script in which to make such changes.
It seems that there is a recurring confusion among mentorship
program applicants that use test_path_* helpers as their practice
material. Perhaps the source of the information that suggests it as
a microproject is poorly phrased and needs to be rewritten to avoid
misleading them.
I found one at https://git.github.io/Outreachy-23-Microprojects/,
which can be one source of such confusion:
Find one test script that verifies the presence/absence of
files/directories with ‘test -(e|f|d|…)’ and replace them
with the appropriate test_path_is_file, test_path_is_dir,
etc. helper functions.
but there may be others.
This task specification does not differenciate "test -[efdx]" used
as a conditional of a control flow statement (which should never be
replaced by test_path_* helpers) and those used to directly fail the
&&-chain in test_expect_success with their exit status (which is the
target that test_path_* helpers are meant to improve).
^ permalink raw reply
* [PATCH] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-02-29 19:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34v6gswv.fsf@gitster.g>
What -n actually does in addition to its documented behavior is
ignoring of configuration variable clean.requireForce, that makes
sense provided -n prevents files removal anyway.
So, first, document this in the manual, and then modify implementation
to make this more explicit in the code.
Improved implementation also stops to share single internal variable
'force' between command-line -f option and configuration variable
clean.requireForce, resulting in more clear logic.
The error messages now do not mention -n as well, as it seems
unnecessary and does not reflect clarified implementation.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
Documentation/git-clean.txt | 2 ++
builtin/clean.c | 26 +++++++++++++-------------
2 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 69331e3f05a1..662eebb85207 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -49,6 +49,8 @@ OPTIONS
-n::
--dry-run::
Don't actually remove anything, just show what would be done.
+ Configuration variable clean.requireForce is ignored, as
+ nothing will be deleted anyway.
-q::
--quiet::
diff --git a/builtin/clean.c b/builtin/clean.c
index d90766cad3a0..fcc50d08ee9b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -25,7 +25,7 @@
#include "help.h"
#include "prompt.h"
-static int force = -1; /* unset */
+static int require_force = -1; /* unset */
static int interactive;
static struct string_list del_list = STRING_LIST_INIT_DUP;
static unsigned int colopts;
@@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
}
if (!strcmp(var, "clean.requireforce")) {
- force = !git_config_bool(var, value);
+ require_force = git_config_bool(var, value);
return 0;
}
@@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
{
int i, res;
int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
- int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
+ int ignored_only = 0, force = 0, errors = 0, gone = 1;
int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
struct strbuf abs_path = STRBUF_INIT;
struct dir_struct dir = DIR_INIT;
@@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
};
git_config(git_clean_config, NULL);
- if (force < 0)
- force = 0;
- else
- config_set = 1;
argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
0);
- if (!interactive && !dry_run && !force) {
- if (config_set)
- die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
+ /* Dry run won't remove anything, so requiring force makes no sense */
+ if(dry_run)
+ require_force = 0;
+
+ if (!force && !interactive) {
+ if (require_force > 0)
+ die(_("clean.requireForce set to true and neither -f, nor -i given; "
+ "refusing to clean"));
+ else if (require_force < 0)
+ die(_("clean.requireForce defaults to true and neither -f, nor -i given; "
"refusing to clean"));
- else
- die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;"
- " refusing to clean"));
}
if (force > 1)
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
--
2.25.1
^ permalink raw reply related
* Re: git-config doc: variables sorting bug
From: Eric Sunshine @ 2024-02-29 19:08 UTC (permalink / raw)
To: Bruno Haible; +Cc: git
In-Reply-To: <6139934.yKrmzQ4Hd0@nimes>
On Wed, Feb 28, 2024 at 5:15 AM Bruno Haible <bruno@clisp.org> wrote:
> In https://git-scm.com/docs/git-config the variables (advice.* ...
> worktree.guessRemote) are apparently meant to be in alphabetical order.
>
> However,
> filter.<driver>.clean
> filter.<driver>.smudge
> come after format.*. They should come before format.*, since 'i' < 'o'.
A possible fix has been posted here:
https://lore.kernel.org/git/20240229190229.20222-1-ericsunshine@charter.net/
^ permalink raw reply
* Re: [PATCH] setup: clarify TODO comment about ignoring core.bare
From: Junio C Hamano @ 2024-02-29 19:15 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git, johannes.schindelin, newren, christian.couder
In-Reply-To: <20240229134114.285393-2-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> /*
> - * TODO: heed core.bare from config file in templates if no
> - * command-line override given
> + * Note: The below line simply checks the presence of worktree (the
> + * simplification of which is given after the line) and core.bare from
> + * config file is not taken into account when deciding if the worktree
> + * should be created or not, even if no command line override given.
> + * That is intentional. Therefore, if in future we want to heed
> + * core.bare from config file, we should do it before we create any
> + * subsequent directories for worktree or repo because until this point
> + * they should already be created.
> */
> is_bare_repository_cfg = prev_bare_repository || !work_tree;
I do not recall the discussion; others may want to discuss if the
change above is desirable, before I come back to the topic later.
But I see this long comment totally unnecessary and distracting.
> - /* TODO (continued):
> + /* Note (continued):
> *
> - * Unfortunately, the line above is equivalent to
> + * The line above is equivalent to
> * is_bare_repository_cfg = !work_tree;
> - * which ignores the config entirely even if no `--[no-]bare`
> - * command line option was present.
> *
> * To see why, note that before this function, there was this call:
> * prev_bare_repository = is_bare_repository()
If it can be proven that the assignment can be simplified to lose
the "prev_bare_repository ||" part, then the above comment can be
used as part of the proposed log message for a commit that makes
such a change. There is no reason to leave such a long comment to
leave the more complex "A || B" expression when it can be simplified
to "B", no?
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