* [PATCH 4/4] i18n: diff: mark warnings for translation
From: Vasco Almeida @ 2016-10-17 13:15 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA
In-Reply-To: <20161017131529.27856-1-vascomalmeida@sapo.pt>
Mark rename_limit_warning and degrade_cc_to_c_warning and
rename_limit_warning for translation.
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
diff.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/diff.c b/diff.c
index 1d304e0..1687317 100644
--- a/diff.c
+++ b/diff.c
@@ -4638,25 +4638,25 @@ static int is_summary_empty(const struct diff_queue_struct *q)
}
static const char rename_limit_warning[] =
-"inexact rename detection was skipped due to too many files.";
+N_("inexact rename detection was skipped due to too many files.");
static const char degrade_cc_to_c_warning[] =
-"only found copies from modified paths due to too many files.";
+N_("only found copies from modified paths due to too many files.");
static const char rename_limit_advice[] =
-"you may want to set your %s variable to at least "
-"%d and retry the command.";
+N_("you may want to set your %s variable to at least "
+ "%d and retry the command.");
void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
{
if (degraded_cc)
- warning(degrade_cc_to_c_warning);
+ warning(_(degrade_cc_to_c_warning));
else if (needed)
- warning(rename_limit_warning);
+ warning(_(rename_limit_warning));
else
return;
if (0 < needed && needed < 32767)
- warning(rename_limit_advice, varname, needed);
+ warning(_(rename_limit_advice), varname, needed);
}
void diff_flush(struct diff_options *options)
--
2.10.1.459.g5fd885d
^ permalink raw reply related
* [PATCH 2/4] i18n: convert mark error messages for translation
From: Vasco Almeida @ 2016-10-17 13:15 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA
In-Reply-To: <20161017131529.27856-1-vascomalmeida@sapo.pt>
Mark error messages about CRLF for translation.
Update test to reflect changes.
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
convert.c | 12 ++++++++----
t/t0020-crlf.sh | 6 +++++-
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/convert.c b/convert.c
index 077f5e6..0ad39b1 100644
--- a/convert.c
+++ b/convert.c
@@ -197,17 +197,21 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
* CRLFs would not be restored by checkout
*/
if (checksafe == SAFE_CRLF_WARN)
- warning("CRLF will be replaced by LF in %s.\nThe file will have its original line endings in your working directory.", path);
+ warning(_("CRLF will be replaced by LF in %s.\n"
+ "The file will have its original line"
+ " endings in your working directory."), path);
else /* i.e. SAFE_CRLF_FAIL */
- die("CRLF would be replaced by LF in %s.", path);
+ die(_("CRLF would be replaced by LF in %s."), path);
} else if (old_stats->lonelf && !new_stats->lonelf ) {
/*
* CRLFs would be added by checkout
*/
if (checksafe == SAFE_CRLF_WARN)
- warning("LF will be replaced by CRLF in %s.\nThe file will have its original line endings in your working directory.", path);
+ warning(_("LF will be replaced by CRLF in %s.\n"
+ "The file will have its original line"
+ " endings in your working directory."), path);
else /* i.e. SAFE_CRLF_FAIL */
- die("LF would be replaced by CRLF in %s", path);
+ die(_("LF would be replaced by CRLF in %s"), path);
}
}
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index f94120a..71350e0 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -83,7 +83,11 @@ test_expect_success 'safecrlf: print warning only once' '
git add doublewarn &&
git commit -m "nowarn" &&
for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn &&
- test $(git add doublewarn 2>&1 | grep "CRLF will be replaced by LF" | wc -l) = 1
+ git add doublewarn 2>err &&
+ if test_have_prereq C_LOCALE_OUTPUT
+ then
+ test $(grep "CRLF will be replaced by LF" err | wc -l) = 1
+ fi
'
--
2.10.1.459.g5fd885d
^ permalink raw reply related
* [PATCH 3/4] i18n: credential-cache--daemon: mark advice for translation
From: Vasco Almeida @ 2016-10-17 13:15 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA
In-Reply-To: <20161017131529.27856-1-vascomalmeida@sapo.pt>
Mark permissions_advice for translation.
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
credential-cache--daemon.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 1e5f16a..46c5937 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -219,11 +219,11 @@ static void serve_cache(const char *socket_path, int debug)
close(fd);
}
-static const char permissions_advice[] =
+static const char permissions_advice[] = N_(
"The permissions on your socket directory are too loose; other\n"
"users may be able to read your cached credentials. Consider running:\n"
"\n"
-" chmod 0700 %s";
+" chmod 0700 %s");
static void init_socket_directory(const char *path)
{
struct stat st;
@@ -232,7 +232,7 @@ static void init_socket_directory(const char *path)
if (!stat(dir, &st)) {
if (st.st_mode & 077)
- die(permissions_advice, dir);
+ die(_(permissions_advice), dir);
} else {
/*
* We must be sure to create the directory with the correct mode,
--
2.10.1.459.g5fd885d
^ permalink raw reply related
* [PATCH 1/4] i18n: apply: mark error message for translation
From: Vasco Almeida @ 2016-10-17 13:15 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA
Update test to reflect changes.
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
apply.c | 4 ++--
t/t4254-am-corrupt.sh | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/apply.c b/apply.c
index 8215874..705cf56 100644
--- a/apply.c
+++ b/apply.c
@@ -1586,8 +1586,8 @@ static int find_header(struct apply_state *state,
patch->new_name = xstrdup(patch->def_name);
}
if (!patch->is_delete && !patch->new_name) {
- error("git diff header lacks filename information "
- "(line %d)", state->linenr);
+ error(_("git diff header lacks filename information "
+ "(line %d)"), state->linenr);
return -128;
}
patch->is_toplevel_relative = 1;
diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
index 9bd7dd2..168739c 100755
--- a/t/t4254-am-corrupt.sh
+++ b/t/t4254-am-corrupt.sh
@@ -31,7 +31,7 @@ test_expect_success 'try to apply corrupted patch' '
test_expect_success 'compare diagnostic; ensure file is still here' '
echo "error: git diff header lacks filename information (line 4)" >expected &&
test_path_is_file f &&
- test_cmp expected actual
+ test_i18ncmp expected actual
'
test_done
--
2.10.1.459.g5fd885d
^ permalink raw reply related
* RE: Uninitialized submodules as symlinks
From: David Turner @ 2016-10-17 15:11 UTC (permalink / raw)
To: 'Heiko Voigt'; +Cc: git@vger.kernel.org
In-Reply-To: <20161013161017.GC29710@book.hvoigt.net>
> -----Original Message-----
> From: Heiko Voigt [mailto:hvoigt@hvoigt.net]
> Sent: Thursday, October 13, 2016 12:10 PM
> To: David Turner
> Cc: git@vger.kernel.org
> Subject: Re: Uninitialized submodules as symlinks
>
> On Fri, Oct 07, 2016 at 06:17:05PM +0000, David Turner wrote:
> > Presently, uninitialized submodules are materialized in the working
> > tree as empty directories. We would like to consider having them be
> > symlinks. Specifically, we'd like them to be symlinks into a FUSE
> > filesystem which retrieves files on demand.
>
> How about portability? This feature would only work on Unix like operating
> systems. You have to be careful to not break Windows since they do not
> have symlinks.
Windows doesn't support FUSE either IIRC. Since this would be an alternate mode of operation, Windows would still work fine on the old model.
^ permalink raw reply
* RE: Uninitialized submodules as symlinks
From: David Turner @ 2016-10-17 15:12 UTC (permalink / raw)
To: 'Duy Nguyen'; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <CACsJy8BAB4sqnv1GBk=K9yUuqVhW4Y=CHdON7mnM9PY08HwdGw@mail.gmail.com>
> -----Original Message-----
> From: Duy Nguyen [mailto:pclouds@gmail.com]
> Sent: Monday, October 17, 2016 5:46 AM
> To: David Turner
> Cc: Stefan Beller; git@vger.kernel.org
> Subject: Re: Uninitialized submodules as symlinks
>
> On Sat, Oct 8, 2016 at 2:59 AM, David Turner <David.Turner@twosigma.com>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stefan Beller [mailto:sbeller@google.com]
> >> Sent: Friday, October 07, 2016 2:56 PM
> >> To: David Turner
> >> Cc: git@vger.kernel.org
> >> Subject: Re: Uninitialized submodules as symlinks
> >>
> >> On Fri, Oct 7, 2016 at 11:17 AM, David Turner
> >> <David.Turner@twosigma.com>
> >> wrote:
> >> > Presently, uninitialized submodules are materialized in the working
> >> > tree as empty directories.
> >>
> >> Right, there has to be something, to hint at the user that creating a
> >> file with that path is probably not what they want.
> >>
> >> > We would like to consider having them be symlinks. Specifically,
> >> > we'd like them to be symlinks into a FUSE filesystem which
> >> > retrieves files on demand.
> >> >
> >> > We've actually already got a FUSE filesystem written, but we use a
> >> > different (semi-manual) means to connect it to the initialized
> submodules.
> >>
> >> So you currently do a
> >>
> >> git submodule init <pathspec>
> >> custom-submodule make-symlink <pathspec>
> >>
> >> ?
> >
> > We do something like
> >
> > For each initialized submodule: symlink it into the right place in
> > .../somedir For each uninitialized submodule: symlink from the FUSE
> > into the right place in .../somedir
> >
> > So .../somedir has the structure of the git main repo, but is all
> symlinks -- some into FUSE, some into the git repo.
> >
> > This means that when we initialize (or deinitialize) a submodule, we
> need to re-run the linking script.
>
> Do .git files work? If .git files point to somewhere in fuse, I guess you
> still have file retrieval on demand. It depends on what files to retrieve
> I guess. If you want worktree files, not object database then .git files
> won't work because worktree remains in the same filesystem as the super
> repo.
Yes, we want worktree files (or even worktree files + built artifacts).
^ permalink raw reply
* Re: Merge conflicts in .gitattributes can cause trouble
From: Johannes Schindelin @ 2016-10-17 16:11 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, Jeff King, me
In-Reply-To: <248A6E81-8D5C-4183-9756-51A0D5193E3E@gmail.com>
Hi Lars,
On Tue, 4 Oct 2016, Lars Schneider wrote:
> If there is a conflict in the .gitattributes during a merge then it looks
> like as if the attributes are not applied
I tried to replicate this behavior, to the point where I wrote a patch
that demonstrates the breakage so I could single-step in a debugger to
find out where things go wrong, and fix them.
Alas, I found out that the .gitattributes are read *before* any merge
conflict arises in the case you demonstrated. Which kind of makes sense,
because the gitattributes decide over which merge driver to use, among
other things.
So in your example:
> Consider this script on Windows:
>
> $ git init .
> $ touch first.commit
> $ git add .
> $ git commit -m "first commit"
>
> $ git checkout -b branch
> $ printf "*.bin binary\n" >> .gitattributes
> $ git add .
> $ git commit -m "tracking *.bin files"
>
> $ git checkout master
> $ printf "binary\ndata\n" > file.dat # <-- Unix line ending!
> $ printf "*.dat binary\n" >> .gitattributes # <-- Tell Git to keep Unix line ending!
> $ git add .
> $ git commit -m "tracking *.dat files"
> $ git cat-file -p :file.dat | od -c
> 0000000 b i n a r y \n d a t a \n
> ^^^^ ^^^^ <-- Correct!
> $ git checkout branch
At this point, the .gitattributes list only .bin files as binary. That is
the revision of the .gitattributes used by this command:
> $ git merge master # <-- Causes merge conflict!
And as a consequence, the .gitattributes do not tell Git that it should
handle .dat files as binary. Which means that...
> $ printf "*.bin binary\n*.dat binary\n" > .gitattributes # <-- Fix merge conflict!
> $ git add .
> $ git commit -m "merged"
> $ git cat-file -p :file.dat | od -c
> 0000000 b i n a r y \r \n d a t a \r \n
> ^^^^^^^^ ^^^^^^^^ <-- Wrong!
... this is actually expected! Why? Because the .gitattributes that were
in effect when the user asked to perform a merge said so.
If you adjust .gitattributes *before* merging `master`, it works as you
would expect: the line endings are not changed.
The reason to do it this way: we want to respect the .gitattributes as per
the current worktree. We go even so far that we respect uncommitted
changes to said file...
> Possible solutions:
>
> 1. We could print an appropriate warning if we detect a merge conflict
> in .gitattributes
>
> 2. We could disable all line ending conversions in case of a merge conflict
> (I am not exactly sure about all the implications, though)
>
> 3. We could salvage what we could of the .gitattributes file,
> perhaps by using the version from HEAD (or more likely, the ours stage of
> the index) -- suggested by Peff on the related GitHub issue mentioned below
I would vote for:
4. We keep letting Git read in the *current* version of .gitattributes
*before* the merge, and apply those attributes while performing the
merge.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+
From: Junio C Hamano @ 2016-10-17 16:54 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jakub Narębski, Git Mailing List
In-Reply-To: <CACBZZX4FK+zrVyBTpbJAbSAxQ9LuCTXcExeEqZE6D3nwHbNxZA@mail.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> As far as I can tell the only outstanding "change this" is your
> s/SHA1/SHA-1/ in <xmqq37k9jm86.fsf@gitster.mtv.corp.google.com>, do
> you want to fix that up or should I submit another series?
I think I did that already myself while queuing. Could you fetch
what I queued on 'pu' to double check?
I think the diff between what was posted and what is queued (I just
checked) looks like this:
-gitweb: Link to 7-char+ SHA1s, not only 8-char+
+gitweb: link to 7-char+ SHA-1s, not only 8-char+
Change the minimum length of an abbreviated object identifier in the
commit message gitweb tries to turn into link from 8 hexchars to 7.
@@ -5,16 +12,18 @@
SHA-1 in commit log message links to "object" view", 2006-12-10), but
the default abbreviation length is 7, and has been for a long time.
-It's still possible to reference SHA1s down to 4 characters in length,
+It's still possible to reference SHA-1s down to 4 characters in length,
see v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make
git actually produce that, so I doubt anyone is putting that into log
-messages in practice, but people definitely do put 7 character SHA1s
+messages in practice, but people definitely do put 7 character SHA-1s
into log messages.
I think it's fairly dubious to link to things matching [0-9a-fA-F]
here as opposed to just [0-9a-f], that dates back to the initial
version of gitweb from 161332a ("first working version",
-2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce
+2005-08-07). Git will accept all-caps SHA-1s, but didn't ever produce
them as far as I can tell.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+Acked-by: Jakub Narębski <jnareb@gmail.com>
+Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Junio C Hamano @ 2016-10-17 17:05 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Lars Schneider, git, Jakub Narębski, peff
In-Reply-To: <20161011100946.GA13745@tb-raspi>
Torsten Bögershausen <tboegi@web.de> writes:
>> +test_cmp_count () {
>> + expect=$1 actual=$2
>
> That could be
> expect="$1"
> actual="$2"
Yes, but it does not have to ;-).
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Junio C Hamano @ 2016-10-17 17:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <alpine.DEB.2.20.1610151020480.197091@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> I'll mark it as "wait for follow-up fix" in whats-cooking.txt (on
>> 'todo' branch) to remind myself not to merge it yet.
>
> May I request your guidance as to your preference how to proceed?
> ...
I guess I didn't see this before I sent my response to the review
thread, which was in my pile of "these need more thought than others
before responding" topics.
> Here are the options I see:
>
> A) remove the tests in question
>
> B) mark them as !MINGW instead
>
> C) change just those two tests from using `$PWD` (pseudo-Unix path) to
> `$(pwd)` (native path)
>
> I would like to hear your feedback about your preference, but not without
> priming you a little bit by detailing my current opinion on the matter:
>
> While I think B) would be the easiest to read, C) would document the
> expected behavior better. A) would feel to me like shrugging, i.e. the
> lazy, wrong thing to do.
>
> What do you think?
As to my preference on tests, I guess what I suggested was a cross
between your B and C below, and I can go with either one as an
abbreviated version of my preference ;-)
I am still wondering if the test is expecting the right behaviour,
though. If some codepaths rely on a question "please resolve '../.'
relative to 'path/to/dir/.'" being answered as "that's path/to/dir
itself", it smells to me that the downstream of the dataflow that
expects such an answer, as well as the machinery that produces such
an answer, are acting as two wrongs that happen to cancel each
other. Am I grossly misunderstanding what that test is doing?
^ permalink raw reply
* Re: [PATCH] convert: mark a file-local symbol static
From: Ramsay Jones @ 2016-10-17 17:15 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Junio C Hamano, GIT Mailing-list
In-Reply-To: <20161017021825.jgpsew7qqfjrirhw@sigill.intra.peff.net>
On 17/10/16 03:18, Jeff King wrote:
> On Mon, Oct 17, 2016 at 02:37:58AM +0100, Ramsay Jones wrote:
>
>> Hmm, well, you have to remember that 'make clean' sometimes
>> doesn't make clean. Ever since the Makefile was changed to only
>> remove $(OBJECTS), rather than *.o xdiff/*.o etc., you have to
>> remember to 'make clean' _before_ you switch branches. Otherwise,
>> you risk leaving some objects laying around. Since the script
>> runs 'nm' on all objects it finds, any stale ones can cause problems.
>> (Of course, I almost always forget, so I frequently have to manually
>> check for and remove stale objects!)
>
> Gross. I would not be opposed to a Makefile rule that outputs the
> correct set of OBJECTS so this (or other) scripts could build on it.
>
> IIRC, BSD make has an option to do this "make -V OBJECTS" or something,
> but I don't thnk there's an easy way to do so.
Hmm, I would go in the opposite direction and take a leaf out of
Ævar's book (see commit bc548efe) and this one-liner:
diff --git a/Makefile b/Makefile
index ee89c06..c08c25e 100644
--- a/Makefile
+++ b/Makefile
@@ -2506,7 +2506,7 @@ profile-clean:
clean: profile-clean coverage-clean
$(RM) *.res
- $(RM) $(OBJECTS)
+ $(RM) $(addsuffix *.o,$(object_dirs))
$(RM) $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
This would actually solve my problem, but it actually isn't a
_complete_ solution. (Hint: think about what isn't in $(OBJECTS),
depending on the configuration). ;-)
> Or, since it seems to find useful results quite frequently, maybe it
> would be worth including the script inside git (and triggering it with
> an optional Makefile rule). It sounds like we'd need a way to annotate
> known false positives, but if it were in common use, it would be easier
> to get people to keep that list up to date.
Hmm, I suspect that wouldn't happen, which would reduce it usefulness
and ultimately lead to it not being used. (Updating the 'stop list' would
fast become a burden.)
I find it useful to flag these issues automatically, but I still need
to look at each symbol and decide what to do (you may not agree with
some of my choices either - take a look at the output on the master
branch!).
The way I use it, I effectively ignore the 'stop list' maintenance issues.
ATB,
Ramsay Jones
^ permalink raw reply related
* Re: [PATCH v3 14/25] sequencer: introduce a helper to read files written by scripts
From: Junio C Hamano @ 2016-10-17 17:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <b554346c600fafa71d2a3461fda8402e377b596e.1476120229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> +/*
> + * Reads a file that was presumably written by a shell script, i.e.
> + * with an end-of-line marker that needs to be stripped.
> + *
> + * Returns 1 if the file was read, 0 if it could not be read or does not exist.
> + */
> +static int read_oneliner(struct strbuf *buf,
> + const char *path, int skip_if_empty)
> +...
> + if (strbuf_read_file(buf, path, 0) < 0) {
> + warning_errno(_("could not read '%s'"), path);
> + return 0;
> + }
> + if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
> + if (--buf->len > orig_len && buf->buf[buf->len - 1] == '\r')
> + --buf->len;
> + buf->buf[buf->len] = '\0';
> + }
The name says "oneliner" but this reads the whole thing and trims
only the last line of the input. Which is correct?
Do we want to error out if we got more than one line? That makes it
more strict. Going in the other direction, do we want to just read
the first line and ignore the remainder? That allows users to leave
cruft after what matters. I _think_ the existing code is closer to
the latter, i.e. something along the lines of ...
struct strbuf oneline = STRBUF_INIT;
FILE *fp = fopen(path, "r");
if (!fp) {
warning_errno(_("could not open '%s'"), path);
return 0;
}
if (strbuf_getline(&oneline, fp) < 0)
; /* EOF - empty */
else {
strbuf_addbuf(buf, &oneline);
}
^ permalink raw reply
* Re: [PATCH v3 15/25] sequencer: allow editing the commit message on a case-by-case basis
From: Junio C Hamano @ 2016-10-17 17:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <0577f66a108dced07775e10b87292c0eabfcc514.1476120229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> In the upcoming commits, we will implement more and more of rebase -i's
> functionality inside the sequencer. One particular feature of the
> commands to come is that some of them allow editing the commit message
> while others don't, i.e. we cannot define in the replay_opts whether the
> commit message should be edited or not.
>
> Let's add a new parameter to the run_git_commit() function. Previously,
> it was the duty of the caller to ensure that the opts->edit setting
> indicates whether to let the user edit the commit message or not,
> indicating that it is an "all or nothing" setting, i.e. that the
> sequencer wants to let the user edit *all* commit message, or none at
> all. In the upcoming rebase -i mode, it will depend on the particular
> command that is currently executed, though.
Makes tons of sense.
^ permalink raw reply
* Re: [PATCH] convert: mark a file-local symbol static
From: Ramsay Jones @ 2016-10-17 17:21 UTC (permalink / raw)
To: Jeff King, Johannes Schindelin
Cc: Lars Schneider, Junio C Hamano, GIT Mailing-list
In-Reply-To: <20161017093754.wah5d6cg4qgtw7ln@sigill.intra.peff.net>
On 17/10/16 10:37, Jeff King wrote:
> On Mon, Oct 17, 2016 at 11:04:19AM +0200, Johannes Schindelin wrote:
>
>>> Gross. I would not be opposed to a Makefile rule that outputs the
>>> correct set of OBJECTS so this (or other) scripts could build on it.
>>
>> You could also use the method I use in Git for Windows to "extend" the
>> Makefile:
>>
>> -- snipsnap --
>> cat >dummy.mak <<EOF
>> include Makefile
>>
>> blub: $(OBJECTS)
>> do-something-with $^
>> EOF
>>
>> make -f dummy.mak blub
>
> Hacky but clever. I like it.
>
> In the particular case of git, I think I've cheated similarly before by
> putting things in config.mak, though of course an arbitrary script can't
> assume it can overwrite that file.
Heh, I actually have the following in my config.mak already:
extra-clean: clean
find . -iname '*.o' -exec rm {} \;
But for some reason I _always_ type 'make clean' and then, to top
it off, I _always_ type the 'find' command by hand (I have no idea
why) :-D
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH v3 16/25] sequencer: support amending commits
From: Junio C Hamano @ 2016-10-17 17:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <ec146dc2f3a3698bd433bbec4d7ecbab762c1bfb.1476120229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> This teaches the run_git_commit() function to take an argument that will
> allow us to implement "todo" commands that need to amend the commit
> messages ("fixup", "squash" and "reword").
Likewise to 15/25, i.e. Good, though the growth by these two steps
starts to make me wonder if these three options should be crammed
into an unsigned "flags" bitword.
I see you have v4, so I'll ignore the remainder of this stale round
and start reading that updated one instead.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index b621f4b..403a4f0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -481,7 +481,7 @@ static char **read_author_script(void)
> * author metadata.
> */
> static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> - int allow_empty, int edit)
> + int allow_empty, int edit, int amend)
> {
> char **env = NULL;
> struct argv_array array;
> @@ -510,6 +510,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> argv_array_push(&array, "commit");
> argv_array_push(&array, "-n");
>
> + if (amend)
> + argv_array_push(&array, "--amend");
> if (opts->gpg_sign)
> argv_array_pushf(&array, "-S%s", opts->gpg_sign);
> if (opts->signoff)
> @@ -785,7 +787,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> }
> if (!opts->no_commit)
> res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> - opts, allow, opts->edit);
> + opts, allow, opts->edit, 0);
>
> leave:
> free_message(commit, &msg);
^ permalink raw reply
* Re: [PATCH] fetch: use "quick" has_sha1_file for tag following
From: Junio C Hamano @ 2016-10-17 17:30 UTC (permalink / raw)
To: Jeff King
Cc: Vegard Nossum, git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20161014185953.k4b5xwihlgvxurjc@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Still an impressive speedup as a percentage, but negligible in absolute
> terms. But that's on a local filesystem on a Linux machine. I'd worry
> much more about a system with a slow readdir(), e.g., due to NFS.
> Somebody's real-world NFS case[1] was what prompted us to do 0eeb077
> (index-pack: avoid excessive re-reading of pack directory, 2015-06-09).
Yes.
> It looks like I _did_ look into optimizing this into a single stat()
> call in the thread at [1]. I completely forgot about that. I did find
> there that naively using stat_validity() on a directory is racy, though
> I wonder if we could do something clever with gettimeofday() instead.
It feels funny to hear an idea to compare fs timestamp with gettimeofday
immedately after hearing the word NFS, though ;-).
>> I agree that the fallout from the inaccuracy of "quick" approach is
>> probably acceptable and the next "fetch" will correct it anyway, so
>> let's do the "quick but inaccurate" for now and perhaps cook it in
>> 'next' for a bit longer than other topics?
>
> I doubt that cooking in 'next' for longer will turn up anything useful.
> The case we care about is the race between a repack and a fetch. We
> lived with the "quick" version of has_sha1_file() everywhere for 8
> years.
A very convincing argument. I stand corrected.
Thanks.
^ permalink raw reply
* Re: [RFC] test-lib: detect common misuse of test_expect_failure
From: Junio C Hamano @ 2016-10-17 17:36 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161014235718.2rbejrzs5axhopt7@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I like the general idea, but I'm not sure how this would interact with
> the tests in t0000 that test the test suite.
I tried but gave up adding a new test for this to t0000 ;-)
>> test_expect_failure () {
>> + if test "$test_in_progress" = 1
>> + then
>> + error "bug in the test script: did you mean test_must_fail instead of test_expect_failure?"
>> + fi
>
> This follows existing practice for things like the &&-lint-checker, and
> bails out on the whole test script.
Yes, you guessed correctly where the above came from.
> That sometimes makes it hard to find
> the problematic test, especially if you're running via something like
> "prove", because it doesn't make valid TAP output.
Yeah, true.
> It might be nicer if we just said "this test is malformed, and therefore
> fails", and then you get all the usual niceties for recording and
> finding the failed test.
>
> I don't think it would be robust enough to try to propagate the error up
> to the outer test_expect_success block (and anyway, you'd also want to
> know about it in a test_expect_failure block; it's a bug in the test,
> not a known breakage). But perhaps error() could dump some TAP-like
> output with a "virtual" failed test.
>
> Something like:
> ...
> which lets "make prove" collect the broken test number.
>
> It would perhaps need to cover the case when $test_count is "0"
> separately. I dunno. It would be nicer still if we could continue
> running other tests in the script, but I think it's impossible to
> robustly jump back to the outer script.
>
> These kinds of "bug in the test suite" are presumably rare enough that
> the niceties don't matter that much, but I trigger the &&-checker
> reasonably frequently (that and test_line_count, because I can never
> remember the correct invocation).
>
> Anyway. That's all orthogonal to your patch. I just wondered if we could
> do better, but AFAICT the right way to do better is to hook into
> error(), which means your patch would not have to care exactly how it
> fails.
Yeah, the change to error() may be a good thing to do, but it has
quite a many callers in t/*lib*.sh and definitely deserves to be a
separate patch, not tied to this single test.
^ permalink raw reply
* Re: [RFC] Case insensitive Git attributes
From: Junio C Hamano @ 2016-10-17 17:50 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Johannes Schindelin, Stefan Beller, Lars Schneider, git
In-Reply-To: <CACsJy8ASnvryrrWvr4ACWL_9oeKgE_4SUqcRHGf+Y6kcaeDUhg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> I agree. Which is why I wrote "we probably want something in the same
> spirit but limited to .gitattributes and .gitignore only". In other
> words we could have core.someName that makes .gitattributes and
> .gitignore patterns case-insensitive (or core-sensitive). If it's
> present, it overrides core.ignoreCase. If it's not present,
> core.ignoreCase decides. I'm just not sure if the new config should
> cover everything involving filename's case in git. That's too big to
> fit in my head.
Once I stopped thinking about this as "filename's case", it does fit
my head ;-)
I view the proposed knob as making patterns in .gitattributes and
.gitignore case insensitive, iow, it is a lazy and useful short-hand
for (mentally) editing "*.c attr" to "*.[cC] attr" without touching
these files.
And I agree that the knob that is missing in today's Git should
default to whatever core.ignoreCase's value is, iow, on case
insensitive filesystem, attr and ignore may match case insensitively
in today's Git, but when the knob is introduced, it should allow
forcing case sensitive match there by setting it to false, just like
the knob is proposed to be used in the oppositite direction to force
case insensitive match regardless of the case insensitiveness of the
underlying filesystem.
^ permalink raw reply
* Re: Merge conflicts in .gitattributes can cause trouble
From: Junio C Hamano @ 2016-10-17 17:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Lars Schneider, git, Jeff King, me
In-Reply-To: <alpine.DEB.2.20.1610171803400.197091@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I would vote for:
>
> 4. We keep letting Git read in the *current* version of .gitattributes
> *before* the merge, and apply those attributes while performing the
> merge.
Even though this needs a major surgery to the way the attr subsystem
reads from these files, I think it is conceptually the cleanest.
^ permalink raw reply
* Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL
From: Stefan Beller @ 2016-10-17 17:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker
In-Reply-To: <xmqqfunvxxgu.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 17, 2016 at 12:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> > expecting success:
>>> > actual=$(git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../.') &&
>>> > test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
>>> >
>>> > +++ git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../.
>>> > ++ actual=C:/git-sdk-64/usr/src/git/wip/t/.
>>> > ++ test C:/git-sdk-64/usr/src/git/wip/t/. = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
>
> This may well be total misunderstanding on my side, but is the
> expectation of this test even correct? If it wants to take "../."
> relative to "$LEAD/t/trash utils/.", should't it go one level up
> with ".." to $LEAD/t and then stay there with ".", expecting
> "$LEAD/t" which is what the above is giving us?
>
> IOW, the above makes me wonder why having one of these as the base
>
> A - path/to/dir
> B - path/to/dir/
> C - path/to/dir/.
>
> to resolve the relative "../." give different results.
Because the shell script originally did "just"
relative="../."
if path/to/dir/ ends with slash, chop it off.
while $relative starts with "../";
do
chop off starting '../' of relative
chop of last '/' and following from "path/to/dir/."
done
(Linux:)
As B was made to A first, only C differs as a result, because
you had one more '/' in there.
(Windows:)
However Windows also detects '/.' (C) and makes it an A,
(in C only, because shell code was not treated Windows-sy)
which is where the incompatibility between Windows and other
platforms arises.
So we have a couple of choices (for Git)now:
* go back to using shell only for submodule things as that doesn't
have the regression and it alos plays nicely with Git for Windows.
* use C for the submodule code in Git and revert the regression fix
"because consistency across platforms trumps
consistency over time"
* use C for the submodule code in Git and keep the regression fix
"because consistency over time in Git proper is more important
than playing nicely with Git for Windows"
* would it be possible to revert this to shell on Windows only?
> Whether bash
> on Windows removes the dot at the end of C to turn it into B, as
> long as A and B give us the same result we wouldn't be hitting the
> problem, no?
Well in Git proper A,B are the same and C is different.
(B was fixed as a regression)
In Windows C is like B, which was different without the regression
fix, but now it is the same as A, too.
>
>>> > test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
>>> > test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
>>> > -test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
>>> > +test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."
>
>>> > The reasons this is ugly: we specifically test for *Unixy* paths when we
>>> > use $PWD, as opposed to *Windowsy* paths when using $(pwd).
>
> Just to ensure I am following this correctly, two tests that come
> before the one you are touching above have $PWD on the input side
> and $(pwd) on the expectation side. That is what you mean by the
> next paragraph, right? They want to make sure that you honor the
> Unixy user input on Windows and still produce Windowsy result, that
> is.
>
>>> > We do this to
>>> > ensure a certain level of confidence that running things such as
>>> >
>>> > git clone --recurse-submodules /z/project/.
>>> >
>>> > work. And now that does not work anymore.
>
> And I agree from that point of view that having to spell both sides
> as $(pwd) would mean you are not testing that "Unixy input to
> Windowsy output" expectation, but at the same time, I think you
> would want "Windowsy input to Windowsy output" combination also does
> produce correct result, which is not tested in the three tests shown
> above. IOW, probably you would want to test both (at least on any
> platform where $PWD and $(pwd) textually disagree) for all these
> [*1*], and the pair
>
> "../." taken relative to "$(pwd)/." must be "$(pwd)/."
> "../." taken relative to "$PWD/." must be "$(pwd)/."
>
> test, because of the limitation of your bash, cannot have the latter
> half of the pair, so you'd need to comment it out with in-code
> explanation, perhaps? IOW something along the lines of...
>
> -- >8 -- snip -- >8 --
>
> test_submodule_relative_url "(null)" "$(pwd)/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
> test_submodule_relative_url "(null)" "$(pwd)/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
> test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."
>
> if test_have_prereq MINGW
> then
>
> test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
> test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
> # This does not work correctly because Win-Bash strips . at the end
> # "of $PWD/."
> # test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
>
> fi
>
> -- >8 -- snip -- >8 --
>
> In any case, I find it more disturbing that we somehow ended up with
> a system where these three things are expected to behave differently:
>
> A - path/to/dir
> B - path/to/dir/
> C - path/to/dir/.
>
> Is that something we can fix?
Well A, B are the same.
C is "obviously" different, when it comes to counting slashes for relative
path/url purposes, in the way that there are characters after the last slash
and just by coincidence '.' refers to the directory itself, C behaving like
'path/to/dir/sub' seems right to me.
So how do you imagine this fix going forward?
* Breaking existing users with /. at the end? by treating it the same as A,B
* Do some check based on time/version of Git and cover the old data?
* Forbid /. at the end from now on?
>
>
> [Footnote]
>
> *1* It is tempting to update the above test sequence using
> a helper like:
>
> tsru () {
> test_submodule_relative_url "(null)" "$(pwd)/$1" "$2" "$(pwd)/$3"
> if test_have_prereq MINGW
> then
> test_submodule_relative_url "(null)" "$PWD/$1" "$2" "$(pwd)/$3"
> fi
> }
>
> then write the above three tests like so:
>
> tsru subsuper_update_r ../subsubsuper_update_r subsubsuper_update_r
> tsru super_update_r2 ../subsuper_update_r subsuper_update_r
> tsru . ../. .
>
> but you would want to disable the MINGW half for only the third
> test, we cannot quite do that.
^ permalink raw reply
* Re: [PATCH] cocci: avoid self-references in object_id transformations
From: Junio C Hamano @ 2016-10-17 18:08 UTC (permalink / raw)
To: René Scharfe; +Cc: brian m. carlson, Git List
In-Reply-To: <20161015134503.u3aznujploqee2le@vauxhall.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> On Sat, Oct 15, 2016 at 10:25:34AM +0200, René Scharfe wrote:
>> The object_id functions oid_to_hex, oid_to_hex_r, oidclr, oidcmp, and
>> oidcpy are defined as wrappers of their legacy counterparts sha1_to_hex,
>> sha1_to_hex_r, hashclr, hashcmp, and hashcpy, respectively. Make sure
>> that the Coccinelle transformations for converting legacy function calls
>> are not applied to these wrappers themselves, which would result in
>> tautological declarations.
>
> Ah, yes, this is a good idea. I've had to hack around this, but this is
> much better than having to fix it up by hand.
Yes, seeing an empty *.cocci.patch files after running coccicheck is
a great feeling, but without something like this patch, we can never
reach that goal ;-)
Thanks.
... oops. Totally unrelated to this patch, but I see these in
strbuf.cocci.patch (this is at the tip of 'pu'), which are total
nonsense. Perhaps I am running a way-stale spatch? It claims to be
"spatch version 1.0.0-rc19 with Python support and with PCRE support"
--- date.c
+++ /tmp/cocci-output-21568-bd3448-date.c
@@ -179,7 +179,7 @@ const char *show_date(unsigned long time
if (mode->type == DATE_UNIX) {
strbuf_reset(&timebuf);
- strbuf_addf(&timebuf, "%lu", time);
+ strbuf_addstr(&timebuf, time);
return timebuf.buf;
}
--- log-tree.c
+++ /tmp/cocci-output-21608-b02087-log-tree.c
@@ -400,7 +400,7 @@ void log_write_email_headers(struct rev_
extra_headers = subject_buffer;
if (opt->numbered_files)
- strbuf_addf(&filename, "%d", opt->nr);
+ strbuf_addstr(&filename, opt->nr);
else
fmt_output_commit(&filename, commit, opt);
snprintf(buffer, sizeof(buffer) - 1,
^ permalink raw reply
* Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL
From: Junio C Hamano @ 2016-10-17 18:28 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker
In-Reply-To: <CAGZ79kYW2qK1GKxoKy_mkVkjjqEUzkh5aPSzDEfRd6U=PYDdzw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> In any case, I find it more disturbing that we somehow ended up with
>> a system where these three things are expected to behave differently:
>>
>> A - path/to/dir
>> B - path/to/dir/
>> C - path/to/dir/.
>>
>> Is that something we can fix?
>
> Well A, B are the same.
> C is "obviously" different, when it comes to counting slashes for relative
> path/url purposes, in the way that there are characters after the last slash
> and just by coincidence '.' refers to the directory itself, C behaving like
> 'path/to/dir/sub' seems right to me.
It doesn't look right to me at all. If you were contrasting
cd path/to/dir/sub && cd ..
cd path/to/dir/bus && cd ..
then I would understand, but why should these two
cd path/to/dir/. && cd ..
cd path/to/dir/sub && cd ..
behave the same?
> So how do you imagine this fix going forward?
> * Breaking existing users with /. at the end? by treating it the same as A,B
> * Do some check based on time/version of Git and cover the old data?
> * Forbid /. at the end from now on?
Where at the end-user facing level does this trailing "/." surface
and how does the difference appear to them? I think that is the
crucial question.
Unless there is some convincing argument why "." is not special
(i.e. counter-argument to the above "bus vs sub" and ". vs sub"
example), I would think "existing users with /." does not matter.
If they are "relying" on the behaviour, I would think it is not
because they find that behaviour intuitive, but only because they
learned to live with it. IOW, treating all of A/B/C the same way
would appear to them a strict bugfix, I would think.
It is totally a different matter if OUR code that consumes the
output from the submodule-helper --resolve-relative" internally is
confused and relies on "../. relative to path/to/dir/. is the same
as ../. relative to path/to/dir/sub" for whatever reason. Without
fixing that, I would not surprised if fixing things to treat A/B/C
the same way would surface differences in the end-user observable
behaviour in a negative way.
^ permalink raw reply
* Re: [PATCH v11 00/14] Git filter protocol
From: Junio C Hamano @ 2016-10-17 18:47 UTC (permalink / raw)
To: larsxschneider; +Cc: git, jnareb, peff, ramsay, tboegi
In-Reply-To: <20161016232038.84951-1-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
> The goal of this series is to avoid launching a new clean/smudge filter
> process for each file that is filtered.
>
> A short summary about v1 to v5 can be found here:
> https://git.github.io/rev_news/2016/08/17/edition-18/
>
> This series is also published on web:
> https://github.com/larsxschneider/git/pull/15
>
> Patches 1 and 2 are cleanups and not strictly necessary for the series.
> Patches 3 to 12 are required preparation. Patch 13 is the main patch.
> Patch 14 adds an example how to use the Git filter protocol in contrib.
Will replace. If you ever need tor reroll 13, please squash the
following in (which I already did locally so there is no need to
resend only to correct it).
Thanks.
convert.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/convert.c b/convert.c
index 9d2aa68df9..bc242276ff 100644
--- a/convert.c
+++ b/convert.c
@@ -535,7 +535,8 @@ static int packet_write_list(int fd, const char *line, ...)
return packet_flush_gently(fd);
}
-static void read_multi_file_filter_status(int fd, struct strbuf *status) {
+static void read_multi_file_filter_status(int fd, struct strbuf *status)
+{
struct strbuf **pair;
char *line;
for (;;) {
--
2.10.1-613-g6ad57fc60c
^ permalink raw reply related
* Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL
From: Stefan Beller @ 2016-10-17 18:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker
In-Reply-To: <xmqqh98avnhh.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 17, 2016 at 11:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> In any case, I find it more disturbing that we somehow ended up with
>>> a system where these three things are expected to behave differently:
>>>
>>> A - path/to/dir
>>> B - path/to/dir/
>>> C - path/to/dir/.
>>>
>>> Is that something we can fix?
>>
>> Well A, B are the same.
>> C is "obviously" different, when it comes to counting slashes for relative
>> path/url purposes, in the way that there are characters after the last slash
>> and just by coincidence '.' refers to the directory itself, C behaving like
>> 'path/to/dir/sub' seems right to me.
>
> It doesn't look right to me at all. If you were contrasting
>
> cd path/to/dir/sub && cd ..
> cd path/to/dir/bus && cd ..
>
> then I would understand, but why should these two
>
> cd path/to/dir/. && cd ..
> cd path/to/dir/sub && cd ..
>
> behave the same?
>
>> So how do you imagine this fix going forward?
>> * Breaking existing users with /. at the end? by treating it the same as A,B
>> * Do some check based on time/version of Git and cover the old data?
>> * Forbid /. at the end from now on?
>
> Where at the end-user facing level does this trailing "/." surface
> and how does the difference appear to them? I think that is the
> crucial question.
>
> Unless there is some convincing argument why "." is not special
> (i.e. counter-argument to the above "bus vs sub" and ". vs sub"
> example), I would think "existing users with /." does not matter.
> If they are "relying" on the behaviour, I would think it is not
> because they find that behaviour intuitive, but only because they
> learned to live with it. IOW, treating all of A/B/C the same way
> would appear to them a strict bugfix, I would think.
I see, so we should adapt the windows style and chop off '/.'
to make A,B,C all the same, because internally we never produced
C AFAICT.
These came in via hand edited .gitmodules files.
>
> It is totally a different matter if OUR code that consumes the
> output from the submodule-helper --resolve-relative" internally is
> confused and relies on "../. relative to path/to/dir/. is the same
> as ../. relative to path/to/dir/sub" for whatever reason. Without
> fixing that, I would not surprised if fixing things to treat A/B/C
> the same way would surface differences in the end-user observable
> behaviour in a negative way.
>
^ permalink raw reply
* Re: [PATCH v4 05/25] sequencer: eventually release memory allocated for the option values
From: Junio C Hamano @ 2016-10-17 19:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <b771bfbc88734b74acda0c377446e984ca81fd9c.1476450940.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
> like a one-shot command when it reads its configuration: memory is
> allocated and released only when the command exits.
>
> This is kind of okay for git-cherry-pick, which *is* a one-shot
> command. All the work to make the sequencer its work horse was
> done to allow using the functionality as a library function, though,
> including proper clean-up after use.
>
> To remedy that, we now take custody of the option values in question,
> requiring those values to be malloc()ed or strdup()ed
That is the approach this patch takes, so "eventually release" in
the title is no longer accurate, I would think.
> Sadly, the current approach makes the code uglier, as we now have to
> take care to strdup() the values passed via the command-line.
I obviously disagree with that statement and the _entrust was too
ugly to live, but it is obviously subjective, and it boils down to
who has a better taste. Let's not go there.
> +
> + /* These option values will be free()d */
> + opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
> + opts->strategy = xstrdup_or_null(opts->strategy);
xstrdup-or-null does make things cleaner.
> +static int git_config_string_dup(char **dest,
> + const char *var, const char *value)
> +{
> + if (!value)
> + return config_error_nonbool(var);
> + free(*dest);
> + *dest = xstrdup(value);
> + return 0;
> +}
So does this.
^ 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