Re: [PATCH] strbuf: clear errno before calling getdelim(3)
René Scharfe writes:
> getdelim(3) returns -1 at the end of the file and if it encounters an
> error, but sets errno only in the latter case. Set errno to zero before
> calling it to avoid misdiagnosing an out-of-memory condition due to a
> left-over value from some other function call.
>
> Reported-by: Yaroslav Halchenko
> Suggested-by: Junio C Hamano
> Signed-off-by: Rene Scharfe
Heh. I mumble something vague then people more capable than me jump
in to take it to the conclusion, and still I get the credit. I wish
all the debugging sessions were this easy ;-)
> ---
> Do we need to save and restore the original value of errno? I doubt it,
> but didn't think deeply about it, yet.
We probably don't need to---a caller who knows it got an error
before calling this function and wants to use errno after doing so
should be stashing it away; after all, this function will clobber
errno when any of the library calls it makes fails and this is on
the I/O codepath, so anything can go wrong.
Thanks.
>
> strbuf.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/strbuf.c b/strbuf.c
> index 89d22e3b09..323c49ceb3 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -476,6 +476,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int
> term)
> /* Translate slopbuf to NULL, as we cannot call realloc on it */
> if (!sb->alloc)
> sb->buf = NULL;
> + errno = 0;
> r = getdelim(&sb->buf, &sb->alloc, term, fp);
>
> if (r > 0) {
Re: [PATCH 3/4] Convert zlib.c to size_t
Martin Koegler writes: > From: Martin Koegler > > Signed-off-by: Martin Koegler > --- Thanks. I haven't thought things through but this looks sensible. Will queue.
Re: [PATCH v2 00/25] Move exported packfile funcs to its own file
On Thu, 10 Aug 2017 14:19:59 -0700 Junio C Hamano wrote: > Jonathan Tan writes: > > > Here is the complete patch set. I have only moved the exported functions > > that operate with packfiles and their static helpers - for example, > > static functions like freshen_packed_object() that are used only by > > non-pack-specific functions are not moved. > > This will interfere with smaller changes and fixes we want to have > early in the 'master' branch, so while I think it is a good idea to > do something like this in the longer term, I'd have to ask you to > either hold on or rebase this on them (you'll know what else you are > conflicting with when you try to merge this to 'pu' yourself). > > Thanks. OK, I'll wait until you have updated the master branch, then I'll try to rebase on it.
Re: [PATCH V2 2/2] Convert size datatype to size_t
Martin Koegler writes: > For next. As this touches core functions, it will likely produce > conflicts with other changes. Please provide the commit you want > to rebase the patch on and I'll produce a V3. No matter what base you pick, by the time the series is merged with other topics in flight to form an updated 'pu' branch, any series of this invasiveness will cause conflict. So from that point of view, picking 'master' or 'next' as the base would not make much difference. However, picking 'next' (or 'pu') as the base is definitely worse than 'master' for a different reason. Anything based on 'next', even though it may apply cleanly there, will not be able to graduate to 'master' without dragging all the other topics that are in 'next' with it. Immediately after a feature release is the worst time, as we will rewind and rebuild 'next' on top of 'master'. In practice, the only sensible base for an invasive change is the mimimum one you create yourself. You would: (1) Start from a reasonably stable base, like 'master'. (2) Among topics that are in flight but not in 'master', find the ones that materially interfere with your changes. Merge them on top of (1). (3) Then build your change on top. In the patch series you create in step 3, you would note which base you chosen (e.g. "v2.14.1") in step 1, plus the names of the topics you merged in step 2, after three-dash lines. The set of topics you find in step 2 might end up including a topic that is of dubious doneness (e.g. especially the ones that are not yet in 'next'). In such a case, you or the other topic may have to yield and wait for the other to stabilize. Git is not a substitute for inter-developer communication, and you'd talk to the author of the other topic and coordinate between yourselves when it happens. Thanks.
Re: [PATCH 0/4] dropping support for older curl
Jeff King writes: > On Thu, Aug 10, 2017 at 11:36:41AM +0200, Johannes Schindelin wrote: > >> Hopefully I had better luck expressing my concerns this time? > > I understand your argument much better now. I'm still not sure I agree. > > -Peff I do not think "there are a dozen #ifdefs and I don't know whether they still work. I don't know whether anybody (who most likely has better things to do than read the Git mailing list) is still using those. So let's just remove them." was why you were suggesting to clean up the (apparent) support of older curl in the code, though. Isn't the reason why your series simplifies these #ifdefs away because we by accident started using some features that require a version that is even newer than any of these #ifdef's try to cater to and yet nobody complained? That is a lot more similar to the removal of rsync transport that happened in a not so distant past, where the reason for removal was "We have been shipping code that couldn't have possibly worked for some time and nobody complained ---we know nobody is depending on it." Or "We accidentally started shipping code with comma after the last element of enum decl and nobody compalined---everybody's compiler must be ready" ;-)
Re: [PATCH] commit: skip discarding the index if there is no pre-commit hook
Kevin Willford writes:
> If there is not a pre-commit hook, there is no reason to discard
> the index and reread it.
>
> This change checks to presence of a pre-commit hook and then only
> discards the index if there was one.
>
> Signed-off-by: Kevin Willford
> ---
Peff already has done a good job reviewing the patch text, and I
agree that this is a worthwhile optimization.
Could Microsoft folks all make sure that their signed-off-by lines
match their From: address (or leave an in-body From: to override
the From: address your MUA places in your messages)?
Thanks.
> builtin/commit.c | 29 +
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index e7a2cb6285..443949d87b 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -671,12 +671,22 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> const char *hook_arg2 = NULL;
> int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
> int old_display_comment_prefix;
> + const char *precommit_hook = NULL;
>
> /* This checks and barfs if author is badly specified */
> determine_author_info(author_ident);
>
> - if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit",
> NULL))
> - return 0;
> +
> + if (!no_verify) {
> + /*
> + * Check to see if there is a pre-commit hook
> + * If there not one we can skip discarding the index later on
> + */
> + precommit_hook = find_hook("pre-commit");
> + if (precommit_hook &&
> + run_commit_hook(use_editor, index_file, "pre-commit", NULL))
> + return 0;
> + }
>
> if (squash_message) {
> /*
> @@ -940,12 +950,15 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> return 0;
> }
>
> - /*
> - * Re-read the index as pre-commit hook could have updated it,
> - * and write it out as a tree. We must do this before we invoke
> - * the editor and after we invoke run_status above.
> - */
> - discard_cache();
> + if (!no_verify && precommit_hook) {
> + /*
> + * Re-read the index as pre-commit hook could have updated it,
> + * and write it out as a tree. We must do this before we invoke
> + * the editor and after we invoke run_status above.
> + */
> + discard_cache();
> + }
> +
> read_cache_from(index_file);
> if (update_main_cache_tree(0)) {
> error(_("Error building trees"));
Re: [PATCH 0/4] dropping support for older curl
On 10/08/17 23:32, Jeff King wrote:
On Thu, Aug 10, 2017 at 10:33:18PM +0200, Tom G. Christensen wrote:
You've totally ignored the argument I made back then[1], and which I
reiterated in this thread. So I'll say it one more time: the more
compelling reason is not the #ifdefs, but the fact that the older
versions are totally untested.
Perhaps you forgot but I stated in the original thread that I build RPMS for
RHEL/CentOS 3, 4, 5, 6 and 7. I still do and I run the testsuite every
single time.
I didn't forget. I actually double-checked the patches you sent at the
time, but I didn't see one for the CURLPROTO issue. And indeed, it is
still broken for me:
$ cd /path/to/curl/repo
$ git checkout curl-7_15_5
$ ./buildconf && ./configure --prefix=/tmp/foo && make install
$ cd /path/to/git
$ git checkout v2.14.0
$ make CURLDIR=/tmp/foo V=1 http.o
gcc -o http.o -c -MF ./.depend/http.o.d -MQ http.o -MMD -MP -g -O0 -Wall -Werror -Wdeclaration-after-statement -Wpointer-arith -Wstrict-prototypes -Wvla
-Wold-style-declaration -Wold-style-definition -Wno-error -Wno-cpp -Wno-unused-value -Wno-strict-prototypes -I. -DUSE_LIBPCRE1 -DHAVE_ALLOCA_H
-I/tmp/foo/include -DUSE_CURL_FOR_IMAP_SEND -DNO_GETTEXT -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0
-DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_C="\"sha1dc_git.c\""
-DSHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H="\"sha1dc_git.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\""
-DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='
"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' http.c
http.c: In function ‘get_curl_allowed_protocols’:
http.c:685:24: error: ‘CURLPROTO_HTTP’ undeclared (first use in this
function); did you mean ‘CURLPROXY_HTTP’?
allowed_protocols |= CURLPROTO_HTTP;
^~
CURLPROXY_HTTP
[and so on]
I just built a pristine 2.14.0 on CentOS 5 with curl 7.15.5. No problems at
all neither with building nor with running the testsuite.
As you can see, this does not compile for me. What's going on?
The call site for get_curl_allowed_protocols() in http.c is still
protected by an #if:
#if LIBCURL_VERSION_NUM >= 0x071304
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
get_curl_allowed_protocols(0));
curl_easy_setopt(result, CURLOPT_PROTOCOLS,
get_curl_allowed_protocols(-1));
#else
warning("protocol restrictions not applied to curl redirects
because\n"
"your curl version is too old (>= 7.19.4)");
#endif
I don't see how it could work, as CURLPROTO_HTTP is not defined at all
in that version of curl.
Indeed but the #if will handle that.
Can you please double-check that you're
building against the correct version of curl, and that you are building
the HTTP parts of Git (which _are_ optional, and the test suite will
pass without them).
I use a mock buildroot and there is no other curl than the vendor
supplied 7.15.5 installed:
# pwd
/var/lib/mock/jrpms-el5-x86_64/root
# find . -name 'curlver.h'
./usr/include/curl/curlver.h
# grep LIBCURL_VERSION_NUM ./usr/include/curl/curlver.h
parsing and comparions by programs. The LIBCURL_VERSION_NUM define will
#define LIBCURL_VERSION_NUM 0x070f05
#
[root@c5-32bit-01 ~]# rpm -q git
git-2.14.1-1.el5.jr
[root@c5-32bit-01 ~]# ldd /usr/libexec/git-core/git-http-fetch |grep libcurl
libcurl.so.3 => /usr/lib/libcurl.so.3 (0x001e7000)
[root@c5-32bit-01 ~]# rpm -qf /usr/lib/libcurl.so.3
curl-7.15.5-17.el5_9
[root@c5-32bit-01 ~]# git --version
git version 2.14.1
[root@c5-32bit-01 ~]# git clone
https://github.com/tgc/tgcware-for-solaris.git
Cloning into 'tgcware-for-solaris'...
warning: protocol restrictions not applied to curl redirects because
your curl version is too old (>= 7.19.4)
remote: Counting objects: 2793, done.
remote: Total 2793 (delta 0), reused 0 (delta 0), pack-reused 2793
Receiving objects: 100% (2793/2793), 780.88 KiB | 639.00 KiB/s, done.
Resolving deltas: 100% (1233/1233), done.
[root@c5-32bit-01 ~]#
I also won't claim any absolutes. I think we all agree this is a
cost/benefit tradeoff. But there are a lot of options for building on a
very old system. For instance, building without http if you don't need
it. Or building a more recent libcurl (and even linking statically for
simplicity).
Of course that is always an option but it does complicate things.
I'd find arguments against the latter more compelling if recent curl
were hard to compile on old systems. I don't know whether that's the
case (certainly on a modern system, it's much easier to get newer
versions of curl to compile than older ones).
I have no experience with building curl on older Linux systems. I know
that I can build it on old Solaris releases but that is not q
Re: [PATCH 5/9] Convert sha1_file.c to size_t
Martin Koegler writes:
> From: Martin Koegler
>
> Signed-off-by: Martin Koegler
> ---
Please do not start your patch series from 5/9 when there is no 1/9,
2/9, 3/9, and 4/9. It is seriously confusing.
I am guessing that you are trying to split the series into
manageable pieces by going per call graph and codeflow. I think it
is a more sensible approach than a single huge ball of wax we saw
earlier.
It may take me a while to get back to this topic before I finish
reviewing other new topics in flight and also merging down existing
topics so that the codebase will become reasonably stable for a
topic that is invasive like this one can safely land. Please be
patient.
Thanks.
> cache.h | 16 +++
> sha1_file.c | 68
> ++---
> streaming.c | 2 +-
> 3 files changed, 43 insertions(+), 43 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 9185763..9322303 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1189,15 +1189,15 @@ static inline const unsigned char
> *lookup_replace_object(const unsigned char *sh
>
> /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
> extern int sha1_object_info(const unsigned char *, size_t *);
> -extern int hash_sha1_file(const void *buf, unsigned long len, const char
> *type, unsigned char *sha1);
> -extern int write_sha1_file(const void *buf, unsigned long len, const char
> *type, unsigned char *return_sha1);
> -extern int hash_sha1_file_literally(const void *buf, unsigned long len,
> const char *type, unsigned char *sha1, unsigned flags);
> -extern int pretend_sha1_file(void *, unsigned long, enum object_type,
> unsigned char *);
> +extern int hash_sha1_file(const void *buf, size_t len, const char *type,
> unsigned char *sha1);
> +extern int write_sha1_file(const void *buf, size_t len, const char *type,
> unsigned char *return_sha1);
> +extern int hash_sha1_file_literally(const void *buf, size_t len, const char
> *type, unsigned char *sha1, unsigned flags);
> +extern int pretend_sha1_file(void *, size_t, enum object_type, unsigned char
> *);
> extern int force_object_loose(const unsigned char *sha1, time_t mtime);
> extern int git_open_cloexec(const char *name, int flags);
> #define git_open(name) git_open_cloexec(name, O_RDONLY)
> -extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
> -extern int unpack_sha1_header(git_zstream *stream, unsigned char *map,
> unsigned long mapsize, void *buffer, unsigned long bufsiz);
> +extern void *map_sha1_file(const unsigned char *sha1, size_t *size);
> +extern int unpack_sha1_header(git_zstream *stream, unsigned char *map,
> size_t mapsize, void *buffer, size_t bufsiz);
> extern int parse_sha1_header(const char *hdr, size_t *sizep);
>
> /* global flag to enable extra checks when accessing packed objects */
> @@ -1723,8 +1723,8 @@ extern off_t find_pack_entry_one(const unsigned char
> *sha1, struct packed_git *)
>
> extern int is_pack_valid(struct packed_git *);
> extern void *unpack_entry(struct packed_git *, off_t, enum object_type *,
> size_t *);
> -extern unsigned long unpack_object_header_buffer(const unsigned char *buf,
> unsigned long len, enum object_type *type, size_t *sizep);
> -extern unsigned long get_size_from_delta(struct packed_git *, struct
> pack_window **, off_t);
> +extern size_t unpack_object_header_buffer(const unsigned char *buf, size_t
> len, enum object_type *type, size_t *sizep);
> +extern size_t get_size_from_delta(struct packed_git *, struct pack_window
> **, off_t);
> extern int unpack_object_header(struct packed_git *, struct pack_window **,
> off_t *, size_t *);
>
> /*
> diff --git a/sha1_file.c b/sha1_file.c
> index 3428172..1b3efea 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -51,7 +51,7 @@ static struct cached_object {
> unsigned char sha1[20];
> enum object_type type;
> void *buf;
> - unsigned long size;
> + size_t size;
> } *cached_objects;
> static int cached_object_nr, cached_object_alloc;
>
> @@ -818,8 +818,8 @@ static int check_packed_git_idx(const char *path, struct
> packed_git *p)
>* variable sized table containing 8-byte entries
>* for offsets larger than 2^31.
>*/
> - unsigned long min_size = 8 + 4*256 + nr*(20 + 4 + 4) + 20 + 20;
> - unsigned long max_size = min_size;
> + size_t min_size = 8 + 4*256 + nr*(20 + 4 + 4) + 20 + 20;
> + size_t max_size = min_size;
> if (nr)
> max_size += (nr - 1)*8;
> if (idx_size < min_size || idx_size > max_size) {
> @@ -1763,7 +1763,7 @@ static int open_sha1_file(const unsigned char *sha1,
> const char **path)
> */
> static void *map_sha1_file_1(const char *path,
>const unsigned char *sha1,
> - unsigned long *size)
> + size_t *size)
> {
>
Re: [PATCH v2 00/25] Move exported packfile funcs to its own file
Jonathan Tan writes: > On Thu, 10 Aug 2017 14:19:59 -0700 > Junio C Hamano wrote: > >> Jonathan Tan writes: >> >> > Here is the complete patch set. I have only moved the exported functions >> > that operate with packfiles and their static helpers - for example, >> > static functions like freshen_packed_object() that are used only by >> > non-pack-specific functions are not moved. >> >> This will interfere with smaller changes and fixes we want to have >> early in the 'master' branch, so while I think it is a good idea to >> do something like this in the longer term, I'd have to ask you to >> either hold on or rebase this on them (you'll know what else you are >> conflicting with when you try to merge this to 'pu' yourself). >> >> Thanks. > > OK, I'll wait until you have updated the master branch, then I'll try to > rebase on it. That will take a few weeks, and I do not think we want you idling during that time ;-). You'd need to double check, but I think the topics that cause trouble are rs/find-apck-entry-bisection and jk/drop-sha1-entry-pos; you can start from v2.14.1 and merge these topics on top and then build your change on top. That would allow you to start cooking before both of them graduate to 'master', as I expect they are both quick-to-next material. There might be other topics that interfere with what you are doing, but you can easily find out what they are if you do a trial merge to 'next' and 'pu' yourself. Thanks.
Re: [PATCH v2 0/2] Add progress for format-patch and rebase
Kevin Willford writes: > Changes since last patch: > 1. Use start_progress_delay so progress isn't shown if generating >the patches is fast enough > 2. Updated to have text of "Generating patches" > 3. Only show progress when the --progress flag is passed > 4. In the rebase script check stderr and the quiet option is not >set before propagating the progress flag to format-patch > > Kevin Willford (2): > format-patch: have progress option while generating patches > rebase: turn on progress option by default for format-patch Do you have a pointer to the previous discussion handy (if you do not, that is OK---I think I can dig the list archive myself)? Also, your patch messages appear from you at gmail yet signed off by you at microsoft. Please make them consistent (I can fix them up while queuing _this_ time, but I do not want forever doing that for future patches from you or other people). One easy workaround to do so without convincing your mailer to put your microsoft address on the sender's From: line in the e-mail is to put an extra From: Kevin Without line, followed by a blank line, at the very beginning of the body of the e-mail message. Thanks.
Re: [PATCH 0/4] dropping support for older curl
On Fri, Aug 11, 2017 at 12:23:42AM +0200, Tom G. Christensen wrote:
> > > I just built a pristine 2.14.0 on CentOS 5 with curl 7.15.5. No problems
> > > at
> > > all neither with building nor with running the testsuite.
> >
> > As you can see, this does not compile for me. What's going on?
> >
> The call site for get_curl_allowed_protocols() in http.c is still protected
> by an #if:
> #if LIBCURL_VERSION_NUM >= 0x071304
> curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> get_curl_allowed_protocols(0));
> curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> get_curl_allowed_protocols(-1));
> #else
> warning("protocol restrictions not applied to curl redirects
> because\n"
> "your curl version is too old (>= 7.19.4)");
> #endif
>
> > I don't see how it could work, as CURLPROTO_HTTP is not defined at all
> > in that version of curl.
>
> Indeed but the #if will handle that.
Er, sorry if I'm being dense, but how? Are you suggesting that by
removing the callsite of get_curl_allowed_protocols(), the compiler
might elide the now-dead code completely? I could certainly see it being
dropped after the compilation, but I'm surprised that it wouldn't
complain about the undeclared identifiers in the first place.
And if that _is_ what is happening...that seems like a very fragile and
unportable thing to be depending on.
> > Can you please double-check that you're
> > building against the correct version of curl, and that you are building
> > the HTTP parts of Git (which _are_ optional, and the test suite will
> > pass without them).
>
> I use a mock buildroot and there is no other curl than the vendor supplied
> 7.15.5 installed:
> [...]
OK, thanks for double-checking. I'm still puzzled why your build
succeeds and mine does not.
> > I saw that, too. But as I understand it, they provide no code updates:
> > no bugfixes and no security updates. They just promise to answer the
> > phone and help you with troubleshooting. It's possible my perception is
> > wrong, though; I'm certainly not one of their customers.
>
> I am refering to the Extended Life-cycle Support product (ELS), which
> promises:
> "the ELS Add-On delivers certain critical-impact security fixes and selected
> urgent priority bug fixes and troubleshooting for the last minor release"
>
> The full description is here:
> https://access.redhat.com/support/policy/updates/errata#Extended_Life_Cycle_Phase
That was the same page I was looking at. The bit I read was:
For versions of products in the Extended Life Phase, Red Hat will
provide limited ongoing technical support. No bug fixes, security
fixes, hardware enablement or root-cause analysis will be available
during this phase, and support will be provided on existing
installations only.
But I missed the bit about the "ELS add-on" below there, which I guess
is an extra thing. I do suspect that "install arbitrary new versions of
Git" is outside of their scope of "urgent priority bug fixes". But in a
sense it doesn't really matter. What is much more interesting is whether
there's a significant population that is running RHEL5 and has a strong
need for newer versions of Git. That I'm not sure about.
-Peff
Re: [PATCH 4/5] interpret-trailers: add an option to normalize output
On 10/08/17 22:10, Jeff King wrote: > On Thu, Aug 10, 2017 at 11:06:40PM +0200, Christian Couder wrote: > Related to this, I wonder if people might want to "normalize" in different ways later. If that happens, we might regret having called this option "--normalize" instead of "--one-per-line" for example. >>> >>> What is normal? >>> >>> Maybe --style=[one-line, wrapped:72, rfc, json, xml, ...] >>> then? >> >> Yeah, we could go there right now (using perhaps "--pretty" or >> "--format" instead of "--style", so that we are more consistent with >> other commands), but on the other hand we don't know yet if the other >> formats will really be needed. > > But some of those things are not 1:1 mappings with normalization. For > instance, --json presumably implies --only-trailers. Or are we proposing > to break the whole commit message down into components and output it all > as json? Hmm, to me the operation wasn't so much a normalization, rather it was an --unfold (or maybe --un-fold ;-D). I suppose going in the other direction could be --fold=72, or some such. [blue is my favourite colour ... :-P ] ATB, Ramsay Jones
Re: [PATCH 0/4] dropping support for older curl
On Thu, Aug 10, 2017 at 03:17:06PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > On Thu, Aug 10, 2017 at 11:36:41AM +0200, Johannes Schindelin wrote: > > > >> Hopefully I had better luck expressing my concerns this time? > > > > I understand your argument much better now. I'm still not sure I agree. > > > > -Peff > > I do not think "there are a dozen #ifdefs and I don't know whether > they still work. I don't know whether anybody (who most likely has > better things to do than read the Git mailing list) is still using > those. So let's just remove them." was why you were suggesting to > clean up the (apparent) support of older curl in the code, though. > > Isn't the reason why your series simplifies these #ifdefs away > because we by accident started using some features that require a > version that is even newer than any of these #ifdef's try to cater > to and yet nobody complained? That is a lot more similar to the > removal of rsync transport that happened in a not so distant past, > where the reason for removal was "We have been shipping code that > couldn't have possibly worked for some time and nobody complained > ---we know nobody is depending on it." I think there are two questions to be asked, and their answers come from different lines of reasoning. The first is "should we eventually drop support for antiquated versions of dependencies?". And the argument in favor is the one I was making here: besides lowering maintenance cost, it is more honest to our users about what to expect[1]. The second is "how far back should we keep support?". And there are two lines of thought there. One is to do it by date and what dependencies are in long-term OS releases, and then compare that to the benefit. Requiring curl 7.11.1 still keeps us working back to rhel4, which was already end-of-lifed completely after a 12 year run. Bumping to 7.16.0 drops rhel4 and rhel5, the latter of which is in its final "barely supported" phase after 10 years. But it gives us a bit more bang for our buck by making CURL_MULTI uconditional[2]. Requiring 7.19.4 actually doesn't drop any more rhel releases. So by that metric, we might as well go there. And the second line of thought is: it was already broken and nobody reported it or offered up a fix. And that's where the "similar to rsync" thing comes in. Though in this case we do have some evidence that people (at least Tom) was patching and distributing behind the scenes. And our breakage period was much shorter (since v2.12.0, but that's only months or so). -Peff
Re: [PATCH 4/5] interpret-trailers: add an option to normalize output
On Fri, Aug 11, 2017 at 12:02:49AM +0100, Ramsay Jones wrote: > > But some of those things are not 1:1 mappings with normalization. For > > instance, --json presumably implies --only-trailers. Or are we proposing > > to break the whole commit message down into components and output it all > > as json? > > Hmm, to me the operation wasn't so much a normalization, rather > it was an --unfold (or maybe --un-fold ;-D). I suppose going in > the other direction could be --fold=72, or some such. But I really don't want callers to think of it as "unfold". I want it to be "turn this into something I can parse simply". Hence if we were to find another case where the output is irregular, I'd feel comfortable calling that a bug and fixing it (one that I suspect but haven't tested is that alternate separators probably should all be converted to colons). > [blue is my favourite colour ... :-P ] Yes, I'm feeling that, too. :-/ -Peff
Re: [PATCH v2 0/2] Add progress for format-patch and rebase
On Thu, Aug 10, 2017 at 03:48:31PM -0700, Junio C Hamano wrote: > Kevin Willford writes: > > > Changes since last patch: > > 1. Use start_progress_delay so progress isn't shown if generating > >the patches is fast enough > > 2. Updated to have text of "Generating patches" > > 3. Only show progress when the --progress flag is passed > > 4. In the rebase script check stderr and the quiet option is not > >set before propagating the progress flag to format-patch > > > > Kevin Willford (2): > > format-patch: have progress option while generating patches > > rebase: turn on progress option by default for format-patch > > Do you have a pointer to the previous discussion handy (if you do > not, that is OK---I think I can dig the list archive myself)? https://public-inbox.org/git/20170531150427.7820-1-kewi...@microsoft.com/ is what I turned up. Overall this version looks good to me, and addresses all of the previous points. I have two minor points which I'll make inline. -Peff
Re: [PATCH 0/4] dropping support for older curl
On 11/08/17 00:54, Jeff King wrote: On Fri, Aug 11, 2017 at 12:23:42AM +0200, Tom G. Christensen wrote: Er, sorry if I'm being dense, but how? Are you suggesting that by removing the callsite of get_curl_allowed_protocols(), the compiler might elide the now-dead code completely? I could certainly see it being dropped after the compilation, but I'm surprised that it wouldn't complain about the undeclared identifiers in the first place. You're right, that should not be able to handle it. Can you please double-check that you're building against the correct version of curl, and that you are building the HTTP parts of Git (which _are_ optional, and the test suite will pass without them). I use a mock buildroot and there is no other curl than the vendor supplied 7.15.5 installed: [...] OK, thanks for double-checking. I'm still puzzled why your build succeeds and mine does not. I know what's going on now and it's so simple. Red Hats version of curl 7.15.5 includes a number of patches including one that backports support for CURLPROTO_* (as part of a fix for CVE-2009-0037). I haven't checked el6 but I would not be surprised if there where similar things going on there. So in conclusion version based #ifdefs are misleading when used with curl as shipped with RHEL. -tgc
Re: [PATCH v2 1/2] format-patch: have progress option while generating patches
On Thu, Aug 10, 2017 at 02:32:55PM -0400, Kevin Willford wrote:
> @@ -1493,6 +1496,8 @@ int cmd_format_patch(int argc, const char **argv, const
> char *prefix)
> OPT_FILENAME(0, "signature-file", &signature_file,
> N_("add a signature from a file")),
> OPT__QUIET(&quiet, N_("don't print the patch filenames")),
> + OPT_BOOL(0, "progress", &show_progress,
> + N_("show progress while generating patches")),
Earlier I suggested allowing --progress="custom text" since this may be
driven as plumbing for other commands. But I don't think there's any
need to worry about it now. It can be added seamlessly later if we find
such a caller.
> @@ -1752,8 +1757,12 @@ int cmd_format_patch(int argc, const char **argv,
> const char *prefix)
> start_number--;
> }
> rev.add_signoff = do_signoff;
> +
> + if (show_progress)
> + progress = start_progress_delay(_("Generating patches"), total,
> 0, 1);
I don't really have an opinion on a 1 second delay versus 2. I thought
we used 2 pretty consistently, though grepping around I do see a couple
of 1's. It probably doesn't matter, but just a curiosity.
-Peff
Re: [PATCH 0/4] dropping support for older curl
On Fri, Aug 11, 2017 at 01:17:51AM +0200, Tom G. Christensen wrote: > > OK, thanks for double-checking. I'm still puzzled why your build > > succeeds and mine does not. > > I know what's going on now and it's so simple. > Red Hats version of curl 7.15.5 includes a number of patches including one > that backports support for CURLPROTO_* (as part of a fix for CVE-2009-0037). > I haven't checked el6 but I would not be surprised if there where similar > things going on there. el6 should have it already as part of 7.19.7, right? > So in conclusion version based #ifdefs are misleading when used with curl as > shipped with RHEL. Yeah, that's certainly an interesting finding. In this case your builds are missing out on redirect protection that we _could_ be providing. If we do keep the compat ifdefs around this feature, it may be worth converting them to "#ifdef CURLPROTO_HTTP" to more directly check the feature. -Peff
Re: [PATCH 0/4] dropping support for older curl
On 11/08/17 01:23, Jeff King wrote: On Fri, Aug 11, 2017 at 01:17:51AM +0200, Tom G. Christensen wrote: OK, thanks for double-checking. I'm still puzzled why your build succeeds and mine does not. I know what's going on now and it's so simple. Red Hats version of curl 7.15.5 includes a number of patches including one that backports support for CURLPROTO_* (as part of a fix for CVE-2009-0037). I haven't checked el6 but I would not be surprised if there where similar things going on there. el6 should have it already as part of 7.19.7, right? Yes of course. So in conclusion version based #ifdefs are misleading when used with curl as shipped with RHEL. Yeah, that's certainly an interesting finding. In this case your builds are missing out on redirect protection that we _could_ be providing. Yes and I'm looking into that right now. If we do keep the compat ifdefs around this feature, it may be worth converting them to "#ifdef CURLPROTO_HTTP" to more directly check the feature. Yes, a feature test would be better. -tgc
Re: [PATCH 4/5] interpret-trailers: add an option to normalize output
On 11/08/17 00:10, Jeff King wrote: > On Fri, Aug 11, 2017 at 12:02:49AM +0100, Ramsay Jones wrote: > >>> But some of those things are not 1:1 mappings with normalization. For >>> instance, --json presumably implies --only-trailers. Or are we proposing >>> to break the whole commit message down into components and output it all >>> as json? >> >> Hmm, to me the operation wasn't so much a normalization, rather >> it was an --unfold (or maybe --un-fold ;-D). I suppose going in >> the other direction could be --fold=72, or some such. > > But I really don't want callers to think of it as "unfold". I want it to > be "turn this into something I can parse simply". Hence if we were to > find another case where the output is irregular, I'd feel comfortable > calling that a bug and fixing it (one that I suspect but haven't tested > is that alternate separators probably should all be converted to > colons). Ah, yes, good point. ATB, Ramsay Jones
Re: [PATCH] apply: remove prefix_length member from apply_state
On Wed, Aug 09, 2017 at 05:54:46PM +0200, René Scharfe wrote:
> Use a NULL-and-NUL check to see if we have a prefix and consistently use
> C string functions on it instead of storing its length in a member of
> struct apply_state. This avoids strlen() calls and simplifies the code.
I had to read the code to figure out exactly what you meant by
NULL-and-NUL (and even then it took me a minute).
I thought at first the latter half just means "use starts_with to walk
the string incrementally rather than bothering to find the length ahead
of time". Which makes perfect sense to me.
But actually, I think you mean the final block which makes sure we have
a non-empty string.
> diff --git a/apply.c b/apply.c
> index 970bed6d41..168dfe3d16 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -80,7 +80,6 @@ int init_apply_state(struct apply_state *state,
> {
> memset(state, 0, sizeof(*state));
> state->prefix = prefix;
> - state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
So we suspect that state->prefix might be NULL here.
> @@ -786,11 +785,11 @@ static int guess_p_value(struct apply_state *state,
> const char *nameline)
>* Does it begin with "a/$our-prefix" and such? Then this is
>* very likely to apply to our directory.
>*/
> - if (!strncmp(name, state->prefix, state->prefix_length))
> + if (starts_with(name, state->prefix))
> val = count_slashes(state->prefix);
At first this looked wrong to me. Don't we need to check for NULL? But
the check is simply just outside the context, so we are good.
> else {
> cp++;
> - if (!strncmp(cp, state->prefix, state->prefix_length))
> + if (starts_with(cp, state->prefix))
> val = count_slashes(state->prefix) + 1;
> }
And likewise for this one, which is part of the same block.
> @@ -2088,10 +2087,9 @@ static int use_patch(struct apply_state *state, struct
> patch *p)
> int i;
>
> /* Paths outside are not touched regardless of "--include" */
> - if (0 < state->prefix_length) {
> - int pathlen = strlen(pathname);
> - if (pathlen <= state->prefix_length ||
> - memcmp(state->prefix, pathname, state->prefix_length))
> + if (state->prefix && *state->prefix) {
> + const char *rest;
> + if (!skip_prefix(pathname, state->prefix, &rest) || !*rest)
> return 0;
> }
The check for *state->prefix here makes sure the behavior remains
identical. I wondered at first whether it's actually necessary. Wouldn't
an empty prefix always match?
But I think we're looking for the pathname to be a proper superset of
the prefix (hence the "!*rest" check). So I guess an empty path would
not be a proper superset of an empty prefix, even though with the
current code it doesn't hit that block at all.
I'm still not sure it's possible to have an empty pathname, so that
corner case may be moot and we could simplify the condition a little.
But certainly as you've written it, it could not be a regression.
The use of skip_prefix also took me a minute. I wonder if it's worth a
comment with the words "proper superset" or some other indicator (it
also surprised me that we're ok with matching "foobar" in the prefix
"foo", and not demanding "foo/bar". But I didn't look at the context to
know whether that's right or not. This may be a case where the prefix is
supposed to have "/" on it already).
-Peff
Re: [PATCH 0/4] dropping support for older curl
On Thu, Aug 10, 2017 at 07:09:02PM -0400, Jeff King wrote: > The first is "should we eventually drop support for antiquated versions > of dependencies?". And the argument in favor is the one I was making > here: besides lowering maintenance cost, it is more honest to our users > about what to expect[1]. As usual, I forgot all my footnotes. [1] When I've talked about keeping expectations reasonable, I'm a lot less interested in "oops, I built Git and this particular feature didn't work". It's easy to write that off as "well, you have an old version of curl, patches welcome". I'm much more concerned about security issues. Curl is network-facing. Leaving aside security vulnerabilities in curl itself (which hopefully distros with 10-year support periods would fix), I wouldn't be surprised if there are bad interactions possible due to our tangle of ifdefs. One way to address that would be more careful auditing. But then that goes back to the cost/benefit thing. > One is to do it by date and what dependencies are in long-term OS > releases, and then compare that to the benefit. Requiring curl 7.11.1 > still keeps us working back to rhel4, which was already end-of-lifed > completely after a 12 year run. Bumping to 7.16.0 drops rhel4 and rhel5, > the latter of which is in its final "barely supported" phase after 10 > years. But it gives us a bit more bang for our buck by making CURL_MULTI > uconditional[2]. Requiring 7.19.4 actually doesn't drop any more rhel > releases. So by that metric, we might as well go there. [2] The line-count change from dropping CURL_MULTI isn't _too_ exciting. But a lot of the tangled design of our http code revolves around the abstractions we've introduced. I have a feeling that it will enable further cleanups as we move forward (OTOH, a lot of the worst parts of our design are because of _using_ curl_multi for dumb http, which of course hardly anyone does these days. But I have a feeling if I suggested removing that, people would really scream). -Peff
git-describe --contains
Please help me understand how this command works. There is one case in the linux kernel repository that puzzles me. Let's consider patch "drm/i915/ execlists: Reset RING registers upon resume" [1]. This patch was committed 641 commits after version 4.8-rc2: ~$ git describe bafb2f7d4755bf1571bd5e9a03b97f3fc4fe69ae v4.8-rc2-641-gbafb2f7d4755 So I would expect to find it in version 4.8-rc3 and later versions. However, if I search for the tag that follows (and hence contains) that commit, I do not find version 4.8-rc3, nor version 4.8, nor version 4.9, but 4.10-rc1: ~$ git describe --contains bafb2f7d4755bf1571bd5e9a03b97f3fc4fe69ae v4.10-rc1~154^2~44^2~178 Why? Why not v4.8-rc3? This means that the patch has been included neither in v4.8 nor in v4.9, but only in version 4.10-rc1, right? Why so much time was needed, considering it was the 621st commit on top ov v4.8-rc2? BTW, what are the numbers 154^2~44^2~178 that follow the tag name? Thanks & Regards, Davide [1] https://patchwork.freedesktop.org/patch/111587/
Re: [PATCH 4/4] Fix delta offset overflow
On Thu, Aug 10, 2017 at 01:49:24PM -0700, Junio C Hamano wrote: > The lower 4-byte of moff (before incrementing it with msize) were > already encoded to the output stream before this hunk. Shouldn't > we be checking if moff would fit in uint32_t _before_ that happens? moff is otherwise only decremented or assigned with an offset generated by create_delta_index. These offsets are limited by 4GB. Any larger offets would be a programming bug - so qualify for just a "assert". > IOW, in a much earlier part of that "while (data < top)" loop, there > is a code that tries to find a match that gives us a large msize by > iterating over index->hash[], and it sets msize and moff as a potential > location that we may want to use. If moff is already big there, then > we shouldn't consider it a match because we cannot encode its location > using 4-byte anyway, no? Recovering from an incorrect moff would add more complex code for a case, that should not happen. A bailout would be sufficient. > Cutting it off at here by resetting msize to 0 might help the next > iteration (I didn't check, but is the effect of it is to corrupt the > "val" rolling checksum and make it unlikely that the hash > computation would not find a correct match?) but it somehow feels > like closing the barn door after the horse has already bolted... The current code produces incorrect deltas - its not just a checksum issue. By the way: Somebody interested in JGIT should also look at these two bugs: https://github.com/eclipse/jgit/blob/005e5feb4ecd08c4e4d141a38b9e7942accb3212/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/DeltaEncoder.java copy would also encode beyond 4GB - producing truncated delta offset. https://github.com/eclipse/jgit/blob/005e5feb4ecd08c4e4d141a38b9e7942accb3212/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/BinaryDelta.java apply uses int for decoding length values. Regards, Martin
