Re: [PATCH v2 2/3] Allow for MERGE_MODE to specify more then one mode

2012-11-27 Thread Junio C Hamano
Kacper Kornet  writes:

> This way when only no-ff is specified all parsers should be happy. If
> reversed-order is specified together no-ff the "external" parser
> probably would fail. Which in my opinion is a good think at this point,
> as it can't correctly interpret MERGE_MODE anyway.

Amen to that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 2/3] Allow for MERGE_MODE to specify more then one mode

2012-11-27 Thread Kacper Kornet
On Tue, Nov 27, 2012 at 06:17:28PM -0800, Junio C Hamano wrote:
> Kacper Kornet  writes:

> > Presently only one merge mode exists: non-fast-forward. But in future
> > the second one (transpose-parents) will be added, so the need to read
> > all lines of MERGE_MODE.

> > Signed-off-by: Kacper Kornet 
> > ---
> >  builtin/commit.c | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)

> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index 273332f..ee0e884 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -1427,7 +1427,6 @@ int cmd_commit(int argc, const char **argv, const 
> > char *prefix)
> > unsigned char sha1[20];
> > struct ref_lock *ref_lock;
> > struct commit_list *parents = NULL, **pptr = &parents;
> > -   struct stat statbuf;
> > int allow_fast_forward = 1;
> > struct commit *current_head = NULL;
> > struct commit_extra_header *extra = NULL;
> > @@ -1481,11 +1480,12 @@ int cmd_commit(int argc, const char **argv, const 
> > char *prefix)

> > if (!reflog_msg)
> > reflog_msg = "commit (merge)";
> > -   if (!stat(git_path("MERGE_MODE"), &statbuf)) {
> > -   if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 
> > 0)
> > -   die_errno(_("could not read MERGE_MODE"));
> > -   if (!strcmp(sb.buf, "no-ff"))
> > -   allow_fast_forward = 0;
> > +   if((fp = fopen(git_path("MERGE_MODE"), "r"))) {

> Style: s/if((fp/if ((fp/;

> > +   while (strbuf_getline(&m, fp, '\n') != EOF) {
> > +   if (!strcmp(m.buf, "no-ff"))
> > +   allow_fast_forward = 0;
> > +   }
> > +   fclose(fp);

> This needs a bit more careful planning for interacting with other
> people's programs, I suspect.

> Your updated builtin/merge.c may write an extra LF after no-ff to
> make this parser to grok it, but it is entirely plausible that
> people have their own Porcelain that writes "no-ff" without LF
> (because that is what we read from this file, and I suspect the
> current code would ignore "no-ff\n").

> At least strbuf_getline() would give us "no-ff" when either "no-ff"
> or "no-ff\n" terminates the file, so updated code would be able to
> grok what other people would write, but if other people want to read
> MERGE_MODE we write, at least we shouldn't break them when we only
> write no-ff in it (once you start writing "reverse-parents" in the
> file, they will be broken anyway, as they do not currently expect
> such token in this file).

At this stage in the patch series the format of MERGE_MODE is not
changed - "no-ff" is printed without "\n". What should be changed is the
next patch. Relevant part should read:

diff --git a/builtin/merge.c b/builtin/merge.c
index a96e8ea..5ceb291 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -993,6 +999,8 @@ static void write_merge_state(struct commit_list 
*remoteheads)
if (fd < 0)
die_errno(_("Could not open '%s' for writing"), filename);
strbuf_reset(&buf);
+   if (reversed_order)
+   strbuf_addf(&buf, "reversed-order\n");
if (!allow_fast_forward)
strbuf_addf(&buf, "no-ff");
if (write_in_full(fd, buf.buf, buf.len) != buf.len)

This way when only no-ff is specified all parsers should be happy. If
reversed-order is specified together no-ff the "external" parser
probably would fail. Which in my opinion is a good think at this point,
as it can't correctly interpret MERGE_MODE anyway.

-- 
  Kacper
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 2/3] Allow for MERGE_MODE to specify more then one mode

2012-11-27 Thread Junio C Hamano
Kacper Kornet  writes:

> Presently only one merge mode exists: non-fast-forward. But in future
> the second one (transpose-parents) will be added, so the need to read
> all lines of MERGE_MODE.
>
> Signed-off-by: Kacper Kornet 
> ---
>  builtin/commit.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 273332f..ee0e884 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1427,7 +1427,6 @@ int cmd_commit(int argc, const char **argv, const char 
> *prefix)
>   unsigned char sha1[20];
>   struct ref_lock *ref_lock;
>   struct commit_list *parents = NULL, **pptr = &parents;
> - struct stat statbuf;
>   int allow_fast_forward = 1;
>   struct commit *current_head = NULL;
>   struct commit_extra_header *extra = NULL;
> @@ -1481,11 +1480,12 @@ int cmd_commit(int argc, const char **argv, const 
> char *prefix)
>  
>   if (!reflog_msg)
>   reflog_msg = "commit (merge)";
> - if (!stat(git_path("MERGE_MODE"), &statbuf)) {
> - if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 
> 0)
> - die_errno(_("could not read MERGE_MODE"));
> - if (!strcmp(sb.buf, "no-ff"))
> - allow_fast_forward = 0;
> + if((fp = fopen(git_path("MERGE_MODE"), "r"))) {

Style: s/if((fp/if ((fp/;

> + while (strbuf_getline(&m, fp, '\n') != EOF) {
> + if (!strcmp(m.buf, "no-ff"))
> + allow_fast_forward = 0;
> + }
> + fclose(fp);

This needs a bit more careful planning for interacting with other
people's programs, I suspect.

Your updated builtin/merge.c may write an extra LF after no-ff to
make this parser to grok it, but it is entirely plausible that
people have their own Porcelain that writes "no-ff" without LF
(because that is what we read from this file, and I suspect the
current code would ignore "no-ff\n").

At least strbuf_getline() would give us "no-ff" when either "no-ff"
or "no-ff\n" terminates the file, so updated code would be able to
grok what other people would write, but if other people want to read
MERGE_MODE we write, at least we shouldn't break them when we only
write no-ff in it (once you start writing "reverse-parents" in the
file, they will be broken anyway, as they do not currently expect
such token in this file).

I am starting to wonder if this is worth it, though...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html