* [PATCH] fast-import: Create import-marks file when necessary
@ 2011-01-09 19:33 Ramkumar Ramachandra
2011-01-09 22:48 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Ramkumar Ramachandra @ 2011-01-09 19:33 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder
When both --import-marks and --export-marks are given, and specify the
same file, that file should be created if it doesn't exist. Without
this patch, the caller would have to check for the existence of a
marks file and vary its fast-import invocation accordingly.
Requested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
fast-import.c | 17 ++++++++++++++++-
1 files changed, 16 insertions(+), 1 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 5055504..cb7a9c9 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3247,8 +3247,23 @@ static void parse_argv(void)
usage(fast_import_usage);
seen_data_command = 1;
- if (import_marks_file)
+ if (import_marks_file) {
+ FILE *f = fopen(import_marks_file, "r");
+ if (!f) {
+ if (errno == ENOENT &&
+ !strcmp(import_marks_file, export_marks_file)) {
+ FILE *d = fopen(import_marks_file, "w");
+ if (!d)
+ die_errno("cannot create '%s'", import_marks_file);
+ fclose(d);
+ }
+ else
+ die_errno("cannot read '%s'", import_marks_file);
+ }
+ else
+ fclose(f);
read_marks();
+ }
}
int main(int argc, const char **argv)
--
1.7.4.rc1.8.g98938a.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fast-import: Create import-marks file when necessary
2011-01-09 19:33 [PATCH] fast-import: Create import-marks file when necessary Ramkumar Ramachandra
@ 2011-01-09 22:48 ` Junio C Hamano
2011-01-11 17:14 ` [PATCH] fast-import: Don't barf if import-marks file is missing Ramkumar Ramachandra
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2011-01-09 22:48 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Jonathan Nieder
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> When both --import-marks and --export-marks are given, and specify the
> same file, that file should be created if it doesn't exist. Without
> this patch, the caller would have to check for the existence of a
> marks file and vary its fast-import invocation accordingly.
>
> Requested-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Do you mean that you are solving a problem for bootstrapping a script that
says "read from this file if exists, do work, and then write to the same
file for later invocation"?
Assuming that is what you are solving, why do you create the file in your
patch in the import codepath? I would imagine a sane logic flow for "read
if exists, do work and then write to the same" would be more like:
if (import_marks) {
FILE = *f = fopen(import_marks);
if (f)
read_marks();
else if (errno == ENOENT && !strcmp(import_marks, export_marks))
; /* Bootstrapping with "read if exists" */
else
barf();
}
no?
Also I find it a bad taste to use "The user gave the same file to import
and export" as a hint for "read if exists".
The first two lines of the proposed commit message is way suboptimal, if
the problem is "Not allowing read-if-exists-but-do-not-barf-if-missing
makes it hard to bootstrap repeated invocations that use a single
import/export marks file as the state-keeping mechanism".
It does not have to be created if it doesn't exist---the program only
needs not to barf. The condition does not have to be "if they are the
same file". The caller may want to script
command --import-if-exists=state --export=new-state+ &&
mv new-state+ state
to make sure that when the underlying command fails, the previous state is
still kept intact without getting overwritten.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] fast-import: Don't barf if import-marks file is missing
2011-01-09 22:48 ` Junio C Hamano
@ 2011-01-11 17:14 ` Ramkumar Ramachandra
2011-01-11 17:51 ` Jonathan Nieder
2011-01-11 20:05 ` [PATCH] fast-import: Don't barf if import-marks file is missing Junio C Hamano
0 siblings, 2 replies; 6+ messages in thread
From: Ramkumar Ramachandra @ 2011-01-11 17:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Git List
The --import-marks option used to barf when the specified marks file
doesn't exist. Change its meaning to "read marks file if it exists" so
that callers don't have to handle bootstraping as a special case.
Requested-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks-to: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
Documentation/git-fast-import.txt | 2 +-
fast-import.c | 6 +++++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index f56dfca..94fbe54 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -72,7 +72,7 @@ OPTIONS
--import-marks=<file>::
Before processing any input, load the marks specified in
- <file>. The input file must exist, must be readable, and
+ <file>, if it exists. The input file must be readable, and
must use the same format as produced by \--export-marks.
Multiple options may be supplied to import more than one
set of marks. If a mark is defined to different values,
diff --git a/fast-import.c b/fast-import.c
index 7857760..cbd5124 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1795,7 +1795,11 @@ static void read_marks(void)
{
char line[512];
FILE *f = fopen(import_marks_file, "r");
- if (!f)
+ if (f)
+ ;
+ else if (errno == ENOENT)
+ return; /* Marks file does not exist */
+ else
die_errno("cannot read '%s'", import_marks_file);
while (fgets(line, sizeof(line), f)) {
uintmax_t mark;
--
1.7.2.2.409.gdbb11.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fast-import: Don't barf if import-marks file is missing
2011-01-11 17:14 ` [PATCH] fast-import: Don't barf if import-marks file is missing Ramkumar Ramachandra
@ 2011-01-11 17:51 ` Jonathan Nieder
2011-01-15 6:31 ` [PATCH] fast-import: Introduce --import-marks-if-exists Ramkumar Ramachandra
2011-01-11 20:05 ` [PATCH] fast-import: Don't barf if import-marks file is missing Junio C Hamano
1 sibling, 1 reply; 6+ messages in thread
From: Jonathan Nieder @ 2011-01-11 17:51 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
Ramkumar Ramachandra wrote:
> The --import-marks option used to barf when the specified marks file
> doesn't exist.
Micronit: change descriptions should contrast present behavior
The --import-marks options errors out when the marks file is
missing.
with user requirements
But if a frontend is just using a marks file to ensure its state
persists between runs, it would be simpler if missing marks
files were tolerated. In some cases the frontend cannot even
check for the existence of the marks file itself because with
--relative-marks, the location of the marks file is
backend-dependent.
and the proposed solution
Change the meaning of --import-marks to "read marks file if it
exists so such frontends don't have to use a different command
line when bootstrapping.
I.e., the preferred style is to use present tense for the description
of present behavior, so the patch can be read as a bug report.
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -72,7 +72,7 @@ OPTIONS
>
> --import-marks=<file>::
> Before processing any input, load the marks specified in
> - <file>. The input file must exist, must be readable, and
> + <file>, if it exists.
This means removing a typo detection facility.
git fast-import --import-marks=something-like-a-real-marks-file.marks
would be accepted and result in an empty marks table instead of an
error message. Is that considered worth it? (My hunch is no, but
either way, I think it ought to be mentioned in the change
description.)
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1795,7 +1795,11 @@ static void read_marks(void)
Nice implementation. Maybe something like this on top?
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Documentation/git-fast-import.txt | 6 +++-
fast-import.c | 12 ++++++--
t/t9300-fast-import.sh | 55 +++++++++++++++++++++++++++++++++++++
3 files changed, 69 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 94fbe54..32a062c 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -72,12 +72,16 @@ OPTIONS
--import-marks=<file>::
Before processing any input, load the marks specified in
- <file>, if it exists. The input file must be readable, and
+ <file>. The input file must exist, must be readable, and
must use the same format as produced by \--export-marks.
Multiple options may be supplied to import more than one
set of marks. If a mark is defined to different values,
the last file wins.
+--import-marks-if-exists=<file>::
+ Like --import-marks but instead of erroring out, silently
+ skips the file if it does not exist.
+
--relative-marks::
After specifying --relative-marks= the paths specified
with --import-marks= and --export-marks= are relative
diff --git a/fast-import.c b/fast-import.c
index cbd5124..c525fda 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -329,6 +329,7 @@ static struct mark_set *marks;
static const char *export_marks_file;
static const char *import_marks_file;
static int import_marks_file_from_stream;
+static int import_marks_file_ignore_missing;
static int relative_marks_paths;
/* Our last blob */
@@ -1797,7 +1798,7 @@ static void read_marks(void)
FILE *f = fopen(import_marks_file, "r");
if (f)
;
- else if (errno == ENOENT)
+ else if (import_marks_file_ignore_missing && errno == ENOENT)
return; /* Marks file does not exist */
else
die_errno("cannot read '%s'", import_marks_file);
@@ -2865,7 +2866,8 @@ static char* make_fast_import_path(const char *path)
return strbuf_detach(&abs_path, NULL);
}
-static void option_import_marks(const char *marks, int from_stream)
+static void option_import_marks(const char *marks,
+ int from_stream, int ignore_missing)
{
if (import_marks_file) {
if (from_stream)
@@ -2879,6 +2881,7 @@ static void option_import_marks(const char *marks, int from_stream)
import_marks_file = make_fast_import_path(marks);
safe_create_leading_directories_const(import_marks_file);
import_marks_file_from_stream = from_stream;
+ import_marks_file_ignore_missing = ignore_missing;
}
static void option_date_format(const char *fmt)
@@ -2978,7 +2981,10 @@ static int parse_one_feature(const char *feature, int from_stream)
if (!prefixcmp(feature, "date-format=")) {
option_date_format(feature + 12);
} else if (!prefixcmp(feature, "import-marks=")) {
- option_import_marks(feature + 13, from_stream);
+ option_import_marks(feature + 13, from_stream, 0);
+ } else if (!prefixcmp(feature, "import-marks-if-exists=")) {
+ option_import_marks(feature + strlen("import-marks-if-exists="),
+ from_stream, 1);
} else if (!prefixcmp(feature, "export-marks=")) {
option_export_marks(feature + 13);
} else if (!strcmp(feature, "cat-blob")) {
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 222d105..870e55b 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1706,6 +1706,61 @@ test_expect_success \
'cat input | git fast-import --export-marks=other.marks &&
grep :1 other.marks'
+test_expect_success 'R: catch typo in marks file name' '
+ test_must_fail git fast-import --import-marks=nonexistent.marks </dev/null &&
+ echo "feature import-marks=nonexistent.marks" |
+ test_must_fail git fast-import
+'
+
+test_expect_success 'R: import and output marks can be the same file' '
+ rm -f io.marks &&
+ blob=$(echo hi | git hash-object --stdin) &&
+ cat >expect <<-EOF &&
+ :1 $blob
+ :2 $blob
+ EOF
+ git fast-import --export-marks=io.marks <<-\EOF &&
+ blob
+ mark :1
+ data 3
+ hi
+
+ EOF
+ git fast-import --import-marks=io.marks --export-marks=io.marks <<-\EOF &&
+ blob
+ mark :2
+ data 3
+ hi
+
+ EOF
+ test_cmp expect io.marks
+'
+
+test_expect_success 'R: --import-marks=foo --output-marks=foo to create foo fails' '
+ rm -f io.marks &&
+ test_must_fail git fast-import --import-marks=io.marks --export-marks=io.marks <<-\EOF
+ blob
+ mark :1
+ data 3
+ hi
+
+ EOF
+'
+
+test_expect_success 'R: --import-marks-if-exists' '
+ rm -f io.marks &&
+ blob=$(echo hi | git hash-object --stdin) &&
+ echo ":1 $blob" >expect &&
+ git fast-import --import-marks-if-exists=io.marks --export-marks=io.marks <<-\EOF &&
+ blob
+ mark :1
+ data 3
+ hi
+
+ EOF
+ test_cmp expect io.marks
+'
+
cat >input << EOF
feature import-marks=marks.out
feature export-marks=marks.new
--
1.7.4.rc1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fast-import: Don't barf if import-marks file is missing
2011-01-11 17:14 ` [PATCH] fast-import: Don't barf if import-marks file is missing Ramkumar Ramachandra
2011-01-11 17:51 ` Jonathan Nieder
@ 2011-01-11 20:05 ` Junio C Hamano
1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2011-01-11 20:05 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Jonathan Nieder, Git List
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> The --import-marks option used to barf when the specified marks file
> doesn't exist. Change its meaning to "read marks file if it exists" so
> that callers don't have to handle bootstraping as a special case.
It is very plausible that people other than you are relying on "barf if it
does not exist" semantics, and the above does not justify breaking their
workflow.
I'd suggest making this an optional feature.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] fast-import: Introduce --import-marks-if-exists
2011-01-11 17:51 ` Jonathan Nieder
@ 2011-01-15 6:31 ` Ramkumar Ramachandra
0 siblings, 0 replies; 6+ messages in thread
From: Ramkumar Ramachandra @ 2011-01-15 6:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Git List
The --import-marks option errors out when the specified marks file
doesn't exist. If a frontend is just using a marks file to ensure its
state persists between runs, it would be simpler if missing marks
files were tolerated. In some cases, the frontend cannot even check
for the existence of the marks file because with --relative-marks, the
location of the marks file is backend-dependent. This can be solved by
changing the meaning of --import-marks, but will result in trading off
functionality that some frontends might desire. So, add new
command-line option --import-marks-if-exists that's like
--import-marks, except that it tolerates missing marks files.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
Changes since last time: Junio suggested making this an optional
feature, and Jonathan wrote a fixup patch for the same.
Documentation/git-fast-import.txt | 4 +++
fast-import.c | 16 +++++++++--
t/t9300-fast-import.sh | 55 +++++++++++++++++++++++++++++++++++++
3 files changed, 72 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index f56dfca..32a062c 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -78,6 +78,10 @@ OPTIONS
set of marks. If a mark is defined to different values,
the last file wins.
+--import-marks-if-exists=<file>::
+ Like --import-marks but instead of erroring out, silently
+ skips the file if it does not exist.
+
--relative-marks::
After specifying --relative-marks= the paths specified
with --import-marks= and --export-marks= are relative
diff --git a/fast-import.c b/fast-import.c
index 7857760..c525fda 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -329,6 +329,7 @@ static struct mark_set *marks;
static const char *export_marks_file;
static const char *import_marks_file;
static int import_marks_file_from_stream;
+static int import_marks_file_ignore_missing;
static int relative_marks_paths;
/* Our last blob */
@@ -1795,7 +1796,11 @@ static void read_marks(void)
{
char line[512];
FILE *f = fopen(import_marks_file, "r");
- if (!f)
+ if (f)
+ ;
+ else if (import_marks_file_ignore_missing && errno == ENOENT)
+ return; /* Marks file does not exist */
+ else
die_errno("cannot read '%s'", import_marks_file);
while (fgets(line, sizeof(line), f)) {
uintmax_t mark;
@@ -2861,7 +2866,8 @@ static char* make_fast_import_path(const char *path)
return strbuf_detach(&abs_path, NULL);
}
-static void option_import_marks(const char *marks, int from_stream)
+static void option_import_marks(const char *marks,
+ int from_stream, int ignore_missing)
{
if (import_marks_file) {
if (from_stream)
@@ -2875,6 +2881,7 @@ static void option_import_marks(const char *marks, int from_stream)
import_marks_file = make_fast_import_path(marks);
safe_create_leading_directories_const(import_marks_file);
import_marks_file_from_stream = from_stream;
+ import_marks_file_ignore_missing = ignore_missing;
}
static void option_date_format(const char *fmt)
@@ -2974,7 +2981,10 @@ static int parse_one_feature(const char *feature, int from_stream)
if (!prefixcmp(feature, "date-format=")) {
option_date_format(feature + 12);
} else if (!prefixcmp(feature, "import-marks=")) {
- option_import_marks(feature + 13, from_stream);
+ option_import_marks(feature + 13, from_stream, 0);
+ } else if (!prefixcmp(feature, "import-marks-if-exists=")) {
+ option_import_marks(feature + strlen("import-marks-if-exists="),
+ from_stream, 1);
} else if (!prefixcmp(feature, "export-marks=")) {
option_export_marks(feature + 13);
} else if (!strcmp(feature, "cat-blob")) {
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 222d105..870e55b 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1706,6 +1706,61 @@ test_expect_success \
'cat input | git fast-import --export-marks=other.marks &&
grep :1 other.marks'
+test_expect_success 'R: catch typo in marks file name' '
+ test_must_fail git fast-import --import-marks=nonexistent.marks </dev/null &&
+ echo "feature import-marks=nonexistent.marks" |
+ test_must_fail git fast-import
+'
+
+test_expect_success 'R: import and output marks can be the same file' '
+ rm -f io.marks &&
+ blob=$(echo hi | git hash-object --stdin) &&
+ cat >expect <<-EOF &&
+ :1 $blob
+ :2 $blob
+ EOF
+ git fast-import --export-marks=io.marks <<-\EOF &&
+ blob
+ mark :1
+ data 3
+ hi
+
+ EOF
+ git fast-import --import-marks=io.marks --export-marks=io.marks <<-\EOF &&
+ blob
+ mark :2
+ data 3
+ hi
+
+ EOF
+ test_cmp expect io.marks
+'
+
+test_expect_success 'R: --import-marks=foo --output-marks=foo to create foo fails' '
+ rm -f io.marks &&
+ test_must_fail git fast-import --import-marks=io.marks --export-marks=io.marks <<-\EOF
+ blob
+ mark :1
+ data 3
+ hi
+
+ EOF
+'
+
+test_expect_success 'R: --import-marks-if-exists' '
+ rm -f io.marks &&
+ blob=$(echo hi | git hash-object --stdin) &&
+ echo ":1 $blob" >expect &&
+ git fast-import --import-marks-if-exists=io.marks --export-marks=io.marks <<-\EOF &&
+ blob
+ mark :1
+ data 3
+ hi
+
+ EOF
+ test_cmp expect io.marks
+'
+
cat >input << EOF
feature import-marks=marks.out
feature export-marks=marks.new
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-01-15 6:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-09 19:33 [PATCH] fast-import: Create import-marks file when necessary Ramkumar Ramachandra
2011-01-09 22:48 ` Junio C Hamano
2011-01-11 17:14 ` [PATCH] fast-import: Don't barf if import-marks file is missing Ramkumar Ramachandra
2011-01-11 17:51 ` Jonathan Nieder
2011-01-15 6:31 ` [PATCH] fast-import: Introduce --import-marks-if-exists Ramkumar Ramachandra
2011-01-11 20:05 ` [PATCH] fast-import: Don't barf if import-marks file is missing Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.