* [PATCH] config: don't segfault when given --path with a missing value
From: Carlos Martín Nieto @ 2012-11-15 18:10 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20121115161758.GC6157@sigill.intra.peff.net>
When given a variable without a value, such as '[section] var' and
asking git-config to treat it as a path, git_config_pathname returns
an error and doesn't modify its output parameter. show_config assumes
that the call is always successful and sets a variable to indicate
that vptr should be freed. In case of an error however, trying to do
this will cause the program to be killed, as it's pointing to memory
in the stack.
Detect the error and return immediately to avoid freeing or accessing
the uninitialed memory in the stack.
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
On Thu, Nov 15, 2012 at 08:11:50AM -0800, Jeff King wrote:
> Hmm, actually, we should probably propagate the error (I was thinking
> for some reason this was in the listing code, but it is really about
> getting a specific variable, and that variable does not have a sane
> format. We'll already have printed the non-bool error, so we should
> probably die. So more like:
>
> if (git_config_pathname(&vptr, key_, value_) < 0)
> return -1;
> must_free_vptr = 1;
Yeah, that's more sensible. I didn't notice that the buffer never gets
written to in this codepath, and the trying to print it out is silly
when we know that there is nothing valid to print. Thanks for the
review. I've included your test as well, which really makes all of
this your code. Do we have some equivalent of a Basically-writen-by
line?
builtin/config.c | 3 ++-
t/t1300-repo-config.sh | 5 +++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/builtin/config.c b/builtin/config.c
index 442ccc2..4dc5ffa 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -129,7 +129,8 @@ static int show_config(const char *key_, const char *value_, void *cb)
else
sprintf(value, "%d", v);
} else if (types == TYPE_PATH) {
- git_config_pathname(&vptr, key_, value_);
+ if (git_config_pathname(&vptr, key_, value_) < 0)
+ return -1;
must_free_vptr = 1;
} else if (value_) {
vptr = value_;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index a477453..17272e0 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -803,6 +803,11 @@ test_expect_success NOT_MINGW 'get --path copes with unset $HOME' '
test_cmp expect result
'
+test_expect_success 'get --path barfs on boolean variable' '
+ echo "[path]bool" >.git/config &&
+ test_must_fail git config --get --path path.bool
+'
+
cat > expect << EOF
[quote]
leading = " test"
--
1.8.0.316.g291341c
^ permalink raw reply related
* Re: [PATCH] config: don't segfault when given --path with a missing value
From: Jeff King @ 2012-11-15 18:15 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git
In-Reply-To: <1353003001-22600-1-git-send-email-cmn@elego.de>
On Thu, Nov 15, 2012 at 10:10:01AM -0800, Carlos Martín Nieto wrote:
> When given a variable without a value, such as '[section] var' and
> asking git-config to treat it as a path, git_config_pathname returns
> an error and doesn't modify its output parameter. show_config assumes
> that the call is always successful and sets a variable to indicate
> that vptr should be freed. In case of an error however, trying to do
> this will cause the program to be killed, as it's pointing to memory
> in the stack.
>
> Detect the error and return immediately to avoid freeing or accessing
> the uninitialed memory in the stack.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
Acked-by: Jeff King <peff@peff.net>
> Yeah, that's more sensible. I didn't notice that the buffer never gets
> written to in this codepath, and the trying to print it out is silly
> when we know that there is nothing valid to print.
> Thanks for the review. I've included your test as well, which really
> makes all of this your code.
Eh, I guess so. You did the hard part of finding it, though. ;)
> Do we have some equivalent of a Basically-writen-by line?
Nothing structured. But I am comfortable enough with the number of times
I am mentioned in "git log" already, so don't worry about it.
-Peff
^ permalink raw reply
* `git mv` has ambiguous error message for non-existing target
From: Patrick Lehner @ 2012-11-15 18:54 UTC (permalink / raw)
To: git
Hey guys,
as was brought up on #github today, the "git mv" command has a bit of a
little-helping output message when the target directory (or any
intermediate directories) dont exist.
To reproduce:
- cd into a git repo
- assuming "filea.txt" is an existing file in the CWD, and "dirb" is
neither a file nor a directory in the CWD, use the command "git mv
filea.txt dirb/filea.txt"
- this will produce an error message like `fatal: renaming 'filea.sh'
failed: No such file or directory`
It does not mention that the problem is, in fact, the target directory
not existing. This seems to be mostly a problem for users unfamiliar
with bash/*nix console commands. Although it is documented that git mv
will not create intermediate folders (which is fine, because neither
does mv), the error message might lead to believe a problem exists with
the source file.
Expanding the error message to "No such file or directory: 'dirb/' "
would probably clear this up.
Best regards and thanks to anyone who could improve this,
Patrick
^ permalink raw reply
* Re: [regression] Newer gits cannot clone any remote repos
From: Ramsay Jones @ 2012-11-15 18:55 UTC (permalink / raw)
To: Douglas Mencken; +Cc: git, Nguyen Thai Ngoc Duy, Junio C Hamano
In-Reply-To: <CACYvZ7jMC5xw4LxiuG5m+=grpQEg+wZb_7BaU4Xn-r7ix=S-bw@mail.gmail.com>
Douglas Mencken wrote:
>> Could you try re-building git with the
>> NO_THREAD_SAFE_PREAD build variable set?
>
> Yeah! It works!!!
>
> --- evil/Makefile
> +++ good/Makefile
> @@ -957,6 +957,7 @@
> HAVE_PATHS_H = YesPlease
> LIBC_CONTAINS_LIBINTL = YesPlease
> HAVE_DEV_TTY = YesPlease
> + NO_THREAD_SAFE_PREAD = YesPlease
> endif
> ifeq ($(uname_S),GNU/kFreeBSD)
> NO_STRLCPY = YesPlease
>
> With this, I do have correctly working git clone.
OK, good.
You didn't mention which platform you are on; from the above Makefile
hunk, however, I can deduce that you are on *some* version of Linux.
Hmm, it doesn't seem too likely that your pread() is thread-unsafe
(possible, just unlikely), so it could be a more fundamental problem
with the threaded index-pack code (ie commit b8a2486f1 et. seq.).
However, as a first step could you try running the test program
(given below) on your system to determine if your pread() is thread-safe
or not. (gcc -I. -o test-pread test-pread.c; ./test-pread)
Also, what is the output of "uname -a".
ATB,
Ramsay Jones
-- >8 --
#include "git-compat-util.h"
#include "thread-utils.h"
#define DATA_FILE "junk.data"
#define MAX_DATA 256 * 1024
#define NUM_THREADS 3
#define TRIALS 500000
struct thread_data {
pthread_t t;
int fd;
int cnt;
int fails;
unsigned long n;
};
static struct thread_data t[NUM_THREADS+1];
int create_data_file(void)
{
int i, fd = open(DATA_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600);
if (fd < 0)
return -1;
for (i = 0; i < MAX_DATA; i++)
if (write(fd, &i, sizeof(int)) < 0) {
close(fd);
unlink(DATA_FILE);
return -1;
}
close(fd);
return 0;
}
void *read_thread(void *data)
{
struct thread_data *d = (struct thread_data *)data;
int i, j, rd;
for (i = 0; i < TRIALS; i += MAX_DATA) {
for (j = 0; j < MAX_DATA; j++) {
ssize_t sz = read(d->fd, &rd, sizeof(int));
if (sz < 0 || rd != j)
d->fails++;
d->cnt++;
}
lseek(d->fd, 0, SEEK_SET);
}
return NULL;
}
void *pread_thread(void *data)
{
struct thread_data *d = (struct thread_data *)data;
int i, j, rd;
for (i = 0; i < TRIALS; i++) {
ssize_t sz;
d->n = d->n * 1103515245 + 12345;
j = d->n % MAX_DATA;
sz = pread(d->fd, &rd, sizeof(int), j * sizeof(int));
if (sz < 0 || rd != j)
d->fails++;
d->cnt++;
}
return NULL;
}
int main(int argc, char *argv[])
{
int fd, i;
if (create_data_file() < 0) {
printf("can't create data file\n");
return 1;
}
if ((fd = open(DATA_FILE, O_RDONLY)) < 0) {
printf("can't open data file\n");
unlink(DATA_FILE);
return 1;
}
for (i = 0; i < NUM_THREADS+1; i++) {
int ret;
t[i].fd = fd;
t[i].cnt = 0;
t[i].fails = 0;
t[i].n = i * 16381;
ret = pthread_create(&t[i].t, NULL,
(i == 0) ? read_thread : pread_thread,
&t[i]);
if (ret) {
printf("can't create thread %d (%s)\n", i, strerror(ret));
unlink(DATA_FILE);
return 1;
}
}
for (i = 0; i < NUM_THREADS+1; i++)
pthread_join(t[i].t, NULL);
close(fd);
for (i = 0; i < NUM_THREADS+1; i++)
printf("%2d: trials %d, failed %d\n", i, t[i].cnt, t[i].fails);
unlink(DATA_FILE);
return 0;
}
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2012, #03; Tue, 13)
From: Ramsay Jones @ 2012-11-15 19:05 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Jeff King, mlevedahl, git
In-Reply-To: <50A2B14C.9040608@web.de>
Torsten Bögershausen wrote:
>> * ml/cygwin-mingw-headers (2012-11-12) 1 commit
>> - Update cygwin.c for new mingw-64 win32 api headers
>>
>> Make git work on newer cygwin.
>>
>> Will merge to 'next'.
>
> (Sorry for late answer, I managed to test the original patch minutes before Peff merged it to pu)
> (And thanks for maintaining git)
>
> Is everybody using cygwin happy with this?
I am still on cygwin 1.5.22 and quite happy that this patch does
not (seem) to cause any problems. ;-P
> I managed to compile on a fresh installed cygwin,
> but failed to compile under 1.7.7, see below.
> Is there a way we can achieve to compile git both under "old" and "new" cygwin 1.7 ?
> Or is this not worth the effort?
Did the cygwin project not bump an api version number somewhere?
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [regression] Newer gits cannot clone any remote repos
From: Ramsay Jones @ 2012-11-15 19:17 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Douglas Mencken, git
In-Reply-To: <50A2B2DF.1080704@web.de>
Torsten Bögershausen wrote:
> On 13.11.12 19:55, Ramsay Jones wrote:
>> Douglas Mencken wrote:
>>> *Any* git clone fails with:
>>>
>>> fatal: premature end of pack file, 106 bytes missing
>>> fatal: index-pack failed
>>>
>>> At first, I tried 1.8.0, and it failed. Then I tried to build 1.7.10.5
>>> then, and it worked. Then I tried 1.7.12.2, but it fails the same way
>>> as 1.8.0.
>>> So I decided to git bisect.
>>>
>>> b8a2486f1524947f232f657e9f2ebf44e3e7a243 is the first bad commit
>>> ``index-pack: support multithreaded delta resolving''
>>
>> This looks like the same problem I had on cygwin, which lead to
>> commit c0f86547c ("index-pack: Disable threading on cygwin", 26-06-2012).
>>
>> I didn't notice which platform you are on, but maybe you also have a
>> thread-unsafe pread()? Could you try re-building git with the
>> NO_THREAD_SAFE_PREAD build variable set?
>>
>> HTH.
>>
>> ATB,
>> Ramsay Jones
>
> This is interesting.
> I had the same problem on a PowerPC
> (Old PowerBook G4 running Linux).
>
> Using NO_THREAD_SAFE_PREAD helped, thanks for the hint.
> (After recompiling without NO_THREAD_SAFE_PREAD I could clone
> from this machine again, so the problem is not really reproducable)
Yes, the failures would be intermittent (and often not easily
reproducible). The threaded index-pack code did not fail for
me on cygwin at all during development, including tests, but failed
immediately I installed v1.7.11. On real repositories, it failed
intermittently. On some repos it always failed, on some it never
failed and on some others it would sometimes fail, sometimes not.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: Git does not understand absolute Win'dos' path
From: Ramsay Jones @ 2012-11-15 19:20 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Martin Lichtin, git@vger.kernel.org
In-Reply-To: <50A36844.3090000@viscovery.net>
Johannes Sixt wrote:
> Am 11/14/2012 10:12, schrieb Martin Lichtin:
>> Maven's release plugin prepares a call Git like in this example:
>>
>> cmd.exe /X /C "git commit --verbose -F
>> C:\cygwin\tmp\maven-scm-915771020.commit pom.xml"
>>
>> Git doesn't seem to understand the -F argument and treats it like a
>> relative path (relative to the repository root):
>>
>> $ cmd.exe /X /C "git commit --verbose -F C:\cygwin\tmp\commit pom.xml"
>> fatal: could not read log file 'mytestdir/C:\cygwin\tmp\commit': No
>> such file or directory
>
> According to the code, this should not happen if you are using msysgit.
> For this reason, I guess you are using Cygwin git. Right?
>
> I don't know what Cygwin programs are supposed to do if they receive an
> argument that looks like a Windows style absolute path.
>
> OTOH, it could be argued that Maven should not treat a Cygwin program like
> a DOS program, and it should pass the path in the POSIXy form
> /c/cygwin/tmp/commit or /tmp/commit.
I would argue precisely this! :-D
ATB,
Ramsay Jones
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2012, #03; Tue, 13)
From: Torsten Bögershausen @ 2012-11-15 19:35 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Torsten Bögershausen, Jeff King, mlevedahl, git
In-Reply-To: <50A53CF0.7040809@ramsay1.demon.co.uk>
On 11/15/2012 08:05 PM, Ramsay Jones wrote:
> Torsten Bögershausen wrote:
>>> * ml/cygwin-mingw-headers (2012-11-12) 1 commit
>>> - Update cygwin.c for new mingw-64 win32 api headers
>>>
>>> Make git work on newer cygwin.
>>>
>>> Will merge to 'next'.
>>
>> (Sorry for late answer, I managed to test the original patch minutes before Peff merged it to pu)
>> (And thanks for maintaining git)
>>
>> Is everybody using cygwin happy with this?
>
> I am still on cygwin 1.5.22 and quite happy that this patch does
> not (seem) to cause any problems. ;-P
>
>> I managed to compile on a fresh installed cygwin,
>> but failed to compile under 1.7.7, see below.
>> Is there a way we can achieve to compile git both under "old" and "new" cygwin 1.7 ?
>> Or is this not worth the effort?
>
> Did the cygwin project not bump an api version number somewhere?
>
> ATB,
> Ramsay Jones
Ramsay,
you can run uname -r to see the version number.
I myself haven't fully understood all the consequences,
somewhere between 1.7.7 and 1.7.17 the include files had been changed.
If this has consequences for using e.g. winsock2.dll, I want to know
myself ;-)
/Torsten
^ permalink raw reply
* Re: [PATCH] status: add advice on how to push/pull to tracking branch
From: Matthieu Moy @ 2012-11-15 19:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vr4nu92hm.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> - if (!num_theirs)
>> + if (!num_theirs) {
>> strbuf_addf(sb,
>> Q_("Your branch is ahead of '%s' by %d commit.\n",
>> "Your branch is ahead of '%s' by %d commits.\n",
>> num_ours),
>> base, num_ours);
>> - else if (!num_ours)
>> + strbuf_addf(sb,
>> + _(" (use \"git push\" to publish your local commits)\n"));
>> + } else if (!num_ours) {
>
> The message should make it clear that the two words in double quotes
> only hint what command is used to "publish your local commits" and
> not to be taken as literal "here is what you exactly would type",
> but I do not think that is what I would get from this if I were a
> total newbie who would need this advise.
>
> It is even more true given that this is showing an arbitrary, and
> more likely than not a non-current branch, especially with the
> recent move from "matching" to "simple" where a naive use of "git
> push" is to push the branch that is currently checked out and no
> other branches.
I don't understand what you mean by "non-current". If you mean a local
branch not pointed to by HEAD, then I don't understand the remark, as
the message is shown by "git status" (looking more closely, it is also
shown by "git checkout", but after switching branch so also showing a
message about the current branch) and precisely talks about the current
branch. If you mean that the upsteam branch has a name different from
the local one, then with "push.default=simple", argumentless "git push"
will fail and show a detailed explanation to the user, which I find
acceptable.
I can tweak the advice to show the full "git push" command with
push.default=matching/current, but first, I'd like to understand your
remark.
>> + strbuf_addf(sb,
>> + _(" (use \"git pull\" to update your local branch)\n"));
>> + } else {
>
> Likewise, and the non-currentness of the branch being described is
> even worse in here, as unlike "git push" that can still be used to
> push a non-current branch, "git pull" is never to be used to update
> local branch that is not current, which means the advice must mention
> "git checkout" somewhere.
I understand this remark even less. We're showing a message about the
current branch and its upstream branch. In which case would "git pull"
not do the right thing?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* gitpacker progress report and a question
From: Eric S. Raymond @ 2012-11-15 21:28 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]
Some days ago I reported that I was attempting to write a tool that could
(a) take a git repo and unpack it into a tarball sequence plus a metadata log,
(b) reverse that operation, packing a tarball and log sequence into a repo.
Thanks in part to advice by Andreas Schwab and in part to looking at the
text of the p4 import script, this effort has succeeded. A proof of
concept is enclosed. It isn't documented yet, and has not been tested
on a repository with branches or merges in the history, but I am confident
that the distance from here to a finished and tested tool is short.
The immediate intended use is for importing older projects that are
available only as sequences of release tarballs, but there are other
sorts of repository surgery that would become easier using it.
I'm still looking for a better name for it and would welcome suggestions.
Before I do much further work, I need to determine how this will be shipped.
I see two possibilities: either I ship it as a small standalone project,
or it becomes a git subcommand shipped with the git suite. How I document
it and set up its tests would differ between these two cases.
Is there a process for submitting new subcommands? What are the
test-suite and documentation requirements?
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
[-- Attachment #2: gitpacker --]
[-- Type: text/plain, Size: 12708 bytes --]
#!/usr/bin/env python
"""
gitpacker - assemble tree sequences into repository histories
Requires git and cpio.
"""
import sys, os, getopt, subprocess, time, tempfile
DEBUG_GENERAL = 1
DEBUG_PROGRESS = 2
DEBUG_COMMANDS = 3
class Fatal(Exception):
"Unrecoverable error."
def __init__(self, msg):
Exception.__init__(self)
self.msg = msg
class Baton:
"Ship progress indications to stdout."
def __init__(self, prompt, endmsg='done', enable=False):
self.prompt = prompt
self.endmsg = endmsg
self.countfmt = None
self.counter = 0
if enable:
self.stream = sys.stdout
else:
self.stream = None
self.count = 0
self.time = 0
def __enter__(self):
if self.stream:
self.stream.write(self.prompt + "...")
if os.isatty(self.stream.fileno()):
self.stream.write(" \010")
self.stream.flush()
self.count = 0
self.time = time.time()
return self
def startcounter(self, countfmt, initial=1):
self.countfmt = countfmt
self.counter = initial
def bumpcounter(self):
if self.stream is None:
return
if os.isatty(self.stream.fileno()):
if self.countfmt:
update = self.countfmt % self.counter
self.stream.write(update + ("\010" * len(update)))
self.stream.flush()
else:
self.twirl()
self.counter = self.counter + 1
def endcounter(self):
if self.stream:
w = len(self.countfmt % self.count)
self.stream.write((" " * w) + ("\010" * w))
self.stream.flush()
self.countfmt = None
def twirl(self, ch=None):
"One twirl of the baton."
if self.stream is None:
return
if os.isatty(self.stream.fileno()):
if ch:
self.stream.write(ch)
self.stream.flush()
return
else:
update = "-/|\\"[self.count % 4]
self.stream.write(update + ("\010" * len(update)))
self.stream.flush()
self.count = self.count + 1
def __exit__(self, extype, value_unused, traceback_unused):
if extype == KeyboardInterrupt:
self.endmsg = "interrupted"
if extype == Fatal:
self.endmsg = "aborted by error"
if self.stream:
self.stream.write("...(%2.2f sec) %s.\n" \
% (time.time() - self.time, self.endmsg))
return False
def do_or_die(dcmd, legend=""):
"Either execute a command or raise a fatal exception."
if legend:
legend = " " + legend
if verbose >= DEBUG_COMMANDS:
sys.stdout.write("executing '%s'%s\n" % (dcmd, legend))
try:
retcode = subprocess.call(dcmd, shell=True)
if retcode < 0:
raise Fatal("child was terminated by signal %d." % -retcode)
elif retcode != 0:
raise Fatal("child returned %d." % retcode)
except (OSError, IOError) as e:
raise Fatal("execution of %s%s failed: %s" % (dcmd, legend, e))
def capture_or_die(dcmd, legend=""):
"Either execute a command and capture its output or die."
if legend:
legend = " " + legend
if verbose >= DEBUG_COMMANDS:
sys.stdout.write("executing '%s'%s\n" % (dcmd, legend))
try:
return subprocess.check_output(dcmd, shell=True)
except subprocess.CalledProcessError as e:
if e.returncode < 0:
raise Fatal("child was terminated by signal %d." % -e.returncode)
elif e.returncode != 0:
sys.stderr.write("gitpacker: child returned %d." % e.returncode)
sys.exit(1)
def git_pack(indir, outdir, quiet=False):
"Pack a tree sequence and associated logfile into a repository"
do_or_die("mkdir %s; git init -q %s" % (outdir, outdir))
logfile = os.path.join(indir, "log")
commit_id = [None]
state = 0
parents = []
comment = committername = authorname = ""
commitdate = authordate = commitemail = authoremail = ""
commitcount = 1;
linecount = 0
with Baton("Packing", enable=not quiet) as baton:
for line in open(logfile):
if verbose > DEBUG_PROGRESS:
print "Looking at: '%s'" % repr(line)
if state == 0:
if line == '\n':
state = 1
else:
try:
space = line.index(' ')
leader = line[:space]
follower = line[space:].strip()
if leader == "commit":
commit = follower
elif leader == "parent":
parents.append(follower)
elif leader not in ("author", "committer"):
raise Fatal("unexpected log attribute at %s" \
% repr(line))
elif leader == "committer":
(committername, committeremail, committerdate) = [x.strip() for x in follower.replace('>','<').split('<')]
elif leader == "author":
(authorname, authoremail, authordate) = [x.strip() for x in follower.replace('>','<').split('<')]
except ValueError:
raise Fatal('"%s", line %d: ill-formed log entry' % (logfile, linecount))
elif state == 1:
if line == ".\n":
if verbose > DEBUG_PROGRESS:
print "Interpretation begins"
os.chdir(outdir)
if commitcount > 1:
do_or_die("rm `git ls-tree --name-only HEAD`")
if verbose > DEBUG_PROGRESS:
print "Copying"
os.chdir("%s/%d" % (indir, commitcount))
do_or_die("find . -print | cpio -pd --quiet %s" % (outdir,))
os.chdir(outdir)
do_or_die("git add -A")
tree_id = capture_or_die("git write-tree").strip()
if verbose > DEBUG_PROGRESS:
print "Tree ID is", tree_id
(_, commentfile) = tempfile.mkstemp()
with open(commentfile, "w") as cfp:
cfp.write(comment)
command = "git commit-tree %s " % tree_id
command += " ".join(map(lambda p: "-p " + commit_id[int(p)],parents))
command += "<'%s'" % commentfile
environment = ""
environment += " GIT_AUTHOR_NAME='%s' " % authorname
environment += " GIT_AUTHOR_EMAIL='%s' " % authoremail
environment += " GIT_AUTHOR_DATE='%s' " % authordate
environment += " GIT_COMMITTER_NAME='%s' " % committername
environment += " GIT_COMMITTER_EMAIL='%s' " % committeremail
environment += " GIT_COMMITTER_DATE='%s' " % committerdate
commit_id.append(capture_or_die(environment + command).strip())
do_or_die("git update-ref HEAD %s" % commit_id[-1])
os.remove(commentfile)
state = 0
parents = []
comment = committername = authorname = ""
committerdate = authordate = committeremail = authoremail = ""
commitcount += 1
baton.twirl()
if maxcommit != 0 and commitcount >= maxcommit:
break
else:
if line.startswith("."):
line = line[1:]
comment += line
def git_unpack(indir, outdir, quiet=False):
"Unpack a repository into a tree sequence and associated logfile."
rawlogfile = os.path.join(outdir, "rawlog")
with Baton("Unpacking", enable=not quiet) as baton:
do_or_die("rm -fr %s; mkdir %s" % (outdir, outdir))
baton.twirl()
do_or_die("cd %s; git log --all --reverse --format=raw >%s" % (indir, rawlogfile))
baton.twirl()
commitcount = 1
commit_map = {}
os.chdir(indir)
try:
for line in open(rawlogfile):
baton.twirl()
if line.startswith("commit "):
commit = line.split()[1]
commit_map[commit] = commitcount
do_or_die("git checkout %s 2>/dev/null; mkdir %s/%d" \
% (commit, outdir, commitcount))
do_or_die("git ls-tree -r --name-only --full-tree %s | cpio -pd --quiet %s/%d"
% (commit, outdir, commitcount))
commitcount += 1
finally:
do_or_die("git reset --hard >/dev/null; git checkout master >/dev/null 2>&1")
cooked = os.path.join(outdir, "log")
body_latch = False
try:
with open(cooked, "w") as wfp:
linecount = 0
for line in open(rawlogfile):
linecount += 1
if line[0].isspace():
if line.startswith(" " * 4):
line = line[4:]
# Old-school byte stuffing.
if line.startswith("."):
line = "." + line
else:
space = line.index(' ')
leader = line[:space]
follower = line[space:].strip()
if leader == "tree":
continue
if leader == "commit" and linecount > 1:
wfp.write(".\n")
# FIXME: Check that log raw emits one parent per line
if leader in ("commit", "parent"):
line = "%s %s\n" % (leader, commit_map[follower])
body_latch = False
elif leader not in ("author", "committer"):
raise Fatal("unexpected log attribute at %s" \
% repr(line))
if line == '\n':
if not body_latch:
body_latch = True
else:
continue
wfp.write(line)
wfp.write(".\n")
except (ValueError, IndexError, KeyError):
raise Fatal("log rewrite failed on %s" % repr(line))
os.remove(rawlogfile)
if __name__ == '__main__':
(options, arguments) = getopt.getopt(sys.argv[1:], "ci:m:o:qxv")
mode = 'auto'
indir = '.'
outdir = None
quiet = False
maxcommit = 0
verbose = 0
for (opt, val) in options:
if opt == '-x':
mode = 'unpack'
elif opt == '-c':
mode = 'pack'
elif opt == '-m':
indir = int(val)
elif opt == '-i':
indir = val
elif opt == '-o':
outdir = val
elif opt == '-q':
quiet = True
elif opt == '-v':
verbose += 1
if not os.path.exists(indir):
sys.stderr.write("gitpacker: input directory %s must exist.\n" % indir)
sys.exit(1)
if mode == 'auto':
if os.path.exists(os.path.join(indir, ".git")):
mode = 'unpack'
else:
mode = 'pack'
assert mode == 'pack' or mode == 'unpack'
if outdir is None:
if mode == 'pack':
outdir = indir + "/packed"
elif mode == 'unpack':
outdir = indir + "/unpacked"
if os.path.exists(outdir):
sys.stderr.write("gitpacker: output directory %s must not exist.\n" % outdir)
sys.exit(1)
indir = os.path.abspath(indir)
outdir = os.path.abspath(outdir)
if verbose >= DEBUG_PROGRESS:
sys.stderr.write("gitpacker: %s from %s to %s.\n" % (mode, indir, outdir))
try:
try:
here = os.getcwd()
if mode == 'pack':
git_pack(indir, outdir, quiet=quiet)
elif mode == 'unpack':
git_unpack(indir, outdir, quiet=quiet)
finally:
os.chdir(here)
except Fatal, e:
sys.stderr.write(e.msg + "\n")
sys.exit(1)
except KeyboardInterrupt:
pass
# end
^ permalink raw reply
* Re: [PATCH] Unify appending signoff in format-patch, commit and sequencer
From: Brandon Casey @ 2012-11-15 20:42 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git@vger.kernel.org
In-Reply-To: <1352982778-28631-1-git-send-email-pclouds@gmail.com>
On Thu, Nov 15, 2012 at 4:32 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> There are two implementations of append_signoff in log-tree.c and
> sequencer.c, which do more or less the same thing. This patch
>
> - teaches sequencer.c's append_signoff() not to append the signoff if
> it's already there but not at the bottom
>
> - removes log-tree.c's
>
> - make sure "Signed-off-by: foo" in the middle of a line is not
> counted as a sign off
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Interestingly this patch triggers the fault that it fixes.
> I was surprised that there was no blank line before my S-o-b
> and thought I broke something. It turns out I used unmodified
> format-patch and it mistook the S-o-b quote as true S-o-b line.
> The modified one puts the blank line back.
Heh, yeah I noticed this bug yesterday when I submitted my changes
affecting the same area of code (sequence.c). Glad I didn't waste too
much time fixing it.
Have you looked at this:
http://thread.gmane.org/gmane.comp.version-control.git/209781
That series doesn't duplicate your work, but the two series's should
be resolved.
-Brandon
^ permalink raw reply
* Re: [PATCH] status: add advice on how to push/pull to tracking branch
From: Junio C Hamano @ 2012-11-15 21:49 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <vpqmwyid59i.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> I don't understand what you mean by "non-current". If you mean a local
> branch not pointed to by HEAD, then I don't understand the remark, as
> the message is shown by "git status" (looking more closely, it is also
> shown by "git checkout", but after switching branch so also showing a
> message about the current branch) and precisely talks about the current
> branch.
Ah, Ok, I somehow thought that "branch -v" would also use this
information, and/or during my absense this function from remote.c
got linked into "git remote show" ;-)
So it is not an issue right now, but we will have to worry about the
messaging when we start using this to describe a branch that is not
currently checked out.
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2012, #04; Wed, 14)
From: Junio C Hamano @ 2012-11-15 22:47 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git
In-Reply-To: <CAMP44s1-bMjr3_XUwkG2XSNzq_aUzHXrs0ZieHDCn+JBYOQMoQ@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Wed, Nov 14, 2012 at 11:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> * fc/completion-test-simplification (2012-10-29) 2 commits
>> - completion: simplify __gitcomp test helper
>> - completion: refactor __gitcomp related tests
>>
>> Clean up completion tests.
>>
>> There were some comments on the list.
>>
>> Expecting a re-roll.
>
> This was already re-rolled
> http://article.gmane.org/gmane.comp.version-control.git/209382
Thanks --- will take a look and replace but not today (I was
spending most of my time on the 'master' integration front).
^ permalink raw reply
* Re: gitpacker progress report and a question
From: Max Horn @ 2012-11-15 22:35 UTC (permalink / raw)
To: esr; +Cc: git
In-Reply-To: <20121115212818.GA21558@thyrsus.com>
On 15.11.2012, at 22:28, Eric S. Raymond wrote:
> Some days ago I reported that I was attempting to write a tool that could
> (a) take a git repo and unpack it into a tarball sequence plus a metadata log,
> (b) reverse that operation, packing a tarball and log sequence into a repo.
Ah, I could have used such a tool a year or so ago. Sounds useful to me, anyway :)
>
> Thanks in part to advice by Andreas Schwab and in part to looking at the
> text of the p4 import script, this effort has succeeded. A proof of
> concept is enclosed. It isn't documented yet, and has not been tested
> on a repository with branches or merges in the history, but I am confident
> that the distance from here to a finished and tested tool is short.
>
> The immediate intended use is for importing older projects that are
> available only as sequences of release tarballs, but there are other
> sorts of repository surgery that would become easier using it.
>
> I'm still looking for a better name for it and would welcome suggestions.
Isn't "gitar" the kind of natural choice? ;) At least for a stand-alone tool, not for a git subcommand.
Cheers,
Max
>
> Before I do much further work, I need to determine how this will be shipped.
> I see two possibilities: either I ship it as a small standalone project,
> or it becomes a git subcommand shipped with the git suite. How I document
> it and set up its tests would differ between these two cases.
>
> Is there a process for submitting new subcommands? What are the
> test-suite and documentation requirements?
> --
> <a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
> <gitpacker.txt>
^ permalink raw reply
* Re: gitpacker progress report and a question
From: Eric S. Raymond @ 2012-11-15 23:05 UTC (permalink / raw)
To: Max Horn; +Cc: git
In-Reply-To: <877E14DE-313D-4863-980B-13D077230F7F@quendi.de>
Max Horn <postbox@quendi.de>:
> > I'm still looking for a better name for it and would welcome suggestions.
>
> Isn't "gitar" the kind of natural choice? ;) At least for a stand-alone tool, not for a git subcommand.
I just renamed it git-weave. I keep talking about tarballs because I keep
thinking about using it archeologically on projects that only exist as
tarball sequences, but the tool actually oacks and unpacks *file tree*
sequences.
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
^ permalink raw reply
* [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines
From: Brandon Casey @ 2012-11-15 23:24 UTC (permalink / raw)
To: pclouds; +Cc: git, Brandon Casey, Brandon Casey
In-Reply-To: <1352943474-15573-5-git-send-email-drafnel@gmail.com>
Detect whether the s-o-b already exists in the commit footer and refrain
from adding a duplicate.
Update t3511 to test new behavior.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
Hi Duy,
How about this patch on top of my series as a base for your patch to
unify the code paths that append signoff in format-patch, commit, and
sequencer?
-Brandon
sequencer.c | 28 ++++++++++++++++++----------
t/t3511-cherry-pick-x.sh | 20 ++++++++++++++++++--
2 files changed, 36 insertions(+), 12 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 7ad1163..546dacb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -42,13 +42,15 @@ static int is_cherry_pick_from_line(const char *buf, int len)
!prefixcmp(buf, cherry_picked_prefix);
}
-static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
+static int ends_rfc2822_footer(struct strbuf *sb, struct strbuf *sob,
+ int ignore_footer)
{
int hit = 0;
int i, k;
int len = sb->len - ignore_footer;
int last_was_rfc2822 = 0;
const char *buf = sb->buf;
+ int found_sob = 0;
for (i = len - 1; i > 0; i--) {
if (hit && buf[i] == '\n')
@@ -66,12 +68,15 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
if (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t'))
continue;
+ if ((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) &&
+ sob && !found_sob &&
+ !strncasecmp(buf+i, sob->buf, sob->len))
+ found_sob = 1;
- if (!((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) ||
- is_cherry_pick_from_line(buf+i, k-i)))
+ if (!(last_was_rfc2822 || is_cherry_pick_from_line(buf+i, k-i)))
return 0;
}
- return 1;
+ return 1 + found_sob;
}
static void remove_sequencer_state(void)
@@ -547,7 +552,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
if (opts->record_origin) {
- if (!ends_rfc2822_footer(&msgbuf, 0))
+ if (!ends_rfc2822_footer(&msgbuf, NULL, 0))
strbuf_addch(&msgbuf, '\n');
strbuf_addstr(&msgbuf, cherry_picked_prefix);
strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
@@ -1077,6 +1082,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
void append_signoff(struct strbuf *msgbuf, int ignore_footer)
{
struct strbuf sob = STRBUF_INIT;
+ int has_footer = 0;
int i;
strbuf_addstr(&sob, sign_off_header);
@@ -1085,10 +1091,12 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
strbuf_addch(&sob, '\n');
for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
; /* do nothing */
- if (prefixcmp(msgbuf->buf + i, sob.buf)) {
- if (!i || !ends_rfc2822_footer(msgbuf, ignore_footer))
- strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
- strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
- }
+ if (!i || !(has_footer =
+ ends_rfc2822_footer(msgbuf, &sob, ignore_footer)))
+ strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
+ "\n", 1);
+ if (has_footer != 2)
+ strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf,
+ sob.len);
strbuf_release(&sob);
}
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index af7a87c..a15b199 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -11,9 +11,10 @@ pristine_detach () {
git clean -d -f -f -q -x
}
-non_rfc2822_mesg='base with footer
+non_rfc2822_mesg="base with footer
-Commit message body is here.'
+Commit message body is here.
+Not an s-o-b Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
rfc2822_mesg="$non_rfc2822_mesg
@@ -25,6 +26,9 @@ rfc2822_cherry_mesg="$rfc2822_mesg
(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
Tested-by: C.U. Thor <cuthor@example.com>"
+rfc2822_cherry_sob_mesg="$rfc2822_cherry_mesg
+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+Signed-off-by: C.U. Thor <cuthor@example.com>"
test_expect_success setup '
git config advice.detachedhead false &&
@@ -36,6 +40,8 @@ test_expect_success setup '
test_commit "$rfc2822_mesg" foo b rfc2822-base &&
git reset --hard initial &&
test_commit "$rfc2822_cherry_mesg" foo b rfc2822-cherry-base &&
+ git reset --hard initial &&
+ test_commit "$rfc2822_cherry_sob_mesg" foo b rfc2822-cherry-sob-base &&
pristine_detach initial &&
test_commit conflicting unrelated
'
@@ -151,4 +157,14 @@ test_expect_success 'cherry-pick treats -x -s "(cherry picked from..." line as p
test_cmp expect actual
'
+test_expect_success 'cherry-pick -s detects committer s-o-b already exists' '
+ pristine_detach initial &&
+ git cherry-pick -s rfc2822-cherry-sob-base &&
+ cat <<-EOF >expect &&
+ $rfc2822_cherry_sob_mesg
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.0
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2012, #03; Tue, 13)
From: Mark Levedahl @ 2012-11-15 23:34 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Ramsay Jones, Jeff King, git
In-Reply-To: <50A54406.5080309@web.de>
On 11/15/2012 02:35 PM, Torsten Bögershausen wrote:
> On 11/15/2012 08:05 PM, Ramsay Jones wrote:
>>
>> Did the cygwin project not bump an api version number somewhere?
>>
>> ATB,
>> Ramsay Jones
> Ramsay,
> you can run uname -r to see the version number.
>
> I myself haven't fully understood all the consequences,
> somewhere between 1.7.7 and 1.7.17 the include files had been changed.
>
> If this has consequences for using e.g. winsock2.dll, I want to know
> myself ;-)
>
> /Torsten
>
>
uname -r gives the version of the dll, not of any particular package,
and all of the various components are versioned independently. The
win32api is spread across several packages, and the recent update
changed the names, There is no api number advertised. You could check
the names of currently installed packages if you assume those names
won't change, but any such method is going to be fragile (and very
unsupported).
Mark
^ permalink raw reply
* Re: `git mv` has ambiguous error message for non-existing target
From: Junio C Hamano @ 2012-11-16 1:34 UTC (permalink / raw)
To: Patrick Lehner; +Cc: git
In-Reply-To: <50A53A80.4080203@gmx.de>
Patrick Lehner <lehner.patrick@gmx.de> writes:
> To reproduce:
> - cd into a git repo
> - assuming "filea.txt" is an existing file in the CWD, and "dirb" is
> neither a file nor a directory in the CWD, use the command "git mv
> filea.txt dirb/filea.txt"
> - this will produce an error message like `fatal: renaming 'filea.sh'
> failed: No such file or directory`
>
> It does not mention that the problem is, in fact, the target directory
> not existing. This seems to be mostly a problem for users unfamiliar
> with bash/*nix console commands. Although it is documented that git mv
> will not create intermediate folders (which is fine, because neither
> does mv), the error message might lead to believe a problem exists
> with the source file.
$ rm -fr xxx
$ >yyy
$ mv yyy xxx/yyy
mv: cannot move `yyy' to `xxx/yyy': No such file or directory
It doesn't mention that the problem is with 'xxx' and not 'yyy'
either.
^ permalink raw reply
* Re: [PATCH] tcsh-completion re-using git-completion.bash
From: Felipe Contreras @ 2012-11-16 1:41 UTC (permalink / raw)
To: Marc Khouzam; +Cc: szeder, git
In-Reply-To: <1352980269-15569-1-git-send-email-marc.khouzam@gmail.com>
On Thu, Nov 15, 2012 at 12:51 PM, Marc Khouzam <marc.khouzam@gmail.com> wrote:
> The current tcsh-completion support for Git, as can be found on the
> Internet, takes the approach of defining the possible completions
> explicitly. This has the obvious draw-back to require constant
> updating as the Git code base evolves.
>
> The approach taken by this commit is to to re-use the advanced bash
> completion script and use its result for tcsh completion. This is
> achieved by executing (versus sourcing) the bash script and
> outputting the completion result for tcsh consumption.
>
> Three solutions were looked at to implement this approach with (A)
> being retained:
>
> A) Modifications:
> git-completion.bash and new git-completion.tcsh
As I said, I don't think this is needed. It can be done in a single
stand-alone script without modifications to git-completion.bash.
This works:
set called = ($_)
set script = "${called[2]}.tmp"
cat <<\EOF > $script
source "$HOME/.git-completion.sh"
# Set COMP_WORDS in a way that can be handled by the bash script.
COMP_WORDS=($1)
# Set COMP_CWORD to the cursor location as bash would.
if [ -n "${2-}" ]; then
COMP_CWORD=$2
else
# Assume the cursor is at the end of parameter #1.
# We must check for a space as the last character which will
# tell us that the previous word is complete and the cursor
# is on the next word.
if [ "${1: -1}" == " " ]; then
# The last character is a space, so our location is at the end
# of the command-line array
COMP_CWORD=${#COMP_WORDS[@]}
else
# The last character is not a space, so our location is on the
# last word of the command-line array, so we must decrement the
# count by 1
COMP_CWORD=$((${#COMP_WORDS[@]}-1))
fi
fi
# Call _git() or _gitk() of the bash script, based on the first
# element of the command-line
_${COMP_WORDS[0]}
IFS=$'\n'
echo "${COMPREPLY[*]}"
\EOF
complete git 'p/*/`bash ${script} "${COMMAND_LINE}" | sort | uniq`/'
complete gitk 'p/*/`bash ${script} "${COMMAND_LINE}" | sort | uniq`/'
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s
From: Junio C Hamano @ 2012-11-16 1:58 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, Brandon Casey
In-Reply-To: <1352943474-15573-2-git-send-email-drafnel@gmail.com>
Brandon Casey <drafnel@gmail.com> writes:
> The cherry-pick -s functionality is currently broken in two ways.
>
> 1. handling of rfc2822 continuation lines has a bug, and the
> continuation lines are not handled correctly.
This is not limited to you, but people should think twice when
writing "has a bug" and "are not handled correctly" in their log
message. Did you write what the expected and actual behaviours?
> +rfc2822_mesg="$non_rfc2822_mesg
> +
> +Signed-off-by: A.U. Thor
> + <author@example.com>
> +Signed-off-by: B.U. Thor <buthor@example.com>"
The S-o-b: lines are meant to record people's contact info in human
readable forms, and folding the lines like the above makes it a lot
harder to read. They typically do not have to be folded.
Besides, the footer lines are *not* RFC2822 headers (and are not
used as such when send-email comes up with Cc: list) in the first
place; have we ever said anything about supporting the RFC2822 line
folding in the commit footer? If not (and I am reasonably sure we
never have), I personally think we should actively *discourage* line
folding there.
> i.e. we should produce this:
>
> Signed-off-by: A.U. Thor <author@example.com>
> (cherry picked from )
> Signed-off-by: C O Mmitter <committer@example.com>
>
> not
>
> Signed-off-by: A.U. Thor <author@example.com>
> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
>
> Signed-off-by: C O Mmitter <committer@example.com>
I can buy that, but then this makes it very clear that these footer
lines are not shaped like RFC2822 headers, no?
Thanks.
^ permalink raw reply
* Re: [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines
From: Junio C Hamano @ 2012-11-16 2:03 UTC (permalink / raw)
To: Brandon Casey; +Cc: pclouds, git, Brandon Casey
In-Reply-To: <1353021875-7552-1-git-send-email-drafnel@gmail.com>
Brandon Casey <drafnel@gmail.com> writes:
> Detect whether the s-o-b already exists in the commit footer and refrain
> from adding a duplicate.
If you are trying to forbid
git cherry-pick -s $other
from adding s-o-b: A when $other ends with these two existing s-o-b:
s-o-b: A
s-o-b: B
then I think that is a wrong thing to do.
In such a case, the resulting commit should gain another s-o-b from
A to record the provenance as a chain of events. A originally wrote
the patch, B forwarded it (possibly with his own tweaks), and then A
picked it up and recorded the result to the history, possibly with a
final tweak or two.
^ permalink raw reply
* Re: [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s
From: Brandon Casey @ 2012-11-16 2:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Casey
In-Reply-To: <7v8va28g1c.fsf@alter.siamese.dyndns.org>
On Thu, Nov 15, 2012 at 5:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>> The cherry-pick -s functionality is currently broken in two ways.
>>
>> 1. handling of rfc2822 continuation lines has a bug, and the
>> continuation lines are not handled correctly.
>
> This is not limited to you, but people should think twice when
> writing "has a bug" and "are not handled correctly" in their log
> message. Did you write what the expected and actual behaviours?
Yeah, I wasn't clear here. The "bug" was that the incorrect index
variable was being used, which caused the wrong line to be examined to
see if it was an rfc2822 continuation line. I could have mentioned
that.
>> +rfc2822_mesg="$non_rfc2822_mesg
>> +
>> +Signed-off-by: A.U. Thor
>> + <author@example.com>
>> +Signed-off-by: B.U. Thor <buthor@example.com>"
>
> The S-o-b: lines are meant to record people's contact info in human
> readable forms, and folding the lines like the above makes it a lot
> harder to read. They typically do not have to be folded.
Well, I wasn't adding functionality here, I was only fixing what I
noticed was broken when I started touching this code.
> Besides, the footer lines are *not* RFC2822 headers (and are not
> used as such when send-email comes up with Cc: list) in the first
> place; have we ever said anything about supporting the RFC2822 line
> folding in the commit footer? If not (and I am reasonably sure we
> never have), I personally think we should actively *discourage* line
> folding there.
That's fine with me. I can't think of a reason that would make it
necessary to support line continuation.
>> i.e. we should produce this:
>>
>> Signed-off-by: A.U. Thor <author@example.com>
>> (cherry picked from )
>> Signed-off-by: C O Mmitter <committer@example.com>
>>
>> not
>>
>> Signed-off-by: A.U. Thor <author@example.com>
>> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
>>
>> Signed-off-by: C O Mmitter <committer@example.com>
>
> I can buy that, but then this makes it very clear that these footer
> lines are not shaped like RFC2822 headers, no?
The lines that are _not_ "(cherry picked from ...)" lines do follow
the format defined by rfc2822 for header lines (mostly). That's
probably why the author of the function in sequencer.c that checks for
a s-o-b footer named it "ends_rfc2822_footer".
I'll remove the *broken* existing code that was intended to support
continuation lines and submit a new patch.
-Brandon
^ permalink raw reply
* Re: [PATCH] Unify appending signoff in format-patch, commit and sequencer
From: Nguyen Thai Ngoc Duy @ 2012-11-16 4:13 UTC (permalink / raw)
To: Brandon Casey; +Cc: git@vger.kernel.org
In-Reply-To: <CA+sFfMcfGbGDeXKZD1o4pmhbrJ1E5WgV3qrQBPT3eH=W54tNGw@mail.gmail.com>
On Fri, Nov 16, 2012 at 3:42 AM, Brandon Casey <drafnel@gmail.com> wrote:
>> Interestingly this patch triggers the fault that it fixes.
>> I was surprised that there was no blank line before my S-o-b
>> and thought I broke something. It turns out I used unmodified
>> format-patch and it mistook the S-o-b quote as true S-o-b line.
>> The modified one puts the blank line back.
>
> Heh, yeah I noticed this bug yesterday when I submitted my changes
> affecting the same area of code (sequence.c). Glad I didn't waste too
> much time fixing it.
>
> Have you looked at this:
>
> http://thread.gmane.org/gmane.comp.version-control.git/209781
>
> That series doesn't duplicate your work, but the two series's should
> be resolved.
Thanks I'm watching that discussion now and probably will rebase on
top of yours once it's finalized. I actually wrote this patch a while
ago, just waiting for nd/builtin-to-libgit to graduate because of some
linking issues this patch causes. I can wait a bit longer until yours
graduates.
--
Duy
^ permalink raw reply
* Re: [PATCH] wildmatch: correct isprint and isspace
From: Nguyen Thai Ngoc Duy @ 2012-11-16 4:19 UTC (permalink / raw)
To: Jan H. Schönherr
Cc: git, Junio C Hamano, rene.scharfe, Johannes Sixt, torvalds
In-Reply-To: <50A522B5.7080206@cs.tu-berlin.de>
On Fri, Nov 16, 2012 at 12:13 AM, "Jan H. Schönherr"
<schnhrr@cs.tu-berlin.de> wrote:
>> #define isprint(x) (sane_istest(x, GIT_ALPHA | GIT_DIGIT | GIT_SPACE | \
>> GIT_PUNCT | GIT_REGEX_SPECIAL | GIT_GLOB_SPECIAL | \
>> - GIT_PATHSPEC_MAGIC))
>> + GIT_PATHSPEC_MAGIC) && \
>> + (x) >= 32)
>
> May I suggest the current is_print() implementation in master:
>
> #define isprint(x) ((x) >= 0x20 && (x) <= 0x7e)
>
>
> To summarize my opinion:
>
> I no longer see a reason to correct isspace() (unless somebody with an actual
> use case complains), and a more POSIXly isprint() is already in master.
>
> => Nothing to do. :)
Yeah. I remember to remind myself to check "the implementation in
master" you mentioned but I probably failed at that. Just checked that
isprint() is already in master, and your comment about isspace() use
in wildmatch.c makes sense too. So I'm all for doing nothing.
--
Duy
^ permalink raw reply
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
From: Junio C Hamano @ 2012-11-16 5:17 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Thomas Rast, Jonathan Nieder
In-Reply-To: <20121115172845.GA20298@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> That is a good question. That confirmation step does come after they
> have typed their cover letter. However, if they are using --compose,
> they are dumped in their editor with something like:
>
> From Jeff King <peff@peff.net> # This line is ignored.
> GIT: Lines beginning in "GIT:" will be removed.
> GIT: Consider including an overall diffstat or table of contents
> GIT: for the patch you are writing.
> GIT:
> GIT: Clear the body content if you don't wish to send a summary.
> From: Jeff King <peff@peff.net>
> Subject:
> In-Reply-To:
>
> which I think would count as sufficient notice of the address being
> used.
OK. Tentatively I replaced your old series with these 8 patches
including the last one, as I tend to agree with the value the
earlier clean-up in the series gives us in the longer term. As you
and Felipe discussed, we may want to replace the last one with a
simpler "don't bother asking" patch, but I think that is more or
less an orthogonal issue.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox