Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hey Junio, On Tue, Jul 26, 2016 at 11:02 PM, Junio C Hamanowrote: > Torsten Bögershausen writes: > >> On 07/25/2016 06:53 PM, Junio C Hamano wrote: >>> Pranit Bauva writes: >>> >> >>> +enum terms_defined { >> >>> + TERM_BAD = 1, >> >>> + TERM_GOOD = 2, >> >>> + TERM_NEW = 4, >> >>> + TERM_OLD = 8 >> >>> +}; >> >>> + > >> ... >> Is there any risk that a more generic term like "TERM_BAD" may collide >> with some other definition some day ? >> >> Would it make sense to call it GIT_BISECT_TERM_BAD, GBS_TERM_BAD, >> BIS_TERM_BAD or something more unique ? > > I am not sure if the scope of these symbols would ever escape > outside bisect-helper.c (and builtin/bisect.c eventually when we > retire git-bisect.sh), but BISECT_TERM_{GOOD,BAD,OLD,NEW} would not > be too bad. I agree that it wouldn't be too bad. This can be considered as low hanging fruit and picked up after the completion of the project as after the whole conversion, some re-ordering of code would need to be done. For eg. there is read_bisect_terms() is in bisect.c while get_terms() is in builtin/bisect--helper.c but they both do the same stuff except the later one uses strbuf and a lot more important stuff. Maybe after the whole conversion, the above enum (or #define bitmap) should also be moved to bisect.h and be used consistently in bisect.c too. Regards, Pranit Bauva -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/4] submodule: support running in multiple worktree setup
Hi. On Wed, Jul 20, 2016 at 07:24:18PM +0200, Nguyễn Thái Ngọc Duy wrote: > + - `remote.*` added by submodules may be per working directory as > + well, unless you are sure remotes from all possible submodules in > + history are consistent. ... > @@ -1114,7 +1114,7 @@ cmd_sync() > sanitize_submodule_env > cd "$sm_path" > remote=$(get_default_remote) > - git config remote."$remote".url > "$sub_origin_url" > + git config --worktree remote."$remote".url > "$sub_origin_url" > > if test -n "$recursive" > then I don't think remote.* should be per-worktree. * note that it is sumodule repository, not superproject. It does not even have to have multiple worktrees. * it is quite bad to have it different in worktree, because git fetch then results in different ref updates depending on where it was called. So whatever issue it was intended to solve, it hardly made things better. * I'm not sure I know all use cases of "submodule sync", but as far as I understand, it should be called when the submodule repository stays the "same" (however user defines the "same"), but older url does not work for some reason. Then I think it is correct to change the remote url for all worktrees. -- Max -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/5] convert: generate large test files only once
On 07/27/2016 02:06 AM, larsxschnei...@gmail.com wrote: From: Lars SchneiderGenerate a more interesting large test file with random characters in between and reuse this test file in multiple tests. Run tests formerly marked as EXPENSIVE every time but with a smaller test file. Signed-off-by: Lars Schneider --- t/t0021-conversion.sh | 35 +-- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh index 7b45136..b9911a4 100755 --- a/t/t0021-conversion.sh +++ b/t/t0021-conversion.sh @@ -4,6 +4,13 @@ test_description='blob conversion via gitattributes' . ./test-lib.sh +if test_have_prereq EXPENSIVE +then + T0021_LARGE_FILE_SIZE=2048 +else + T0021_LARGE_FILE_SIZE=30 +fi + cat test.i && git add test test.t test.i && rm -f test test.t test.i && - git checkout -- test test.t test.i + git checkout -- test test.t test.i && + + mkdir -p generated-test-data && + for i in $(test_seq 1 $T0021_LARGE_FILE_SIZE) + do + # Generate 1MB of empty data and 100 bytes of random characters + printf "%1048576d" 1 + printf "$(LC_ALL=C tr -dc "A-Za-z0-9" >8)) count=1 2>/dev/null)" I'm not sure how portable /dev/urandom is. The other thing, that "really random" numbers are an overkill, and it may be easier to use pre-defined numbers, The rest of 1..4 looks good, I will look at 5/5 later. + done >generated-test-data/large.file ' script='s/^\$Id: \([0-9a-f]*\) \$/\1/p' @@ -199,9 +214,9 @@ test_expect_success 'required filter clean failure' ' test_expect_success 'filtering large input to small output should use little memory' ' test_config filter.devnull.clean "cat >/dev/null" && test_config filter.devnull.required true && - for i in $(test_seq 1 30); do printf "%1048576d" 1; done >30MB && - echo "30MB filter=devnull" >.gitattributes && - GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB + cp generated-test-data/large.file large.file && + echo "large.file filter=devnull" >.gitattributes && + GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add large.file ' test_expect_success 'filter that does not read is fine' ' @@ -214,15 +229,15 @@ test_expect_success 'filter that does not read is fine' ' test_cmp expect actual ' -test_expect_success EXPENSIVE 'filter large file' ' +test_expect_success 'filter large file' ' test_config filter.largefile.smudge cat && test_config filter.largefile.clean cat && - for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB && - echo "2GB filter=largefile" >.gitattributes && - git add 2GB 2>err && + echo "large.file filter=largefile" >.gitattributes && + cp generated-test-data/large.file large.file && + git add large.file 2>err && test_must_be_empty err && - rm -f 2GB && - git checkout -- 2GB 2>err && + rm -f large.file && + git checkout -- large.file 2>err && test_must_be_empty err ' -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] convert: add filter..process option
On Wed, Jul 27, 2016 at 02:06:05AM +0200, larsxschnei...@gmail.com wrote: > +static off_t multi_packet_read(struct strbuf *sb, const int fd, const size_t > size) > +{ > + off_t bytes_read; > + off_t total_bytes_read = 0; I haven't looked carefully at the whole patch yet, but there seems to be some type issues here. off_t is a good type for storing the whole size of a file (which may be larger than the amount of memory we can allocate). But size_t is the right size for an in-memory object. This function takes a size_t size, which makes sense if it is meant to read everything into a strbuf. So I think our total_bytes_read would probably want to be a size_t here, too, because it cannot possibly grow larger than that (and that is enforced by the loop below). Otherwise you get weirdness like "sb->buf + total_bytes_ref" possibly overflowing memory. > + strbuf_grow(sb, size + 1); // we need one extra byte for the > packet flush What happens if size is the maximum for size_t here (i.e., 4GB-1 on a 32-bit system)? > + do { > + bytes_read = packet_read( > + fd, NULL, NULL, > + sb->buf + total_bytes_read, sb->len - total_bytes_read > - 1, > + PACKET_READ_GENTLE_ON_EOF > + ); packet_read() actually returns an int, and may return "-1" on EOF (and int is fine because we know that we are constrained to 16-bit values by the pkt-line definition). You read it into an "off_t". I _think_ that is OK, because I believe POSIX says off_t must be signed. But probably "int" is the more correct type here. > + total_bytes_read += bytes_read; If you do get "-1", I think you need to detect it here before adjusting total_bytes_read. > + while ( > + bytes_read > 0 && // the > last packet was no flush > + sb->len - total_bytes_read - 1 > 0 // we still have space > left in the buffer > + ); And I'm not sure if you need to distinguish between "0" and "-1" when checking byte_read here. > + strbuf_setlen(sb, total_bytes_read); Passing an off_t to something expecting a size_t, which can involve truncation (though I think in practice you really are limited to size_t). > +static int multi_packet_write(const char *src, size_t len, const int in, > const int out) > +{ > + int ret = 1; > + char header[4]; > + char buffer[8192]; > + off_t bytes_to_write; > + while (ret) { > + if (in >= 0) { > + bytes_to_write = xread(in, buffer, sizeof(buffer)); Likewise here, xread() is returning ssize_t. Again, OK if we can assume off_t is signed, but it probably makes sense to use the correct type (we also know it cannot be larger than 8K, of course). Why 8K? The pkt-line format naturally restricts us to just under 64K, so why not take advantage of that and minimize the framing overhead for large data? > + if (bytes_to_write < 0) > + ret &= 0; I think "&= 0" is unusual for our codebase? Would just writing "= 0" be more clear? We do sometimes do "ret |= something()" but that is in cases where "ret" is zero for success, and non-zero (usually -1) otherwise. Perhaps your function's error-reporting is inverted from our usual style? > + set_packet_header(header, bytes_to_write + 4); > + ret &= write_in_full(out, , sizeof(header)) == > sizeof(header); > + ret &= write_in_full(out, src, bytes_to_write) == > bytes_to_write; > + } If you look at format_packet(), it pulls a slight trick: we have a buffer 4 bytes larger than we need, format into "buf + 4", and then write the final size at the beginning. That lets us write() it all in one go. At first I thought this function was simply reinventing packet_write(), but I guess you are trying to avoid the extra copy of the data (once into the buffer from xread, and then again via format_packet just to add the extra bytes at the beginning). I agree with what Junio said elsewhere, that there may be a way to make the pkt-line code handle this zero-copy situation better. Perhaps something like: struct pktline { /* first 4 bytes are reserved for length header */ char buf[LARGE_PACKET_MAX]; }; #define PKTLINE_DATA_START(pkt) ((pkt)->buf + 4) #define PKTLINE_DATA_LEN (LARGE_PACKET_MAX - 4) ... struct pktline pkt; ssize_t len = xread(fd, PKTLINE_DATA_START(), PKTLINE_DATA_LEN); packet_send(, len); Then packet_send() knows that the first 4 bytes are reserved for it. I suspect that the strbuf used by format_packet() could get away with using such a "struct pktline" too, though in practice I doubt there's any real efficiency to be gained (we generally reuse the same strbuf over and over, so it will grow once to 64K and get reused). > + ret &= write_in_full(out, "", 4) == 4; packet_flush() ? I know the packet functions are keen on
[PATCH v2 3/3] subtree: adjust style to match CodingGuidelines
Prefer "test" over "[ ... ]", use double-quotes around variables, break long lines, and properly indent "case" statements. Helped-by: Johannes SixtSigned-off-by: David Aguilar --- This is a replacement patch that addresses the notes from Hannes' review. contrib/subtree/git-subtree.sh | 570 + 1 file changed, 354 insertions(+), 216 deletions(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index b567eae..9fd3b6a 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -4,8 +4,9 @@ # # Copyright (C) 2009 Avery Pennarun # -if [ $# -eq 0 ]; then -set -- -h +if test $# -eq 0 +then + set -- -h fi OPTS_SPEC="\ git subtree add --prefix= @@ -50,87 +51,145 @@ prefix= debug() { - if [ -n "$debug" ]; then + if test -n "$debug" + then printf "%s\n" "$*" >&2 fi } say() { - if [ -z "$quiet" ]; then + if test -z "$quiet" + then printf "%s\n" "$*" >&2 fi } progress() { - if [ -z "$quiet" ]; then + if test -z "$quiet" + then printf "%s\r" "$*" >&2 fi } assert() { - if "$@"; then - : - else + if ! "$@" + then die "assertion failed: " "$@" fi } -#echo "Options: $*" - -while [ $# -gt 0 ]; do +while test $# -gt 0 +do opt="$1" shift + case "$opt" in - -q) quiet=1 ;; - -d) debug=1 ;; - --annotate) annotate="$1"; shift ;; - --no-annotate) annotate= ;; - -b) branch="$1"; shift ;; - -P) prefix="${1%/}"; shift ;; - -m) message="$1"; shift ;; - --no-prefix) prefix= ;; - --onto) onto="$1"; shift ;; - --no-onto) onto= ;; - --rejoin) rejoin=1 ;; - --no-rejoin) rejoin= ;; - --ignore-joins) ignore_joins=1 ;; - --no-ignore-joins) ignore_joins= ;; - --squash) squash=1 ;; - --no-squash) squash= ;; - --) break ;; - *) die "Unexpected option: $opt" ;; + -q) + quiet=1 + ;; + -d) + debug=1 + ;; + --annotate) + annotate="$1" + shift + ;; + --no-annotate) + annotate= + ;; + -b) + branch="$1" + shift + ;; + -P) + prefix="${1%/}" + shift + ;; + -m) + message="$1" + shift + ;; + --no-prefix) + prefix= + ;; + --onto) + onto="$1" + shift + ;; + --no-onto) + onto= + ;; + --rejoin) + rejoin=1 + ;; + --no-rejoin) + rejoin= + ;; + --ignore-joins) + ignore_joins=1 + ;; + --no-ignore-joins) + ignore_joins= + ;; + --squash) + squash=1 + ;; + --no-squash) + squash= + ;; + --) + break + ;; + *) + die "Unexpected option: $opt" + ;; esac done command="$1" shift + case "$command" in - add|merge|pull) default= ;; - split|push) default="--default HEAD" ;; - *) die "Unknown command '$command'" ;; +add|merge|pull) + default= + ;; +split|push) + default="--default HEAD" + ;; +*) + die "Unknown command '$command'" + ;; esac -if [ -z "$prefix" ]; then +if test -z "$prefix" +then die "You must provide the --prefix option." fi case "$command" in - add) [ -e "$prefix" ] && - die "prefix '$prefix' already exists." ;; - *) [ -e "$prefix" ] || - die "'$prefix' does not exist; use 'git subtree add'" ;; +add) + test -e "$prefix" && die "prefix '$prefix' already exists." + ;; +*) + test -e "$prefix" || + die "'$prefix' does not exist; use 'git subtree add'" + ;; esac dir="$(dirname "$prefix/.")" -if [ "$command" != "pull" -a "$command" != "add" -a "$command" != "push" ]; then +if test "$command" != "pull" && + test "$command" != "add" && + test "$command" != "push" +then revs=$(git rev-parse $default --revs-only "$@") || exit $? - dirs="$(git rev-parse --no-revs --no-flags "$@")" || exit $? - if [ -n "$dirs" ]; then + dirs=$(git rev-parse --no-revs --no-flags "$@") || exit $? + if test -n "$dirs" + then die "Error:
Re: [PATCH v2 3/5] pkt-line: extract and use `set_packet_header` function
larsxschnei...@gmail.com writes: > From: Lars Schneider> > `set_packet_header` converts an integer to a 4 byte hex string. Make > this function publicly available so that other parts of Git can easily > generate a pkt-line. I think that having to do this is a strong sign that the design of this series is going in a wrong direction. If you need a helper function that writes a pkt-line format that behaves differently from what is already available (for example, packet_write()), it would be much better to design that new function so that it would be generally useful and add that to pkt-line.[ch], instead of creating random helper functions that use write(2) directly, bypassing pkt-line API, to write stuff. In other words, do not _mimick_ pkt-line; enhance pkt-line as necessary and use it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/5] Git filter protocol
From: Lars SchneiderHi, thanks a lot for the extensive reviews. I tried to address all mentioned concerns and summarized them below. The most prominent changes since v1 are the following: * pipe communication uses a packet format (pkt-line) based protocol * a long running filter application is defined with "filter..process" * Git offers a number of filter capabilties that a filter can request (right now only "smudge" and "clean" - in the future maybe "cleanFromFile", "smudgeToFile", and/or "stream") Cheers, Lars ## Torsten: * add "\n" line terminator after version in init sequence * prepare big file for EXPENSIVE tests once * set "#!/usr/bin/perl" as shebang for rot13.pl to mimic other Perl test scripts * add test_have_prereq PERL to t0021 ## Ramsay: * use write_in_full(process->in, nbuf.buf, nbuf.len) to avoid unneccsary strlen call * use read_in_full to read data that exceeds MAX_IO_SIZE properly * fix test case to check for large file filtering ## Jakub: * use standard input/standard output instead of stdin/stdout * replace global variable "cmd_process_map" with a function parameter where possible * avoid "strbuf_reset" after STRBUF_INIT * align test_config_global * rename rot13.pl to rot13-filter.pl * make Perl style consistent * describe hard coded filenames in test filter header * improve docs * add filter capabilities field (enables cleanToFile, smudgeFromFile, and/or stream later) * explain that content size in bytes is encoded in ASCII * consistent line ending for die call in Perl (without "\n") * make rot13 test filter die in case of failure (instead of returning "fail") ## Eric: * flush explicitly in Perl test filter * do not initialize variables to NULL if they are set unconditionally * fix no-op stop_protocol_filter * use off_t instead of size_t * improve test filter int parsing ($filelen =~ /\A\d+\z/ or die "bad filelen: $filelen") ## Peff: * use pkt-line protocol * do not use Perl autodie ## Remi: * remove spaces after '<' Lars Schneider (5): convert: quote filter names in error messages convert: modernize tests pkt-line: extract and use `set_packet_header` function convert: generate large test files only once convert: add filter..process option Documentation/gitattributes.txt | 54 +++- convert.c | 281 +--- pkt-line.c | 15 ++- pkt-line.h | 1 + t/t0021-conversion.sh | 272 -- t/t0021/rot13-filter.pl | 146 + 6 files changed, 704 insertions(+), 65 deletions(-) create mode 100755 t/t0021/rot13-filter.pl -- 2.9.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/5] convert: modernize tests
From: Lars SchneiderUse `test_config` to set the config, check that files are empty with `test_must_be_empty`, compare files with `test_cmp`, and remove spaces after ">". Signed-off-by: Lars Schneider --- t/t0021-conversion.sh | 62 +-- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh index 7bac2bc..7b45136 100755 --- a/t/t0021-conversion.sh +++ b/t/t0021-conversion.sh @@ -13,8 +13,8 @@ EOF chmod +x rot13.sh test_expect_success setup ' - git config filter.rot13.smudge ./rot13.sh && - git config filter.rot13.clean ./rot13.sh && + test_config filter.rot13.smudge ./rot13.sh && + test_config filter.rot13.clean ./rot13.sh && { echo "*.t filter=rot13" @@ -38,8 +38,8 @@ script='s/^\$Id: \([0-9a-f]*\) \$/\1/p' test_expect_success check ' - cmp test.o test && - cmp test.o test.t && + test_cmp test.o test && + test_cmp test.o test.t && # ident should be stripped in the repository git diff --raw --exit-code :test :test.i && @@ -47,10 +47,10 @@ test_expect_success check ' embedded=$(sed -ne "$script" test.i) && test "z$id" = "z$embedded" && - git cat-file blob :test.t > test.r && + git cat-file blob :test.t >test.r && - ./rot13.sh < test.o > test.t && - cmp test.r test.t + ./rot13.sh test.t && + test_cmp test.r test.t ' # If an expanded ident ever gets into the repository, we want to make sure that @@ -130,7 +130,7 @@ test_expect_success 'filter shell-escaped filenames' ' # delete the files and check them out again, using a smudge filter # that will count the args and echo the command-line back to us - git config filter.argc.smudge "sh ./argc.sh %f" && + test_config filter.argc.smudge "sh ./argc.sh %f" && rm "$normal" "$special" && git checkout -- "$normal" "$special" && @@ -141,7 +141,7 @@ test_expect_success 'filter shell-escaped filenames' ' test_cmp expect "$special" && # do the same thing, but with more args in the filter expression - git config filter.argc.smudge "sh ./argc.sh %f --my-extra-arg" && + test_config filter.argc.smudge "sh ./argc.sh %f --my-extra-arg" && rm "$normal" "$special" && git checkout -- "$normal" "$special" && @@ -154,9 +154,9 @@ test_expect_success 'filter shell-escaped filenames' ' ' test_expect_success 'required filter should filter data' ' - git config filter.required.smudge ./rot13.sh && - git config filter.required.clean ./rot13.sh && - git config filter.required.required true && + test_config filter.required.smudge ./rot13.sh && + test_config filter.required.clean ./rot13.sh && + test_config filter.required.required true && echo "*.r filter=required" >.gitattributes && @@ -165,17 +165,17 @@ test_expect_success 'required filter should filter data' ' rm -f test.r && git checkout -- test.r && - cmp test.o test.r && + test_cmp test.o test.r && ./rot13.sh expected && git cat-file blob :test.r >actual && - cmp expected actual + test_cmp expected actual ' test_expect_success 'required filter smudge failure' ' - git config filter.failsmudge.smudge false && - git config filter.failsmudge.clean cat && - git config filter.failsmudge.required true && + test_config filter.failsmudge.smudge false && + test_config filter.failsmudge.clean cat && + test_config filter.failsmudge.required true && echo "*.fs filter=failsmudge" >.gitattributes && @@ -186,9 +186,9 @@ test_expect_success 'required filter smudge failure' ' ' test_expect_success 'required filter clean failure' ' - git config filter.failclean.smudge cat && - git config filter.failclean.clean false && - git config filter.failclean.required true && + test_config filter.failclean.smudge cat && + test_config filter.failclean.clean false && + test_config filter.failclean.required true && echo "*.fc filter=failclean" >.gitattributes && @@ -197,8 +197,8 @@ test_expect_success 'required filter clean failure' ' ' test_expect_success 'filtering large input to small output should use little memory' ' - git config filter.devnull.clean "cat >/dev/null" && - git config filter.devnull.required true && + test_config filter.devnull.clean "cat >/dev/null" && + test_config filter.devnull.required true && for i in $(test_seq 1 30); do printf "%1048576d" 1; done >30MB && echo "30MB filter=devnull" >.gitattributes && GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB @@ -207,7 +207,7 @@ test_expect_success 'filtering large input to small output should use little mem test_expect_success
[PATCH v2 1/5] convert: quote filter names in error messages
From: Lars SchneiderGit filter with spaces (e.g. `filter.sh foo`) are hard to read in error messages. Quote them to improve the readability. Signed-off-by: Lars Schneider --- convert.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/convert.c b/convert.c index b1614bf..522e2c5 100644 --- a/convert.c +++ b/convert.c @@ -397,7 +397,7 @@ static int filter_buffer_or_fd(int in, int out, void *data) child_process.out = out; if (start_command(_process)) - return error("cannot fork to run external filter %s", params->cmd); + return error("cannot fork to run external filter '%s'", params->cmd); sigchain_push(SIGPIPE, SIG_IGN); @@ -415,13 +415,13 @@ static int filter_buffer_or_fd(int in, int out, void *data) if (close(child_process.in)) write_err = 1; if (write_err) - error("cannot feed the input to external filter %s", params->cmd); + error("cannot feed the input to external filter '%s'", params->cmd); sigchain_pop(SIGPIPE); status = finish_command(_process); if (status) - error("external filter %s failed %d", params->cmd, status); + error("external filter '%s' failed %d", params->cmd, status); strbuf_release(); return (write_err || status); @@ -462,15 +462,15 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd, return 0; /* error was already reported */ if (strbuf_read(, async.out, len) < 0) { - error("read from external filter %s failed", cmd); + error("read from external filter '%s' failed", cmd); ret = 0; } if (close(async.out)) { - error("read from external filter %s failed", cmd); + error("read from external filter '%s' failed", cmd); ret = 0; } if (finish_async()) { - error("external filter %s failed", cmd); + error("external filter '%s' failed", cmd); ret = 0; } -- 2.9.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/5] convert: generate large test files only once
From: Lars SchneiderGenerate a more interesting large test file with random characters in between and reuse this test file in multiple tests. Run tests formerly marked as EXPENSIVE every time but with a smaller test file. Signed-off-by: Lars Schneider --- t/t0021-conversion.sh | 35 +-- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh index 7b45136..b9911a4 100755 --- a/t/t0021-conversion.sh +++ b/t/t0021-conversion.sh @@ -4,6 +4,13 @@ test_description='blob conversion via gitattributes' . ./test-lib.sh +if test_have_prereq EXPENSIVE +then + T0021_LARGE_FILE_SIZE=2048 +else + T0021_LARGE_FILE_SIZE=30 +fi + cat test.i && git add test test.t test.i && rm -f test test.t test.i && - git checkout -- test test.t test.i + git checkout -- test test.t test.i && + + mkdir -p generated-test-data && + for i in $(test_seq 1 $T0021_LARGE_FILE_SIZE) + do + # Generate 1MB of empty data and 100 bytes of random characters + printf "%1048576d" 1 + printf "$(LC_ALL=C tr -dc "A-Za-z0-9" >8)) count=1 2>/dev/null)" + done >generated-test-data/large.file ' script='s/^\$Id: \([0-9a-f]*\) \$/\1/p' @@ -199,9 +214,9 @@ test_expect_success 'required filter clean failure' ' test_expect_success 'filtering large input to small output should use little memory' ' test_config filter.devnull.clean "cat >/dev/null" && test_config filter.devnull.required true && - for i in $(test_seq 1 30); do printf "%1048576d" 1; done >30MB && - echo "30MB filter=devnull" >.gitattributes && - GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB + cp generated-test-data/large.file large.file && + echo "large.file filter=devnull" >.gitattributes && + GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add large.file ' test_expect_success 'filter that does not read is fine' ' @@ -214,15 +229,15 @@ test_expect_success 'filter that does not read is fine' ' test_cmp expect actual ' -test_expect_success EXPENSIVE 'filter large file' ' +test_expect_success 'filter large file' ' test_config filter.largefile.smudge cat && test_config filter.largefile.clean cat && - for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB && - echo "2GB filter=largefile" >.gitattributes && - git add 2GB 2>err && + echo "large.file filter=largefile" >.gitattributes && + cp generated-test-data/large.file large.file && + git add large.file 2>err && test_must_be_empty err && - rm -f 2GB && - git checkout -- 2GB 2>err && + rm -f large.file && + git checkout -- large.file 2>err && test_must_be_empty err ' -- 2.9.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/5] convert: add filter..process option
From: Lars SchneiderGit's clean/smudge mechanism invokes an external filter process for every single blob that is affected by a filter. If Git filters a lot of blobs then the startup time of the external filter processes can become a significant part of the overall Git execution time. This patch adds the filter..process string option which, if used, keeps the external filter process running and processes all blobs with the following packet format (pkt-line) based protocol over standard input and standard output. Git starts the filter on first usage and expects a welcome message, protocol version number, and filter capabilities seperated by spaces: packet: git< git-filter-protocol packet: git< version 2 packet: git< clean smudge Supported filter capabilities are "clean" and "smudge". Afterwards Git sends a command (e.g. "smudge" or "clean" - based on the supported capabilities), the filename, the content size as ASCII number in bytes, and the content in packet format with a flush packet at the end: packet: git> smudge packet: git> testfile.dat packet: git> 7 packet: git> CONTENT packet: git> The filter is expected to respond with the result content size as ASCII number in bytes and the result content in packet format with a flush packet at the end: packet: git< 57 packet: git< SMUDGED_CONTENT packet: git< Please note: In a future version of Git the capability "stream" might be supported. In that case the content size must not be part of the filter response. Afterwards the filter is expected to wait for the next command. Signed-off-by: Lars Schneider Helped-by: Martin-Louis Bright Signed-off-by: Lars Schneider --- Documentation/gitattributes.txt | 54 +++- convert.c | 269 ++-- t/t0021-conversion.sh | 175 ++ t/t0021/rot13-filter.pl | 146 ++ 4 files changed, 631 insertions(+), 13 deletions(-) create mode 100755 t/t0021/rot13-filter.pl diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index 8882a3e..8fb40d2 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -300,7 +300,11 @@ checkout, when the `smudge` command is specified, the command is fed the blob object from its standard input, and its standard output is used to update the worktree file. Similarly, the `clean` command is used to convert the contents of worktree file -upon checkin. +upon checkin. By default these commands process only a single +blob and terminate. If a long running filter process (see section +below) is used then Git can process all blobs with a single filter +invocation for the entire life of a single Git command (e.g. +`git add .`). One use of the content filtering is to massage the content into a shape that is more convenient for the platform, filesystem, and the user to use. @@ -375,6 +379,54 @@ substitution. For example: +Long Running Filter Process +^^^ + +If the filter command (string value) is defined via +filter..process then Git can process all blobs with a +single filter invocation for the entire life of a single Git +command by talking with the following packet format (pkt-line) +based protocol over standard input and standard output. + +Git starts the filter on first usage and expects a welcome +message, protocol version number, and filter capabilities +seperated by spaces: + +packet: git< git-filter-protocol +packet: git< version 2 +packet: git< clean smudge + +Supported filter capabilities are "clean" and "smudge". + +Afterwards Git sends a command (e.g. "smudge" or "clean" - based +on the supported capabilities), the filename, the content size as +ASCII number in bytes, and the content in packet format with a +flush packet at the end: + +packet: git> smudge +packet: git> testfile.dat +packet: git> 7 +packet: git> CONTENT +packet: git> + + +The filter is expected to respond with the result content size as +ASCII number in bytes and the result content in packet format with +a flush packet at the end: + +packet: git< 57 +packet: git< SMUDGED_CONTENT +packet: git< + +Please note: In a future version of Git the capability "stream" +might be supported. In that case the content size must not be +part of the filter response. + +Afterwards the filter is expected to
[PATCH v2 3/5] pkt-line: extract and use `set_packet_header` function
From: Lars Schneider`set_packet_header` converts an integer to a 4 byte hex string. Make this function publicly available so that other parts of Git can easily generate a pkt-line. Signed-off-by: Lars Schneider --- pkt-line.c | 15 ++- pkt-line.h | 1 + 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/pkt-line.c b/pkt-line.c index 62fdb37..6820224 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -98,9 +98,17 @@ void packet_buf_flush(struct strbuf *buf) } #define hex(a) (hexchar[(a) & 15]) -static void format_packet(struct strbuf *out, const char *fmt, va_list args) +void set_packet_header(char *buf, const int size) { static char hexchar[] = "0123456789abcdef"; + buf[0] = hex(size >> 12); + buf[1] = hex(size >> 8); + buf[2] = hex(size >> 4); + buf[3] = hex(size); +} + +static void format_packet(struct strbuf *out, const char *fmt, va_list args) +{ size_t orig_len, n; orig_len = out->len; @@ -111,10 +119,7 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args) if (n > LARGE_PACKET_MAX) die("protocol error: impossibly long line"); - out->buf[orig_len + 0] = hex(n >> 12); - out->buf[orig_len + 1] = hex(n >> 8); - out->buf[orig_len + 2] = hex(n >> 4); - out->buf[orig_len + 3] = hex(n); + set_packet_header(>buf[orig_len], n); packet_trace(out->buf + orig_len + 4, n - 4, 1); } diff --git a/pkt-line.h b/pkt-line.h index 3cb9d91..925c6d3 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -23,6 +23,7 @@ void packet_flush(int fd); void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3))); void packet_buf_flush(struct strbuf *buf); void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3))); +void set_packet_header(char *buf, const int size); /* * Read a packetized line into the buffer, which must be at least size bytes -- 2.9.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] fix passing a name for config from submodules
Stefan Bellerwrites: >> @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct >> submodule_cache *cache, >> parameter.commit_sha1 = commit_sha1; >> parameter.gitmodules_sha1 = sha1; >> parameter.overwrite = 0; >> - git_config_from_mem(parse_config, "submodule-blob", "", >> + git_config_from_mem(parse_config, "submodule-blob", rev.buf, >> config, config_size, ); > > Ok, this is the actual fix. Do you want to demonstrate its impact by adding > one or two tests that failed before and now work? > (As I was using the submodule config API most of the time with null_sha1 > to indicate we'd be looking at the current .gitmodules file in the worktree, > the actual bug may have not manifested in the users of this API. > But still, it would be nice to see what was broken?) Sounds like a good idea. I'll keep these two queued on 'pu' and see if Heiko (or somebody else) can find time to do that, so that we can replace them with an improved version when it happens. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pack-objects: break out of want_object loop early
Jeff Kingwrites: > I got side-tracked by adding a t/perf test to show off the improvement. > It's rather tricky to get right and takes a long time to run. I _think_ > I have it now, but am waiting for results. :) Well, then I'd stop here and wait for the reroll to requeue. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation
John Keepingwrites: > Thanks. I'm about to send v3 anyway to pull a test forward to address > Jakub's comment. I also used oidclr() for the last two changes below. Will replace with v3. I think v3 is ready to advance to 'next'. Let's see if we get further comments from others for a few days. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 00/41] libify apply and use lib in am, part 2
Christian Couderwrites: > Sorry if this patch series is still long. I can split it into two or > more series if it is prefered. > ... > Christian Couder (41): > apply: make some names more specific > apply: move 'struct apply_state' to apply.h > builtin/apply: make apply_patch() return -1 or -128 instead of > die()ing > builtin/apply: read_patch_file() return -1 instead of die()ing > builtin/apply: make find_header() return -128 instead of die()ing > builtin/apply: make parse_chunk() return a negative integer on error > builtin/apply: make parse_single_patch() return -1 on error > builtin/apply: make parse_whitespace_option() return -1 instead of > die()ing > builtin/apply: make parse_ignorewhitespace_option() return -1 instead > of die()ing > builtin/apply: move init_apply_state() to apply.c > apply: make init_apply_state() return -1 instead of exit()ing > builtin/apply: make check_apply_state() return -1 instead of die()ing > builtin/apply: move check_apply_state() to apply.c > builtin/apply: make apply_all_patches() return 128 or 1 on error > builtin/apply: make parse_traditional_patch() return -1 on error > builtin/apply: make gitdiff_*() return 1 at end of header > builtin/apply: make gitdiff_*() return -1 on error > builtin/apply: change die_on_unsafe_path() to check_unsafe_path() > builtin/apply: make build_fake_ancestor() return -1 on error > builtin/apply: make remove_file() return -1 on error > builtin/apply: make add_conflicted_stages_file() return -1 on error > builtin/apply: make add_index_file() return -1 on error > builtin/apply: make create_file() return -1 on error > builtin/apply: make write_out_one_result() return -1 on error > builtin/apply: make write_out_results() return -1 on error > builtin/apply: make try_create_file() return -1 on error > builtin/apply: make create_one_file() return -1 on error > builtin/apply: rename option parsing functions > apply: rename and move opt constants to apply.h > Move libified code from builtin/apply.c to apply.{c,h} > apply: make some parsing functions static again > environment: add set_index_file() > write_or_die: use warning() instead of fprintf(stderr, ...) > apply: add 'be_silent' variable to 'struct apply_state' > apply: make 'be_silent' incompatible with 'apply_verbosely' > apply: don't print on stdout when be_silent is set > usage: add set_warn_routine() > usage: add get_error_routine() and get_warn_routine() > apply: change error_routine when be_silent is set > builtin/am: use apply api in run_apply() > apply: use error_errno() where possible I finally found time to get back to finish reviewing this. The early part up to and including "apply: make some parsing functions static again" looked good and we could treat them as a continuation of the earlier cc/apply-introduce-state topic, which has been merged to the 'master' already. I got an impression that the patches in the remainder of the series were not as polished as the earlier ones, except for the last one, which should be reordered and be part of the early preparation step if possible. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/8] status: per-file data collection for --porcelain=v2
From: Jeff HostetlerThe output of `git status --porcelain` leaves out many details about the current status that clients might like to have. This can force them to be less efficient as they may need to launch secondary commands (and try to match the logic within git) to accumulate this extra information. For example, a GUI IDE might want the file mode to display the correct icon for a changed item (without having to stat it afterwards). Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- builtin/commit.c | 3 +++ wt-status.c | 63 +++- wt-status.h | 4 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/builtin/commit.c b/builtin/commit.c index c3ae2c3..93ce28c 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -153,6 +153,8 @@ static int opt_parse_porcelain(const struct option *opt, const char *arg, int un *value = STATUS_FORMAT_PORCELAIN; else if (!strcmp(arg, "v1") || !strcmp(arg,"1")) *value = STATUS_FORMAT_PORCELAIN; + else if (!strcmp(arg, "v2") || !strcmp(arg, "2")) + *value = STATUS_FORMAT_PORCELAIN_V2; else die("unsupported porcelain version '%s'", arg); @@ -1104,6 +1106,7 @@ static struct status_deferred_config { static void finalize_deferred_config(struct wt_status *s) { int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN && + status_format != STATUS_FORMAT_PORCELAIN_V2 && !s->null_termination); if (s->null_termination) { diff --git a/wt-status.c b/wt-status.c index a9031e4..15d3349 100644 --- a/wt-status.c +++ b/wt-status.c @@ -434,6 +434,31 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q, if (S_ISGITLINK(p->two->mode)) d->new_submodule_commits = !!oidcmp(>one->oid, >two->oid); + + switch (p->status) { + case DIFF_STATUS_ADDED: + die("BUG: worktree status add???"); + break; + + case DIFF_STATUS_DELETED: + d->mode_index = p->one->mode; + oidcpy(>oid_index, >one->oid); + /* mode_worktree is zero for a delete. */ + break; + + case DIFF_STATUS_MODIFIED: + case DIFF_STATUS_TYPE_CHANGED: + case DIFF_STATUS_UNMERGED: + d->mode_index = p->one->mode; + d->mode_worktree = p->two->mode; + oidcpy(>oid_index, >one->oid); + break; + + case DIFF_STATUS_UNKNOWN: + die("BUG: worktree status unknown???"); + break; + } + } } @@ -479,12 +504,36 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q, if (!d->index_status) d->index_status = p->status; switch (p->status) { + case DIFF_STATUS_ADDED: + /* Leave {mode,oid}_head zero for an add. */ + d->mode_index = p->two->mode; + oidcpy(>oid_index, >two->oid); + break; + case DIFF_STATUS_DELETED: + d->mode_head = p->one->mode; + oidcpy(>oid_head, >one->oid); + /* Leave {mode,oid}_index zero for a delete. */ + break; + case DIFF_STATUS_COPIED: case DIFF_STATUS_RENAMED: d->head_path = xstrdup(p->one->path); + d->score = p->score * 100 / MAX_SCORE; + /* fallthru */ + case DIFF_STATUS_MODIFIED: + case DIFF_STATUS_TYPE_CHANGED: + d->mode_head = p->one->mode; + d->mode_index = p->two->mode; + oidcpy(>oid_head, >one->oid); + oidcpy(>oid_index, >two->oid); break; case DIFF_STATUS_UNMERGED: d->stagemask = unmerged_mask(p->two->path); + /* +* Don't bother setting {mode,oid}_{head,index} since the print +* code will output the stage values directly and not use the +* values in these fields. +*/ break; } } @@ -565,9 +614,18 @@ static void wt_status_collect_changes_initial(struct wt_status *s) if (ce_stage(ce)) { d->index_status =
[PATCH v3 5/8] status: print per-file porcelain v2 status data
From: Jeff HostetlerPrint per-file information in porcelain v2 format. Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- wt-status.c | 283 +++- 1 file changed, 282 insertions(+), 1 deletion(-) diff --git a/wt-status.c b/wt-status.c index 15d3349..46061d4 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1813,6 +1813,287 @@ static void wt_porcelain_print(struct wt_status *s) wt_shortstatus_print(s); } +/* + * Convert various submodule status values into a + * fixed-length string of characters in the buffer provided. + */ +static void wt_porcelain_v2_submodule_state( + struct wt_status_change_data *d, + char sub[5]) +{ + if (S_ISGITLINK(d->mode_head) || + S_ISGITLINK(d->mode_index) || + S_ISGITLINK(d->mode_worktree)) { + sub[0] = 'S'; + sub[1] = d->new_submodule_commits ? 'C' : '.'; + sub[2] = (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED) ? 'M' : '.'; + sub[3] = (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ? 'U' : '.'; + } else { + sub[0] = 'N'; + sub[1] = '.'; + sub[2] = '.'; + sub[3] = '.'; + } + sub[4] = 0; +} + +/* + * Fix-up changed entries before we print them. + */ +static void wt_porcelain_v2_fix_up_changed( + struct string_list_item *it, + struct wt_status *s) +{ + struct wt_status_change_data *d = it->util; + + if (!d->index_status) { + /* +* This entry is unchanged in the index (relative to the head). +* Therefore, the collect_updated_cb was never called for this +* entry (during the head-vs-index scan) and so the head column +* fields were never set. +* +* We must have data for the index column (from the +* index-vs-worktree scan (otherwise, this entry should not be +* in the list of changes)). +* +* Copy index column fields to the head column, so that our +* output looks complete. +*/ + assert(d->mode_head == 0); + d->mode_head = d->mode_index; + oidcpy(>oid_head, >oid_index); + } + + if (!d->worktree_status) { + /* +* This entry is unchanged in the worktree (relative to the index). +* Therefore, the collect_changed_cb was never called for this entry +* (during the index-vs-worktree scan) and so the worktree column +* fields were never set. +* +* We must have data for the index column (from the head-vs-index +* scan). +* +* Copy the index column fields to the worktree column so that +* our output looks complete. +* +* Note that we only have a mode field in the worktree column +* because the scan code tries really hard to not have to compute it. +*/ + assert(d->mode_worktree == 0); + d->mode_worktree = d->mode_index; + } +} + +/* + * Print porcelain v2 info for tracked entries with changes. + */ +static void wt_porcelain_v2_print_changed_entry( + struct string_list_item *it, + struct wt_status *s) +{ + struct wt_status_change_data *d = it->util; + struct strbuf buf_current = STRBUF_INIT; + struct strbuf buf_src = STRBUF_INIT; + const char *path_current = NULL; + const char *path_src = NULL; + char key[3]; + char submodule_token[5]; + char sep_char, eol_char; + + wt_porcelain_v2_fix_up_changed(it, s); + wt_porcelain_v2_submodule_state(d, submodule_token); + + key[0] = d->index_status ? d->index_status : '.'; + key[1] = d->worktree_status ? d->worktree_status : '.'; + key[2] = 0; + + if (s->null_termination) { + /* +* In -z mode, we DO NOT C-Quote pathnames. Current path is ALWAYS first. +* A single NUL character separates them. +*/ + sep_char = '\0'; + eol_char = '\0'; + path_current = it->string; + path_src = d->head_path; + } else { + /* +* Path(s) are C-Quoted if necessary. Current path is ALWAYS first. +* The source path is only present when necessary. +* A single TAB separates them (because paths can contain spaces +* which are not escaped and C-Quoting does escape TAB characters). +*/ + sep_char = '\t'; + eol_char = '\n'; + path_current =
[PATCH v3 7/8] status: update git-status.txt for --porcelain=v2
From: Jeff HostetlerUpdate status manpage to include information about porcelain v2 format. Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- Documentation/git-status.txt | 93 ++-- 1 file changed, 90 insertions(+), 3 deletions(-) diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index 6b1454b..ed3590d 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -185,10 +185,10 @@ If -b is used the short-format status is preceded by a line ## branchname tracking info -Porcelain Format - +Porcelain Format Version 1 +~~ -The porcelain format is similar to the short format, but is guaranteed +Version 1 porcelain format is similar to the short format, but is guaranteed not to change in a backwards-incompatible way between Git versions or based on user configuration. This makes it ideal for parsing by scripts. The description of the short format above also describes the porcelain @@ -210,6 +210,93 @@ field from the first filename). Third, filenames containing special characters are not specially formatted; no quoting or backslash-escaping is performed. +Porcelain Format Version 2 +~~ + +Version 2 format adds more detailed information about the state of +the worktree and changed items. + +If `--branch` is given, a series of header lines are printed with +information about the current branch. + +Line Notes + +# branch.oid | (initial)Current commit +# branch.head | (detached) Current branch +# branch.upstream If set +# branch.ab + - If set and present + + +A series of lines are then displayed for the tracked entries. +Ordinary changed entries have the following format: + +1 + +Renamed or copied entries have the following format: + +2 \t + +Field Meaning + +A 2 character field containing the staged and +unstaged XY values described in the short format, +with unchanged indicated by a "." rather than +a space. + A 4 character field describing the submodule state. +"N..." when the entry is not a submodule. +"S" when the entry is a submodule. + is "C" if the commit changed; otherwise ".". + is "M" if it has tracked changes; otherwise ".". + is "U" if there are untracked changes; otherwise ".". +The 6 character octal file mode in the HEAD. +The octal file mode in the index. +The octal file mode in the worktree. +The SHA1 value in the HEAD. +The SHA1 value in the index. + The rename or copied percentage score. For example "R100" +or "C75". + The current pathname. + The original path. This is only present when the entry +has been renamed or copied. + + +Unmerged entries have the following format; the first character is +a "u" to distinguish from ordinary changed entries. + +u + +Field Meaning + +A 2 character field describing the conflict type +as described in the short format. + A 4 character field describing the submodule state +as described above. +The octal file mode for stage 1. +The octal file mode for stage 2. +The octal file mode for stage 3. +The octal file mode in the worktree. +The SHA1 value for stage 1. +The SHA1 value for stage 2. +The SHA1 value for stage 3. + The current pathname. + + +A series of lines are then displayed for untracked and ignored entries. + + + +Where is "?" for untracked entries and "!" for ignored entries. + +In all 3 line formats, pathnames will be "C Quoted" if they contain +any of the following characters: TAB, LF, double quotes, or backslashes. +These characters will be replaced with \t, \n, \", and \\, respectively, +and the pathname will be enclosed in double quotes. + +When the `-z` option is given, a NUL (zero) byte follows each pathname; +serving as both a separator and line termination. No pathname quoting +or backslash escaping is performed. All fields are output in the same +order. + CONFIGURATION - -- 2.8.0.rc4.17.gac42084.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message
[PATCH v3 8/8] status: tests for --porcelain=v2
From: Jeff HostetlerUnit tests for porcelain v2 status format. Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- t/t7064-wtstatus-pv2.sh | 585 1 file changed, 585 insertions(+) create mode 100755 t/t7064-wtstatus-pv2.sh diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh new file mode 100755 index 000..ff0dd3d --- /dev/null +++ b/t/t7064-wtstatus-pv2.sh @@ -0,0 +1,585 @@ +#!/bin/sh + +test_description='git status --porcelain=v2 + +This test exercises porcelain V2 output for git status.' + + +. ./test-lib.sh + +test_expect_success setup ' + test_tick && + git config --local core.autocrlf false && + echo x >file_x && + echo y >file_y && + echo z >file_z && + mkdir dir1 && + echo a >dir1/file_a && + echo b >dir1/file_b +' + + +## +## Confirm output prior to initial commit. +## + +test_expect_success pre_initial_commit_0 ' + cat >expected <<-EOF && + # branch.oid (initial) + # branch.head master + ? actual + ? dir1/ + ? expected + ? file_x + ? file_y + ? file_z + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=normal >actual && + test_cmp expected actual +' + + +test_expect_success pre_initial_commit_1 ' + git add file_x file_y file_z dir1 && + SHA_A=`git hash-object -t blob -- dir1/file_a` && + SHA_B=`git hash-object -t blob -- dir1/file_b` && + SHA_X=`git hash-object -t blob -- file_x` && + SHA_Y=`git hash-object -t blob -- file_y` && + SHA_Z=`git hash-object -t blob -- file_z` && + SHA_ZERO= && + + cat >expected <<-EOF && + # branch.oid (initial) + # branch.head master + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_A dir1/file_a + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_B dir1/file_b + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_X file_x + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Y file_y + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Z file_z + ? actual + ? expected + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual +' + +## Try -z on the above +test_expect_success pre_initial_commit_2 ' + cat >expected.lf <<-EOF && + # branch.oid (initial) + # branch.head master + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_A dir1/file_a + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_B dir1/file_b + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_X file_x + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Y file_y + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Z file_z + ? actual + ? expected + EOF + perl -pe y/\\012/\\000/ expected && + rm expected.lf && + + git status -z --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual +' + +## +## Create first commit. Confirm commit sha in new track header. +## Then make some changes on top of it. +## + +test_expect_success initial_commit_0 ' + git commit -m initial && + H0=`git rev-parse HEAD` && + cat >expected <<-EOF && + # branch.oid $H0 + # branch.head master + ? actual + ? expected + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual +' + + +test_expect_success initial_commit_1 ' + echo x >>file_x && + SHA_X1=`git hash-object -t blob -- file_x` && + rm file_z && + H0=`git rev-parse HEAD` && + + cat >expected <<-EOF && + # branch.oid $H0 + # branch.head master + 1 .M N... 100644 100644 100644 $SHA_X $SHA_X file_x + 1 .D N... 100644 100644 00 $SHA_Z $SHA_Z file_z + ? actual + ? expected + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual +' + + +test_expect_success initial_commit_2 ' + git add file_x && + git rm file_z && + H0=`git rev-parse HEAD` && + + cat >expected <<-EOF && + # branch.oid $H0 + # branch.head master + 1 M. N... 100644 100644 100644 $SHA_X $SHA_X1 file_x + 1 D. N... 100644 00 00 $SHA_Z $SHA_ZERO file_z + ? actual + ? expected + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual +' + + +test_expect_success initial_commit_3 ' +
[PATCH v3 6/8] status: print branch info with --porcelain=v2 --branch
From: Jeff HostetlerExpand porcelain v2 output to include branch and tracking branch information. This includes the commit SHA, the branch, the upstream branch, and the ahead and behind counts. Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- builtin/commit.c | 5 wt-status.c | 90 wt-status.h | 1 + 3 files changed, 96 insertions(+) diff --git a/builtin/commit.c b/builtin/commit.c index 93ce28c..b1fd2d1 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -510,6 +510,8 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int s->fp = fp; s->nowarn = nowarn; s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0; + if (!s->is_initial) + hashcpy(s->sha1_commit, sha1); s->status_format = status_format; s->ignore_submodule_arg = ignore_submodule_arg; @@ -1378,6 +1380,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) fd = hold_locked_index(_lock, 0); s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0; + if (!s.is_initial) + hashcpy(s.sha1_commit, sha1); + s.ignore_submodule_arg = ignore_submodule_arg; s.status_format = status_format; s.verbose = verbose; diff --git a/wt-status.c b/wt-status.c index 46061d4..592fbd2 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1814,6 +1814,92 @@ static void wt_porcelain_print(struct wt_status *s) } /* + * Print branch information for porcelain v2 output. These lines + * are printed when the '--branch' parameter is given. + * + *# branch.oid + *# branch.head + * [# branch.upstream + * [# branch.ab + -]] + * + * ::= the current commit hash or the the literal + * "(initial)" to indicate an initialized repo + * with no commits. + * + * ::= the current branch name or + * "(detached)" literal when detached head or + * "(unknown)" when something is wrong. + * + * ::= the upstream branch name, when set. + * + *::= integer ahead value, when upstream set + * and the commit is present (not gone). + * + * ::= integer behind value, when upstream set + * and commit is present. + * + * + * The end-of-line is defined by the -z flag. + * + * ::= NUL when -z, + * LF when NOT -z. + * + */ +static void wt_porcelain_v2_print_tracking(struct wt_status *s) +{ + struct branch *branch; + const char *base; + const char *branch_name; + struct wt_status_state state; + int ab_info, nr_ahead, nr_behind; + char eol = s->null_termination ? '\0' : '\n'; + + memset(, 0, sizeof(state)); + wt_status_get_state(, s->branch && !strcmp(s->branch, "HEAD")); + + fprintf(s->fp, "# branch.oid %s%c", + (s->is_initial ? "(initial)" : sha1_to_hex(s->sha1_commit)), + eol); + + if (!s->branch) + fprintf(s->fp, "# branch.head %s%c", "(unknown)", eol); + else { + if (!strcmp(s->branch, "HEAD")) { + fprintf(s->fp, "# branch.head %s%c", "(detached)", eol); + + if (state.rebase_in_progress || state.rebase_interactive_in_progress) + branch_name = state.onto; + else if (state.detached_from) + branch_name = state.detached_from; + else + branch_name = ""; + } else { + branch_name = NULL; + skip_prefix(s->branch, "refs/heads/", _name); + + fprintf(s->fp, "# branch.head %s%c", branch_name, eol); + } + + /* Lookup stats on the upstream tracking branch, if set. */ + branch = branch_get(branch_name); + base = NULL; + ab_info = (stat_tracking_info(branch, _ahead, _behind, ) == 0); + if (base) { + base = shorten_unambiguous_ref(base, 0); + fprintf(s->fp, "# branch.upstream %s%c", base, eol); + free((char *)base); + + if (ab_info) + fprintf(s->fp, "# branch.ab +%d -%d%c", nr_ahead, nr_behind, eol); + } + } + + free(state.branch); + free(state.onto); + free(state.detached_from); +} + +/* * Convert various submodule status values into a * fixed-length string of characters in the buffer provided. */ @@ -2057,6 +2143,7 @@ static void wt_porcelain_v2_print_other( /* * Print porcelain V2 status. * + * [] * []* * []* * []* @@ -2069,6 +2156,9 @@ void
[PATCH v3 1/8] status: rename long-format print routines
From: Jeff HostetlerRenamed the various wt_status_print*() routines to be wt_longstatus_print*() to make it clear that these routines are only concerned with the normal/long status output. This will hopefully reduce confusion as other status formats are added in the future. Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- builtin/commit.c | 4 +- wt-status.c | 110 +++ wt-status.h | 2 +- 3 files changed, 58 insertions(+), 58 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 1f6dbcd..b80273b 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -515,7 +515,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int break; case STATUS_FORMAT_NONE: case STATUS_FORMAT_LONG: - wt_status_print(s); + wt_longstatus_print(s); break; } @@ -1403,7 +1403,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) case STATUS_FORMAT_LONG: s.verbose = verbose; s.ignore_submodule_arg = ignore_submodule_arg; - wt_status_print(); + wt_longstatus_print(); break; } return 0; diff --git a/wt-status.c b/wt-status.c index de62ab2..b9a58fd 100644 --- a/wt-status.c +++ b/wt-status.c @@ -139,7 +139,7 @@ void wt_status_prepare(struct wt_status *s) s->display_comment_prefix = 0; } -static void wt_status_print_unmerged_header(struct wt_status *s) +static void wt_longstatus_print_unmerged_header(struct wt_status *s) { int i; int del_mod_conflict = 0; @@ -191,7 +191,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s) status_printf_ln(s, c, "%s", ""); } -static void wt_status_print_cached_header(struct wt_status *s) +static void wt_longstatus_print_cached_header(struct wt_status *s) { const char *c = color(WT_STATUS_HEADER, s); @@ -207,9 +207,9 @@ static void wt_status_print_cached_header(struct wt_status *s) status_printf_ln(s, c, "%s", ""); } -static void wt_status_print_dirty_header(struct wt_status *s, -int has_deleted, -int has_dirty_submodules) +static void wt_longstatus_print_dirty_header(struct wt_status *s, +int has_deleted, +int has_dirty_submodules) { const char *c = color(WT_STATUS_HEADER, s); @@ -226,9 +226,9 @@ static void wt_status_print_dirty_header(struct wt_status *s, status_printf_ln(s, c, "%s", ""); } -static void wt_status_print_other_header(struct wt_status *s, -const char *what, -const char *how) +static void wt_longstatus_print_other_header(struct wt_status *s, +const char *what, +const char *how) { const char *c = color(WT_STATUS_HEADER, s); status_printf_ln(s, c, "%s:", what); @@ -238,7 +238,7 @@ static void wt_status_print_other_header(struct wt_status *s, status_printf_ln(s, c, "%s", ""); } -static void wt_status_print_trailer(struct wt_status *s) +static void wt_longstatus_print_trailer(struct wt_status *s) { status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", ""); } @@ -304,8 +304,8 @@ static int maxwidth(const char *(*label)(int), int minval, int maxval) return result; } -static void wt_status_print_unmerged_data(struct wt_status *s, - struct string_list_item *it) +static void wt_longstatus_print_unmerged_data(struct wt_status *s, + struct string_list_item *it) { const char *c = color(WT_STATUS_UNMERGED, s); struct wt_status_change_data *d = it->util; @@ -331,9 +331,9 @@ static void wt_status_print_unmerged_data(struct wt_status *s, strbuf_release(); } -static void wt_status_print_change_data(struct wt_status *s, - int change_type, - struct string_list_item *it) +static void wt_longstatus_print_change_data(struct wt_status *s, + int change_type, + struct string_list_item *it) { struct wt_status_change_data *d = it->util; const char *c = color(change_type, s); @@ -378,7 +378,7 @@ static void wt_status_print_change_data(struct wt_status *s, status = d->worktree_status; break; default: - die("BUG: unhandled change_type %d in wt_status_print_change_data", + die("BUG: unhandled
[PATCH v3 2/8] status: cleanup API to wt_status_print
From: Jeff HostetlerRefactor the API between builtin/commit.c and wt-status.[ch]. Hide details of the various wt_*status_print() routines inside wt-status.c behind a single (new) wt_status_print() routine and eliminate the switch statements from builtin/commit.c This will allow us to more easily add new status formats and isolate that within wt-status.c Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- builtin/commit.c | 51 +-- wt-status.c | 25 ++--- wt-status.h | 16 3 files changed, 43 insertions(+), 49 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index b80273b..a792deb 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -142,14 +142,7 @@ static int show_ignored_in_status, have_option_m; static const char *only_include_assumed; static struct strbuf message = STRBUF_INIT; -static enum status_format { - STATUS_FORMAT_NONE = 0, - STATUS_FORMAT_LONG, - STATUS_FORMAT_SHORT, - STATUS_FORMAT_PORCELAIN, - - STATUS_FORMAT_UNSPECIFIED -} status_format = STATUS_FORMAT_UNSPECIFIED; +static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; static int opt_parse_m(const struct option *opt, const char *arg, int unset) { @@ -500,24 +493,11 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int s->fp = fp; s->nowarn = nowarn; s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0; + s->status_format = status_format; + s->ignore_submodule_arg = ignore_submodule_arg; wt_status_collect(s); - - switch (status_format) { - case STATUS_FORMAT_SHORT: - wt_shortstatus_print(s); - break; - case STATUS_FORMAT_PORCELAIN: - wt_porcelain_print(s); - break; - case STATUS_FORMAT_UNSPECIFIED: - die("BUG: finalize_deferred_config() should have been called"); - break; - case STATUS_FORMAT_NONE: - case STATUS_FORMAT_LONG: - wt_longstatus_print(s); - break; - } + wt_status_print(s); return s->commitable; } @@ -1099,7 +1079,7 @@ static const char *read_commit_message(const char *name) * is not in effect here. */ static struct status_deferred_config { - enum status_format status_format; + enum wt_status_format status_format; int show_branch; } status_deferred_config = { STATUS_FORMAT_UNSPECIFIED, @@ -1381,6 +1361,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0; s.ignore_submodule_arg = ignore_submodule_arg; + s.status_format = status_format; + s.verbose = verbose; + wt_status_collect(); if (0 <= fd) @@ -1389,23 +1372,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) if (s.relative_paths) s.prefix = prefix; - switch (status_format) { - case STATUS_FORMAT_SHORT: - wt_shortstatus_print(); - break; - case STATUS_FORMAT_PORCELAIN: - wt_porcelain_print(); - break; - case STATUS_FORMAT_UNSPECIFIED: - die("BUG: finalize_deferred_config() should have been called"); - break; - case STATUS_FORMAT_NONE: - case STATUS_FORMAT_LONG: - s.verbose = verbose; - s.ignore_submodule_arg = ignore_submodule_arg; - wt_longstatus_print(); - break; - } + wt_status_print(); return 0; } diff --git a/wt-status.c b/wt-status.c index b9a58fd..a9031e4 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1447,7 +1447,7 @@ static void wt_longstatus_print_state(struct wt_status *s, show_bisect_in_progress(s, state, state_color); } -void wt_longstatus_print(struct wt_status *s) +static void wt_longstatus_print(struct wt_status *s) { const char *branch_color = color(WT_STATUS_ONBRANCH, s); const char *branch_status_color = color(WT_STATUS_HEADER, s); @@ -1714,7 +1714,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) fputc(s->null_termination ? '\0' : '\n', s->fp); } -void wt_shortstatus_print(struct wt_status *s) +static void wt_shortstatus_print(struct wt_status *s) { int i; @@ -1746,7 +1746,7 @@ void wt_shortstatus_print(struct wt_status *s) } } -void wt_porcelain_print(struct wt_status *s) +static void wt_porcelain_print(struct wt_status *s) { s->use_color = 0; s->relative_paths = 0; @@ -1754,3 +1754,22 @@ void wt_porcelain_print(struct wt_status *s) s->no_gettext = 1; wt_shortstatus_print(s); } + +void wt_status_print(struct wt_status *s) +{ + switch
[PATCH v3 0/8] status: V2 porcelain status
This patch series adds porcelain V2 format to status. This provides detailed information about file changes and about the current branch. The new output is accessed via: git status --porcelain=v2 [--branch] Relative to the v2 patch series, in this v3 patch series I have changed the format of the lines for ordinary changes to make it easier to distinguish renames and copies from normal entries. I also split the --branch header information onto separate lines, since we're not bound by any requirement to have a single ## header line. I document the argument as --porcelain=v2, but silently also allow --porcelain=2. Jeff Hostetler (8): status: rename long-format print routines status: cleanup API to wt_status_print status: support --porcelain[=] status: per-file data collection for --porcelain=v2 status: print per-file porcelain v2 status data status: print branch info with --porcelain=v2 --branch status: update git-status.txt for --porcelain=v2 status: tests for --porcelain=v2 Documentation/git-status.txt | 100 +++- builtin/commit.c | 78 +++--- t/t7060-wtstatus.sh | 21 ++ t/t7064-wtstatus-pv2.sh | 585 +++ wt-status.c | 567 - wt-status.h | 19 +- 6 files changed, 1260 insertions(+), 110 deletions(-) create mode 100755 t/t7064-wtstatus-pv2.sh -- 2.8.0.rc4.17.gac42084.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/8] status: support --porcelain[=]
From: Jeff HostetlerUpdate --porcelain argument to take optional version parameter to allow multiple porcelain formats to be supported in the future. The token "v1" is the default value and indicates the traditional porcelain format. (The token "1" is an alias for that.) Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- Documentation/git-status.txt | 7 +-- builtin/commit.c | 21 ++--- t/t7060-wtstatus.sh | 21 + 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index e1e8f57..6b1454b 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -32,11 +32,14 @@ OPTIONS --branch:: Show the branch and tracking info even in short-format. ---porcelain:: +--porcelain[=]:: Give the output in an easy-to-parse format for scripts. This is similar to the short output, but will remain stable across Git versions and regardless of user configuration. See below for details. ++ +The version parameter is used to specify the format version. +This is optional and defaults to the original version 'v1' format. --long:: Give the output in the long-format. This is the default. @@ -96,7 +99,7 @@ configuration variable documented in linkgit:git-config[1]. -z:: Terminate entries with NUL, instead of LF. This implies - the `--porcelain` output format if no other format is given. + the `--porcelain=v1` output format if no other format is given. --column[=]:: --no-column:: diff --git a/builtin/commit.c b/builtin/commit.c index a792deb..c3ae2c3 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -144,6 +144,21 @@ static struct strbuf message = STRBUF_INIT; static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; +static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset) +{ + enum wt_status_format *value = (enum wt_status_format *)opt->value; + if (unset) + *value = STATUS_FORMAT_NONE; + else if (!arg) + *value = STATUS_FORMAT_PORCELAIN; + else if (!strcmp(arg, "v1") || !strcmp(arg,"1")) + *value = STATUS_FORMAT_PORCELAIN; + else + die("unsupported porcelain version '%s'", arg); + + return 0; +} + static int opt_parse_m(const struct option *opt, const char *arg, int unset) { struct strbuf *buf = opt->value; @@ -1316,9 +1331,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) N_("show status concisely"), STATUS_FORMAT_SHORT), OPT_BOOL('b', "branch", _branch, N_("show branch information")), - OPT_SET_INT(0, "porcelain", _format, - N_("machine-readable output"), - STATUS_FORMAT_PORCELAIN), + { OPTION_CALLBACK, 0, "porcelain", _format, + N_("version"), N_("machine-readable output"), + PARSE_OPT_OPTARG, opt_parse_porcelain }, OPT_SET_INT(0, "long", _format, N_("show status in long format (default)"), STATUS_FORMAT_LONG), diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh index 44bf1d8..00e0ceb 100755 --- a/t/t7060-wtstatus.sh +++ b/t/t7060-wtstatus.sh @@ -228,4 +228,25 @@ test_expect_success 'status --branch with detached HEAD' ' test_i18ncmp expected actual ' +## Duplicate the above test and verify --porcelain=v1 arg parsing. +test_expect_success 'status --porcelain=v1 --branch with detached HEAD' ' + git reset --hard && + git checkout master^0 && + git status --branch --porcelain=v1 >actual && + cat >expected <<-EOF && + ## HEAD (no branch) + ?? .gitconfig + ?? actual + ?? expect + ?? expected + ?? mdconflict/ + EOF + test_i18ncmp expected actual +' + +## Verify parser error on invalid --porcelain argument. +test_expect_success 'status --porcelain=bogus' ' + test_must_fail git status --porcelain=bogus +' + test_done -- 2.8.0.rc4.17.gac42084.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: Run commit-msg hook
Johannes Schindelinwrites: > FWIW I dug out the original submission: > http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435 > > It seems that there was no discussion about the commit-msg. Which makes me > wonder why nobody thought of this. I actually think it was a mistaken argument. We could have devised a mechanism to prevent "git commit" that concludes a conflicted "git merge" from calling the hook to resolve the inconsistency, and the solution would have been equally valid. I'd rather not make things worse by repeating that same mistake. If we were to change anything, we should be adding prepare-merge-msg as you suggested earlier and weaning people off of the current behaviour that was added by mistake. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: Run commit-msg hook
Orgad Shanehwrites: >> Hmm. This is not very convincing to me, as >> >> - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`? > > prepare-commit-msg is already called, a few lines above this addition. I do not see such call in contrib/example/git-merge.sh; could it be a recent bug that it calls it? >> - a merge is a different beast from a simple commit. That is why we have >> two different commands for them. A hook to edit the merge message may >> need to know the *second* parent commit, too, for example to generate >> a diffstat, or to add information about changes in an "evil commit". > > That is correct for a post-merge hook. Why should *message validation* differ > between simple and merge commit? Have you seen a typical merge commit message? They certainly look different to me and different rules would apply. >> - if Gerrit is the intended user, would it not make more sense to >> introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you >> have to teach Gerrit a new trick anyway? > > Why is that new? Every commit in gerrit has a Change-Id footer, which is > generated by commit-msg hook. Hmm, I didn't know Gerrit gave Change-ID to merges. In any case, I would think this is completely new. Without this change, commit-msg was not called for merges, so Gerrit wouldn't have added Change-ID to merges with that mechanism. A new merge-msg hook would make more sense, if we were making any change. I do not want to get yelled at by those who wrote their commit-msg hooks long time ago and have happily been using them that updated Git started calling them for their merges where their validation logic for their single-parent commit do not apply at all. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] subtree: adjust style to match CodingGuidelines
Johannes Sixtwrites: > These caught my eye browsing through my inbox. I'm not a subtree user. All good comments. Let's queue 1/3 and 2/3 and fast-track them down to 'master'. Style fixes can come independently later. Thanks. > Am 26.07.2016 um 06:14 schrieb David Aguilar: >> @@ -50,87 +51,145 @@ prefix= >> >> debug() >> { >> -if [ -n "$debug" ]; then >> -printf "%s\n" "$*" >&2 >> +if test -n "$debug" >> +then >> +printf "%s\n" "$@" >&2 > > Are you sure you want this? It prints each argument of the 'debug' > invocation on its own line. > >> fi >> } >> >> say() >> { >> -if [ -z "$quiet" ]; then >> -printf "%s\n" "$*" >&2 >> +if test -z "$quiet" >> +then >> +printf "%s\n" "$@" >&2 > > Same here. > >> fi >> } >> >> progress() >> { >> -if [ -z "$quiet" ]; then >> -printf "%s\r" "$*" >&2 >> +if test -z "$quiet" >> +then >> +printf "%s\r" "$@" >&2 > > But here I'm pretty sure that this is not wanted; the original is > clearly correct. > >> fi >> } > ... >> @@ -139,22 +198,27 @@ debug "command: {$command}" >> debug "quiet: {$quiet}" >> debug "revs: {$revs}" >> debug "dir: {$dir}" >> -debug "opts: {$*}" >> +debug "opts: {$@}" > > When the arguments of a script or function are to be printed for the > user's entertainment/education, then it is safer (and, therefore, > idiomatic) to use "$*". > >> debug > ... >> cache_get() >> { >> -for oldrev in $*; do >> -if [ -r "$cachedir/$oldrev" ]; then >> +for oldrev in "$@" >> +do > > It is idiomatic to write this as > > for oldrev > do > > (But your move from bare $* to quoted "$@" fits better under the "fix > quoting" topic of this patch.) > >> +if test -r "$cachedir/$oldrev" >> +then >> read newrev <"$cachedir/$oldrev" >> echo $newrev >> fi > ... >> @@ -631,17 +749,19 @@ cmd_split() >> debug " parents: $parents" >> newparents=$(cache_get $parents) >> debug " newparents: $newparents" >> - >> + >> tree=$(subtree_for_commit $rev "$dir") >> debug " tree is: $tree" >> >> check_parents $parents >> - >> + >> # ugly. is there no better way to tell if this is a subtree >> # vs. a mainline commit? Does it matter? >> -if [ -z $tree ]; then >> +if test -z $tree > > This works by accident. When $tree is empty, this reduces to 'test > -z', which happens to evaluate to true, just what we want. But it be > appropriate to put $tree in double-quotes nevertheless. > >> +then >> set_notree $rev >> -if [ -n "$newparents" ]; then >> +if test -n "$newparents" >> +then >> cache_set $rev $rev >> fi >> continue > > -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]submodule deinit: remove outdated comment
Stefan Bellerwrites: > Signed-off-by: Stefan Beller > --- > > This is logically part of origin/sb/submodule-deinit-all, but this change > failed to be there on time. Thanks; let's queue it there and fast-track it down to 'master'. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pack-objects: break out of want_object loop early
On Tue, Jul 26, 2016 at 01:38:47PM -0700, Junio C Hamano wrote: > Jeff Kingwrites: > > >> I do not mind too much about having to check two bools twice. But > >> given that the reason why I was confused was because I didn't see > >> why we need to pass the two "return 0" conditions at least once > >> before we decide that we do not need the "return 0" thing at all, > >> and started constructing a case where this might break by writing > >> "Suppose you have two packs, one remote and one local in packed_git > >> list in this order, and ..." before I realized that the new "early > >> break" can be hoisted up like the above, I definitely feel that "we > >> found one, and we aren't conditionally pretending that this thing > >> does not need to be packed at all, so return early and say we want > >> to pack it" is easier to understand before the two existing "if" > >> statements. > > > > Ah, right. Now you had me second-guessing for a moment that there might > > be a bad case in hoisting it up where we would want to return 0 but > > would break out early to the "return 1". > > > > But it cannot be the case, because the break is mutually exclusive with > > the two conditions. > > Here is what I amended looks like (with s/local/non-local/ in the > log message). Thanks, I was actually just preparing a very similar patch (to move the condition and to add a comment, since clearly it is tricky). I got side-tracked by adding a t/perf test to show off the improvement. It's rather tricky to get right and takes a long time to run. I _think_ I have it now, but am waiting for results. :) > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index a2f8cfd..a46bf5b 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -977,6 +977,21 @@ static int want_object_in_pack(const unsigned char *sha1, > return 1; > if (incremental) > return 0; > + > + /* > + * When asked to do --local (do not include an > + * object that appears in a pack we borrow > + * from elsewhere) or --honor-pack-keep (do not > + * include an object that appears in a pack marked > + * with .keep), we need to make sure no copy of this > + * object come from in _any_ pack that causes us to > + * omit it, and need to complete this loop. When > + * neither option is in effect, we know the object > + * we just found is going to be packed, so break > + * out of the loop to return 1 now. > + */ > + if (!ignore_packed_keep && !local) > + break; This looks great. Given the explanation in the comment, it might be more clear to switch the break to "return 1", but I could go either way. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/3] push: allow pushing new branches with --force-with-lease
Changes in v3: - Use hashclr() and oidclr() where appropriate instead of memset() - Pull a test forward from patch 3 to patch 2 John Keeping (3): Documentation/git-push: fix placeholder formatting push: add shorthand for --force-with-lease branch creation push: allow pushing new branches with --force-with-lease Documentation/git-push.txt | 5 +++-- remote.c | 9 + remote.h | 1 - t/t5533-push-cas.sh| 38 ++ 4 files changed, 46 insertions(+), 7 deletions(-) -- 2.9.2.639.g855ae9f -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/3] Documentation/git-push: fix placeholder formatting
Format the placeholder as monospace to match other occurrences in this file and obey CodingGuidelines. Signed-off-by: John Keeping--- No changes in v3. Documentation/git-push.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 93c3527..bf7c9a2 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -198,7 +198,7 @@ branch we have for it. + `--force-with-lease=:` will protect the named ref (alone), if it is going to be updated, by requiring its current value to be -the same as the specified value (which is allowed to be +the same as the specified value `` (which is allowed to be different from the remote-tracking branch we have for the refname, or we do not even have to have such a remote-tracking branch when this form is used). -- 2.9.2.639.g855ae9f -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/3] push: allow pushing new branches with --force-with-lease
If there is no upstream information for a branch, it is likely that it is newly created and can safely be pushed under the normal fast-forward rules. Relax the --force-with-lease check so that we do not reject these branches immediately but rather attempt to push them as new branches, using the null SHA-1 as the expected value. In fact, it is already possible to push new branches using the explicit --force-with-lease=: syntax, so all we do here is make this behaviour the default if no explicit "expect" value is specified. Signed-off-by: John Keeping--- Changes in v3: - use oidclr() - final test is now added in the previous patch and now uses the explicit --force-with-lease syntax remote.c| 7 +++ remote.h| 1 - t/t5533-push-cas.sh | 12 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/remote.c b/remote.c index 42c4a34..d29850a 100644 --- a/remote.c +++ b/remote.c @@ -1544,8 +1544,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, * branch. */ if (ref->expect_old_sha1) { - if (ref->expect_old_no_trackback || - oidcmp(>old_oid, >old_oid_expect)) + if (oidcmp(>old_oid, >old_oid_expect)) reject_reason = REF_STATUS_REJECT_STALE; else /* If the ref isn't stale then force the update. */ @@ -2345,7 +2344,7 @@ static void apply_cas(struct push_cas_option *cas, if (!entry->use_tracking) hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect); else if (remote_tracking(remote, ref->name, >old_oid_expect)) - ref->expect_old_no_trackback = 1; + oidclr(>old_oid_expect); return; } @@ -2355,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas, ref->expect_old_sha1 = 1; if (remote_tracking(remote, ref->name, >old_oid_expect)) - ref->expect_old_no_trackback = 1; + oidclr(>old_oid_expect); } void apply_push_cas(struct push_cas_option *cas, diff --git a/remote.h b/remote.h index c21fd37..9248811 100644 --- a/remote.h +++ b/remote.h @@ -89,7 +89,6 @@ struct ref { force:1, forced_update:1, expect_old_sha1:1, - expect_old_no_trackback:1, deletion:1, matched:1; diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh index ed631c3..09899af 100755 --- a/t/t5533-push-cas.sh +++ b/t/t5533-push-cas.sh @@ -191,6 +191,18 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' ' test_cmp expect actual ' +test_expect_success 'new branch covered by force-with-lease' ' + setup_srcdst_basic && + ( + cd dst && + git branch branch master && + git push --force-with-lease=branch origin branch + ) && + git ls-remote dst refs/heads/branch >expect && + git ls-remote src refs/heads/branch >actual && + test_cmp expect actual +' + test_expect_success 'new branch covered by force-with-lease (explicit)' ' setup_srcdst_basic && ( -- 2.9.2.639.g855ae9f -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] push: add shorthand for --force-with-lease branch creation
Allow the empty string to stand in for the null SHA-1 when pushing a new branch, like we do when deleting branches. This means that the following command ensures that `new-branch` is created on the remote (that is, is must not already exist): git push --force-with-lease=new-branch: origin new-branch Signed-off-by: John Keeping--- Changes in v3: - use hashclr() - pull 'new branch already exists' test forward from patch 3 and use explicit --force-with-lease syntax Documentation/git-push.txt | 3 ++- remote.c | 2 ++ t/t5533-push-cas.sh| 26 ++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index bf7c9a2..927a034 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current value to be the same as the specified value `` (which is allowed to be different from the remote-tracking branch we have for the refname, or we do not even have to have such a remote-tracking branch when -this form is used). +this form is used). If `` is the empty string, then the named ref +must not already exist. + Note that all forms other than `--force-with-lease=:` that specifies the expected current value of the ref explicitly are diff --git a/remote.c b/remote.c index a326e4e..42c4a34 100644 --- a/remote.c +++ b/remote.c @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse entry = add_cas_entry(cas, arg, colon - arg); if (!*colon) entry->use_tracking = 1; + else if (!colon[1]) + hashclr(entry->expect); else if (get_sha1(colon + 1, entry->expect)) return error("cannot parse expected object name '%s'", colon + 1); return 0; diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh index c732012..ed631c3 100755 --- a/t/t5533-push-cas.sh +++ b/t/t5533-push-cas.sh @@ -191,4 +191,30 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' ' test_cmp expect actual ' +test_expect_success 'new branch covered by force-with-lease (explicit)' ' + setup_srcdst_basic && + ( + cd dst && + git branch branch master && + git push --force-with-lease=branch: origin branch + ) && + git ls-remote dst refs/heads/branch >expect && + git ls-remote src refs/heads/branch >actual && + test_cmp expect actual +' + +test_expect_success 'new branch already exists' ' + setup_srcdst_basic && + ( + cd src && + git checkout -b branch master && + test_commit c + ) && + ( + cd dst && + git branch branch master && + test_must_fail git push --force-with-lease=branch: origin branch + ) +' + test_done -- 2.9.2.639.g855ae9f -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] commit: Fix description of no-verify
org...@gmail.com writes: > Subject: Re: [PATCH v2] commit: Fix description of no-verify Following the prevailing style from "git shortlog --no-merges -100" would make it "commit: fix description of no-verify", but "fix" is too generic a word and does not convey as much information as it wastes bits ;-) Subject: commit: --no-verify option skips commit-msg hook, too perhaps? > From: Orgad Shaneh> > include also commit-msg hook. Then this half-sentence can go completely. Thanks; I'll do the above myself while queuing, so there is no need to resend. > diff --git a/builtin/commit.c b/builtin/commit.c > index 163dbca..2725712 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1616,7 +1616,7 @@ int cmd_commit(int argc, const char **argv, const char > *prefix) > OPT_BOOL(0, "interactive", , N_("interactively add > files")), > OPT_BOOL('p', "patch", _interactive, N_("interactively > add changes")), > OPT_BOOL('o', "only", , N_("commit only specified files")), > - OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit > hook")), > + OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit > and commit-msg hooks")), > OPT_BOOL(0, "dry-run", _run, N_("show what would be > committed")), > OPT_SET_INT(0, "short", _format, N_("show status > concisely"), > STATUS_FORMAT_SHORT), -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git cherry-pick conflict error message is deceptive when cherry-picking multiple commits
On Tue, Jul 26, 2016 at 4:30 PM, Jeff Kingwrote: > On Tue, Jul 26, 2016 at 01:18:55PM -0700, Stefan Beller wrote: > >> > Would it be possible to expand the hint message to tell users to run >> > 'git cherry-pick --continue' >> >> Instead of expanding I'd go for replacing? >> >> I'd say the user is tempted for 2 choices, >> a) aborting (for various reasons) >> b) fix and continue. > > Yeah, I'd agree with this. > > I think that advice comes from a time when you could only cherry-pick a > single commit. These days you can do several in a single run, and that's > why "git cherry-pick --continue" was invented. > > So I think we would need to make sure that the "cherry-pick --continue" > advice applies in both cases (and that we do not need to give different > advice depending on whether we are in a single or multiple cherry-pick). > > I did some basic tests and it _seems_ to work to use --continue in > either case. Probably due to 093a309 (revert: allow cherry-pick > --continue to commit before resuming, 2011-12-10), but I didn't dig. > > -Peff The 'git status' text for a rebase/am/cherry-pick is fix conflicts and then run "git --continue" use "git --skip" to skip this patch" use "git --abort" to cancel the operation (The --cancel text varies a bit actually, but that's the gist of it.) The rebase/cherry-pick conflict case should really indicate how to mark the conflict as resolved as that's the specific situation the user is in. I don't know if there are guidelines to hint line length, or how many actions should be on one line but if the above text was changed to have this as the "fix" text, possibly over two lines, I think that would do it. fix conflicts with 'git add ' or 'git rm '" and then run "git --continue" Stephen -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation
On Tue, Jul 26, 2016 at 12:59:04PM -0700, Junio C Hamano wrote: > John Keepingwrites: > > >> > @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option > >> > *cas, const char *arg, int unse > >> > entry = add_cas_entry(cas, arg, colon - arg); > >> > if (!*colon) > >> > entry->use_tracking = 1; > >> > +else if (!colon[1]) > >> > +memset(entry->expect, 0, sizeof(entry->expect)); > >> > >> hashclr()? > > > > Yes (and in the following patch as well). I hadn't realised that > > function exists. > > Thanks; I've locally tweaked these two patches; the interdiff looks > like this. Thanks. I'm about to send v3 anyway to pull a test forward to address Jakub's comment. I also used oidclr() for the last two changes below. > remote.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/remote.c b/remote.c > index e8b7bac..7eaf3c8 100644 > --- a/remote.c > +++ b/remote.c > @@ -2304,7 +2304,7 @@ int parse_push_cas_option(struct push_cas_option *cas, > const char *arg, int unse > if (!*colon) > entry->use_tracking = 1; > else if (!colon[1]) > - memset(entry->expect, 0, sizeof(entry->expect)); > + hashclr(entry->expect); > else if (get_sha1(colon + 1, entry->expect)) > return error("cannot parse expected object name '%s'", colon + > 1); > return 0; > @@ -2354,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas, > if (!entry->use_tracking) > hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect); > else if (remote_tracking(remote, ref->name, > >old_oid_expect)) > - memset(>old_oid_expect, 0, > sizeof(ref->old_oid_expect)); > + hashclr(ref->old_oid_expect.hash); > return; > } > > @@ -2364,7 +2364,7 @@ static void apply_cas(struct push_cas_option *cas, > > ref->expect_old_sha1 = 1; > if (remote_tracking(remote, ref->name, >old_oid_expect)) > - memset(>old_oid_expect, 0, sizeof(ref->old_oid_expect)); > + hashclr(ref->old_oid_expect.hash); > } > > void apply_push_cas(struct push_cas_option *cas, -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 2/3] convert: modernize tests
On Tue, Jul 26, 2016 at 8:18 AM, Remi Galan Alfonsowrote: > Considering how close it is to your patch, you might also want to > remove spaces after '<'. > > There is only one occurrence in this file and it's in a line you are > already modifying. Good eyes. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pack-objects: break out of want_object loop early
Jeff Kingwrites: >> I do not mind too much about having to check two bools twice. But >> given that the reason why I was confused was because I didn't see >> why we need to pass the two "return 0" conditions at least once >> before we decide that we do not need the "return 0" thing at all, >> and started constructing a case where this might break by writing >> "Suppose you have two packs, one remote and one local in packed_git >> list in this order, and ..." before I realized that the new "early >> break" can be hoisted up like the above, I definitely feel that "we >> found one, and we aren't conditionally pretending that this thing >> does not need to be packed at all, so return early and say we want >> to pack it" is easier to understand before the two existing "if" >> statements. > > Ah, right. Now you had me second-guessing for a moment that there might > be a bad case in hoisting it up where we would want to return 0 but > would break out early to the "return 1". > > But it cannot be the case, because the break is mutually exclusive with > the two conditions. Here is what I amended looks like (with s/local/non-local/ in the log message). diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index a2f8cfd..a46bf5b 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -977,6 +977,21 @@ static int want_object_in_pack(const unsigned char *sha1, return 1; if (incremental) return 0; + + /* +* When asked to do --local (do not include an +* object that appears in a pack we borrow +* from elsewhere) or --honor-pack-keep (do not +* include an object that appears in a pack marked +* with .keep), we need to make sure no copy of this +* object come from in _any_ pack that causes us to +* omit it, and need to complete this loop. When +* neither option is in effect, we know the object +* we just found is going to be packed, so break +* out of the loop to return 1 now. +*/ + if (!ignore_packed_keep && !local) + break; if (local && !p->pack_local) return 0; if (ignore_packed_keep && p->pack_local && p->pack_keep) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git cherry-pick conflict error message is deceptive when cherry-picking multiple commits
On Tue, Jul 26, 2016 at 01:18:55PM -0700, Stefan Beller wrote: > > Would it be possible to expand the hint message to tell users to run > > 'git cherry-pick --continue' > > Instead of expanding I'd go for replacing? > > I'd say the user is tempted for 2 choices, > a) aborting (for various reasons) > b) fix and continue. Yeah, I'd agree with this. I think that advice comes from a time when you could only cherry-pick a single commit. These days you can do several in a single run, and that's why "git cherry-pick --continue" was invented. So I think we would need to make sure that the "cherry-pick --continue" advice applies in both cases (and that we do not need to give different advice depending on whether we are in a single or multiple cherry-pick). I did some basic tests and it _seems_ to work to use --continue in either case. Probably due to 093a309 (revert: allow cherry-pick --continue to commit before resuming, 2011-12-10), but I didn't dig. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git cherry-pick conflict error message is deceptive when cherry-picking multiple commits
> Would it be possible to expand the hint message to tell users to run > 'git cherry-pick --continue' Instead of expanding I'd go for replacing? I'd say the user is tempted for 2 choices, a) aborting (for various reasons) b) fix and continue. So we'd want to point out the way for those two ways and not give an additional arbitrary way that is not quite (b) but goes that direction? I realize this is what the patch does, but I wanted to point out the difference on expanding vs replacing. I think a 4 line hint is "enough", so I'd object adding more lines, but changing existing lines is great! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5510: skip tests under GETTEXT_POISON build
On Tue, Jul 26, 2016 at 6:53 PM, Junio C Hamanowrote: > [...] Back when 5e9637c6 (i18n: add infrastructure for > translating Git with gettext, 2011-11-18) introduced the former, > test_have_prereq did not support a negated prerequisite, so the > commit added GETTEXT_POISON prerequisite; if we had the modern > test_have_prereq, we would have written > > test_expect_success GETTEXT_POISON '...' > > that appear in t0205 as > > test_expect_success !C_LOCALE_OUTPUT '...' > > I would think. Maybe the names of the test prerequisites should be merged. I can't think of a rea As for the GETTEXT_POISON facility in general, I haven't worked much if at all on the i18n toolchain since I initially wrote the gettext support so I think at this point it's for others to say whether stuff like this is useful. But for what it's worth the v1.7.4.1-65-gbb946bb commit explains better what it's for: This is a debugging aid for people who are working on the i18n part of the system, to make sure that they are not marking plumbing messages that should never be translated with _(). I.e. so the person gettext-izing something can actively spot issues with marking strings for translations right away. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 0/3] Git filter protocol
On Sun, Jul 24, 2016 at 01:24:29PM +0200, Lars Schneider wrote: > What if we would keep the config option "protocol" and make it an "int"? > Undefined or version "1" would describe the existing clean/smudge > protocol via command line and pipe. Version "2" would be the new protocol? FWIW, that is what I expected when I saw the word "protocol". It's possible that we might never need a "v3" protocol specified here, because your v2 protocol should be able to auto-upgrade. That is, if we start a filter and it says "hi, I am speaking protocol 3", then Git knows to speak the requested version from there on (or will barf if it doesn't understand the version). So you'd only need to say "filter.foo.protocol=v3" if there was some protocol change that broke the initial conversation. That does mean it is the filter which sets the maximum protocol level, not git. So a filter which can speak v3 or v2 (to work with older versions of git) does not know which to use. That could be solved by specifying [filter "foo"] smudge = my-filter --version=3 or something. I'm not sure it's worth thinking too hard about what-ifs here. We should do the simplest thing that will work and avoid painting ourselves into a corner for future upgrades. > > * The way the serialized access to these long-running processes > > work in 3/3 would make it harder or impossible to later > > parallelize conversion? I am imagining a far future where we > > would run "git checkout ." using (say) two threads, one > > responsible for active_cache[0..active_nr/2] and the other > > responsible for the remainder. > I hope this future is not too far away :-) > However, I don't think that would be a problem as we could start the > long-running process once for each checkout thread, no? That's reasonable if we have a worker-thread model (which seems likely, as that's what we use elsewhere in git), and if the main cost you want to amortize is just process startup (so you pay the cost once per worker, which is a constant factor and not too bad). It's not a good model if the long-running process wants to amortize other shared costs. For example, persistent https connections. Or even user-interactive authentication steps, where you really would prefer to do them once. The filter can implement its own ad-hoc sharing of resources, but doing that portably is complicated. Of course having an async protocol between git and the filter is also complicated. Perhaps that's something that could wait for a v3 if somebody really wants it. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] convert: add filter..useProtocol option
On Sat, Jul 23, 2016 at 07:27:21AM +, Eric Wong wrote: > Jakub Narębskiwrote: > > W dniu 2016-07-22 o 17:49, larsxschnei...@gmail.com pisze: > > > +use strict; > > > +use warnings; > > > +use autodie; > > > > autodie? > > "set -e" for Perl (man autodie) > > It's been a part of Perl for ages, but I've never used it > myself, either; I suppose it's fine for tests... autodie has been around for a long time, but it only became part of the perl core in v5.10.1 (according to Module::CoreList). I think the code in perl/ requires only 5.8, but whenever we unconditionally use perl without respect to NO_PERL (like in the test scripts), we usually shoot for even antique versions of perl like 5.005. So by those rules, we should avoid "autodie" here, though I wouldn't be surprised if it takes a while for people to complain in practice (most modern systems will have a recent enough perl, but it seems we go through cycles where every few years somebody posts a bunch of patches for ancient versions of IRIX or some other platform, cleaning up all of these sorts of portability problems). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation
John Keepingwrites: >> > @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option >> > *cas, const char *arg, int unse >> >entry = add_cas_entry(cas, arg, colon - arg); >> >if (!*colon) >> >entry->use_tracking = 1; >> > + else if (!colon[1]) >> > + memset(entry->expect, 0, sizeof(entry->expect)); >> >> hashclr()? > > Yes (and in the following patch as well). I hadn't realised that > function exists. Thanks; I've locally tweaked these two patches; the interdiff looks like this. remote.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/remote.c b/remote.c index e8b7bac..7eaf3c8 100644 --- a/remote.c +++ b/remote.c @@ -2304,7 +2304,7 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse if (!*colon) entry->use_tracking = 1; else if (!colon[1]) - memset(entry->expect, 0, sizeof(entry->expect)); + hashclr(entry->expect); else if (get_sha1(colon + 1, entry->expect)) return error("cannot parse expected object name '%s'", colon + 1); return 0; @@ -2354,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas, if (!entry->use_tracking) hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect); else if (remote_tracking(remote, ref->name, >old_oid_expect)) - memset(>old_oid_expect, 0, sizeof(ref->old_oid_expect)); + hashclr(ref->old_oid_expect.hash); return; } @@ -2364,7 +2364,7 @@ static void apply_cas(struct push_cas_option *cas, ref->expect_old_sha1 = 1; if (remote_tracking(remote, ref->name, >old_oid_expect)) - memset(>old_oid_expect, 0, sizeof(ref->old_oid_expect)); + hashclr(ref->old_oid_expect.hash); } void apply_push_cas(struct push_cas_option *cas, -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 40/41] builtin/am: use apply api in run_apply()
Christian Couderwrites: > static int run_apply(const struct am_state *state, const char *index_file) > { > - struct child_process cp = CHILD_PROCESS_INIT; > + struct argv_array apply_paths = ARGV_ARRAY_INIT; > + struct argv_array apply_opts = ARGV_ARRAY_INIT; > + struct apply_state apply_state; > + int res, opts_left; > + char *save_index_file; > + static struct lock_file lock_file; > + > + struct option am_apply_options[] = { > + { OPTION_CALLBACK, 0, "whitespace", _state, N_("action"), > + N_("detect new or modified lines that have whitespace > errors"), > + 0, apply_option_parse_whitespace }, > + { OPTION_CALLBACK, 0, "ignore-space-change", _state, NULL, > + N_("ignore changes in whitespace when finding context"), > + PARSE_OPT_NOARG, apply_option_parse_space_change }, > + { OPTION_CALLBACK, 0, "ignore-whitespace", _state, NULL, > + N_("ignore changes in whitespace when finding context"), > + PARSE_OPT_NOARG, apply_option_parse_space_change }, > + { OPTION_CALLBACK, 0, "directory", _state, N_("root"), > + N_("prepend to all filenames"), > + 0, apply_option_parse_directory }, > + { OPTION_CALLBACK, 0, "exclude", _state, N_("path"), > + N_("don't apply changes matching the given path"), > + 0, apply_option_parse_exclude }, > + { OPTION_CALLBACK, 0, "include", _state, N_("path"), > + N_("apply changes matching the given path"), > + 0, apply_option_parse_include }, > + OPT_INTEGER('C', NULL, _state.p_context, > + N_("ensure at least lines of context > match")), > + { OPTION_CALLBACK, 'p', NULL, _state, N_("num"), > + N_("remove leading slashes from traditional diff > paths"), > + 0, apply_option_parse_p }, > + OPT_BOOL(0, "reject", _state.apply_with_reject, > + N_("leave the rejected hunks in corresponding *.rej > files")), > + OPT_END() > + }; Is this an indication that this step came too prematurely? Can we avoid having to maintain semi-duplicated options array if cmd_apply() is properly refactored before this step? The resulting code is unmaintainable as long as we have this duplication. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/8] status: update git-status.txt for --porcelain=v2
On 07/25/2016 06:43 PM, Jakub Narębski wrote: W dniu 2016-07-25 o 21:25, Jeff Hostetler pisze: +Porcelain Format Version 2 +~~ + +Version 2 format adds more detailed information about the state of +the worktree and the changed items. I think it should be "and changed items", but I am not a native speaker. fixed. +If `--branch` is given, a header line showing branch tracking information +is printed. This line begins with "### branch: ". Fields are separated +by a single space. + +FieldMeaning + + | (initial)Current commit + | (detached)Current branch + Upstream branch, if set ++ Ahead count, if upstream present +-Behind count, if upstream present + + +A series of lines are then displayed for the tracked entries. +Ordinary changed entries have the following format; the first +character is a 'c' to distinguish them from unmerged entries. It would be nice (though not necessary) to have an example, either here or at the end. I'll try to fit that in at the bottom. + +cR [\t] + +Field Meaning + +A 2 character field containing the staged and +unstaged XY values described in the short format, +with unchanged indicated by a "." rather than +a space. + A 4 character field describing the submodule state. +"N..." when the entry is not a submodule. +"S" when the entry is a submodule. + is "C" if the commit changed; otherwise ".". + is "M" if it has tracked changes; otherwise ".". + is "U" if there are untracked changes; otherwise ".". +The 6 character octal file modes for head, index, +and worktree. I think it might be more readable to be explicit: "for HEAD (), index (), and worktree ()." I'll try to clarify that. + The head and index SHA1 values. +R The rename percentage score. I assume this would be C copy detection heuristics percentage score in case of copy detection, and B break percentage score in case of breaking change into addition and deletion of file. Or am I confused? I'm updating to include rename and copied scores. I haven't seen anything in the code about a break percentage in this context. I see it listed with -B in the git-diff-* pages, but not in status. + The current pathname. It is C-Quoted if it contains +special control characters. I assume that "\t" tab character between and is here to be able to not C-Quote sane filenames with internal whitespace, isn't it? I'm using the same quoting routines as the existing porcelain format. So yes, that looks to be the case. It only converts if there are insane characters in pathnames. + The original path. This is only present for staged renames. +It is C-Quoted if necessary. I assume that "C-Quoted if necessary" is the same as "C-Quoted if it contains special control characters"; also: '"' quote character, '\' backlash escape character and "\t" horizontal tab are not control characters per se., but still need C-Quoting. The rules are the same as for the rest of Git, e.g. for `git diff`, isn't it? yes, i'm using the same quoting routine. i'll clarify. thanks. + + +Unmerged entries have the following format; the first character is +a "u" to distinguish from ordinary changed entries. + +u + +Field Meaning + +A 2 character field describing the conflict type +as described in the short format. + A 4 character field describing the submodule state +as described above. + The 6 character octal file modes for the stage 1, +stage 2, stage 3, and worktree. Errr... the pattern has only _3_ character octal modes, . A question: what happens during octopus merge? +For regular entries, these are the head, index, and +worktree modes; the fourth is zero. This is remnant of the previous version of "v2" format, isn't it? yes, sorry. I missed that one. + The stage 1, stage 2, and stage 3 SHA1 values. + The current pathname. It is C-Quoted if necessary. + + +A series of lines are then displayed for untracked and ignored entries. + + + +Where is "?" for untracked entries and "!" for ignored entries. I assume that here also is C-Quoted if necessary. yes. + +When the `-z` option is
Re: [PATCH v8 36/41] apply: don't print on stdout when be_silent is set
Christian Couderwrites: > This variable should prevent anything to be printed on both stderr > and stdout. You have to mention that skipping the entire callchain, not just the "printing" part, is safe. I can see numstat_patch_list() is probably safe as it does not do any computation other than calling printf() and write_name_quoted(), but other two are not immediately obvious that what they compute are only used for their own printing and there is no other side effects left to affect what happens after this function returns. > Signed-off-by: Christian Couder > --- > apply.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/apply.c b/apply.c > index 1435f85..e2acc18 100644 > --- a/apply.c > +++ b/apply.c > @@ -4698,13 +4698,13 @@ static int apply_patch(struct apply_state *state, > goto end; > } > > - if (state->diffstat) > + if (state->diffstat && !state->be_silent) > stat_patch_list(state, list); > > - if (state->numstat) > + if (state->numstat && !state->be_silent) > numstat_patch_list(state, list); > > - if (state->summary) > + if (state->summary && !state->be_silent) > summary_patch_list(list); > > end: -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git cherry-pick conflict error message is deceptive when cherry-picking multiple commits
When I cherry-pick n commits and one of the first (n-1), fails, what I should do is resolve the conflict, 'git add' it, and then run 'git cherry-pick --continue'. However git advises me to error: could not apply d0fb756... Commit message for test commit hint: after resolving the conflicts, mark the corrected paths hint: with 'git add ' or 'git rm ' hint: and commit the result with 'git commit' I suspect this is not just a little bit deceptive but actually a bit destructive, as doing a 'git commit' removes some of the sequencing information. If I do a 'git commit', while there is .git/sequencer information and 'git cherry-pick --continue' will work, there is no indication in 'git status' that a cherry-pick is in progress. It would be extremely easy for somebody to follow cherry-pick's hints by running 'git commit' and think that they were done, not realizing that there were m more commits remaining to be cherry-picked. Would it be possible to expand the hint message to tell users to run 'git cherry-pick --continue' This patch is *not* meant as a serious patch for submission --I'm sure it's all wrong-- it's just a proof of concept and showing some goodwill on my part that I'm trying to help and I'm willing to put in some work if I can be pointed more in the right direction. Regards, Stephen diff --git a/sequencer.c b/sequencer.c index cdfac82..b8fa534 100644 --- a/sequencer.c +++ b/sequencer.c @@ -171,14 +171,21 @@ static void print_advice(int show_hint, struct replay_opts *opts) if (show_hint) { if (opts->no_commit) advise(_("after resolving the conflicts, mark the corrected paths\n" "with 'git add ' or 'git rm '")); - else - advise(_("after resolving the conflicts, mark the corrected paths\n" -"with 'git add ' or 'git rm '\n" -"and commit the result with 'git commit'")); + else if (! file_exists(git_path_seq_dir())) { + advise(_("SCM: after resolving the conflicts, mark the corrected paths\n" + "with 'git add ' or 'git rm '\n" + "and commit the result with 'git commit'")); +} +else +{ +advise(_("SCM: after resolving the conflicts, mark the corrected paths\n" + "with 'git add ' or 'git rm '\n" + "and continue the %s with 'git %s --continue'"), action_name(opts), action_name(opts)); +} } } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 35/41] apply: make 'be_silent' incompatible with 'apply_verbosely'
Christian Couderwrites: > It should be an error to have both be_silent and apply_verbosely set, > so let's check that in check_apply_state(). Doesn't that suggest that we do not want to have a new be_silent field at all? Perhaps we used to have apply_verbosely = resulting in only two verbosity levels, "verbose" and "normal", and you want to have another new one "total silence" or something? If so perhaps it would be more appropriate to rename apply_verbosely to apply_verbosity that is no longer a boolean? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 34/41] apply: add 'be_silent' variable to 'struct apply_state'
Christian Couderwrites: > This variable should prevent anything to be printed on both stderr > and stdout. It is far more important to describe "why" this is needed than what it does, the latter of which can be read from the patch text. And I do not see any "why" here. Is this "when the current caller wanted to silence us, it spawned us in a separate process and redirected our output to /dev/null, but we no longer can do that because we will change the calling convention to allow direct calls into us"? Do we have a precedent to name a switch that we usually call "quiet" or "silent" as "be_{silent,quiet}"? Is there already a "silent" nearby that records what the end-user gave us (e.g. via "--silent" option), a new name may be needed, but if that is the motivation, I'd probably call it something more specific, "apply_silently" or somesuch. > Let's not take care of stdout and apply_verbosely for now though, > as that will be taken care of in following patches. > > Signed-off-by: Christian Couder > --- > apply.c | 43 +-- > apply.h | 1 + > 2 files changed, 30 insertions(+), 14 deletions(-) > > diff --git a/apply.c b/apply.c > index 7bf12a7..802fa79 100644 > --- a/apply.c > +++ b/apply.c > @@ -1617,8 +1617,9 @@ static void record_ws_error(struct apply_state *state, > return; > > err = whitespace_error_string(result); > - fprintf(stderr, "%s:%d: %s.\n%.*s\n", > - state->patch_input_file, linenr, err, len, line); > + if (!state->be_silent) > + fprintf(stderr, "%s:%d: %s.\n%.*s\n", > + state->patch_input_file, linenr, err, len, line); > free(err); > } > > @@ -1813,7 +1814,7 @@ static int parse_single_patch(struct apply_state *state, > return error(_("new file %s depends on old contents"), > patch->new_name); > if (0 < patch->is_delete && newlines) > return error(_("deleted file %s still has contents"), > patch->old_name); > - if (!patch->is_delete && !newlines && context) > + if (!patch->is_delete && !newlines && context && !state->be_silent) > fprintf_ln(stderr, > _("** warning: " >"file %s becomes empty but is not deleted"), > @@ -3038,8 +3039,8 @@ static int apply_one_fragment(struct apply_state *state, >* Warn if it was necessary to reduce the number >* of context lines. >*/ > - if ((leading != frag->leading) || > - (trailing != frag->trailing)) > + if ((leading != frag->leading || > + trailing != frag->trailing) && !state->be_silent) > fprintf_ln(stderr, _("Context reduced to (%ld/%ld)" >" to apply fragment at %d"), > leading, trailing, applied_pos+1); > @@ -3536,7 +3537,8 @@ static int try_threeway(struct apply_state *state, >read_blob_object(, pre_sha1, patch->old_mode)) > return error("repository lacks the necessary blob to fall back > on 3-way merge."); > > - fprintf(stderr, "Falling back to three-way merge...\n"); > + if (!state->be_silent) > + fprintf(stderr, "Falling back to three-way merge...\n"); > > img = strbuf_detach(, ); > prepare_image(_image, img, len, 1); > @@ -3566,7 +3568,9 @@ static int try_threeway(struct apply_state *state, > status = three_way_merge(image, patch->new_name, >pre_sha1, our_sha1, post_sha1); > if (status < 0) { > - fprintf(stderr, "Failed to fall back on three-way merge...\n"); > + if (!state->be_silent) > + fprintf(stderr, > + "Failed to fall back on three-way merge...\n"); > return status; > } > > @@ -3578,9 +3582,15 @@ static int try_threeway(struct apply_state *state, > hashcpy(patch->threeway_stage[0].hash, pre_sha1); > hashcpy(patch->threeway_stage[1].hash, our_sha1); > hashcpy(patch->threeway_stage[2].hash, post_sha1); > - fprintf(stderr, "Applied patch to '%s' with conflicts.\n", > patch->new_name); > + if (!state->be_silent) > + fprintf(stderr, > + "Applied patch to '%s' with conflicts.\n", > + patch->new_name); > } else { > - fprintf(stderr, "Applied patch to '%s' cleanly.\n", > patch->new_name); > + if (!state->be_silent) > + fprintf(stderr, > + "Applied patch to '%s' cleanly.\n", > + patch->new_name); > } > return 0; > } > @@ -4483,7 +4493,8 @@ static int write_out_one_reject(struct apply_state >
Re: [PATCH v8 32/41] environment: add set_index_file()
Christian Couderwrites: > Introduce set_index_file() to be able to temporarily change the index file. > > It should be used like this: > > /* Save current index file */ > old_index_file = get_index_file(); > set_index_file((char *)tmp_index_file); > > /* Do stuff that will use tmp_index_file as the index file */ > ... > > /* When finished reset the index file */ > set_index_file(old_index_file); WHY is glaringly missing. > Signed-off-by: Christian Couder > --- Doesn't calling this have a global effect that is flowned upon when used by a library-ish function? Who is the expected caller in this series that wants to "libify" a part of the system? > cache.h | 1 + > environment.c | 10 ++ > 2 files changed, 11 insertions(+) > > diff --git a/cache.h b/cache.h > index c73becb..8854365 100644 > --- a/cache.h > +++ b/cache.h > @@ -461,6 +461,7 @@ extern int is_inside_work_tree(void); > extern const char *get_git_dir(void); > extern const char *get_git_common_dir(void); > extern char *get_object_directory(void); > +extern void set_index_file(char *index_file); > extern char *get_index_file(void); > extern char *get_graft_file(void); > extern int set_git_dir(const char *path); > diff --git a/environment.c b/environment.c > index ca72464..7a53799 100644 > --- a/environment.c > +++ b/environment.c > @@ -292,6 +292,16 @@ int odb_pack_keep(char *name, size_t namesz, const > unsigned char *sha1) > return open(name, O_RDWR|O_CREAT|O_EXCL, 0600); > } > > +/* > + * Temporarily change the index file. > + * Please save the current index file using get_index_file() before changing > + * the index file. And when finished, reset it to the saved value. > + */ > +void set_index_file(char *index_file) > +{ > + git_index_file = index_file; > +} > + > char *get_index_file(void) > { > if (!git_index_file) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] date: document and test "raw-local" mode
On Sat, Jul 23, 2016 at 12:15:37PM +0200, Jakub Narębski wrote: > > diff --git a/Documentation/rev-list-options.txt > > b/Documentation/rev-list-options.txt > > index 5d1de06..3ec75d4 100644 > > --- a/Documentation/rev-list-options.txt > > +++ b/Documentation/rev-list-options.txt > > @@ -725,8 +725,8 @@ include::pretty-options.txt[] > > `iso-local`), the user's local time zone is used instead. > > + > > `--date=relative` shows dates relative to the current time, > > -e.g. ``2 hours ago''. The `-local` option cannot be used with > > -`--raw` or `--relative`. > > +e.g. ``2 hours ago''. The `-local` option has no effect for > > +`--relative`. > > Do I understand it correctly: --relative is a short form for more > generic --date=relative (which probably should be spelled > --date-format=relative), and that --date=relative-local is the > same as --date=relative, that is *-local suffix does not change > how date is formatted? > > Because I don't think you can say --relative-local ("The `-local` > option has no effect on `--relative`"), can you? All correct. There is no --relative-local because "--relative" is a historical artifact. We could support --foo for every --date=foo, but I don't think there is a reason to do so (and reasons not to, like avoiding cluttering the option space). > > -`--date=raw` shows the date in the internal raw Git format `%s %z` format. > > +`--date=raw` shows the date in the internal raw Git format `%s %z` > > +format. Note that the `-local` option does not affect the > > +seconds-since-epoch value (which is always measured in UTC), but does > > +switch the accompanying timezone value. > > Which is correct, logical, and next to useless, I think. This was discussed in the earlier review. It's basically only useful if you are feeding the output to another script which will format the date and want to change what that script shows. > BTW. one kind of format that Git does not support (I think) is the > varying kind, where the precision changes with the distance from now, > so that we can get most precision in limited width. That's what > `ls --long` does: > > * 'Jun 29 16:47' for dates falling in current year (more precision) > * 'Nov 23 2015' for dates outside (less precision, same width) > > Many other programs switch from relative to absolute time when date > in question is far in the past that relative is not very good. Yes, this was discussed on the list not too long ago. I think it would be useful, but is obviously orthogonal to this series. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] A Change to Commit IDs Too Ridiculous to Consider?
From: "Jon Forrest"On 7/24/2016 11:51 AM, Rodrigo Campos wrote: And what is the problem with that, if you are doing it with instructional purposes? Let's assume that this helps and not confuses later when the commits *do* change. What is the problem you face? A lot of instructional material contains stuff like "Do [xxx] and you'll see [zzz]. If you don't then something went wrong so try to figure out what happened and do it again." Git, as it stands, for good reason doesn't allow this approach. You may want to look at how the test suite handles the need for well defined commit sequences. It's not something I've really studied, but I am aware of the test_tick to increment the time and similar helpers. There is a big learning step that needs to be got over by many beginners who have no concept of a DVCS, nor of multiple master copies (which to most is an oxymoron!), nor why the sha is a good solution and serial numbers are a bad solution!. Being able to do a few "Hello World" commits starting at unix t=0, and then progressing on to see how they differ when it's unix=now time, or they use their own user IDs could be a useful step for those that need it. I don't think a Git beginner, when using a version of Git that somehow works the way I proposed, will be confused. The fact that performing the same steps results in the same commit IDs won't be something that they'll care about or even notice. The material can include a callout mentioning the difference between "real" Git and "learners" Git. I mean, for some examples you can use HEAD, HEAD^, HEAD~4, etc. and that always works, no matter the commit id. This will work in some cases, but should come later in a Git book. But, in many cases using relative commit IDs, rather than absolute, will be less clear (I believe). In which cases do you want/need the commit ids to be equal? Can you be more specific? Sure. Take a look at the 2nd or 3rd chapter of Pro Git Reedited, 2nd Edition (or just Pro Git 2nd Edition - it doesn't matter). You see lots of output showing 'git commit' commands and the commit IDs that result. I suspect you'd see the same in almost any book about Git. Jon -- Philip -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/4] submodule: support running in multiple worktree setup
On Tue, Jul 26, 2016 at 10:20 AM, Duy Nguyenwrote: > On Tue, Jul 26, 2016 at 1:25 AM, Stefan Beller wrote: >> So what is the design philosophy in worktrees? How much independence does >> one working tree have? > > git-worktree started out as an alternative for git-stash: hmm.. i need > to make some changes in another branch, okay let's leave this worktree > (with all its messy stuff) as-is, create another worktree, make those > changes, then delete the worktree and go back here. There's already > another way of doing that without git-stash: you clone the repo, fix > your stuff, push back and delete the new repo. > > I know I have not really answered your questions. But I think it gives > an idea what are the typical use cases for multiple worktrees. How > much independence would need to be decided case-by-case, I think. Thanks! > >> So here is what I did: >> * s/git submodule init/git submodule update --init/ >> * added a test_pause to the last test on the last line >> * Then: >> >> $ find . |grep da5e6058 >> ./addtest/.git/modules/submod/objects/08/da5e6058267d6be703ae058d173ce38ed53066 >> ./addtest/.git/worktrees/super-elsewhere/modules/submod/objects/08/da5e6058267d6be703ae058d173ce38ed53066 >> ./addtest/.git/worktrees/super-elsewhere/modules/submod2/objects/08/da5e6058267d6be703ae058d173ce38ed53066 >> ./.git/objects/08/da5e6058267d6be703ae058d173ce38ed53066 >> >> The last entry is the "upstream" for the addtest clone, so that is fine. >> However inside the ./addtest/ (and its worktrees, which currently are >> embedded in there?) we only want to have one object store for a given >> submodule? > > How to store stuff in .git is the implementation details that the user > does not care about. They do unfortunately. :( Some teams here are trying to migrate from the repo[1] tool to submodules, and they usually have large code bases. (e.g. The Android Open Source Project[2], put into a superproject has a .git dir size of 17G. The 17G are partitioned as follows: .../.git$ du --max-depth=1 -h 44K ./hooks 32K ./refs 36K ./logs 17G ./modules 4.0K ./branches 8.0K ./info 4.7M ./objects 17G . i.e. roughly all in submodules. So our users do care about both what is on disk, as well as what goes over the wire (network traffic). My sudden interest in worktrees came up when I learned the `--reference` flag for submodule operations is broken for our use case, and instead of fixing the `--reference` flag, I think the worktree approach is generally saner (i.e. with the references you may have nasty gc issues IIUC, but in the worktree world gc knows about all the working trees, detached heads and branches.) [1] https://source.android.com/source/developing.html [2] https://android.googlesource.com/ > As long as we keep the behavior the same (they > can still "git submodule init" and stuff in the new worktree), sharing > the same object store makes sense (pros: lower disk consumption, cons: > none). So I think the current workflow for submodules may need some redesign anyway as the submodule commands were designed with a strict "one working tree only" assumption. Submodule URLs are stored in 3 places: A) In the .gitmodules file of the superproject B) In the option submodule..URL in the superproject C) In the remote.origin.URL in the submodule A) is a recommendation from the superproject to make life of downstream easier to find and setup the whole thing. You can ignore that if you want, though generally a caring upstream provides good URLs here. C) is where we actually fetch from (and hope it has all the sha1s that are recorded as gitlinks in the superproejct) B) seems like a hack to enable the workflow as below: Current workflow for handling submodule URLs: 1) Clone the superproject 2) Run git submodule init on desired submodules 3) Inspect .git/config to see if any submodule URL needs adaption 4) Run git submodule update to obtain the submodules from the configured place 5) In case of superproject adapting the URL -> git submodule sync, which overwrites the submodule..URL in the superprojects .git/config as well as configuring the remote."$remote".url in the submodule 6) In case of users desire to change the URL -> No one command to solve it; possible workaround: edit .gitmodules and git submodule sync, or configure the submodule..URL in the superprojects .git/config as well as configuring the remote."$remote".url in the submodule separately. Although just changing the submodules remote works just as well (until you remove and re-clone the submodule) One could imagine another workflow: 1) clone the superproject, which creates empty repositories for the submodules (2) from the prior workflow is gone 3) instead of inspecting .git/config you can directly manipulate the remote.$remote.url configuration in the submodule. 4) Run git submodule update to obtain the submodules from
Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Torsten Bögershausenwrites: > On 07/25/2016 06:53 PM, Junio C Hamano wrote: >> Pranit Bauva writes: >> > >>> +enum terms_defined { > >>> + TERM_BAD = 1, > >>> + TERM_GOOD = 2, > >>> + TERM_NEW = 4, > >>> + TERM_OLD = 8 > >>> +}; > >>> + >> ... > Is there any risk that a more generic term like "TERM_BAD" may collide > with some other definition some day ? > > Would it make sense to call it GIT_BISECT_TERM_BAD, GBS_TERM_BAD, > BIS_TERM_BAD or something more unique ? I am not sure if the scope of these symbols would ever escape outside bisect-helper.c (and builtin/bisect.c eventually when we retire git-bisect.sh), but BISECT_TERM_{GOOD,BAD,OLD,NEW} would not be too bad. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] submodule-config: combine error checking if clauses
On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigtwrote: > So we have less return handling code. > > Signed-off-by: Heiko Voigt Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] fix passing a name for config from submodules
On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigtwrote: Thanks for continuing on the submodule cache! > In commit 959b5455 we implemented the initial version of the submodule Usually we refer to the commit by a triple of "abbrev. sha1 (date, subject). See d201a1ecd (2015-05-21, test_bitmap_walk: free bitmap with bitmap_free) for an example. Or ce41720ca (2015-04-02, blame, log: format usage strings similarly to those in documentation). Apparently we put the subject first and then the date. I always did it the other way round, to there is no strict coding guide line, though it helps a lot to have an understanding for a) how long are we in the "broken" state already as well as b) what was the rationale for introducing it. > @@ -397,8 +397,10 @@ static const struct submodule *config_from(struct > submodule_cache *cache, > return entry->config; > } > > - if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) > + if (!gitmodule_sha1_from_commit(commit_sha1, sha1, )) { > + strbuf_release(); > return NULL; This is a reoccuring pattern below. Maybe it might make sense to just do a s/return.../ goto out/ and at that label we cleanup `rev` and `config` and return a result value? There are currently 6 early returns (not counting the 3 from the last switch), 4 of them return NULL, so that would result in just a "goto out", whereas 2 return an actual value, they would need to assign the result value first before jumping out of the logic. I dunno, just food for though. > @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct > submodule_cache *cache, > parameter.commit_sha1 = commit_sha1; > parameter.gitmodules_sha1 = sha1; > parameter.overwrite = 0; > - git_config_from_mem(parse_config, "submodule-blob", "", > + git_config_from_mem(parse_config, "submodule-blob", rev.buf, > config, config_size, ); Ok, this is the actual fix. Do you want to demonstrate its impact by adding one or two tests that failed before and now work? (As I was using the submodule config API most of the time with null_sha1 to indicate we'd be looking at the current .gitmodules file in the worktree, the actual bug may have not manifested in the users of this API. But still, it would be nice to see what was broken?) Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/4] submodule: support running in multiple worktree setup
On Tue, Jul 26, 2016 at 1:25 AM, Stefan Bellerwrote: > So what is the design philosophy in worktrees? How much independence does > one working tree have? git-worktree started out as an alternative for git-stash: hmm.. i need to make some changes in another branch, okay let's leave this worktree (with all its messy stuff) as-is, create another worktree, make those changes, then delete the worktree and go back here. There's already another way of doing that without git-stash: you clone the repo, fix your stuff, push back and delete the new repo. I know I have not really answered your questions. But I think it gives an idea what are the typical use cases for multiple worktrees. How much independence would need to be decided case-by-case, I think. > So here is what I did: > * s/git submodule init/git submodule update --init/ > * added a test_pause to the last test on the last line > * Then: > > $ find . |grep da5e6058 > ./addtest/.git/modules/submod/objects/08/da5e6058267d6be703ae058d173ce38ed53066 > ./addtest/.git/worktrees/super-elsewhere/modules/submod/objects/08/da5e6058267d6be703ae058d173ce38ed53066 > ./addtest/.git/worktrees/super-elsewhere/modules/submod2/objects/08/da5e6058267d6be703ae058d173ce38ed53066 > ./.git/objects/08/da5e6058267d6be703ae058d173ce38ed53066 > > The last entry is the "upstream" for the addtest clone, so that is fine. > However inside the ./addtest/ (and its worktrees, which currently are > embedded in there?) we only want to have one object store for a given > submodule? How to store stuff in .git is the implementation details that the user does not care about. As long as we keep the behavior the same (they can still "git submodule init" and stuff in the new worktree), sharing the same object store makes sense (pros: lower disk consumption, cons: none). > After playing with this series a bit more, I actually like the UI as it is an > easy mental model "submodules behave completely independent". > > However in 3/4 you said: > > + - `submodule.*` in current state should not be shared because the > + information is tied to a particular version of .gitmodules in a > + working directory. > > This is already a problem with say different branches/versions. > That has been solved by duplicating that information to .git/config > as a required step. (I don't like that approach, as it is super confusing > IMHO) Hmm.. I didn't realize this. But then I have never given much thought about submodules, probably because I have an alternative solution for it (or some of its use cases) anyway :) OK so it's already a problem. But if we keep sharing submodule stuff in .git/config, there's a _new_ problem: when you "submodule init" a worktree, .git/config is now tailored for the current worktree, when you move back to the previous worktree, you need to "submodule init" again. So moving to multiple worktrees setup changes how the user uses submodule, not good in my opinion. If you have a grand plan to make submodule work at switching branches (without reinit) and if it happens to work the same way when we have multiple worktrees, great. > I am back to the drawing board for the submodule side of things, > but I guess this series could be used once we figure out how to > have just one object database for a submodule. I would leave this out for now. Let's make submodule work with multiple worktrees first (and see how the users react to this). Then we can try to share object database. Object database and refs are tied closely together so you may run into other problems soon. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 11/16] am -3: use merge_recursive() directly again
Johannes Schindelinwrites: > If you want, I can break out the subsequent patches into a separate > series. I do not think that would help anybody, as we'll have to review them anyway. If some of the the later ones were "oops, this earlier step did an incomplete job and here is a fix-up", then squashing them into the step where such a mistake happens may reduce the review load, but I suspect that is not the case (iow, they are enhancements and the earlier ones can stand on their own). > I may have missed something as stupid as an unclosed file handle, after > all. Yes, reviewing the same change over and over, especially if the change is your own, tends to have diminishing rate of bug yields, which is why they need to be reviewed by fresh set of eyes. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i18n: notes: mark comment for translation
Vasco Almeidawrites: > A Seg, 25-07-2016 às 10:49 -0700, Junio C Hamano escreveu: >> Vasco Almeida writes: >> >> > >> > static const char note_template[] = >> > - "\nWrite/edit the notes for the following object:\n"; >> > + N_("Write/edit the notes for the following object:"); >> >> I do not quite understand why you want the blank lines surrounding >> the message outside add_commented_lines() call. I think the intent >> is to produce >> >> # >> # Write/edit the notes for the following object: >> # > > Yes, this was my intention. The original does: > > # > # Write/edit the notes for the following object: > Ah, of course, I misspoke. The original "\n\n" requests that an empty line that is commented, followed by a line with the message that is also commented, is given at that point. The last commented blank was my mistake. In any case, I do not understand why you want to exclude the LFs from the message. If a translation for a particular language is very long and would not fit on a single line, the translator is allowed to make the message much longer, i.e. the translated version of the part may contain one or more LFs (which is what the add-commented-lines function was invented for). I'd think having LFs in the to-be-translated-original will serve as a good hint to signal translators that is the case. Thansk. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] i18n: notes: mark comment for translation
Vasco Almeidawrites: > + strbuf_add_commented_lines(, "\n", strlen("\n")); > + strbuf_add_commented_lines(, _(note_template), > strlen(_(note_template))); > + strbuf_add_commented_lines(, "\n", strlen("\n")); Hmm, do we really need to make three separate calls? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5510: skip tests under GETTEXT_POISON build
Vasco Almeidawrites: > Skip tests when running under GETTEXT_POISON build and run them with > C_LOCALE_OUTPUT prerequisite. > > These tests are irrelevant under GETTEXT_POISON because they test text > output alignment which GETTEXT_POISON turns useless. > > Signed-off-by: Vasco Almeida > --- > t/t5510-fetch.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 6bd4853..668c54b 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -688,7 +688,7 @@ test_expect_success 'fetching with auto-gc does not lock > up' ' > ) > ' > > -test_expect_success 'fetch aligned output' ' > +test_expect_success C_LOCALE_OUTPUT 'fetch aligned output' ' > git clone . full-output && > test_commit long-tag && > ( > @@ -703,7 +703,7 @@ test_expect_success 'fetch aligned output' ' > test_cmp expect actual > ' > > -test_expect_success 'fetch compact output' ' > +test_expect_success C_LOCALE_OUTPUT 'fetch compact output' ' > git clone . compact && > test_commit extraaa && > ( Makes sense, will queue. This is a tangent, but it may make sense for us to start thinking about retiring one of the two prerequisites, GETTEXT_POISON and C_LOCALE_OUTPUT. Back when 5e9637c6 (i18n: add infrastructure for translating Git with gettext, 2011-11-18) introduced the former, test_have_prereq did not support a negated prerequisite, so the commit added GETTEXT_POISON prerequisite; if we had the modern test_have_prereq, we would have written test_expect_success GETTEXT_POISON '...' that appear in t0205 as test_expect_success !C_LOCALE_OUTPUT '...' I would think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 16/16] merge-recursive: flush output buffer even when erroring out
Ever since 66a155b (Enable output buffering in merge-recursive., 2007-01-14), we had a problem: When the merge failed in a fatal way, all regular output was swallowed because we called die() and did not get a chance to drain the output buffers. To fix this, several modifications were necessary: - we needed to stop die()ing, to give callers a chance to do something when an error occurred (in this case, flush the output buffers), - we needed to delay printing the error message so that the caller can print the buffered output before that, and - we needed to make sure that the output buffers are flushed even when the return value indicates an error. The first two changes were introduced through earlier commits in this patch series, and this commit addresses the third one. Signed-off-by: Johannes Schindelin--- merge-recursive.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/merge-recursive.c b/merge-recursive.c index a16b150..66e93e0 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2069,6 +2069,7 @@ int merge_recursive(struct merge_options *o, o->ancestor = "merged common ancestors"; clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree, ); + flush_output(o); if (clean < 0) return clean; @@ -2077,7 +2078,6 @@ int merge_recursive(struct merge_options *o, commit_list_insert(h1, &(*result)->parents); commit_list_insert(h2, &(*result)->parents->next); } - flush_output(o); if (o->buffer_output < 2) strbuf_release(>obuf); if (show(o, 2)) -- 2.9.0.281.g286a8d9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 13/16] merge-recursive: write the commit title in one go
In 66a155b (Enable output buffering in merge-recursive., 2007-01-14), we changed the code such that it prints the output in one go, to avoid interfering with the progress output. Let's make sure that the same holds true when outputting the commit title: previously, we used several printf() statements to stdout and speculated that stdout's buffer is large enough to hold the entire commit title. Apart from making that speculation unnecessary, we change the code to add the message to the output buffer before flushing for another reason: the next commit will introduce a new level of output buffering, where the caller can request the output not to be flushed, but to be retained for further processing. This latter feature will be needed when teaching the sequencer to do rebase -i's brunt work: it wants to control the output of the cherry-picks (i.e. recursive merges). Signed-off-by: Johannes Schindelin--- merge-recursive.c | 17 + 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 71a0aa0..723b8d0 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -191,25 +191,26 @@ static void output(struct merge_options *o, int v, const char *fmt, ...) static void output_commit_title(struct merge_options *o, struct commit *commit) { - int i; - flush_output(o); - for (i = o->call_depth; i--;) - fputs(" ", stdout); + strbuf_addchars(>obuf, ' ', o->call_depth * 2); if (commit->util) - printf("virtual %s\n", merge_remote_util(commit)->name); + strbuf_addf(>obuf, "virtual %s\n", + merge_remote_util(commit)->name); else { - printf("%s ", find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV)); + strbuf_addf(>obuf, "%s ", + find_unique_abbrev(commit->object.oid.hash, + DEFAULT_ABBREV)); if (parse_commit(commit) != 0) - printf(_("(bad commit)\n")); + strbuf_addf(>obuf, _("(bad commit)\n")); else { const char *title; const char *msg = get_commit_buffer(commit, NULL); int len = find_commit_subject(msg, ); if (len) - printf("%.*s\n", len, title); + strbuf_addf(>obuf, "%.*s\n", len, title); unuse_commit_buffer(commit, msg); } } + flush_output(o); } static int add_cacheinfo(struct merge_options *o, -- 2.9.0.281.g286a8d9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 11/16] am -3: use merge_recursive() directly again
Last October, we had to change this code to run `git merge-recursive` in a child process: git-am wants to print some helpful advice when the merge failed, but the code in question was not prepared to return, it die()d instead. We are finally at a point when the code *is* prepared to return errors, and can avoid the child process again. This reverts commit c63d4b2 (am -3: do not let failed merge from completing the error codepath, 2015-10-09), with the necessary changes to adjust for the fact that Git's source code changed in the meantime (such as: using OIDs instead of hashes in the recursive merge, and a removed gender bias). Note: the code now calls merge_recursive_generic() again. Unlike merge_trees() and merge_recursive(), this function returns 0 upon success, as most of Git's functions. Therefore, the error value -1 naturally is handled correctly, and we do not have to take care of it specifically. Signed-off-by: Johannes Schindelin--- builtin/am.c | 62 +--- 1 file changed, 22 insertions(+), 40 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index b77bf11..cfb79ea 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1579,47 +1579,18 @@ static int build_fake_ancestor(const struct am_state *state, const char *index_f } /** - * Do the three-way merge using fake ancestor, their tree constructed - * from the fake ancestor and the postimage of the patch, and our - * state. - */ -static int run_fallback_merge_recursive(const struct am_state *state, - unsigned char *orig_tree, - unsigned char *our_tree, - unsigned char *their_tree) -{ - struct child_process cp = CHILD_PROCESS_INIT; - int status; - - cp.git_cmd = 1; - - argv_array_pushf(_array, "GITHEAD_%s=%.*s", -sha1_to_hex(their_tree), linelen(state->msg), state->msg); - if (state->quiet) - argv_array_push(_array, "GIT_MERGE_VERBOSITY=0"); - - argv_array_push(, "merge-recursive"); - argv_array_push(, sha1_to_hex(orig_tree)); - argv_array_push(, "--"); - argv_array_push(, sha1_to_hex(our_tree)); - argv_array_push(, sha1_to_hex(their_tree)); - - status = run_command() ? (-1) : 0; - discard_cache(); - read_cache(); - return status; -} - -/** * Attempt a threeway merge, using index_path as the temporary index. */ static int fall_back_threeway(const struct am_state *state, const char *index_path) { - unsigned char orig_tree[GIT_SHA1_RAWSZ], their_tree[GIT_SHA1_RAWSZ], - our_tree[GIT_SHA1_RAWSZ]; + struct object_id orig_tree, their_tree, our_tree; + const struct object_id *bases[1] = { _tree }; + struct merge_options o; + struct commit *result; + char *their_tree_name; - if (get_sha1("HEAD", our_tree) < 0) - hashcpy(our_tree, EMPTY_TREE_SHA1_BIN); + if (get_oid("HEAD", _tree) < 0) + hashcpy(our_tree.hash, EMPTY_TREE_SHA1_BIN); if (build_fake_ancestor(state, index_path)) return error("could not build fake ancestor"); @@ -1627,7 +1598,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa discard_cache(); read_cache_from(index_path); - if (write_index_as_tree(orig_tree, _index, index_path, 0, NULL)) + if (write_index_as_tree(orig_tree.hash, _index, index_path, 0, NULL)) return error(_("Repository lacks necessary blobs to fall back on 3-way merge.")); say(state, stdout, _("Using index info to reconstruct a base tree...")); @@ -1643,7 +1614,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa init_revisions(_info, NULL); rev_info.diffopt.output_format = DIFF_FORMAT_NAME_STATUS; diff_opt_parse(_info.diffopt, _filter_str, 1, rev_info.prefix); - add_pending_sha1(_info, "HEAD", our_tree, 0); + add_pending_sha1(_info, "HEAD", our_tree.hash, 0); diff_setup_done(_info.diffopt); run_diff_index(_info, 1); } @@ -1652,7 +1623,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa return error(_("Did you hand edit your patch?\n" "It does not apply to blobs recorded in its index.")); - if (write_index_as_tree(their_tree, _index, index_path, 0, NULL)) + if (write_index_as_tree(their_tree.hash, _index, index_path, 0, NULL)) return error("could not write tree"); say(state, stdout, _("Falling back to patching base and 3-way merge...")); @@ -1668,11 +1639,22 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa * changes.
[PATCH v5 10/16] merge-recursive: switch to returning errors instead of dying
The recursive merge machinery is supposed to be a library function, i.e. it should return an error when it fails. Originally the functions were part of the builtin "merge-recursive", though, where it was simpler to call die() and be done with error handling. The existing callers were already prepared to detect negative return values to indicate errors and to behave as previously: exit with code 128 (which is the same thing that die() does, after printing the message). Signed-off-by: Johannes Schindelin--- merge-recursive.c | 62 +++ 1 file changed, 35 insertions(+), 27 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 6beb1e4..bc59815 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -275,8 +275,10 @@ struct tree *write_tree_from_memory(struct merge_options *o) active_cache_tree = cache_tree(); if (!cache_tree_fully_valid(active_cache_tree) && - cache_tree_update(_index, 0) < 0) - die(_("error building trees")); + cache_tree_update(_index, 0) < 0) { + error(_("error building trees")); + return NULL; + } result = lookup_tree(active_cache_tree->sha1); @@ -716,12 +718,10 @@ static int make_room_for_path(struct merge_options *o, const char *path) /* Make sure leading directories are created */ status = safe_create_leading_directories_const(path); if (status) { - if (status == SCLD_EXISTS) { + if (status == SCLD_EXISTS) /* something else exists */ - error(msg, path, _(": perhaps a D/F conflict?")); - return -1; - } - die(msg, path, ""); + return error(msg, path, _(": perhaps a D/F conflict?")); + return error(msg, path, ""); } /* @@ -749,6 +749,8 @@ static int update_file_flags(struct merge_options *o, int update_cache, int update_wd) { + int ret = 0; + if (o->call_depth) update_wd = 0; @@ -769,9 +771,11 @@ static int update_file_flags(struct merge_options *o, buf = read_sha1_file(oid->hash, , ); if (!buf) - die(_("cannot read object %s '%s'"), oid_to_hex(oid), path); - if (type != OBJ_BLOB) - die(_("blob expected for %s '%s'"), oid_to_hex(oid), path); + return error(_("cannot read object %s '%s'"), oid_to_hex(oid), path); + if (type != OBJ_BLOB) { + ret = error(_("blob expected for %s '%s'"), oid_to_hex(oid), path); + goto free_buf; + } if (S_ISREG(mode)) { struct strbuf strbuf = STRBUF_INIT; if (convert_to_working_tree(path, buf, size, )) { @@ -792,8 +796,11 @@ static int update_file_flags(struct merge_options *o, else mode = 0666; fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode); - if (fd < 0) - die_errno(_("failed to open '%s'"), path); + if (fd < 0) { + ret = error_errno(_("failed to open '%s'"), + path); + goto free_buf; + } write_in_full(fd, buf, size); close(fd); } else if (S_ISLNK(mode)) { @@ -801,18 +808,18 @@ static int update_file_flags(struct merge_options *o, safe_create_leading_directories_const(path); unlink(path); if (symlink(lnk, path)) - die_errno(_("failed to symlink '%s'"), path); + ret = error_errno(_("failed to symlink '%s'"), path); free(lnk); } else - die(_("do not know what to do with %06o %s '%s'"), - mode, oid_to_hex(oid), path); + ret = error(_("do not know what to do with %06o %s '%s'"), + mode, oid_to_hex(oid), path); free_buf: free(buf); } update_index: - if (update_cache) + if (!ret && update_cache) add_cacheinfo(mode, oid, path, 0, update_wd, ADD_CACHE_OK_TO_ADD); - return 0; + return ret; } static int update_file(struct merge_options *o, @@ -938,20 +945,22 @@ static int merge_file_1(struct merge_options *o, oidcpy(>oid, >oid); else if (S_ISREG(a->mode)) { mmbuffer_t
[PATCH v5 15/16] Ensure that the output buffer is released after calling merge_trees()
The recursive merge machinery accumulates its output in an output buffer, to be flushed at the end of merge_recursive(). At this point, we forgot to release the output buffer. When calling merge_trees() (i.e. the non-recursive part of the recursive merge) directly, the output buffer is never flushed because the caller may be merge_recursive() which wants to flush the output itself. For the same reason, merge_trees() cannot release the output buffer: it may still be needed. Forgetting to release the output buffer did not matter much when running git-checkout, or git-merge-recursive, because we exited after the operation anyway. Ever since cherry-pick learned to pick a commit range, however, this memory leak had the potential of becoming a problem. Signed-off-by: Johannes Schindelin--- builtin/checkout.c | 1 + merge-recursive.c | 2 ++ sequencer.c| 1 + 3 files changed, 4 insertions(+) diff --git a/builtin/checkout.c b/builtin/checkout.c index 07dea3b..8d852d4 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -573,6 +573,7 @@ static int merge_working_tree(const struct checkout_opts *opts, exit(128); ret = reset_tree(new->commit->tree, opts, 0, writeout_error); + strbuf_release(); if (ret) return ret; } diff --git a/merge-recursive.c b/merge-recursive.c index 311cfa4..a16b150 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2078,6 +2078,8 @@ int merge_recursive(struct merge_options *o, commit_list_insert(h2, &(*result)->parents->next); } flush_output(o); + if (o->buffer_output < 2) + strbuf_release(>obuf); if (show(o, 2)) diff_warn_rename_limit("merge.renamelimit", o->needed_rename_limit, 0); diff --git a/sequencer.c b/sequencer.c index 286a435..ec50519 100644 --- a/sequencer.c +++ b/sequencer.c @@ -293,6 +293,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, clean = merge_trees(, head_tree, next_tree, base_tree, ); + strbuf_release(); if (clean < 0) return clean; -- 2.9.0.281.g286a8d9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 12/16] merge-recursive: flush output buffer before printing error messages
The data structure passed to the recursive merge machinery has a feature where the caller can ask for the output to be buffered into a strbuf, by setting the field 'buffer_output'. Previously, we simply swallowed the buffered output when showing error messages. With this patch, we show the output first, and only then print the error message. Currently, the only user of that buffering is merge_recursive() itself, to avoid the progress output to interfere. In the next patches, we will introduce a new buffer_output mode that forces merge_recursive() to retain the output buffer for further processing by the caller. If the caller asked for that, we will then also write the error messages into the output buffer. This is necessary to give the caller more control not only how to react in case of errors but also control how/if to display the error messages. Signed-off-by: Johannes Schindelin--- merge-recursive.c | 116 -- 1 file changed, 68 insertions(+), 48 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index bc59815..71a0aa0 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -23,6 +23,28 @@ #include "dir.h" #include "submodule.h" +static void flush_output(struct merge_options *o) +{ + if (o->obuf.len) { + fputs(o->obuf.buf, stdout); + strbuf_reset(>obuf); + } +} + +static int err(struct merge_options *o, const char *err, ...) +{ + va_list params; + + va_start(params, err); + flush_output(o); + strbuf_vaddf(>obuf, err, params); + error("%s", o->obuf.buf); + strbuf_reset(>obuf); + va_end(params); + + return -1; +} + static struct tree *shift_tree_object(struct tree *one, struct tree *two, const char *subtree_shift) { @@ -148,14 +170,6 @@ static int show(struct merge_options *o, int v) return (!o->call_depth && o->verbosity >= v) || o->verbosity >= 5; } -static void flush_output(struct merge_options *o) -{ - if (o->obuf.len) { - fputs(o->obuf.buf, stdout); - strbuf_reset(>obuf); - } -} - __attribute__((format (printf, 3, 4))) static void output(struct merge_options *o, int v, const char *fmt, ...) { @@ -198,7 +212,8 @@ static void output_commit_title(struct merge_options *o, struct commit *commit) } } -static int add_cacheinfo(unsigned int mode, const struct object_id *oid, +static int add_cacheinfo(struct merge_options *o, + unsigned int mode, const struct object_id *oid, const char *path, int stage, int refresh, int options) { struct cache_entry *ce; @@ -206,7 +221,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid, ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage, 0); if (!ce) - return error(_("addinfo_cache failed for path '%s'"), path); + return err(o, _("addinfo_cache failed for path '%s'"), path); ret = add_cache_entry(ce, options); if (refresh) { @@ -276,7 +291,7 @@ struct tree *write_tree_from_memory(struct merge_options *o) if (!cache_tree_fully_valid(active_cache_tree) && cache_tree_update(_index, 0) < 0) { - error(_("error building trees")); + err(o, _("error building trees")); return NULL; } @@ -544,7 +559,8 @@ static struct string_list *get_renames(struct merge_options *o, return renames; } -static int update_stages(const char *path, const struct diff_filespec *o, +static int update_stages(struct merge_options *opt, const char *path, +const struct diff_filespec *o, const struct diff_filespec *a, const struct diff_filespec *b) { @@ -563,13 +579,13 @@ static int update_stages(const char *path, const struct diff_filespec *o, if (remove_file_from_cache(path)) return -1; if (o) - if (add_cacheinfo(o->mode, >oid, path, 1, 0, options)) + if (add_cacheinfo(opt, o->mode, >oid, path, 1, 0, options)) return -1; if (a) - if (add_cacheinfo(a->mode, >oid, path, 2, 0, options)) + if (add_cacheinfo(opt, a->mode, >oid, path, 2, 0, options)) return -1; if (b) - if (add_cacheinfo(b->mode, >oid, path, 3, 0, options)) + if (add_cacheinfo(opt, b->mode, >oid, path, 3, 0, options)) return -1; return 0; } @@ -720,8 +736,8 @@ static int make_room_for_path(struct merge_options *o, const char *path) if (status) { if (status == SCLD_EXISTS) /* something else exists */ - return error(msg, path, _(": perhaps a D/F
[PATCH v5 00/16] Use merge_recursive() directly in the builtin am
This is the fifth iteration of the long-awaited re-roll of the attempt to avoid spawning merge-recursive from the builtin am and use merge_recursive() directly instead. The *real* reason for the reroll is that I need a libified recursive merge to accelerate the interactive rebase by teaching the sequencer to do rebase -i's grunt work. Coming with a very nice 3x-5x speedup of `rebase -i`. In this endeavor, we need to be extra careful to retain backwards compatibility. The test script t6022-merge-rename.sh, for example, verifies that `git pull` exits with status 128 in case of a fatal error. To that end, we need to make sure that fatal errors are handled by existing (builtin) users via exit(128) (or die(), which calls exit(128) at the end). New users (such as a builtin helper doing rebase -i's grunt work) may want to print some helpful advice what happened and how to get out of this mess before erroring out. The changes relative to the fourth iteration of this patch series: - the first patch which introduces a tests case to t5520 was prettified: - it uses test_seq now, - it avoids `printf` when `echo` does the job, too, - it adds a missing empty line between test cases, and - it clarifies why we need to check out with force. - the change that would have made the bug report about a too-small buffer in imap-send was reverted, because it did more than was claimed by the commit message. - the "Let's teach them manners" part of one commit message was replaced with a less flippant description. - the comment before merge_recursive() that claims that the return value is ignored now clarifies that it is ignored unless it indicates an error. - during the rebase to `master`, a trivial merge conflict with the `jc/renormalize-merge-kill-safer-crlf` branch was resolved. This patch series touches rather important code. Now that I addressed concerns such as prettifying some test code, I would appreciate thorough reviews with a focus on the critical parts of the code, those that could result in regressions. Johannes Schindelin (16): t5520: verify that `pull --rebase` shows the helpful advice when failing Report bugs consistently Avoid translating bug messages merge-recursive: clarify code in was_tracked() Prepare the builtins for a libified merge_recursive() merge_recursive: abort properly upon errors merge-recursive: avoid returning a wholesale struct merge-recursive: allow write_tree_from_memory() to error out merge-recursive: handle return values indicating errors merge-recursive: switch to returning errors instead of dying am -3: use merge_recursive() directly again merge-recursive: flush output buffer before printing error messages merge-recursive: write the commit title in one go merge-recursive: offer an option to retain the output in 'obuf' Ensure that the output buffer is released after calling merge_trees() merge-recursive: flush output buffer even when erroring out builtin/am.c | 62 ++ builtin/checkout.c | 5 +- builtin/ls-files.c | 3 +- builtin/merge.c| 2 + builtin/update-index.c | 2 +- grep.c | 8 +- imap-send.c| 2 +- merge-recursive.c | 578 + merge-recursive.h | 2 +- sequencer.c| 5 + sha1_file.c| 4 +- t/t5520-pull.sh| 32 +++ trailer.c | 2 +- transport.c| 2 +- wt-status.c| 4 +- 15 files changed, 418 insertions(+), 295 deletions(-) Published-As: https://github.com/dscho/git/releases/tag/am-3-merge-recursive-direct-v5 Interdiff vs v4: diff --git a/imap-send.c b/imap-send.c index 67d67f8..0f5f476 100644 --- a/imap-send.c +++ b/imap-send.c @@ -506,12 +506,12 @@ static char *next_arg(char **s) static int nfsnprintf(char *buf, int blen, const char *fmt, ...) { - int ret = -1; + int ret; va_list va; va_start(va, fmt); if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= (unsigned)blen) - die("BUG: buffer too small (%d < %d)", ret, blen); + die("BUG: buffer too small. Please report a bug."); va_end(va); return ret; } diff --git a/merge-recursive.c b/merge-recursive.c index a3d12e6..66e93e0 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2041,9 +2041,10 @@ int merge_recursive(struct merge_options *o, /* * When the merge fails, the result contains files * with conflict markers. The cleanness flag is - * ignored, it was never actually used, as result of - * merge_trees has always overwritten it: the committed - * "conflicts" were already resolved. + * ignored (unless indicating an error), it was never + * actually used, as result of merge_trees has always + *
[PATCH v5 04/16] merge-recursive: clarify code in was_tracked()
It can be puzzling to see that was_tracked() asks to get an index entry by name, but does not take a negative return value for an answer. The reason we have to do this is that cache_name_pos() only looks for entries in stage 0, even if nobody asked for any stage in particular. Let's rewrite the logic a little bit, to handle the easy case early: if cache_name_pos() returned a non-negative position, we know it is a match, and we do not even have to compare the name again (cache_name_pos() did that for us already). We can say right away: yes, this file was tracked. Only if there was no exact match do we need to look harder for any matching entry in stage 2. Signed-off-by: Johannes Schindelin--- merge-recursive.c | 30 ++ 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 1b6db87..3a652b7 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -667,23 +667,21 @@ static int was_tracked(const char *path) { int pos = cache_name_pos(path, strlen(path)); - if (pos < 0) - pos = -1 - pos; - while (pos < active_nr && - !strcmp(path, active_cache[pos]->name)) { - /* -* If stage #0, it is definitely tracked. -* If it has stage #2 then it was tracked -* before this merge started. All other -* cases the path was not tracked. -*/ - switch (ce_stage(active_cache[pos])) { - case 0: - case 2: + if (0 <= pos) + /* we have been tracking this path */ + return 1; + + /* +* Look for an unmerged entry for the path, +* specifically stage #2, which would indicate +* that "our" side before the merge started +* had the path tracked (and resulted in a conflict). +*/ + for (pos = -1 - pos; +pos < active_nr && !strcmp(path, active_cache[pos]->name); +pos++) + if (ce_stage(active_cache[pos]) == 2) return 1; - } - pos++; - } return 0; } -- 2.9.0.281.g286a8d9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 08/16] merge-recursive: allow write_tree_from_memory() to error out
It is possible that a tree cannot be written (think: disk full). We will want to give the caller a chance to clean up instead of letting the program die() in such a case. Signed-off-by: Johannes Schindelin--- merge-recursive.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 2be1e17..1f86338 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1888,8 +1888,8 @@ int merge_trees(struct merge_options *o, else clean = 1; - if (o->call_depth) - *result = write_tree_from_memory(o); + if (o->call_depth && !(*result = write_tree_from_memory(o))) + return -1; return clean; } -- 2.9.0.281.g286a8d9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 14/16] merge-recursive: offer an option to retain the output in 'obuf'
Since 66a155b (Enable output buffering in merge-recursive., 2007-01-14), we already accumulate the output in a buffer. The idea was to avoid interfering with the progress output that goes to stderr, which is unbuffered, when we write to stdout, which is buffered. We extend that buffering to allow the caller to handle the output (possibly suppressing it). This will help us when extending the sequencer to do rebase -i's brunt work: it does not want the picks to print anything by default but instead determine itself whether to print the output or not. Note that we also redirect the error messages into the output buffer when the caller asked not to flush the output buffer, for two reasons: 1) to retain the correct output order, and 2) to allow the caller to suppress *all* output. Signed-off-by: Johannes Schindelin--- merge-recursive.c | 17 + merge-recursive.h | 2 +- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 723b8d0..311cfa4 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -25,7 +25,7 @@ static void flush_output(struct merge_options *o) { - if (o->obuf.len) { + if (o->buffer_output < 2 && o->obuf.len) { fputs(o->obuf.buf, stdout); strbuf_reset(>obuf); } @@ -36,10 +36,19 @@ static int err(struct merge_options *o, const char *err, ...) va_list params; va_start(params, err); - flush_output(o); + if (o->buffer_output < 2) + flush_output(o); + else { + strbuf_complete(>obuf, '\n'); + strbuf_addstr(>obuf, "error: "); + } strbuf_vaddf(>obuf, err, params); - error("%s", o->obuf.buf); - strbuf_reset(>obuf); + if (o->buffer_output > 1) + strbuf_addch(>obuf, '\n'); + else { + error("%s", o->obuf.buf); + strbuf_reset(>obuf); + } va_end(params); return -1; diff --git a/merge-recursive.h b/merge-recursive.h index d415724..340704c 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -13,7 +13,7 @@ struct merge_options { MERGE_RECURSIVE_THEIRS } recursive_variant; const char *subtree_shift; - unsigned buffer_output : 1; + unsigned buffer_output : 2; /* 1: output at end, 2: keep buffered */ unsigned renormalize : 1; long xdl_opts; int verbosity; -- 2.9.0.281.g286a8d9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/8] status: tests for --porcelain=v2
Hi Jeff, On Mon, 25 Jul 2016, Jeff Hostetler wrote: > +## > +## Confirm VVP output prior to initial commit. > +## s/VVP/porcelain v2/... Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 05/16] Prepare the builtins for a libified merge_recursive()
Previously, callers of merge_trees() or merge_recursive() expected that code to die() with an error message. This used to be okay because we called those commands from scripts, and had a chance to print out a message in case the command failed fatally (read: with exit code 128). As scripting incurs its own set of problems (portability, speed, idiosynchracies of different shells, limited data structures leading to inefficient code), we are converting more and more of these scripts into builtins, using library functions directly. We already tried to use merge_recursive() directly in the builtin git-am, for example. Unfortunately, we had to roll it back temporarily because some of the code in merge-recursive.c still deemed it okay to call die(), when the builtin am code really wanted to print out a useful advice after the merge failed fatally. In the next commits, we want to fix that. The code touched by this commit expected merge_trees() to die() with some useful message when there is an error condition, but merge_trees() is going to be improved by converting all die() calls to return error() instead (i.e. return value -1 after printing out the message as before), so that the caller can react more flexibly. This is a step to prepare for the version of merge_trees() that no longer dies, even if we just imitate the previous behavior by calling exit(128): this is what callers of e.g. `git merge` have come to expect. Note that the callers of the sequencer (revert and cherry-pick) already fail fast even for the return value -1; The only difference is that they now get a chance to say " failed". A caller of merge_trees() might want handle error messages themselves (or even suppress them). As this patch is already complex enough, we leave that change for a later patch. Signed-off-by: Johannes Schindelin--- builtin/checkout.c | 4 +++- builtin/merge.c| 2 ++ sequencer.c| 4 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 27c1a05..07dea3b 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -567,8 +567,10 @@ static int merge_working_tree(const struct checkout_opts *opts, o.ancestor = old->name; o.branch1 = new->name; o.branch2 = "local"; - merge_trees(, new->commit->tree, work, + ret = merge_trees(, new->commit->tree, work, old->commit->tree, ); + if (ret < 0) + exit(128); ret = reset_tree(new->commit->tree, opts, 0, writeout_error); if (ret) diff --git a/builtin/merge.c b/builtin/merge.c index 19b3bc2..148a9a5 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -673,6 +673,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, hold_locked_index(, 1); clean = merge_recursive(, head, remoteheads->item, reversed, ); + if (clean < 0) + exit(128); if (active_cache_changed && write_locked_index(_index, , COMMIT_LOCK)) die (_("unable to write %s"), get_index_file()); diff --git a/sequencer.c b/sequencer.c index cdfac82..286a435 100644 --- a/sequencer.c +++ b/sequencer.c @@ -293,6 +293,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, clean = merge_trees(, head_tree, next_tree, base_tree, ); + if (clean < 0) + return clean; if (active_cache_changed && write_locked_index(_index, _lock, COMMIT_LOCK)) @@ -559,6 +561,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) { res = do_recursive_merge(base, next, base_label, next_label, head, , opts); + if (res < 0) + return res; write_message(, git_path_merge_msg()); } else { struct commit_list *common = NULL; -- 2.9.0.281.g286a8d9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 09/16] merge-recursive: handle return values indicating errors
We are about to libify the recursive merge machinery, where we only die() in case of a bug or memory contention. To that end, we must heed negative return values as indicating errors. This requires our functions to be careful to pass through error conditions in call chains, and for quite a few functions this means that they have to return values to begin with. The next step will be to convert the places where we currently die() to return negative values (read: -1) instead. Note that we ignore errors reported by make_room_for_path(), consistent with the previous behavior (update_file_flags() used the return value of make_room_for_path() only to indicate an early return, but not a fatal error): if the error is really a fatal error, we will notice later; If not, it was not that serious a problem to begin with. (Witnesses in favor of this reasoning are t4151-am-abort and t7610-mergetool, which would start failing if we stopped on errors reported by make_room_for_path()). Also note: while this patch makes the code slightly less readable in update_file_flags() (we introduce a new "goto free_buf;" instead of an explicit "free(buf); return;"), it is a preparatory change for the next patch where we will convert all of the die() calls in the same function to go through the free_buf return path instead. Signed-off-by: Johannes Schindelin--- merge-recursive.c | 252 -- 1 file changed, 150 insertions(+), 102 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 1f86338..6beb1e4 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -742,12 +742,12 @@ static int make_room_for_path(struct merge_options *o, const char *path) return error(msg, path, _(": perhaps a D/F conflict?")); } -static void update_file_flags(struct merge_options *o, - const struct object_id *oid, - unsigned mode, - const char *path, - int update_cache, - int update_wd) +static int update_file_flags(struct merge_options *o, +const struct object_id *oid, +unsigned mode, +const char *path, +int update_cache, +int update_wd) { if (o->call_depth) update_wd = 0; @@ -783,8 +783,7 @@ static void update_file_flags(struct merge_options *o, if (make_room_for_path(o, path) < 0) { update_wd = 0; - free(buf); - goto update_index; + goto free_buf; } if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) { int fd; @@ -807,20 +806,22 @@ static void update_file_flags(struct merge_options *o, } else die(_("do not know what to do with %06o %s '%s'"), mode, oid_to_hex(oid), path); + free_buf: free(buf); } update_index: if (update_cache) add_cacheinfo(mode, oid, path, 0, update_wd, ADD_CACHE_OK_TO_ADD); + return 0; } -static void update_file(struct merge_options *o, - int clean, - const struct object_id *oid, - unsigned mode, - const char *path) +static int update_file(struct merge_options *o, + int clean, + const struct object_id *oid, + unsigned mode, + const char *path) { - update_file_flags(o, oid, mode, path, o->call_depth || clean, !o->call_depth); + return update_file_flags(o, oid, mode, path, o->call_depth || clean, !o->call_depth); } /* Low level file merging, update and removal */ @@ -1019,7 +1020,7 @@ static int merge_file_one(struct merge_options *o, return merge_file_1(o, , , , branch1, branch2, mfi); } -static void handle_change_delete(struct merge_options *o, +static int handle_change_delete(struct merge_options *o, const char *path, const struct object_id *o_oid, int o_mode, const struct object_id *a_oid, int a_mode, @@ -1027,6 +1028,7 @@ static void handle_change_delete(struct merge_options *o, const char *change, const char *change_past) { char *renamed = NULL; + int ret = 0; if (dir_in_way(path, !o->call_depth)) { renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2); } @@ -1037,21 +1039,23 @@ static void handle_change_delete(struct merge_options *o, * correct; since there is no true "middle point" between * them, simply reuse the
[PATCH v5 06/16] merge_recursive: abort properly upon errors
There are a couple of places where return values never indicated errors before, as wie simply died instead of returning. But now negative return values mean that there was an error and we have to abort the operation. Let's do exactly that. Signed-off-by: Johannes Schindelin--- merge-recursive.c | 17 - 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 3a652b7..58ced25 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1949,17 +1949,19 @@ int merge_recursive(struct merge_options *o, /* * When the merge fails, the result contains files * with conflict markers. The cleanness flag is -* ignored, it was never actually used, as result of -* merge_trees has always overwritten it: the committed -* "conflicts" were already resolved. +* ignored (unless indicating an error), it was never +* actually used, as result of merge_trees has always +* overwritten it: the committed "conflicts" were +* already resolved. */ discard_cache(); saved_b1 = o->branch1; saved_b2 = o->branch2; o->branch1 = "Temporary merge branch 1"; o->branch2 = "Temporary merge branch 2"; - merge_recursive(o, merged_common_ancestors, iter->item, - NULL, _common_ancestors); + if (merge_recursive(o, merged_common_ancestors, iter->item, + NULL, _common_ancestors) < 0) + return -1; o->branch1 = saved_b1; o->branch2 = saved_b2; o->call_depth--; @@ -1975,6 +1977,8 @@ int merge_recursive(struct merge_options *o, o->ancestor = "merged common ancestors"; clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree, ); + if (clean < 0) + return clean; if (o->call_depth) { *result = make_virtual_commit(mrtree, "merged tree"); @@ -2031,6 +2035,9 @@ int merge_recursive_generic(struct merge_options *o, hold_locked_index(lock, 1); clean = merge_recursive(o, head_commit, next_commit, ca, result); + if (clean < 0) + return clean; + if (active_cache_changed && write_locked_index(_index, lock, COMMIT_LOCK)) return error(_("Unable to write index.")); -- 2.9.0.281.g286a8d9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 07/16] merge-recursive: avoid returning a wholesale struct
It is technically allowed, as per C89, for functions' return type to be complete structs (i.e. *not* just pointers to structs). However, it was just an oversight of this developer when converting Python code to C code in 6d297f8 (Status update on merge-recursive in C, 2006-07-08) which introduced such a return type. Besides, by converting this construct to pass in the struct, we can now start returning a value that can indicate errors in future patches. This will help the current effort to libify merge-recursive.c. Signed-off-by: Johannes Schindelin--- merge-recursive.c | 106 -- 1 file changed, 56 insertions(+), 50 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 58ced25..2be1e17 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -894,47 +894,47 @@ static int merge_3way(struct merge_options *o, return merge_status; } -static struct merge_file_info merge_file_1(struct merge_options *o, +static int merge_file_1(struct merge_options *o, const struct diff_filespec *one, const struct diff_filespec *a, const struct diff_filespec *b, const char *branch1, - const char *branch2) + const char *branch2, + struct merge_file_info *result) { - struct merge_file_info result; - result.merge = 0; - result.clean = 1; + result->merge = 0; + result->clean = 1; if ((S_IFMT & a->mode) != (S_IFMT & b->mode)) { - result.clean = 0; + result->clean = 0; if (S_ISREG(a->mode)) { - result.mode = a->mode; - oidcpy(, >oid); + result->mode = a->mode; + oidcpy(>oid, >oid); } else { - result.mode = b->mode; - oidcpy(, >oid); + result->mode = b->mode; + oidcpy(>oid, >oid); } } else { if (!oid_eq(>oid, >oid) && !oid_eq(>oid, >oid)) - result.merge = 1; + result->merge = 1; /* * Merge modes */ if (a->mode == b->mode || a->mode == one->mode) - result.mode = b->mode; + result->mode = b->mode; else { - result.mode = a->mode; + result->mode = a->mode; if (b->mode != one->mode) { - result.clean = 0; - result.merge = 1; + result->clean = 0; + result->merge = 1; } } if (oid_eq(>oid, >oid) || oid_eq(>oid, >oid)) - oidcpy(, >oid); + oidcpy(>oid, >oid); else if (oid_eq(>oid, >oid)) - oidcpy(, >oid); + oidcpy(>oid, >oid); else if (S_ISREG(a->mode)) { mmbuffer_t result_buf; int merge_status; @@ -946,64 +946,66 @@ static struct merge_file_info merge_file_1(struct merge_options *o, die(_("Failed to execute internal merge")); if (write_sha1_file(result_buf.ptr, result_buf.size, - blob_type, result.oid.hash)) + blob_type, result->oid.hash)) die(_("Unable to add %s to database"), a->path); free(result_buf.ptr); - result.clean = (merge_status == 0); + result->clean = (merge_status == 0); } else if (S_ISGITLINK(a->mode)) { - result.clean = merge_submodule(result.oid.hash, + result->clean = merge_submodule(result->oid.hash, one->path, one->oid.hash, a->oid.hash, b->oid.hash, !o->call_depth); } else if (S_ISLNK(a->mode)) { - oidcpy(, >oid); + oidcpy(>oid, >oid); if (!oid_eq(>oid, >oid)) - result.clean = 0; +
[PATCH v5 01/16] t5520: verify that `pull --rebase` shows the helpful advice when failing
It was noticed by Brendan Forster last October that the builtin `git am` regressed on that. Our hot fix reverted to spawning the recursive merge instead of using it as a library function. As we are about to revert that hot fix, after making the recursive merge a true library function (i.e. a function that does not die() in case of "normal" errors), let's add a test that verifies that we do not regress on the same problem which made the hot fix necessary in the first place. Signed-off-by: Johannes Schindelin--- t/t5520-pull.sh | 32 1 file changed, 32 insertions(+) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 37ebbcf..6ad37b5 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -255,6 +255,38 @@ test_expect_success '--rebase' ' test new = "$(git show HEAD:file2)" ' +test_expect_success '--rebase with conflicts shows advice' ' + test_when_finished "git rebase --abort; git checkout -f to-rebase" && + git checkout -b seq && + test_seq 5 >seq.txt && + git add seq.txt && + test_tick && + git commit -m "Add seq.txt" && + echo 6 >>seq.txt && + test_tick && + git commit -m "Append to seq.txt" seq.txt && + git checkout -b with-conflicts HEAD^ && + echo conflicting >>seq.txt && + test_tick && + git commit -m "Create conflict" seq.txt && + test_must_fail git pull --rebase . seq 2>err >out && + grep "When you have resolved this problem" out +' + +test_expect_success 'failed --rebase shows advice' ' + test_when_finished "git rebase --abort; git checkout -f to-rebase" && + git checkout -b diverging && + test_commit attributes .gitattributes "* text=auto" attrs && + sha1="$(printf "1\\r\\n" | git hash-object -w --stdin)" && + git update-index --cacheinfo 0644 $sha1 file && + git commit -m v1-with-cr && + # force checkout because `git reset --hard` will not leave clean `file` + git checkout -f -b fails-to-rebase HEAD^ && + test_commit v2-without-cr file "2" file2-lf && + test_must_fail git pull --rebase . diverging 2>err >out && + grep "When you have resolved this problem" out +' + test_expect_success '--rebase fails with multiple branches' ' git reset --hard before-rebase && test_must_fail git pull --rebase . copy master 2>err && -- 2.9.0.281.g286a8d9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 03/16] Avoid translating bug messages
While working on the patch series that avoids die()ing in recursive merges, the issue came up that bug reports (i.e. die("BUG: ...") constructs) should never be translated, as the target audience is the Git developer community, not necessarily the current user, and hence a translated message would make it *harder* to address the problem. So let's stop translating the obvious ones. As it is really, really outside the purview of this patch series to see whether there are more die() statements that report bugs and are currently translated, that task is left for another day and patch. Signed-off-by: Johannes Schindelin--- merge-recursive.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 4338b73..1b6db87 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -967,7 +967,7 @@ static struct merge_file_info merge_file_1(struct merge_options *o, if (!oid_eq(>oid, >oid)) result.clean = 0; } else - die(_("BUG: unsupported object type in the tree")); + die("BUG: unsupported object type in the tree"); } return result; @@ -1811,7 +1811,7 @@ static int process_entry(struct merge_options *o, */ remove_file(o, 1, path, !a_mode); } else - die(_("BUG: fatal merge failure, shouldn't happen.")); + die("BUG: fatal merge failure, shouldn't happen."); return clean_merge; } @@ -1869,7 +1869,7 @@ int merge_trees(struct merge_options *o, for (i = 0; i < entries->nr; i++) { struct stage_data *e = entries->items[i].util; if (!e->processed) - die(_("BUG: unprocessed path??? %s"), + die("BUG: unprocessed path??? %s", entries->items[i].string); } -- 2.9.0.281.g286a8d9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 02/16] Report bugs consistently
The vast majority of error messages in Git's source code which report a bug use the convention to prefix the message with "BUG:". As part of cleaning up merge-recursive to stop die()ing except in case of detected bugs, let's just make the remainder of the bug reports consistent with the de facto rule. Signed-off-by: Johannes Schindelin--- builtin/ls-files.c | 3 ++- builtin/update-index.c | 2 +- grep.c | 8 imap-send.c| 2 +- merge-recursive.c | 15 +++ sha1_file.c| 4 ++-- trailer.c | 2 +- transport.c| 2 +- wt-status.c| 4 ++-- 9 files changed, 21 insertions(+), 21 deletions(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index f02e3d2..00ea91a 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -118,7 +118,8 @@ static void show_killed_files(struct dir_struct *dir) */ pos = cache_name_pos(ent->name, ent->len); if (0 <= pos) - die("bug in show-killed-files"); + die("BUG: killed-file %.*s not found", + ent->len, ent->name); pos = -pos - 1; while (pos < active_nr && ce_stage(active_cache[pos])) diff --git a/builtin/update-index.c b/builtin/update-index.c index 6cdfd5f..ba04b19 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -1146,7 +1146,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) report(_("Untracked cache enabled for '%s'"), get_git_work_tree()); break; default: - die("Bug: bad untracked_cache value: %d", untracked_cache); + die("BUG: bad untracked_cache value: %d", untracked_cache); } if (active_cache_changed) { diff --git a/grep.c b/grep.c index 394c856..22cbb73 100644 --- a/grep.c +++ b/grep.c @@ -693,10 +693,10 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt) for (p = opt->header_list; p; p = p->next) { if (p->token != GREP_PATTERN_HEAD) - die("bug: a non-header pattern in grep header list."); + die("BUG: a non-header pattern in grep header list."); if (p->field < GREP_HEADER_FIELD_MIN || GREP_HEADER_FIELD_MAX <= p->field) - die("bug: unknown header field %d", p->field); + die("BUG: unknown header field %d", p->field); compile_regexp(p, opt); } @@ -709,7 +709,7 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt) h = compile_pattern_atom(); if (!h || pp != p->next) - die("bug: malformed header expr"); + die("BUG: malformed header expr"); if (!header_group[p->field]) { header_group[p->field] = h; continue; @@ -1514,7 +1514,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle case GREP_BINARY_TEXT: break; default: - die("bug: unknown binary handling mode"); + die("BUG: unknown binary handling mode"); } } diff --git a/imap-send.c b/imap-send.c index db0fafe..0f5f476 100644 --- a/imap-send.c +++ b/imap-send.c @@ -511,7 +511,7 @@ static int nfsnprintf(char *buf, int blen, const char *fmt, ...) va_start(va, fmt); if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= (unsigned)blen) - die("Fatal: buffer too small. Please report a bug."); + die("BUG: buffer too small. Please report a bug."); va_end(va); return ret; } diff --git a/merge-recursive.c b/merge-recursive.c index a4a1195..4338b73 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -268,7 +268,7 @@ struct tree *write_tree_from_memory(struct merge_options *o) fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce), (int)ce_namelen(ce), ce->name); } - die("Bug in merge-recursive.c"); + die("BUG: unmerged index entries in merge-recursive.c"); } if (!active_cache_tree) @@ -966,9 +966,8 @@ static struct merge_file_info merge_file_1(struct merge_options *o, if (!oid_eq(>oid, >oid)) result.clean = 0; - } else { - die(_("unsupported object type in the tree")); - } + } else +
Re: [PATCH v2 7/8] status: update git-status.txt for --porcelain=v2
Hi Kuba, On Tue, 26 Jul 2016, Jakub Narębski wrote: > W dniu 2016-07-25 o 21:25, Jeff Hostetler pisze: > > > +Field Meaning > > + > > +A 2 character field describing the conflict type > > +as described in the short format. > > + A 4 character field describing the submodule state > > +as described above. > > +The 6 character octal file modes for the stage 1, > > +stage 2, stage 3, and worktree. > > Errr... the pattern has only _3_ character octal modes, . > A question: what happens during octopus merge? From git-merge-octopus.sh: # We allow only last one to have a hand-resolvable # conflicts. Last round failed and we still had # a head to merge. In other words, octopus merges do not use higher stages than 3. Ciao, Dscho
[PATCH v2] merge: Run commit-msg hook
From: Orgad Shanehcommit-msg is needed to either validate the commit message or edit it. Gerrit for instance uses this hook to append its Change-Id footer. The hook is installed on the user's machine, and it is expected to append the footer for each commit that the user creates. This is relevant to merge commit just like any other commit. Currently this hook is only called for simple commits, so Gerrit user that tries to push a merge commit has to amend it first in order to execute the hook that appends the footer. prepare-commit-msg hook is already called on merge, but commit-msg isn't. It looks like unlike pre-commit not being executed, which was a conscious decision[1], commit-msg was never considered. [1] http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435 Signed-off-by: Orgad Shaneh --- Documentation/git-merge.txt | 6 +- builtin/merge.c | 7 ++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt index b758d55..59508aa 100644 --- a/Documentation/git-merge.txt +++ b/Documentation/git-merge.txt @@ -11,7 +11,7 @@ SYNOPSIS [verse] 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit] [-s ] [-X ] [-S[]] - [--[no-]allow-unrelated-histories] + [--[no-]allow-unrelated-histories] [--no-verify] [--[no-]rerere-autoupdate] [-m ] [...] 'git merge' HEAD ... 'git merge' --abort @@ -87,6 +87,10 @@ invocations. The automated message can include the branch description. Allow the rerere mechanism to update the index with the result of auto-conflict resolution if possible. +--no-verify:: + This option bypasses the commit-msg hook. + See also linkgit:githooks[5]. + --abort:: Abort the current conflict resolution process, and try to reconstruct the pre-merge state. diff --git a/builtin/merge.c b/builtin/merge.c index b555a1b..30c03c8 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -51,7 +51,7 @@ static const char * const builtin_merge_usage[] = { static int show_diffstat = 1, shortlog_len = -1, squash; static int option_commit = 1; static int option_edit = -1; -static int allow_trivial = 1, have_message, verify_signatures; +static int allow_trivial = 1, have_message, verify_signatures, no_verify; static int overwrite_ignore = 1; static struct strbuf merge_msg = STRBUF_INIT; static struct strategy **use_strategies; @@ -228,6 +228,7 @@ static struct option builtin_merge_options[] = { { OPTION_STRING, 'S', "gpg-sign", _commit, N_("key-id"), N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored files (default)")), + OPT_BOOL(0, "no-verify", _verify, N_("bypass commit-msg hook")), OPT_END() }; @@ -809,6 +810,10 @@ static void prepare_to_commit(struct commit_list *remoteheads) if (launch_editor(git_path_merge_msg(), NULL, NULL)) abort_commit(remoteheads, NULL); } + if (!no_verify && + run_commit_hook(0 < option_edit, get_index_file(), "commit-msg", + git_path_merge_msg(), NULL)) + abort_commit(remoteheads, NULL); read_merge_msg(); strbuf_stripspace(, 0 < option_edit); if (!msg.len) -- 2.8.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] merge: Run commit-msg hook
Hi again, On Tue, Jul 26, 2016 at 5:54 PM, Johannes Schindelinwrote: > > Hi Orgad, > > On Tue, 26 Jul 2016, Orgad Shaneh wrote: > > > On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin > > wrote: > > > > > > On Tue, 26 Jul 2016, Orgad Shaneh wrote: > > > > > >> commit-msg is needed to either validate the commit message or edit it. > > >> Gerrit for instance uses this hook to append its Change-Id footer. > > >> > > >> This is relevant to merge commit just like any other commit. > > > > > > Hmm. This is not very convincing to me, as > > > > > > - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`? > > > > prepare-commit-msg is already called, a few lines above this addition. > > Oh. That would have made a heck of a convincing argument in the commit > message. Pity it was not mentioned. (Yes, please read that as a strong > suggestion to fix that in the next patch iteration.) Done. > > > FWIW I dug out the original submission: > http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435 > > It seems that there was no discussion about the commit-msg. Which makes me > wonder why nobody thought of this. > > Now, you could make a case that you want to run the commit-msg hook in the > same spirit as prepare-commit-msg (and mention that apparently nobody > thought of that when the patch was accepted into builtin/merge.c). > > But then I wonder what your argument would be for *not* running the > pre-commit and the post-commit hooks in `git merge` as well? > > Seems like a big inconsistency to me, one that would not be helped by a > piecemeal patch that does only half the job of resolving the inconsistency. > > There was actually a question why the post-commit hook was not run in `git > merge`: > http://thread.gmane.org/gmane.comp.version-control.git/168716/focus=168773 > (but it seems nobody cared all that much). pre-commit seems to have been rejected, though I must admit I don't fully understand why. post-commit might make sense, but I wouldn't include it in the same patch. These are different issues. > > > > - a merge is a different beast from a simple commit. That is why we > > > have two different commands for them. A hook to edit the merge message > > > may need to know the *second* parent commit, too, for example to > > > generate a diffstat, or to add information about changes in an "evil > > > commit". > > > > That is correct for a post-merge hook. Why should *message validation* > > differ between simple and merge commit? > > You yourself do not use the hook for validation. You use it to *edit* the > message. My examples do the very same thing. Why should they wait for a > *post-merge* hook to amend the message? Because commit-msg doesn't know anything about the commit. It only refers to the message. The commit might even not exist when it is called. If you need data from the commit, post-merge is the right place. > > Otherwise, why wouldn't you use the post-merge hook to add the Change-Id: > in your use case, too... > > > > - if Gerrit is the intended user, would it not make more sense to > > > introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you > > > have to teach Gerrit a new trick anyway? > > > > Why is that new? Every commit in gerrit has a Change-Id footer, which is > > generated by commit-msg hook. > > So it already works for Gerrit? Why is this patch needed, then? This is > confusing. Gerrit is a server, the user installs a commit-msg hook provided by Gerrit on the local machine. This hook currently works only for simple commits and not for (trivial) merge commits. That's where this patch comes to the rescue. > > > What I currently do for merges that succeed without conflicts is > > unconditional commit --amend --no-edit just to run the hook. > > So you do that manually? Or you taught Gerrit to do that? Please clarify. Gerrit can't do anything on my machine. It's a web/ssh server. I have my own post-merge hook that runs commit --amend > > > - if Gerrit is the intended user, why does it not simply edit the merge > > > message itself? After all, it executes it, and probably crafts a merge > > > message mentioning that this is an automatic merge, anyway, so why not > > > add the Change-Id *then*? > > > > Most Gerrit setups require Change-Id in the commit message that the user > > pushes. It is possible to disable this setting, and then you don't need the > > Change-Id at all, but then you can't push a new patch set for the change > > (unless you copy the Change-Id from gerrit to your commit message). > > That's a real pain, I'm not aware of any public gerrit server that > > disables this :) > > Forgive me, I never used Gerrit, and neither do I intend to do so. Too bad, great system :) > I would appreciate a self-contained commit message that explains just > enough about Gerrit to understand what the patch tries to solve, and in a > manner such that even
Re: [PATCH v1 2/3] convert: modernize tests
Hi Lars, Sorry, minor nit that I noticed a couple of days ago but didn't comment on the moment and forgot until now. Lars Schneiderwrote: > Use `test_config` to set the config, check that files are empty with > `test_must_be_empty`, compare files with `test_cmp`, and remove spaces > after ">". Considering how close it is to your patch, you might also want to remove spaces after '<'. There is only one occurrence in this file and it's in a line you are already modifying. See below: > test_expect_success check ' > > -cmp test.o test && > -cmp test.o test.t && > +test_cmp test.o test && > +test_cmp test.o test.t && > > # ident should be stripped in the repository > git diff --raw --exit-code :test :test.i && > @@ -47,10 +47,10 @@ test_expect_success check ' > embedded=$(sed -ne "$script" test.i) && > test "z$id" = "z$embedded" && > > -git cat-file blob :test.t > test.r && > +git cat-file blob :test.t >test.r && > > -./rot13.sh < test.o > test.t && > -cmp test.r test.t > +./rot13.sh < test.o >test.t && Here. > +test_cmp test.r test.t > ' Thanks, Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/4] worktree: add per-worktree config files
On Tue, Jul 26, 2016 at 2:59 AM, Stefan Bellerwrote: > I like the user facing design, but how am I supposed to use it internally? > > Say I want to read a value preferably from the worktree I'd do a > /* > * maybe I don't even have to set it to 1 as > * the user is supposed to do that? > */ > repository_format_worktree_config = 1; > git_config_get_{string,bool,int} (... as usual ...) > > and if I want to read the value globally I would set the variable to 0 > and read? (I would need to restore it, so I'll have a temporary variable > to keep the original value of repository_format_worktree_config) I would understand if you need an api to write to worktree config or the shared one. But choosing to _read_ from a specific source sounds wrong. The common rule should apply everywhere: read from worktree first, if not found, read again from shared config. Why do you need this? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: Run commit-msg hook
Hi Orgad, On Tue, 26 Jul 2016, Orgad Shaneh wrote: > On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin >wrote: > > > > On Tue, 26 Jul 2016, Orgad Shaneh wrote: > > > >> commit-msg is needed to either validate the commit message or edit it. > >> Gerrit for instance uses this hook to append its Change-Id footer. > >> > >> This is relevant to merge commit just like any other commit. > > > > Hmm. This is not very convincing to me, as > > > > - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`? > > prepare-commit-msg is already called, a few lines above this addition. Oh. That would have made a heck of a convincing argument in the commit message. Pity it was not mentioned. (Yes, please read that as a strong suggestion to fix that in the next patch iteration.) FWIW I dug out the original submission: http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435 It seems that there was no discussion about the commit-msg. Which makes me wonder why nobody thought of this. Now, you could make a case that you want to run the commit-msg hook in the same spirit as prepare-commit-msg (and mention that apparently nobody thought of that when the patch was accepted into builtin/merge.c). But then I wonder what your argument would be for *not* running the pre-commit and the post-commit hooks in `git merge` as well? Seems like a big inconsistency to me, one that would not be helped by a piecemeal patch that does only half the job of resolving the inconsistency. There was actually a question why the post-commit hook was not run in `git merge`: http://thread.gmane.org/gmane.comp.version-control.git/168716/focus=168773 (but it seems nobody cared all that much). > > - a merge is a different beast from a simple commit. That is why we > > have two different commands for them. A hook to edit the merge message > > may need to know the *second* parent commit, too, for example to > > generate a diffstat, or to add information about changes in an "evil > > commit". > > That is correct for a post-merge hook. Why should *message validation* > differ between simple and merge commit? You yourself do not use the hook for validation. You use it to *edit* the message. My examples do the very same thing. Why should they wait for a *post-merge* hook to amend the message? Otherwise, why wouldn't you use the post-merge hook to add the Change-Id: in your use case, too... > > - if Gerrit is the intended user, would it not make more sense to > > introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you > > have to teach Gerrit a new trick anyway? > > Why is that new? Every commit in gerrit has a Change-Id footer, which is > generated by commit-msg hook. So it already works for Gerrit? Why is this patch needed, then? This is confusing. > What I currently do for merges that succeed without conflicts is > unconditional commit --amend --no-edit just to run the hook. So you do that manually? Or you taught Gerrit to do that? Please clarify. > > - if Gerrit is the intended user, why does it not simply edit the merge > > message itself? After all, it executes it, and probably crafts a merge > > message mentioning that this is an automatic merge, anyway, so why not > > add the Change-Id *then*? > > Most Gerrit setups require Change-Id in the commit message that the user > pushes. It is possible to disable this setting, and then you don't need the > Change-Id at all, but then you can't push a new patch set for the change > (unless you copy the Change-Id from gerrit to your commit message). > That's a real pain, I'm not aware of any public gerrit server that > disables this :) Forgive me, I never used Gerrit, and neither do I intend to do so. I would appreciate a self-contained commit message that explains just enough about Gerrit to understand what the patch tries to solve, and in a manner such that even developers who decided to ignore Gerrit understand. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANN] Pro Git Reedited 2nd Edition
On 7/26/2016 2:15 AM, Manlio Perillo wrote: I have noted a problem when reading the PDF with Chromium: the anchors/links do not work. I see what you mean. I was able to replicate the problem with Acrobat Reader on Windows 10. This seems to happen only with internal links - links to external URLs work fine. I don't know if this is an issue with the conversion to PDF or an issue with Chromium. It's not Chromium. This is going to be tricky to fix. I'm just using the tool chain that Scott and Ben set up. I've found other issues too that will be hard to resolve. Thanks for the report. I guess for now the best thing to do is to use the HTML version. Jon -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] convert: add filter..useProtocol option
W dniu 2016-07-25 o 22:09, Lars Schneider pisze: > On 24 Jul 2016, at 22:14, Jakub Narębskiwrote: >> W dniu 2016-07-24 o 20:36, Lars Schneider pisze: >>> On 23 Jul 2016, at 02:11, Jakub Narębski wrote: W dniu 2016-07-22 o 17:49, larsxschnei...@gmail.com pisze: > From: Lars Schneider Nb. this line is only needed if you want author name and/or date different from the email sender, or if you have sender line misconfigured (e.g. lacking the human readable name). >>> >>> I use "git format-patch" to generate these emails: >>> >>> git format-patch --cover-letter --subject-prefix="PATCH ..." -M $BASE -o >>> $OUTPUT >>> >>> How would I disable this line? (I already checked the man page to no avail). >> >> If you are using `git send-email` or equivalent, I think it is >> stripped automatically if it is not needed (in you case it was, >> because Sender was lacking human readable name... at least I think >> it was because of what my email reader inserted as reply line). >> If you are using an ordinary email client, you need to remove it >> yourself, if needed. > > Weird. I am sending the patches with this command: > > git send-email mystuff/* --to=git@vger.kernel.org --cc=... > > Maybe I need to try the "--suppress-from" switch?! No, the "From:" line is needed, but only because your Sender seems to be lacking human-readable name (for some strange reason). But let's stop this here. [...] I also agree that we might want to be able to keep clean and smudge filters separate, but be able to run a single program if they are both the same. I think there is a special case for filter unset, and/or filter being "cat" -- we would want to keep that. >>> >>> Since 1a8630d there is a more efficient way to unset a filter ;-) >>> Can you think of other cases where the separation would be useful? >> >> I can't think of any, but it doesn't mean that it does not exist. >> It also does not mean that you need to consider situation that may >> not happen. Covering one-way filters, like "indent" filter for `clean`, >> should be enough... they do work with your proposal, don't they? > > This should work right now but it would be a bit inefficient (the filter > would just pass the data unchanged through the smudge command). I plan to > add a "capabilities" flag to the protocol. Then you can define only > the "clean" capability and nothing or the current filter mechanism > would happen for smudge (I will make a test case to demonstrate that > behavior in v2). Isn't no-op filter (value not set, value set to empty string, "cat") caught earlier in a common code? We would certainly want to keep one-way filter configuration mechanism without many changes. Also, this should be of course tested. Git <-- Filter: "capabilities clean smudge\n" Or we can add capabilities in later version... >>> >>> That is an interesting idea. My initial thought was to make the capabilities >>> of a certain version fix. If we want to add new capabilities then we would >>> bump the version. I wonder what others think about your suggestion! >> >> Using capabilities (like git-upload-pack / git-receive-pack, that is >> smart Git transfer protocols do) is probably slightly more difficult on >> the Git side (assuming no capabilities negotiation), but also much more >> flexible than pure version numbers. >> >> One possible idea for a capability is support for passing input >> and output of a filter via filesystem, like cleanToFile and smudgeFromFile >> proposal in 'jh/clean-smudge-annex' (in 'pu'). >> >> For example: >> >> Git <-- Filter: "capabilities clean smudge cleanToFile smudgeFromFile\n" > > Yes, I like that very much. As stated above, I will add that in v2. I guess that you would add the idea of capabilities (though this could be left for v3), not support for "cleanToFile" / "smudgeFromFile" capabilities, and accompanying extension to the protocol, isn't it? > Git --> Filter: "testfile.dat\n" Unfortunately, while sane filenames should not contain newlines[1], the unfortunate fact is that *filenames can include newlines*, and you need to be able to handle that[2]. Therefore you need either to choose a different separator (the only one that can be safely used is "\0", i.e. the NUL character - but it is not something easy to handle by shell scripts), or C-quote filenames as needed, or always C-quote filenames. C-quoting at minimum should include quoting newline character, and the escape character itself. BTW. is it the basename of a file, or a full pathname? [1]: http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html [2]: http://www.dwheeler.com/essays/filenames-in-shell.html >>> >>> Thanks for this explanation. A bash version of the protocol is not >>> trivial (I tried it but ended up using Perl). Therefore I think "\0" >>>
[PATCH v2] commit: Fix description of no-verify
From: Orgad Shanehinclude also commit-msg hook. This brings the short help in line with the documentation. Signed-off-by: Orgad Shaneh --- builtin/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/commit.c b/builtin/commit.c index 163dbca..2725712 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1616,7 +1616,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "interactive", , N_("interactively add files")), OPT_BOOL('p', "patch", _interactive, N_("interactively add changes")), OPT_BOOL('o', "only", , N_("commit only specified files")), - OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit hook")), + OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit and commit-msg hooks")), OPT_BOOL(0, "dry-run", _run, N_("show what would be committed")), OPT_SET_INT(0, "short", _format, N_("show status concisely"), STATUS_FORMAT_SHORT), -- 2.8.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/8] status: print per-file porcelain v2 status data
On 07/25/2016 04:23 PM, Junio C Hamano wrote: Jeff Hostetlerwrites: +static void wt_porcelain_v2_print(struct wt_status *s); + There is no point in this forward declaration, if you just place the implementation of these functions here, no? Right. I just did it that way to make the diffs with the previous commit draw a little cleaner. But I can take it out. +/* + * Print porcelain v2 info for tracked entries with changes. + */ +static void wt_porcelain_v2_print_changed_entry( + struct string_list_item *it, + struct wt_status *s) +{ +... + fprintf(s->fp, "%c %s %s %06o %06o %06o %s %s R%d %s", It is misleading to always say R in the output when there is no rename, isn't it? Yes, especially if we add it for copied entries too. I was just looking for a way to have a fixed format. If we make the R%d field optional, then the first pathname is ambiguous. That gets me back to an earlier draft where we have rename and non-rename line types. I'll split this up into 1 pathname and 2 pathname forms, and only include the R%d (or the C%d) field in the latter. +* Note that this is a last-one-wins for each the individual +* stage [123] columns in the event of multiple cache rows +* for a stage. Just FYI, the usual lingo we use for that is "multiple cache entries for the same stage", I would think. thanks. +*/ + memset(stages, 0, sizeof(stages)); + sum = 0; + pos = cache_name_pos(it->string, strlen(it->string)); + assert(pos < 0); + pos = -pos-1; + while (pos < active_nr) { + ce = active_cache[pos++]; + stage = ce_stage(ce); + if (strcmp(ce->name, it->string) || !stage) + break; + stages[stage - 1].mode = ce->ce_mode; + hashcpy(stages[stage - 1].oid.hash, ce->sha1); + sum++; + } + if (!sum) + die("BUG: unmerged entry without any stages"); Hmm, we seem to already have d->stagemask; if you call that variable "sum" anyway, perhaps its computation can be more like sum |= 1 << (stage - 1); so that you can compare it with d->stagemask for this sanity check? good point. thanks jeff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: Run commit-msg hook
Hi, On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelinwrote: > Hi Orgad, > > On Tue, 26 Jul 2016, Orgad Shaneh wrote: > >> From: Orgad Shaneh > > Again, this is unnecessary if you already send the mail from the same > address. > >> commit-msg is needed to either validate the commit message or edit it. >> Gerrit for instance uses this hook to append its Change-Id footer. >> >> This is relevant to merge commit just like any other commit. > > Hmm. This is not very convincing to me, as > > - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`? prepare-commit-msg is already called, a few lines above this addition. > > - a merge is a different beast from a simple commit. That is why we have > two different commands for them. A hook to edit the merge message may > need to know the *second* parent commit, too, for example to generate > a diffstat, or to add information about changes in an "evil commit". That is correct for a post-merge hook. Why should *message validation* differ between simple and merge commit? > - if Gerrit is the intended user, would it not make more sense to > introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you > have to teach Gerrit a new trick anyway? Why is that new? Every commit in gerrit has a Change-Id footer, which is generated by commit-msg hook. What I currently do for merges that succeed without conflicts is unconditional commit --amend --no-edit just to run the hook. This doesn't make sense. > - if Gerrit is the intended user, why does it not simply edit the merge > message itself? After all, it executes it, and probably crafts a merge > message mentioning that this is an automatic merge, anyway, so why not > add the Change-Id *then*? Most Gerrit setups require Change-Id in the commit message that the user pushes. It is possible to disable this setting, and then you don't need the Change-Id at all, but then you can't push a new patch set for the change (unless you copy the Change-Id from gerrit to your commit message). That's a real pain, I'm not aware of any public gerrit server that disables this :) > Ciao, > Dscho - Orgad -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: Fix description of no-verify
Hi Orgad, On Tue, 26 Jul 2016, Orgad Shaneh wrote: > On Tue, Jul 26, 2016 at 3:55 PM, Johannes Schindelin >wrote: > > > > On Tue, 26 Jul 2016, Orgad Shaneh wrote: > > > >> include also commit-msg hook. > > > > This comment was a bit cryptic, until I read the patch. Now I find that > > comment redundant with the patch. > > This brings the short help in line with the documentation. I should > have stated that in the commit message. That would be good. > I don't have much experience with submitting patches to Git. How do I > edit the commit message? Submit it as a new patch? You edit it locally via `git commit --amend` (or using rebase -i's `reword` if you need to adjust a commit message in the middle of a patch series, not at the end). Then you send it as a reply to the first submission (--in-reply-to, if you use format-patch), with the `[PATCH v2]` prefix (--subject-prefix='[PATCH v2]' if you use format-patch). Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: LARGE_PACKET_MAX wrong?
On Tue, Jul 26, 2016 at 03:07:41PM +0200, Lars Schneider wrote: > I am reading the pkt-line code and stumbled across this oddity: > > LARGE_PACKET_MAX is defined as 65520 > https://github.com/git/git/blob/8c6d1f9807c67532e7fb545a944b064faff0f70b/pkt-line.h#L79 > > In `format_packet` we check that the 4 bytes of length data plus payload is > not larger than LARGE_PACKET_MAX (= 65520) > https://github.com/git/git/blob/8c6d1f9807c67532e7fb545a944b064faff0f70b/pkt-line.c#L111-L112 > > However, in the documentation we state that 4 bytes of length data plus > payload must not exceed 65524 > https://github.com/git/git/blob/8c6d1f9807c67532e7fb545a944b064faff0f70b/Documentation/technical/protocol-common.txt#L70-L72 > > Who is right? Code or documentation? I think the documentation is wrong. Git's packet_read() will complain on a 65524-byte incoming packet (it actually handles up to 65523, but that is simply a quirk of the implementation). The sending sides always include the 4-byte header in the LARGE_PACKET_MAX calculations. So I don't know what was intended once upon a time, but I think we have to stick to what the code does, because there are many deployed instances that we cannot break compatibility with. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/8] status: per-file data collection for --porcelain=v2
On 07/25/2016 04:14 PM, Junio C Hamano wrote: Jeff Hostetlerwrites: +static void aux_updated_entry_porcelain_v2( + struct wt_status *s, + struct wt_status_change_data *d, + struct diff_filepair *p) +{ + switch (p->status) { + case DIFF_STATUS_ADDED: + /* {mode,sha1}_head are zero for an add. */ + d->aux.porcelain_v2.mode_index = p->two->mode; I doubt that it makes sense in the longer term to have a new "aux" field. Why isn't it part of the wt_status_change_data structure? For that matter, why should these new functions have both "aux" and "v2" in their names? Imagine what should happen when somebody wants to add --porcelain=v3 format in 6 months. Why must "v3" be treated differently from "v1" and in a way close to "v2"? Why shouldn't all the three be treated in a similar way that "v1" has already? I wasn't sure if we wanted the v2 fields to be isolated and only filled in for v2 requests or whether we wanted them to be common going forward. In the case of the former, I could see the "aux" struct becoming a union and the various aux_*() routines only populating one member in that union. And then the various per-format print routines would know which aux member to access. That may be more complicated that necessary though -- if we assume that any subsequent formats (and possibly any JSON formats) would always want to keep these fields and add more. I'll flatten the fields into the main structure. + oidcpy(>aux.porcelain_v2.oid_index, >two->oid); + break; + + case DIFF_STATUS_DELETED: + d->aux.porcelain_v2.mode_head = p->one->mode; + oidcpy(>aux.porcelain_v2.oid_head, >one->oid); + /* {mode,oid}_index are zero for a delete. */ + break; + + case DIFF_STATUS_RENAMED: + d->aux.porcelain_v2.rename_score = p->score * 100 / MAX_SCORE; I have a slight aversion against losing the precision in a helper function like this that does not do the actual output, but it probably is OK. Don't we have copy detection score that is computed exactly the same way for DIFF_STATUS_COPIED case, too? Yes I believe so. I'll see about adding that. Or rather make the field apply to both. For readability, unless a case arm is completely empty, we should have /* fallthru */ comment where "break;" would go for a normal case arm. will do. thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: Fix description of no-verify
Hi and thanks for your reply. On Tue, Jul 26, 2016 at 3:55 PM, Johannes Schindelinwrote: > Hi Orgad > > On Tue, 26 Jul 2016, Orgad Shaneh wrote: > >> From: Orgad Shaneh > > This is unnecessary, as it matches your email address. > >> include also commit-msg hook. > > This comment was a bit cryptic, until I read the patch. Now I find that > comment redundant with the patch. This brings the short help in line with the documentation. I should have stated that in the commit message. > > However, I think that... > >> - OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit >> hook")), >> + OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit >> and commit-msg hooks")), > > > ... it may be more desirable to future-proof this simply by saying "bypass > hooks". That wouldn't be correct. prepare-commit-msg is not suppressed by this flag. > In the alternative, it would be good if the commit message could > convincingly make the case that there are no other hooks that will be > skipped with -n. > > Of course, I could go and look at the source code to convince myself. But > it is really the duty of the commit message to be already convincing > enough. > > Ciao, > Dscho I don't have much experience with submitting patches to Git. How do I edit the commit message? Submit it as a new patch? - Orgad -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: Run commit-msg hook
Hi Orgad, On Tue, 26 Jul 2016, Orgad Shaneh wrote: > From: Orgad ShanehAgain, this is unnecessary if you already send the mail from the same address. > commit-msg is needed to either validate the commit message or edit it. > Gerrit for instance uses this hook to append its Change-Id footer. > > This is relevant to merge commit just like any other commit. Hmm. This is not very convincing to me, as - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`? - a merge is a different beast from a simple commit. That is why we have two different commands for them. A hook to edit the merge message may need to know the *second* parent commit, too, for example to generate a diffstat, or to add information about changes in an "evil commit". - if Gerrit is the intended user, would it not make more sense to introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you have to teach Gerrit a new trick anyway? - if Gerrit is the intended user, why does it not simply edit the merge message itself? After all, it executes it, and probably crafts a merge message mentioning that this is an automatic merge, anyway, so why not add the Change-Id *then*? Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] t5510: skip tests under GETTEXT_POISON build
Skip tests when running under GETTEXT_POISON build and run them with C_LOCALE_OUTPUT prerequisite. These tests are irrelevant under GETTEXT_POISON because they test text output alignment which GETTEXT_POISON turns useless. Signed-off-by: Vasco Almeida--- t/t5510-fetch.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 6bd4853..668c54b 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -688,7 +688,7 @@ test_expect_success 'fetching with auto-gc does not lock up' ' ) ' -test_expect_success 'fetch aligned output' ' +test_expect_success C_LOCALE_OUTPUT 'fetch aligned output' ' git clone . full-output && test_commit long-tag && ( @@ -703,7 +703,7 @@ test_expect_success 'fetch aligned output' ' test_cmp expect actual ' -test_expect_success 'fetch compact output' ' +test_expect_success C_LOCALE_OUTPUT 'fetch compact output' ' git clone . compact && test_commit extraaa && ( -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: Fix description of no-verify
Hi Orgad On Tue, 26 Jul 2016, Orgad Shaneh wrote: > From: Orgad ShanehThis is unnecessary, as it matches your email address. > include also commit-msg hook. This comment was a bit cryptic, until I read the patch. Now I find that comment redundant with the patch. However, I think that... > - OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit > hook")), > + OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit > and commit-msg hooks")), ... it may be more desirable to future-proof this simply by saying "bypass hooks". In the alternative, it would be good if the commit message could convincingly make the case that there are no other hooks that will be skipped with -n. Of course, I could go and look at the source code to convince myself. But it is really the duty of the commit message to be already convincing enough. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 11/16] am -3: use merge_recursive() directly again
Hi Junio, On Mon, 25 Jul 2016, Junio C Hamano wrote: > Johannes Schindelinwrites: > > > Note: the code now calls merge_recursive_generic() again. Unlike > > merge_trees() and merge_recursive(), this function returns 0 upon success, > > as most of Git's functions. Therefore, the error value -1 naturally is > > handled correctly, and we do not have to take care of it specifically. > > I've finished reading through up to this point and I'd stop for > now. If you want, I can break out the subsequent patches into a separate series. I just thought that you might want to have them here, as I implemented them in response to the concern you raised in a previous iteration of the same patch series: you pointed out that returning a negative error value still does not let the caller handle the error message, and with the subsequent patches that is now possible, too. > Some of the patches I didn't look beyond the context presented in > the patches, so it is very possible that I missed leaks caused by > early returns and things like that, but I didn't see anything > glaringly wrong. Looks very promising. Thanks. I did try my best to catch all resource leaks, but I did stare at those patches so often and so long that it is easy for some obvious bug to have slipped by. Therefore, I would appreciate it if you (or somebody as diligent as you) could have a careful look in particular at the "merge-recursive: switch to returning errors instead of dying" patch. I may have missed something as stupid as an unclosed file handle, after all. Thank you, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html